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

[netdev-bot+sashiko@kernel.org]

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?

> @@ -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?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009192556.272263-1-ivecera%40redhat.com