Re: [PATCH net v4 2/5] dpll: zl3073x: reject output frequencies with too small divisor

[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 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 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] Kept only because requires_validation=true.
> - [Low] The commit message does not say that the new `!freq` check fixes
>    a division by zero that firmware can trigger at probe time.
> 
> --
> 
> dpll: zl3073x: reject output frequencies with too small divisor
> 
> This patch makes zl3073x_pin_check_freq() reject output frequencies from
> firmware when the resulting divisor is below 2. For the N-pin of an
> N-divided output the minimum is 4. The patch also rejects 0 Hz for both
> input and output pins.
> 
>> diff --git a/drivers/dpll/zl3073x/prop.c b/drivers/dpll/zl3073x/prop.c
>> index 18a1bf3103326..4e006ed5950ad 100644
>> --- a/drivers/dpll/zl3073x/prop.c
>> +++ b/drivers/dpll/zl3073x/prop.c
> 
> [ ... ]
> 
>> @@ -30,7 +31,7 @@ static bool
>>   zl3073x_pin_check_freq(struct zl3073x_dev *zldev, enum dpll_pin_direction dir,
>>   		       u8 id, u64 freq)
>>   {
>> -	if (freq > U32_MAX)
>> +	if (!freq || freq > U32_MAX)
>>   		goto err_inv_freq;
> 
> [Severity: Low]
> Should the commit message say that this check also fixes a divide error at
> probe time?
> 
> Before this patch, the output path did:
> 
> 	if (synth_freq % (u32)freq)
> 
> zl3073x_pin_props_get() only skips a supported-frequencies-hz entry when it
> equals curr_freq:
> 
> 	if (freqs[i] == curr_freq)
> 		continue;
> 	if (zl3073x_pin_check_freq(zldev, dir, index, freqs[i])) {
> 
> The dpll-pin.yaml binding doesn't set a minimum value. So an output pin
> entry of 0 in firmware would reach the modulo and oops during probe.
> 
> The same check also changes how input pins behave. Before,
> zl3073x_ref_freq_factorize(0, ...) succeeded, because 0 / 1 <= U16_MAX and
> 0 % 1 == 0. That meant 0 Hz was advertised as a supported input frequency.
> With this patch it is dropped.
> 
> The commit message only says "reject 0 Hz". Could it also describe the
> crash fix and the change for input pins?

I will move the 0Hz check to the first patch and update commit message
accordingly.>
>> @@ -41,18 +42,38 @@ zl3073x_pin_check_freq(struct zl3073x_dev *zldev, enum dpll_pin_direction dir,
> 
> [ ... ]
> 
>>   		/* Check the frequency divides synth frequency */
>> -		if (synth_freq % (u32)freq)
>> +		div = div_u64_rem(synth_freq, freq, &rem);
>> +		if (rem)
>> +			goto err_inv_freq;
>> +
>> +		/* The driver requires an output divisor of at least 2 -
>> +		 * below that the hardware bypasses the divider and ignores
>> +		 * the pulse width and eSync registers the driver programs.
>> +		 * For the N-pin of an N-divided output the effective divisor
>> +		 * also includes the N divider (>= 2), so the minimum is 4.
>> +		 * For N-divided outputs this is only a necessary condition -
>> +		 * whether the frequency can be set also depends on the output
>> +		 * divisor shared by both pins, which is checked when the
>> +		 * frequency is set.
>> +		 */
> 
> [Severity: Medium]
> Is the "checked when the frequency is set" part true at this commit? The
> commit message makes the same claim:
> 
>    "For N-divided outputs this is necessary but not sufficient, the rest
>     is checked by the frequency set callback."
> 
> In zl3073x_dpll_output_pin_frequency_set() in drivers/dpll/zl3073x/dpll.c,
> both N-divided branches only reject an N period of zero:
> 
> 	if (zl3073x_dpll_is_p_pin(pin)) {
> 		...
> 		out.esync_n_period = (out.esync_n_period * out.div) / new_div;
> 		if (!out.esync_n_period)
> 			return -EINVAL;
> 		...
> 	} else {
> 		...
> 		out.esync_n_period = div64_u64(synth_freq, frequency * out.div);
> 		if (!out.esync_n_period)
> 			return -EINVAL;
> 	}
> 
> Neither branch requires an N divider of at least 2, and neither checks for
> a remainder.
> 
> For example, take a 1 GHz synth with out.div == 4:
> 
> - Requesting 250 MHz on the N-pin passes zl3073x_pin_check_freq(), since
>    div 4 >= min_div 4. The setter then programs an N divider of 1.
> - Requesting 100 MHz on the N-pin gives esync_n_period == 2, so the pin
>    runs at 125 MHz instead of 100 MHz.
> 
> Neither request returns an error.
> 
> A later patch in this series, "dpll: zl3073x: reject inexact frequencies
> for N-divided outputs", appears to add these checks. It uses
> div64_u64_rem() and rejects n_period < 2, n_period > U32_MAX and a non-zero
> remainder.
> 
> Could the series be reordered so the setter checks land first?
> Alternatively, could the comment and commit message here be changed to
> match what the code does at this point?
Yes, will reorder.

Thanks,
Ivan