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

Date: Tue, 3 Oct 2023 19:18:55 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>
Cc: Vadim Fedorenko <vadim.fedorenko@...ux.dev>,
	"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

Tue, Oct 03, 2023 at 04:29:13PM CEST, arkadiusz.kubalewski@...el.com wrote:
>>From: Jiri Pirko <jiri@...nulli.us>
>>Sent: Tuesday, October 3, 2023 8:27 AM
>>To: Kubalewski, Arkadiusz <arkadiusz.kubalewski@...el.com>
>>
>>Tue, Oct 03, 2023 at 01:10:39AM CEST, arkadiusz.kubalewski@...el.com wrote:
>>>>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
>>
>>In this case, it is common to return the first error hit and bail out,
>>not trying the rest.
>>
>
>OK, so now I see it like this:
>-> check if all device implement callback, if not return EOPNOTSUPP;
>-> get old phase_adjust
>-> if new == old, return EINVAL

0 would be better, no? User has what he desired.

>-> for each device: call phase_adjust_set, if fails, rollback all previous
>   successful attempts and return the failure code

That would work.

>?
>
>Thank you!
>Arkadiusz
>
>>
>>>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