netdev - RE: [Intel-wired-lan] [PATCH net-next 3/4] dpll: netlink/core: add support for pin-dpll signal phase offset/adjust

Open Source and information security mailing list archives

Message-ID: <DM6PR11MB4657DB3C9BC3E1EFE6A2F3389BC5A@DM6PR11MB4657.namprd11.prod.outlook.com>
Date: Mon, 2 Oct 2023 23:10:39 +0000
From: "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>
To: Vadim Fedorenko <vadim.fedorenko@...ux.dev>, Jiri Pirko <jiri@...nulli.us>
CC: "corbet@....net" <corbet@....net>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "linux-doc@...r.kernel.org"
	<linux-doc@...r.kernel.org>, "Brandeburg, Jesse"
	<jesse.brandeburg@...el.com>, "Nguyen, Anthony L"
	<anthony.l.nguyen@...el.com>, "intel-wired-lan@...ts.osuosl.org"
	<intel-wired-lan@...ts.osuosl.org>, "kuba@...nel.org" <kuba@...nel.org>,
	"pabeni@...hat.com" <pabeni@...hat.com>, "davem@...emloft.net"
	<davem@...emloft.net>
Subject: RE: [Intel-wired-lan] [PATCH net-next 3/4] dpll: netlink/core: add
 support for pin-dpll signal phase offset/adjust

>From: Intel-wired-lan <intel-wired-lan-bounces@...osl.org> On Behalf Of
>Vadim Fedorenko
>Sent: Monday, October 2, 2023 5:09 PM
>
>On 02/10/2023 16:04, Jiri Pirko wrote:
>> Mon, Oct 02, 2023 at 04:32:30PM CEST, arkadiusz.kubalewski@...el.com
>> wrote:
>>>> From: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
>>>> Sent: Wednesday, September 27, 2023 8:09 PM
>>>>
>>>> On 27/09/2023 10:24, Arkadiusz Kubalewski wrote:
>>>>> Add callback op (get) for pin-dpll phase-offset measurment.
>>>>> Add callback ops (get/set) for pin signal phase adjustment.
>>>>> Add min and max phase adjustment values to pin proprties.
>>>>> Invoke get callbacks when filling up the pin details to provide user
>>>>> with phase related attribute values.
>>>>> Invoke phase-adjust set callback when phase-adjust value is provided
>>>>> for
>>>>> pin-set request.
>>>>>
>>>>> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>
>>>>
>>>> [...]
>>>>
>>>>> +static int
>>>>> +dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr
>>>>> *phase_adj_attr,
>>>>> +		       struct netlink_ext_ack *extack)
>>>>> +{
>>>>> +	struct dpll_pin_ref *ref;
>>>>> +	unsigned long i;
>>>>> +	s32 phase_adj;
>>>>> +	int ret;
>>>>> +
>>>>> +	phase_adj = nla_get_s32(phase_adj_attr);
>>>>> +	if (phase_adj > pin->prop->phase_range.max ||
>>>>> +	    phase_adj < pin->prop->phase_range.min) {
>>>>> +		NL_SET_ERR_MSG(extack, "phase adjust value not supported");
>>>>> +		return -EINVAL;
>>>>> +	}
>>>>> +	xa_for_each(&pin->dpll_refs, i, ref) {
>>>>> +		const struct dpll_pin_ops *ops = dpll_pin_ops(ref);
>>>>> +		struct dpll_device *dpll = ref->dpll;
>>>>> +
>>>>> +		if (!ops->phase_adjust_set)
>>>>> +			return -EOPNOTSUPP;
>>>>
>>>> I'm thinking about this part. We can potentially have dpll devices with
>>>> different expectations on phase adjustments, right? And if one of them
>>>> won't be able to adjust phase (or will fail in the next line), then
>>>> netlink will return EOPNOTSUPP while _some_ of the devices will be
>>>> adjusted. Doesn't look great. Can we think about different way to apply
>>>> the change?
>>>>
>>>
>>> Well makes sense to me.
>>>
>>> Does following makes sense as a fix?
>>> We would call op for all devices which has been provided with the op.
>>> If device has no op -> add extack error, continue
>>
>> Is it real to expect some of the device support this and others don't?
>> Is it true for ice?
>> If not, I would got for all-or-nothing here.
>>
>
>But nothing blocks vendors to provide such configuration. Should we
>rollback the configuration? Otherwise we can easily make it
>inconsistent.

Good point, in such case rollback might be required.

>
>I'm more thinking of checking if all the devices returned error (or
>absence of operation callback) and then return error instead of 0 with
>extack filled in.
>

Well, what if different devices would return different errors?
In general we would have to keep track of the error values returned in
such case.. Assuming one is different than the other - still need to error
extack them out? I guess it would be easier to return common error if there
were only failures and let the driver fill the errors on extack, smt like:

	int miss_cb_num = 0, dev_num = 0, err_num;

	xa_for_each(&pin->dpll_refs, i, ref) {
		const struct dpll_pin_ops *ops = dpll_pin_ops(ref);
		struct dpll_device *dpll = ref->dpll;

		dev_num++;
		if (!ops->phase_adjust_set) {
			miss_cb_num++;
			continue;
		}
		ret = ops->phase_adjust_set(pin,
					dpll_pin_on_dpll_priv(dpll, pin),
					dpll, dpll_priv(dpll), phase_adj,
					extack);
		if (ret)
			err_num++;
	}
	if (dev_num == miss_cb_num)
		return -EOPNOTSUPP;
	if (dev_num == err_num)
		return -EINVAL;
	__dpll_pin_change_ntf(pin);
	return 0;

??

Thank you!
Arkadiusz

>>
>>> If device fails to set -> add extack error, continue
>>> Function always returns 0.
>>>
>>> Thank you!
>>> Arkadiusz
>>>
>>>>
>>>>> +		ret = ops->phase_adjust_set(pin,
>>>>> +					    dpll_pin_on_dpll_priv(dpll, pin),
>>>>> +					    dpll, dpll_priv(dpll), phase_adj,
>>>>> +					    extack);
>>>>> +		if (ret)
>>>>> +			return ret;
>>>>> +	}
>>>>> +	__dpll_pin_change_ntf(pin);
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>
>_______________________________________________
>Intel-wired-lan mailing list
>Intel-wired-lan@...osl.org
>https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

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