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

[Jiri Pirko <jiri@resnulli.us>]

Tue, Oct 03, 2023 at 01:10:39AM CEST, arkadiusz.kubalewski@intel.com wrote:
>>From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.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@intel.com
>>> wrote:
>>>>> From: Vadim Fedorenko <vadim.fedorenko@linux.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@intel.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.


>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@osuosl.org
>>https://lists.osuosl.org/mailman/listinfo/intel-wired-lan