| 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: <SJ2PR11MB84521AE1C30176E2A31C4F709BBC2@SJ2PR11MB8452.namprd11.prod.outlook.com>
Date: Thu, 17 Apr 2025 09:23:09 +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>, "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
>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.
>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?
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