| 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: <y5d6hkhdgwprncbnfmznuk5otluqz3vqi6fof6cyr4dc673cqx@5kr6ys6g53ev>
Date: Thu, 17 Apr 2025 11:59:20 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>
Cc: "donald.hunter@...il.com" <donald.hunter@...il.com>,
"kuba@...nel.org" <kuba@...nel.org>, "davem@...emloft.net" <davem@...emloft.net>,
"Dumazet, Eric" <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 v2 3/4] dpll: features_get/set callbacks
Thu, Apr 17, 2025 at 11:23:09AM +0200, arkadiusz.kubalewski@...el.com wrote:
>>From: Jiri Pirko <jiri@...nulli.us>
>>Sent: Wednesday, April 16, 2025 2:11 PM
>>
>>Tue, Apr 15, 2025 at 08:15:42PM +0200, arkadiusz.kubalewski@...el.com
>>wrote:
>>>Add new callback ops for a dpll device.
>>>- features_get(..) - to obtain currently configured features from dpll
>>> device,
>>>- feature_set(..) - to allow dpll device features configuration.
>>>Provide features attribute and allow control over it to the users if
>>>device driver implements callbacks.
>>>
>>>Reviewed-by: Milena Olech <milena.olech@...el.com>
>>>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>
>>>---
>>>v2:
>>>- adapt changes and align wiht new dpll_device_info struct
>>>---
>>> drivers/dpll/dpll_netlink.c | 79 ++++++++++++++++++++++++++++++++++++-
>>> include/linux/dpll.h | 5 +++
>>> 2 files changed, 82 insertions(+), 2 deletions(-)
>>>
>>>diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
>>>index 2de9ec08d551..acfdd87fcffe 100644
>>>--- a/drivers/dpll/dpll_netlink.c
>>>+++ b/drivers/dpll/dpll_netlink.c
>>>@@ -126,6 +126,25 @@ dpll_msg_add_mode_supported(struct sk_buff *msg,
>>>struct dpll_device *dpll,
>>> return 0;
>>> }
>>>
>>>+static int
>>>+dpll_msg_add_features(struct sk_buff *msg, struct dpll_device *dpll,
>>>+ struct netlink_ext_ack *extack)
>>>+{
>>>+ const struct dpll_device_ops *ops = dpll_device_ops(dpll);
>>>+ u32 features;
>>>+ int ret;
>>>+
>>>+ if (!ops->features_get)
>>>+ return 0;
>>>+ ret = ops->features_get(dpll, dpll_priv(dpll), &features, extack);
>>>+ if (ret)
>>>+ return ret;
>>>+ if (nla_put_u32(msg, DPLL_A_FEATURES, features))
>>>+ return -EMSGSIZE;
>>>+
>>>+ return 0;
>>>+}
>>>+
>>> static int
>>> dpll_msg_add_lock_status(struct sk_buff *msg, struct dpll_device *dpll,
>>> struct netlink_ext_ack *extack)
>>>@@ -592,6 +611,11 @@ dpll_device_get_one(struct dpll_device *dpll, struct
>>>sk_buff *msg,
>>> return ret;
>>> if (nla_put_u32(msg, DPLL_A_TYPE, info->type))
>>> return -EMSGSIZE;
>>>+ if (nla_put_u32(msg, DPLL_A_CAPABILITIES, info->capabilities))
>>>+ return -EMSGSIZE;
>>>+ ret = dpll_msg_add_features(msg, dpll, extack);
>>>+ if (ret)
>>>+ return ret;
>>>
>>> return 0;
>>> }
>>>@@ -747,6 +771,34 @@ int dpll_pin_change_ntf(struct dpll_pin *pin)
>>> }
>>> EXPORT_SYMBOL_GPL(dpll_pin_change_ntf);
>>>
>>>+static int
>>>+dpll_features_set(struct dpll_device *dpll, struct nlattr *a,
>>>+ struct netlink_ext_ack *extack)
>>>+{
>>>+ const struct dpll_device_info *info = dpll_device_info(dpll);
>>>+ const struct dpll_device_ops *ops = dpll_device_ops(dpll);
>>>+ u32 features = nla_get_u32(a), old_features;
>>>+ int ret;
>>>+
>>>+ if (features && !(info->capabilities & features)) {
>>>+ NL_SET_ERR_MSG_ATTR(extack, a, "dpll device not capable of this
>>>features");
>>>+ return -EOPNOTSUPP;
>>>+ }
>>>+ if (!ops->features_get || !ops->features_set) {
>>>+ NL_SET_ERR_MSG(extack, "dpll device not supporting any
>>>features");
>>>+ return -EOPNOTSUPP;
>>>+ }
>>>+ ret = ops->features_get(dpll, dpll_priv(dpll), &old_features,
>>>extack);
>>>+ if (ret) {
>>>+ NL_SET_ERR_MSG(extack, "unable to get old features value");
>>>+ return ret;
>>>+ }
>>>+ if (old_features == features)
>>>+ return -EINVAL;
>>>+
>>>+ return ops->features_set(dpll, dpll_priv(dpll), features, extack);
>>
>>So you allow to enable/disable them all in once. What if user want to
>>enable feature A and does not care about feature B that may of may not
>>be previously set?
>
>Assumed that user would always request full set.
Ugh.
>
>>How many of the features do you expect to appear in the future. I'm
>>asking because this could be just a bool attr with a separate op to the
>>driver. If we have 3, no problem. Benefit is, you may also extend it
>>easily to pass some non-bool configuration. My point is, what is the
>>benefit of features bitset here?
>>
>
>Not much, at least right now..
>Maybe one similar in nearest feature. Sure, we can go that way.
>
>But you mean uAPI part also have this enabled somehow per feature or
>leave the feature bits there?
I don't see reason for u32/bitfield32 bits here. Just have attr per
feature to enable/disable it, no problem. It will be easier to work with.
>
>Thank you!
>Arkadiusz
>
>>
>>
>>>+}
>>>+
>>> static int
>>> dpll_pin_freq_set(struct dpll_pin *pin, struct nlattr *a,
>>> struct netlink_ext_ack *extack)
>>>@@ -1536,10 +1588,33 @@ 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) {
>>>+ switch (nla_type(a)) {
>>>+ case DPLL_A_FEATURES:
>>>+ ret = dpll_features_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 0489464af958..90c743daf64b 100644
>>>--- a/include/linux/dpll.h
>>>+++ b/include/linux/dpll.h
>>>@@ -30,6 +30,10 @@ struct dpll_device_ops {
>>> void *dpll_priv,
>>> unsigned long *qls,
>>> struct netlink_ext_ack *extack);
>>>+ int (*features_set)(const struct dpll_device *dpll, void *dpll_priv,
>>>+ u32 features, struct netlink_ext_ack *extack);
>>>+ int (*features_get)(const struct dpll_device *dpll, void *dpll_priv,
>>>+ u32 *features, struct netlink_ext_ack *extack);
>>> };
>>>
>>> struct dpll_pin_ops {
>>>@@ -99,6 +103,7 @@ struct dpll_pin_ops {
>>>
>>> struct dpll_device_info {
>>> enum dpll_type type;
>>>+ u32 capabilities;
>>> const struct dpll_device_ops *ops;
>>> };
>>>
>>>--
>>>2.38.1
>>>
Powered by blists - more mailing lists