linux-kernel - RE: [PATCH net-next v2 3/4] dpll: features

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <SJ2PR11MB8452FFB39384B1671D5D5A809B8BA@SJ2PR11MB8452.namprd11.prod.outlook.com>
Date: Thu, 8 May 2025 12:30:50 +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: Thursday, April 17, 2025 11:59 AM

[...]

>>>>+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.
>
>

OK. Fixed in v3.

Thank you!
Arkadiusz

[...]