Message-ID: <DM6PR11MB4657B52BD09700F49799ED8C9BC4A@DM6PR11MB4657.namprd11.prod.outlook.com>
Date: Tue, 3 Oct 2023 14:29:43 +0000
From: "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>
To: Jiri Pirko <jiri@...nulli.us>
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

>From: Jiri Pirko <jiri@...nulli.us>
>Sent: Tuesday, October 3, 2023 8:32 AM
>
>Tue, Oct 03, 2023 at 01:03:00AM CEST, arkadiusz.kubalewski@...el.com wrote:
>>>From: Jiri Pirko <jiri@...nulli.us>
>>>Sent: Monday, October 2, 2023 5:04 PM
>>>
>>>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.
>>>
>>
>>Let's step back a bit.
>>The op itself is introduced as per pin-dpll tuple.. did this
>>intentionally,
>>to inform each dpll that the offset has been changed - in case dplls are
>>controlled by separated driver/firmware instances but still sharing the
>>pin.
>>Same way a pin frequency is being set, from user perspective on a pin, but
>>callback is called for each dpll the pin was registered with.
>>Whatever we do here, it shall be probably done for frequency_set()
>>callback as
>>well.
>>
>>The answers:
>>So far I don't know the device that might do it this way, it rather
>>supports
>>phase_adjust or not. In theory we allow such behavior to be implemented,
>>i.e.
>>pin is registered with 2 dplls, one has the callback, second not.
>
>If there is only theoretical device like that now, implement
>all-or-nothing. If such theoretical device appears in real, this could
>be changed. The UAPI would not change, no problem.
>

I can live with it :)

>
>>Current hardware of ice sets phase offset for a pin no matter on which
>>dpll
>>device callback was invoked.
>>"all-or-nothing" - do you mean to check all callback returns and then
>>decide
>>if it was successful?
>
>Check if all dplls have ops and only perform the action in such case. In
>case one of the dplls does not have the op filled, return -EOPNOTSUPP.
>
>
>Regarding the successful/failed op, I think you can just return. In
>these cases, when user performs multiaction cmd, he should be prepared
>to deal with consequences if part of this cmd fails. We don't have
>rollback for any other multiaction cmd in dpll, I don't see why this
>should be treated differently.
>

We don't have it because no one have spotted it on review,
as mentioned the frequency_set behaves the same way,
we need one approach for all of those cases.
I am opting for having the rollback as suggested on the other thread.

Thank you!
Arkadiusz

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

Powered by blists - more mailing lists