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

[Ivan Vecera <ivecera@redhat.com>]

On 10/12/26 08:13, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
> 
> Low priority / nit picks for your consideration:
> 
> - [Low] Concern: zl3073x_pin_props_get() always lists the current
>    (truncated) frequency as supported.
> 
> Pre-existing issues:
> - [Low] Concern: userspace never sees the three new extack messages added
>    in `zl3073x_dpll_output_pin_frequency_set()` ("must be at most half",…
> 
> --
> 
> dpll: zl3073x: reject inexact frequencies for N-divided outputs
> 
> This commit tightens zl3073x_dpll_output_pin_frequency_set() for
> N-divided outputs. It computes esync_n_period in 64 bits and requires it
> to be at least 2 and to fit into 32 bits. It also rejects any request
> whose division leaves a remainder, and reports each failure case through
> extack.
> 
>> diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
>> index f2e137475b40f..7a617d9bd6712 100644
>> --- a/drivers/dpll/zl3073x/dpll.c
>> +++ b/drivers/dpll/zl3073x/dpll.c
> 
> [ ... ]
> 
>> @@ -1031,17 +1027,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 {
> 
> [ ... ]
> 
>> +		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);
>> +		return -EINVAL;
>> +	}
> 
> [Severity: Low]
> This is a pre-existing issue and was not introduced by this patch, but
> will userspace ever see this message, or the two added below it?

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

> 
> When ops->frequency_set() fails, the DPLL core overwrites the driver's
> extack text:
> 
> 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() always writes into extack->_msg_buf and resets
> _msg. The "must be at most half", "is too low" and "must divide" messages
> are therefore replaced by the generic text, and they can only be seen
> through the netlink_extack tracepoint. The -EINVAL return is still
> correct.
> 
>> +	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: Low]
> Can this check reject a frequency that zl3073x_pin_props_get() lists as
> supported?
> 
> zl3073x_pin_props_get() always puts the current frequency into the
> supported list. For an N-pin, that value comes from a truncating
> division:
> 
> drivers/dpll/zl3073x/core.h:zl3073x_dev_output_pin_freq_get() {
>      ...
> 	if (zl3073x_out_is_ndiv(out) && zl3073x_is_n_pin(id))
> 		freq /= out->esync_n_period;
>      ...
> }
> 
> Also, zl3073x_out_state_fetch() only refuses esync_n_period == 0, so a
> boot config with esync_n_period == 1 is accepted.
> 
> A request for the current value never reaches the driver, because
> dpll_pin_freq_set() returns early when freq == old_freq. If the pin is
> first moved to another frequency, though, a later request for the
> advertised boot value can fail.
> 
> Take a 600 MHz synth, div=60 and a boot esync_n_period of 7. The N-pin
> is advertised as 1428571 Hz. After N is moved to 1 MHz, a request for
> 1428571 gives rem=180 and returns -EINVAL.
> 
> In the same way, a P-pin frequency that would bring back
> esync_n_period == 1 now fails the n_period < 2 check. According to the
> series commit "dpll: zl3073x: reject output frequencies with too small
> divisor", that state gave no N-pin output.
> 
> This looks consistent with the goal of rejecting inexact N frequencies.
> A later comment in prop.c in this series also says the supported list
> for N-divided outputs is only a necessary condition. The old code
> already rejected some advertised values, such as N-pin frequencies at or
> above the P-pin frequency.
> 
> Is it intended that a user cannot go back to an inexact boot-time N-pin
> setting by asking for its advertised value?

Yes. Such a value can only come from the flash configuration and cannot
be requested exactly in whole Hz. Accepting truncated values would
bring back the silent rounding this patch removes.

Thanks,
Ivan