>From: Jiri Pirko <jiri@xxxxxxxxxxx>
>Sent: Tuesday, October 3, 2023 7:20 PM
>To: Kubalewski, Arkadiusz <arkadiusz.kubalewski@xxxxxxxxx>
>
>Tue, Oct 03, 2023 at 04:29:43PM CEST, arkadiusz.kubalewski@xxxxxxxxx wrote:
>>>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.
>
>Okay, but let's do that consistently.
>
Sure, fixed in v2.
Thanks!
Arkadiusz
>>
>>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;
>>>>>>>> +}
>>>>>>>> +
>>>>
