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

[netdev-bot+sashiko@kernel.org]

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?

>  	}
>

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