Re: [PATCH net v5 1/5] dpll: zl3073x: do not offer 0 Hz as a supported pin frequency

[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] The patch fixes two separate 0 Hz bugs, but its only Fixes: tag
  names 85a9aaac4a38 ("dpll: zl3073x: Include current frequency in…

Pre-existing issues:
- [Medium] zl3073x_pin_props_get() still adds the probe-time current
  frequency at index 0 after only a zero check (prop.c line 302).

--

dpll: zl3073x: do not offer 0 Hz as a supported pin frequency

This patch stops zl3073x_pin_props_get() from adding a 0 Hz current
frequency to the supported list. It also makes zl3073x_pin_check_freq()
reject 0 Hz firmware node entries. If the resulting list is empty, it is
not published.

> diff --git a/drivers/dpll/zl3073x/prop.c b/drivers/dpll/zl3073x/prop.c
> index ac9d41d0f978e..768e5424de2d2 100644
> --- a/drivers/dpll/zl3073x/prop.c
> +++ b/drivers/dpll/zl3073x/prop.c
> @@ -30,7 +30,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]

The commit message also says that a 0 Hz firmware node entry is divided by
in zl3073x_pin_check_freq() at probe time. Does the Fixes: tag cover that
part of the fix?

Before this patch, zl3073x_pin_check_freq() had:

	if (freq > U32_MAX)
		goto err_inv_freq;
	...
		if (synth_freq % (u32)freq)
			goto err_inv_freq;

Both checks came from a99a9f0ebdaa ("dpll: zl3073x: Read DPLL types and pin
properties from system firmware"), not from 85a9aaac4a38. On input pins,
zl3073x_ref_freq_factorize(0) also succeeds, so 0 Hz was accepted there too.

Would it make sense to add this tag as well?

Fixes: a99a9f0ebdaa ("dpll: zl3073x: Read DPLL types and pin properties from system firmware")

Without it, stable trees that have a99a9f0ebdaa but not 85a9aaac4a38 might
not pick this up.

The later patch in this series, "dpll: zl3073x: reject output frequencies
with too small divisor", carries Fixes: a99a9f0ebdaa. It still divides by
freq via div_u64_rem(), so it seems to depend on the !freq guard added here.

>  
>  	if (dir == DPLL_PIN_DIRECTION_INPUT) {
> @@ -295,13 +295,20 @@ struct zl3073x_pin_props *zl3073x_pin_props_get(struct zl3073x_dev *zldev,
>  		goto err_alloc_ranges;
>  	}
>  
> -	/* Start with current frequency at index 0 */
> -	ranges[0] = (struct dpll_pin_frequency)DPLL_PIN_FREQUENCY(curr_freq);
> +	/* Start with current frequency at index 0. A sub-Hz frequency is
> +	 * read back as 0 Hz and cannot be set, so it is not offered.
> +	 */
> +	j = 0;
> +	if (curr_freq) {
> +		struct dpll_pin_frequency freq = DPLL_PIN_FREQUENCY(curr_freq);
> +
> +		ranges[j++] = freq;
> +	}

[Severity: Medium]

This is a pre-existing issue and was not introduced by this patch. The new
comment says a frequency that cannot be set is not offered, but only 0 Hz
is filtered here. Should curr_freq also go through
zl3073x_pin_check_freq(), as the firmware node entries do?

curr_freq comes from truncating divisions:

  - outputs: synth_freq / out->div, then / esync_n_period for N-pins
  - inputs: mul_u64_u32_div(base * mult, M, N)

So a non-zero curr_freq can still be a value that the set callbacks cannot
program.

For output pins, zl3073x_dpll_output_pin_frequency_set() does:

	new_div = synth_freq / (u32)frequency;

The non-N-div path has no remainder or minimum divisor check. Take a 1 GHz
synth with a probe-time divisor of 300000. curr_freq is 3333, and selecting
it later gives new_div = 300030, not 300000.

If the probe-time divisor was 1, a user can switch away and then pick the
still-advertised curr_freq again. That puts the output back into divider
bypass. The later patch in this series says the hardware ignores the width
and eSync registers in that mode.

For input pins, zl3073x_dpll_input_pin_frequency_set() ignores the result
of the factorization:

	/* Update frequency */
	zl3073x_ref_freq_set(&ref, frequency);

	/* Commit reference state */
	return zl3073x_ref_state_set(zldev, ref_id, &ref);

Suppose curr_freq came from an M/N ratio that has no base * mult
factorization, such as a truncated 161132812 Hz. zl3073x_ref_freq_set()
fails and ref stays unchanged. zl3073x_ref_state_set() then finds nothing
to write and returns 0, so the request reports success while the reference
keeps its previous frequency.

The unconditional curr_freq entry comes from 85a9aaac4a38. The ignored
zl3073x_ref_freq_set() return value is older.

[ ... ]

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