>From: Jiri Pirko <[email protected]>
>Sent: Tuesday, October 3, 2023 7:19 PM
>
>Tue, Oct 03, 2023 at 04:29:13PM CEST, [email protected] wrote:
>>>From: Jiri Pirko <[email protected]>
>>>Sent: Tuesday, October 3, 2023 8:27 AM
>>>To: Kubalewski, Arkadiusz <[email protected]>
>>>
>>>Tue, Oct 03, 2023 at 01:10:39AM CEST, [email protected]
>>>wrote:
>>>>>From: Intel-wired-lan <[email protected]> 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, [email protected]
>>>>>> wrote:
>>>>>>>> From: Vadim Fedorenko <[email protected]>
>>>>>>>> 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 <[email protected]>
>>>>>>>>
>>>>>>>> [...]
>>>>>>>>
>>>>>>>>> +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.
>
Yes, that makes sense.
>
>>-> for each device: call phase_adjust_set, if fails, rollback all previous
>> successful attempts and return the failure code
>
>That would work.
>
Great, just sent v2.
Thanks!
Arkadiusz
>
>>?
>>
>>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
>>>>>[email protected]
>>>>>https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
>>
_______________________________________________
Intel-wired-lan mailing list
[email protected]
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
