Thu, May 08, 2025 at 05:20:24PM +0200, arkadiusz.kubalew...@intel.com wrote:
>>From: Jiri Pirko <j...@resnulli.us>
>>Sent: Thursday, May 8, 2025 4:31 PM
>>
>>Thu, May 08, 2025 at 02:21:27PM +0200, arkadiusz.kubalew...@intel.com
>>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 <milena.ol...@intel.com>
>>>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalew...@intel.com>
>>>---
>>>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.
>>>+ }
>>>+ 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.
>
>>
>>>+}
>>>+
>>> 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.
>
>Thank you!
>Arkadiusz
>
>>
>>>+ switch (nla_type(a)) {
>>>+ case DPLL_A_PHASE_OFFSET_MONITOR:
>>>+ ret = dpll_phase_offset_monitor_set(dpll, a,
>>>+ info->extack);
>>>+ if (ret)
>>>+ return ret;
>>>+ break;
>>>+ default:
>>>+ break;
>>>+ }
>>>+ }
>>>+
>>>+ return 0;
>>>+}
>>>+
>>> int dpll_nl_device_set_doit(struct sk_buff *skb, struct genl_info *info)
>>> {
>>>- /* placeholder for set command */
>>>- return 0;
>>>+ struct dpll_device *dpll = info->user_ptr[0];
>>>+
>>>+ return dpll_set_from_nlattr(dpll, info);
>>> }
>>>
>>> int dpll_nl_device_get_dumpit(struct sk_buff *skb, struct
>>>netlink_callback *cb)
>>>diff --git a/include/linux/dpll.h b/include/linux/dpll.h
>>>index 5e4f9ab1cf75..6ad6c2968a28 100644
>>>--- a/include/linux/dpll.h
>>>+++ b/include/linux/dpll.h
>>>@@ -30,6 +30,14 @@ struct dpll_device_ops {
>>> void *dpll_priv,
>>> unsigned long *qls,
>>> struct netlink_ext_ack *extack);
>>>+ int (*phase_offset_monitor_set)(const struct dpll_device *dpll,
>>>+ void *dpll_priv,
>>>+ enum dpll_feature_state state,
>>>+ struct netlink_ext_ack *extack);
>>>+ int (*phase_offset_monitor_get)(const struct dpll_device *dpll,
>>>+ void *dpll_priv,
>>>+ enum dpll_feature_state *state,
>>>+ struct netlink_ext_ack *extack);
>>> };
>>>
>>> struct dpll_pin_ops {
>>>--
>>>2.38.1
>>>
>
