>From: Jiri Pirko <jiri@xxxxxxxxxxx> >Sent: Tuesday, October 3, 2023 8:32 AM > >Tue, Oct 03, 2023 at 01:03:00AM CEST, arkadiusz.kubalewski@xxxxxxxxx wrote: >>>From: Jiri Pirko <jiri@xxxxxxxxxxx> >>>Sent: Monday, October 2, 2023 5:04 PM >>> >>>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. >>> >> >>Let's step back a bit. >>The op itself is introduced as per pin-dpll tuple.. did this >>intentionally, >>to inform each dpll that the offset has been changed - in case dplls are >>controlled by separated driver/firmware instances but still sharing the >>pin. >>Same way a pin frequency is being set, from user perspective on a pin, but >>callback is called for each dpll the pin was registered with. >>Whatever we do here, it shall be probably done for frequency_set() >>callback as >>well. >> >>The answers: >>So far I don't know the device that might do it this way, it rather >>supports >>phase_adjust or not. In theory we allow such behavior to be implemented, >>i.e. >>pin is registered with 2 dplls, one has the callback, second not. > >If there is only theoretical device like that now, implement >all-or-nothing. If such theoretical device appears in real, this could >be changed. The UAPI would not change, no problem. > I can live with it :) > >>Current hardware of ice sets phase offset for a pin no matter on which >>dpll >>device callback was invoked. >>"all-or-nothing" - do you mean to check all callback returns and then >>decide >>if it was successful? > >Check if all dplls have ops and only perform the action in such case. In >case one of the dplls does not have the op filled, return -EOPNOTSUPP. > > >Regarding the successful/failed op, I think you can just return. In >these cases, when user performs multiaction cmd, he should be prepared >to deal with consequences if part of this cmd fails. We don't have >rollback for any other multiaction cmd in dpll, I don't see why this >should be treated differently. > We don't have it because no one have spotted it on review, as mentioned the frequency_set behaves the same way, we need one approach for all of those cases. I am opting for having the rollback as suggested on the other thread. Thank you! Arkadiusz > >> >>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; >>>>>> +} >>>>>> + >>