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

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] [thread-next>] [day] [month] [year] [list]

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

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.


>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;
>>> +}
>>> +