lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 3 Oct 2023 19:19:44 +0200
From: Jiri Pirko <>
To: "Kubalewski, Arkadiusz" <>
Cc: Vadim Fedorenko <>,
	"" <>,
	"" <>,
	"" <>,
	"" <>,
	"" <>,
	"Brandeburg, Jesse" <>,
	"Nguyen, Anthony L" <>,
	"" <>,
	"" <>
Subject: Re: [PATCH net-next 3/4] dpll: netlink/core: add support for
 pin-dpll signal phase offset/adjust

Tue, Oct 03, 2023 at 04:29:43PM CEST, wrote:
>>From: Jiri Pirko <>
>>Sent: Tuesday, October 3, 2023 8:32 AM
>>Tue, Oct 03, 2023 at 01:03:00AM CEST, wrote:
>>>>From: Jiri Pirko <>
>>>>Sent: Monday, October 2, 2023 5:04 PM
>>>>Mon, Oct 02, 2023 at 04:32:30PM CEST,
>>>>>>From: Vadim Fedorenko <>
>>>>>>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 <>
>>>>>>> +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
>>>to inform each dpll that the offset has been changed - in case dplls are
>>>controlled by separated driver/firmware instances but still sharing the
>>>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
>>>The answers:
>>>So far I don't know the device that might do it this way, it rather
>>>phase_adjust or not. In theory we allow such behavior to be implemented,
>>>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
>>>device callback was invoked.
>>>"all-or-nothing" - do you mean to check all callback returns and then
>>>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.

>Thank you!
>>>Thank you!
>>>>>If device fails to set -> add extack error, continue
>>>>>Function always returns 0.
>>>>>Thank you!
>>>>>>> +		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;
>>>>>>> +}
>>>>>>> +

Powered by blists - more mailing lists