Re: [PATCH net v4 4/5] dpll: zl3073x: reject inexact frequencies for N-divided outputs

[Ivan Vecera <ivecera@redhat.com>]

On 10/10/26 21:27, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 0 (1)
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] After this patch, zl3073x_dpll_output_pin_frequency_set()
>    rejects an N-pin frequency that the driver itself advertises as…
> 
> Pre-existing issues:
> - [Low] The three NL_SET_ERR_MSG_FMT() diagnostics this patch adds to
>    zl3073x_dpll_output_pin_frequency_set() never reach userspace.
> 
> --
> 
> dpll: zl3073x: reject inexact frequencies for N-divided outputs
> 
> This patch changes zl3073x_dpll_output_pin_frequency_set() for N-divided
> outputs. The esync_n_period computation now uses a 64-bit product, and the
> request is rejected when the resulting period is below 2, does not fit
> into 32 bits, or the division leaves a remainder.
> 
>> diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
>> index 8bac680394d76..6d9a6d21d30b7 100644
>> --- a/drivers/dpll/zl3073x/dpll.c
>> +++ b/drivers/dpll/zl3073x/dpll.c
> 
> [ ... ]
> 
>> @@ -1033,17 +1029,31 @@ zl3073x_dpll_output_pin_frequency_set(const struct dpll_pin *dpll_pin,
>>   		/* For 50/50 duty cycle the divisor is equal to width */
>>   		out.width = out.div;
>>   	} else {
>> -		/* We are going to change frequency of N-pin but if
>> -		 * the requested freq is greater or equal than freq of P-pin
>> -		 * in the output pair we cannot compute divisor for the N-pin.
>> -		 * In this case indicate a failure.
>> -		 *
>> -		 * Update divisor for N-pin
>> +		/* The N-pin frequency has to divide the P-pin frequency and
>> +		 * be at most half of it.
>>   		 */
>> -		out.esync_n_period = div64_u64(synth_freq, frequency * out.div);
>> -		if (!out.esync_n_period)
>> -			return -EINVAL;
>> +		n_period = frequency * out.div;
>> +		n_period = div64_u64_rem(synth_freq, n_period, &rem);
>> +	}
>> +	if (n_period < 2) {
>> +		NL_SET_ERR_MSG_FMT(extack,
>> +				   "OUT%uN freq must be at most half of OUT%uP freq",
>> +				   out_id, out_id);
> 
> [Severity: Low]
> This isn't a bug introduced by this patch, but will userspace ever see
> these three new extack messages?

No, dpll_pin_freq_set() overwrites them. That is a DPLL core issue and
out of scope for this series.

> The only caller, dpll_pin_freq_set() in drivers/dpll/dpll_netlink.c,
> always writes its own message when the driver callback fails:
> 
> drivers/dpll/dpll_netlink.c:dpll_pin_freq_set() {
>      ...
> 	ret = ops->frequency_set(pin, dpll_pin_on_dpll_priv(dpll, pin),
> 				 dpll, dpll_priv(dpll), freq, extack);
> 	if (ret) {
> 		NL_SET_ERR_MSG_FMT(extack,
> 				   "frequency set failed for dpll_id:%u",
> 				   dpll->id);
> 		return ret;
> 	}
>      ...
> }
> 
> NL_SET_ERR_MSG_FMT() unconditionally replaces extack->_msg, so the
> "OUT%uN freq must ..." strings from zl3073x get replaced by the generic
> core message. ice_dpll_pin_freq_set() loses its messages the same way.
> 
> Would it make sense to switch the core to NL_SET_ERR_MSG_WEAK(), or to
> keep a message the driver has already set?

Yes, it make sense. This will be handled separately (not in this series).

 >> +		return -EINVAL;>> +	}
>> +	if (n_period > U32_MAX) {
>> +		NL_SET_ERR_MSG_FMT(extack,
>> +				   "OUT%uN freq is too low for OUT%uP freq",
>> +				   out_id, out_id);
>> +		return -EINVAL;
>> +	}
>> +	if (rem != 0) {
>> +		NL_SET_ERR_MSG_FMT(extack,
>> +				   "OUT%uN freq must divide OUT%uP freq",
>> +				   out_id, out_id);
>> +		return -EINVAL;
> 
> [Severity: Medium]
> Can this reject an N-pin frequency that the driver itself lists as
> supported?
> 
> The hardware N-pin frequency is synth_freq / (div * esync_n_period), and
> that does not have to be a whole number of Hz. The getter rounds it down:
> 
> drivers/dpll/zl3073x/core.h:zl3073x_dev_output_pin_freq_get() {
>      ...
> 	freq = zl3073x_synth_freq_get(synth) / out->div;
> 
> 	if (zl3073x_out_is_ndiv(out) && zl3073x_is_n_pin(id))
> 		freq /= out->esync_n_period;
>      ...
> }
> 
> zl3073x_pin_props_get() then adds this rounded value to freq_supported.
> Unlike the firmware-listed frequencies, it does not go through
> zl3073x_pin_check_freq():
> 
> 	if (curr_freq) {
> 		struct dpll_pin_frequency freq = DPLL_PIN_FREQUENCY(curr_freq);
> 
> 		ranges[j++] = freq;
> 	}
> 
> For example, take a 600 MHz synth with div=60 and esync_n_period=7. The
> real N-pin frequency is 1428571.43 Hz, and it is advertised as 1428571 Hz.
> 
> Say userspace moves the N-pin to another supported value, such as a
> firmware-listed 1 MHz, and then tries to set it back to 1428571 Hz.
> dpll_pin_is_freq_supported() accepts the value, and then this function
> computes:
> 
>      n_period = 1428571 * 60 = 85714260
>      div64_u64_rem(600000000, 85714260, &rem) -> 7, rem = 180
> 
> The new rem != 0 check returns -EINVAL here. The old truncating division
> gave n_period = 7, which restored the original hardware setting.
> 
> The comment in zl3073x_pin_props_get() says a frequency that "cannot be
> set ... is not offered", which suggests the current frequency added there
> is expected to be settable. Should the advertised current frequency be
> skipped or checked for inexact N-divided outputs, or should the setter
> accept the truncated value?

Neither. A non-integer N-pin frequency can only come from the flash
configuration, and the DPLL API works in whole Hz, so such a frequency
cannot be requested exactly. The old truncating division restored it
only by accident. Accepting truncated values again would bring back the
silent rounding this patch removes.

Thanks,
Ivan