>From: Jiri Pirko <jiri@xxxxxxxxxxx>
>Sent: Tuesday, October 3, 2023 8:27 AM
>To: Kubalewski, Arkadiusz <arkadiusz.kubalewski@xxxxxxxxx>
>
>Tue, Oct 03, 2023 at 01:10:39AM CEST, arkadiusz.kubalewski@xxxxxxxxx wrote:
>>>From: Intel-wired-lan <intel-wired-lan-bounces@xxxxxxxxxx> 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@xxxxxxxxx
>>>> wrote:
>>>>>> From: Vadim Fedorenko <vadim.fedorenko@xxxxxxxxx>
>>>>>> 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@xxxxxxxxx>
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>> +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
-> for each device: call phase_adjust_set, if fails, rollback all previous
successful attempts and return the failure code
?
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@xxxxxxxxxx
>>>https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
