Re: [PATCH net v3 1/4] dpll: zl3073x: reject output frequencies with too small divisor

[Ivan Vecera <ivecera@redhat.com>]

On 10/8/26 17:31, 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] In N-div output mode, the new probe-time check in
>    zl3073x_pin_check_freq() (drivers/dpll/zl3073x/prop.c) only requires…
> 
> --
> 
> dpll: zl3073x: reject output frequencies with too small divisor
> 
> This patch tightens zl3073x_pin_check_freq() so output pin frequencies
> from firmware must divide the synth frequency with a divisor of at
> least 2. For the N-pin of an N-divided output the minimum is 4. It also
> rejects a zero frequency so the division cannot be by zero.
> 
>> diff --git a/drivers/dpll/zl3073x/prop.c b/drivers/dpll/zl3073x/prop.c
>> index ac9d41d0f978e..a4bdc3878590f 100644
>> --- a/drivers/dpll/zl3073x/prop.c
>> +++ b/drivers/dpll/zl3073x/prop.c
> 
> [ ... ]
> 
>> @@ -41,18 +42,32 @@ 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 output divisor has to be at least 2. For the N-pin of an
>> +		 * N-divided output the effective divisor also includes the N
>> +		 * divider (>= 2), so the minimum is 4.
>> +		 */
>> +		out = zl3073x_out_state_get(zldev, out_id);
>> +		if (zl3073x_out_is_ndiv(out) && zl3073x_is_n_pin(id))
>> +			min_div = 4;
>> +		else
>> +			min_div = 2;
>> +		if (div < min_div)
>>   			goto err_inv_freq;
> 
> [Severity: Low]
> For N-divided outputs, the new check compares the effective divisor
> only against the synth. It does not look at the shared output divider
> (out->div). Does that let an N-pin frequency pass this filter even
> though no valid N divider exists for it?
> 
> In this patch, zl3073x_dpll_output_pin_frequency_set() in
> drivers/dpll/zl3073x/dpll.c computes the N divider with a truncating
> division and only rejects a result of 0:
> 
> drivers/dpll/zl3073x/dpll.c:zl3073x_dpll_output_pin_frequency_set() {
>      ...
> 	} else {
> 		...
> 		out.esync_n_period = div64_u64(synth_freq, frequency * out.div);
> 		if (!out.esync_n_period)
> 			return -EINVAL;
> 	}
>      ...
> }
> 
> Take a 1 GHz synth with P-pin divider 10:
> 
>    N-pin request of 100 MHz: div = 10 >= 4, so it passes here.
>    esync_n_period then works out to 1, and that is committed to
>    hardware.
> 
>    N-pin request of 40 MHz: div = 25 >= 4, so it also passes.
>    esync_n_period is truncated from 2.5 to 2. The output runs at
>    50 MHz, but the call reports success.
> 
> The P-pin branch has the same truncation and zero-only check:
> 
> 		out.esync_n_period = (out.esync_n_period * out.div) / new_div;
> 		if (!out.esync_n_period)
> 			return -EINVAL;
> 
> The later commit in this series, "dpll: zl3073x: reject inexact
> frequencies for N-divided outputs", fixes this. It adds remainder
> checks and esync_n_period >= 2 checks to both branches, and returns
> -EINVAL with an extack message.
> 
> After that commit, one gap remains. Some frequencies have an effective
> divisor that cannot be split into out_div >= 2 times n_div >= 2, such
> as synth/5. They still appear in freq_supported, but setting them is
> rejected. Since this check is necessary but not sufficient, should the
> commit message or the comment say so?
> 

Yes, next version will say so in the commit message. The remaining cases are
rejected by the frequency set callback.

Thanks,
Ivan