Fri, May 23, 2025 at 09:45:29AM +0200, [email protected] wrote:
>>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.
Fair.
>
>>
>>>
>>>>
>>>>>+}
>>>>>+
>>>>> 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
>
>[...]
