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. >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