>From: Jiri Pirko <[email protected]>
>Sent: Friday, May 9, 2025 8:15 AM
>
>Thu, May 08, 2025 at 05:20:24PM +0200, [email protected]
>wrote:
>>>From: Jiri Pirko <[email protected]>
>>>Sent: Thursday, May 8, 2025 4:31 PM
>>>
>>>Thu, May 08, 2025 at 02:21:27PM +0200, [email protected]
>>>wrote:
>>>>Add new callback operations for a dpll device:
>>>>- phase_offset_monitor_get(..) - to obtain current state of phase offset
>>>> monitor feature from dpll device,
>>>>- phase_offset_monitor_set(..) - to allow feature configuration.
>>>>
>>>>Obtain the feature state value using the get callback and provide it to
>>>>the user if the device driver implements callbacks.
>>>>
>>>>Execute the set callback upon user requests.
>>>>
>>>>Reviewed-by: Milena Olech <[email protected]>
>>>>Signed-off-by: Arkadiusz Kubalewski <[email protected]>
>>>>---
>>>>v3:
>>>>- remove feature flags and capabilities,
>>>>- add separated (per feature) callback ops,
>>>>- use callback ops to determine feature availability.
>>>>---
>>>> drivers/dpll/dpll_netlink.c | 76 ++++++++++++++++++++++++++++++++++++-
>>>> include/linux/dpll.h | 8 ++++
>>>> 2 files changed, 82 insertions(+), 2 deletions(-)
>>>>
>>>>diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
>>>>index c130f87147fa..6d2980455a46 100644
>>>>--- a/drivers/dpll/dpll_netlink.c
>>>>+++ b/drivers/dpll/dpll_netlink.c
>>>>@@ -126,6 +126,26 @@ dpll_msg_add_mode_supported(struct sk_buff *msg,
>>>>struct dpll_device *dpll,
>>>> return 0;
>>>> }
>>>>
>>>>+static int
>>>>+dpll_msg_add_phase_offset_monitor(struct sk_buff *msg, struct
>>>>dpll_device
>>>>*dpll,
>>>>+ struct netlink_ext_ack *extack)
>>>>+{
>>>>+ const struct dpll_device_ops *ops = dpll_device_ops(dpll);
>>>>+ enum dpll_feature_state state;
>>>>+ int ret;
>>>>+
>>>>+ if (ops->phase_offset_monitor_set && ops->phase_offset_monitor_get) {
>>>>+ ret = ops->phase_offset_monitor_get(dpll, dpll_priv(dpll),
>>>>+ &state, extack);
>>>>+ if (ret)
>>>>+ return -EINVAL;
>>>
>>>Why you don't propagate "ret"?
>>>
>>
>>My bad, will fix that.
>>
>>>
>>>>+ if (nla_put_u32(msg, DPLL_A_PHASE_OFFSET_MONITOR, state))
>>>>+ return -EMSGSIZE;
>>>>+ }
>>>>+
>>>>+ return 0;
>>>>+}
>>>>+
>>>> static int
>>>> dpll_msg_add_lock_status(struct sk_buff *msg, struct dpll_device *dpll,
>>>> struct netlink_ext_ack *extack)
>>>>@@ -591,6 +611,9 @@ dpll_device_get_one(struct dpll_device *dpll, struct
>>>>sk_buff *msg,
>>>> return ret;
>>>> if (nla_put_u32(msg, DPLL_A_TYPE, dpll->type))
>>>> return -EMSGSIZE;
>>>>+ ret = dpll_msg_add_phase_offset_monitor(msg, dpll, extack);
>>>>+ if (ret)
>>>>+ return ret;
>>>>
>>>> return 0;
>>>> }
>>>>@@ -746,6 +769,31 @@ int dpll_pin_change_ntf(struct dpll_pin *pin)
>>>> }
>>>> EXPORT_SYMBOL_GPL(dpll_pin_change_ntf);
>>>>
>>>>+static int
>>>>+dpll_phase_offset_monitor_set(struct dpll_device *dpll, struct nlattr
>>>>*a,
>>>>+ struct netlink_ext_ack *extack)
>>>>+{
>>>>+ const struct dpll_device_ops *ops = dpll_device_ops(dpll);
>>>>+ enum dpll_feature_state state = nla_get_u32(a), old_state;
>>>>+ int ret;
>>>>+
>>>>+ if (!(ops->phase_offset_monitor_set && ops-
>>>>phase_offset_monitor_get)) {
>>>>+ NL_SET_ERR_MSG_ATTR(extack, a, "dpll device not capable of
>>>>phase offset monitor");
>>>>+ return -EOPNOTSUPP;
>>>>+ }
>>>>+ ret = ops->phase_offset_monitor_get(dpll, dpll_priv(dpll),
>>>>&old_state,
>>>>+ extack);
>>>>+ if (ret) {
>>>>+ NL_SET_ERR_MSG(extack, "unable to get current state of phase
>>>>offset monitor");
>>>>+ return -EINVAL;
>
>Propagate ret.
>
Sure, will do.
>
>>>>+ }
>>>>+ if (state == old_state)
>>>>+ return 0;
>>>>+
>>>>+ return ops->phase_offset_monitor_set(dpll, dpll_priv(dpll), state,
>>>>+ extack);
>>>
>>>Why you need to do this get/set dance? I mean, just call the driver
>>>set() op and let it do what is needed there, no?
>>>
>>
>>We did it this way from the beginning (during various pin-set related
>>flows).
>
>Hmm, idk if it is absolutelly necessary to stick with this pattern all
>the time. I mean, what's the benefit here? I don't see any.
>
Driver implementing callback could do that, or can be done here. Here is
earlier/better, right?
Why would we remove this pattern for one flow, and use different for
other flows? Doesn't make much sense to me, we could change for all to
make it consistent.
>
>>
>>>
>>>>+}
>>>>+
>>>> static int
>>>> dpll_pin_freq_set(struct dpll_pin *pin, struct nlattr *a,
>>>> struct netlink_ext_ack *extack)
>>>>@@ -1533,10 +1581,34 @@ int dpll_nl_device_get_doit(struct sk_buff *skb,
>>>>struct genl_info *info)
>>>> return genlmsg_reply(msg, info);
>>>> }
>>>>
>>>>+static int
>>>>+dpll_set_from_nlattr(struct dpll_device *dpll, struct genl_info *info)
>>>>+{
>>>>+ struct nlattr *a;
>>>>+ int rem, ret;
>>>>+
>>>>+ nla_for_each_attr(a, genlmsg_data(info->genlhdr),
>>>>+ genlmsg_len(info->genlhdr), rem) {
>>>
>>>Hmm, why you iterate? Why you just don't parse to attr array, as it is
>>>usually done?
>>>
>>
>>Hmm, AFAIR there are issues when you parse nested stuff with the array
>>approach, here nothing is nested, but we already have the same approach on
>>parsing pin related stuff (dpll_pin_set_from_nlattr(..)), just did the
>>same
>>here.
>
>The only reason to iterate over attrs is then you have multiattr. Is
>ever attr is there only once, no need for iteration.
>
Ok, will do.
Thank you!
Arkadiusz
[...]
