Tue, Oct 03, 2023 at 04:29:13PM CEST, arkadiusz.kubalewski@xxxxxxxxx wrote: >>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 0 would be better, no? User has what he desired. >-> for each device: call phase_adjust_set, if fails, rollback all previous > successful attempts and return the failure code That would work. >? > >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 >