RE: [PATCH net-next v2 3/4] dpll: features_get/set callbacks

[Kubalewski, Arkadiusz]

>From: Jiri Pirko <jiri@xxxxxxxxxxx>
>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

[...]