| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <SA1PR11MB84468A82953226F3C16D9CCB9B98A@SA1PR11MB8446.namprd11.prod.outlook.com>
Date: Fri, 23 May 2025 07:45:29 +0000
From: "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>
To: Jiri Pirko <jiri@...nulli.us>
CC: "donald.hunter@...il.com" <donald.hunter@...il.com>, "kuba@...nel.org"
<kuba@...nel.org>, "davem@...emloft.net" <davem@...emloft.net>,
"edumazet@...gle.com" <edumazet@...gle.com>, "pabeni@...hat.com"
<pabeni@...hat.com>, "horms@...nel.org" <horms@...nel.org>,
"vadim.fedorenko@...ux.dev" <vadim.fedorenko@...ux.dev>, "Nguyen, Anthony L"
<anthony.l.nguyen@...el.com>, "Kitszel, Przemyslaw"
<przemyslaw.kitszel@...el.com>, "andrew+netdev@...n.ch"
<andrew+netdev@...n.ch>, "saeedm@...dia.com" <saeedm@...dia.com>,
"leon@...nel.org" <leon@...nel.org>, "tariqt@...dia.com" <tariqt@...dia.com>,
"jonathan.lemon@...il.com" <jonathan.lemon@...il.com>,
"richardcochran@...il.com" <richardcochran@...il.com>, "Loktionov, Aleksandr"
<aleksandr.loktionov@...el.com>, "Olech, Milena" <milena.olech@...el.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
"linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>
Subject: RE: [PATCH net-next v3 2/3] dpll: add phase_offset_monitor_get/set
callback ops
>From: Jiri Pirko <jiri@...nulli.us>
>Sent: Friday, May 9, 2025 8:15 AM
>
>Thu, May 08, 2025 at 05:20:24PM +0200, arkadiusz.kubalewski@...el.com
>wrote:
>>>From: Jiri Pirko <jiri@...nulli.us>
>>>Sent: Thursday, May 8, 2025 4:31 PM
>>>
>>>Thu, May 08, 2025 at 02:21:27PM +0200, arkadiusz.kubalewski@...el.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.olech@...el.com>
>>>>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.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.
>
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
[...]
Powered by blists - more mailing lists