Re: [PATCH net-next 5/6] dpll: zl3073x: factor out output pin frequency helper

[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 kernel-doc of the new helper
  zl3073x_dpll_output_pin_freq_set() in drivers/dpll/zl3073x/dpll.c says…

Pre-existing issues:
- [High] A user with CAP_NET_ADMIN can trigger a divide-by-zero oops in
  zl3073x_dpll_output_pin_freq_set() (drivers/dpll/zl3073x/dpll.c:1058,…
- [Medium] (Baseline: preexisting, medium; not reported as a finding of
  this patch.) In the N-div P-pin branch of…
- [Medium] (Baseline: preexisting, medium; not reported as a finding of
  this patch.) In zl3073x_dpll_output_pin_freq_set(), the P-pin rescale…
- [Low] (Baseline: not confirmed as a new issue.) The concern says
  zl3073x_dpll_output_pin_freq_set() does not validate its input, which…

--

dpll: zl3073x: factor out output pin frequency helper

This commit moves the divisor and N-division computation out of
zl3073x_dpll_output_pin_frequency_set() into a new helper,
zl3073x_dpll_output_pin_freq_set(). The helper updates a struct
zl3073x_out but does not write it to hardware, so the upcoming PTP
periodic output code can reuse it. No functional change is intended.

> diff --git a/drivers/dpll/zl3073x/dpll.c b/drivers/dpll/zl3073x/dpll.c
> index fcf91aba2988..0a36a2acf15b 100644
> --- a/drivers/dpll/zl3073x/dpll.c
> +++ b/drivers/dpll/zl3073x/dpll.c
> @@ -1032,96 +1032,109 @@ zl3073x_dpll_output_pin_frequency_get(const struct dpll_pin *dpll_pin,

[ ... ]

> +zl3073x_dpll_output_pin_freq_set(struct zl3073x_dpll_pin *pin,
> +				 struct zl3073x_out *out, u64 frequency)
>  {

[ ... ]

> +	synth = zl3073x_out_synth_get(out);
> +	synth_freq = zl3073x_dev_synth_freq_get(zldev, synth);
>  	new_div = synth_freq / (u32)frequency;

[Severity: High]
This isn't a bug introduced by this patch, but can frequency be 0 here?
The old zl3073x_dpll_output_pin_frequency_set() had the same division.

zl3073x_pin_props_get() always puts the current frequency in slot 0 of
the supported list:

    ranges[0] = (struct dpll_pin_frequency)DPLL_PIN_FREQUENCY(curr_freq);

curr_freq comes from zl3073x_dev_output_pin_freq_get(), which returns 0
for an output running below 1 Hz. Two examples:

  - an N-div N-pin with synth 1 GHz, div 10 and esync_n_period 2e8
  - a P-pin with div > synth_freq

zl3073x_out_state_fetch() only rejects a zero div and a zero
esync_n_period, so both setups are accepted.

Suppose the pin is first moved to another advertised frequency (for
example 1 Hz, or through a rescale caused by the sibling P-pin). A later
DPLL_CMD_PIN_SET with frequency 0 then passes
dpll_pin_is_freq_supported() through the {0, 0} range.

It also skips the "freq == old_freq" shortcut in dpll_pin_freq_set(),
because old_freq is no longer 0:

dpll_nl_pin_set_doit()
  dpll_pin_freq_set()
    zl3073x_dpll_output_pin_frequency_set()
      zl3073x_dpll_output_pin_freq_set()
        new_div = synth_freq / (u32)frequency;

On x86 this looks like a divide error oops, raised while zldpll->lock
and the dpll core lock are held.

Would it make sense to return -EINVAL from the helper for !frequency
(and for new_div == 0)? That would also match the new kernel-doc.
Another option is for zl3073x_pin_props_get() to skip a zero curr_freq.

[Severity: Low]
This is a pre-existing issue, but what happens here if the synth reports
a frequency of 0?

zl3073x_synth_state_fetch() only validates freq_n, so a synth_freq of 0
read from the device gives new_div == 0. In that case div = 0 is
committed, and zl3073x_dev_output_pin_freq_get() later divides by
out->div. This case looks speculative, and the netlink path could
already reach it before this patch.

The upcoming PTP caller does not seem to add a new path into this
division. zl3073x_dpll_perout_enable(), added later in the series by
"dpll: zl3073x: add PTP periodic output support", rejects everything
except a 1 second period and passes a constant:

    rc = zl3073x_dpll_output_pin_freq_set(pin, &out, 1);

So the only remaining gap between the helper's "-EINVAL if the
frequency cannot be represented" contract and its behaviour is the
frequency 0 case above and this synth_freq 0 case.

[ ... ]

>  	if (zl3073x_dpll_is_p_pin(pin)) {
> -		/* We are going to change output frequency for P-pin but
> -		 * if the requested frequency is less than current N-pin
> -		 * frequency then indicate a failure as we are not able
> -		 * to compute N-pin divisor to keep its frequency unchanged.
> -		 *
> -		 * Update divisor for N-pin to keep N-pin frequency.
> +		/* Changing the P-pin frequency, rescale the N-pin divisor to
> +		 * keep the N-pin frequency unchanged. Fail if the requested
> +		 * frequency is too low to represent the current N-pin one.
>  		 */
> -		out.esync_n_period = (out.esync_n_period * out.div) / new_div;
> -		if (!out.esync_n_period) {
> -			rc = -EINVAL;
> -			goto unlock;
> -		}
> +		out->esync_n_period = out->esync_n_period * out->div / new_div;
> +		if (!out->esync_n_period)
> +			return -EINVAL;

[Severity: Medium]
This isn't a bug introduced by this patch, but does this rescale really
keep the N-pin frequency unchanged? The division truncates, and only a
zero result is rejected.

For example, synth 1 GHz, div 100 and esync_n_period 2 give P = 10 MHz
and N = 5 MHz. Setting P to 8 MHz passes zl3073x_pin_check_freq(). That
gives new_div = 125 and esync_n_period = 200 / 125 = 1, so N becomes
8 MHz while the helper returns 0.

zl3073x_dpll_output_pin_frequency_set() only looks up the sibling for
non N-divided formats:

    if (!zl3073x_out_is_ndiv(&out))
        sibling = zl3073x_dpll_output_pin_sibling_get(pin);

So no change notification is sent for the N-pin even though its
frequency changed.

The old code had the same (out.esync_n_period * out.div) / new_div
rescale. The earlier commit "dpll: zl3073x: notify sibling pin when
shared output config changes" limited the notification to non N-divided
formats.

Could the helper return -EINVAL when the rescale is not exact?

[Severity: Medium]
This is also a pre-existing issue, but can out->esync_n_period *
out->div overflow here? Both are u32, so the product is computed in 32
bits before the division by new_div.

If div * period exceeds 2^32 (an N-pin below about 0.23 Hz on a 1 GHz
synth), the product wraps:

  - div 8, period 536870912 and a new P of 62.5 MHz (new_div 16): the
    product wraps to exactly 0, so a representable setup is rejected
    with -EINVAL.
  - period 600000000: the wrapped product is 505032704, which gives
    period 31564544. The N-pin then runs at about 1.98 Hz instead of
    about 0.208 Hz.

Would something like div_u64((u64)out->esync_n_period * out->div,
new_div), with a range check on the result, be safer?

[ ... ]

>  	} else {
> -		/* We are going to change frequency of N-pin but if
> -		 * the requested freq is greater or equal than freq of P-pin
> -		 * in the output pair we cannot compute divisor for the N-pin.
> -		 * In this case indicate a failure.
> -		 *
> -		 * Update divisor for N-pin
> +		/* Changing the N-pin frequency. Fail if the requested
> +		 * frequency is higher than or does not divide the P-pin one.
>  		 */
> -		out.esync_n_period = div64_u64(synth_freq, frequency * out.div);
> -		if (!out.esync_n_period) {
> -			rc = -EINVAL;
> -			goto unlock;
> -		}
> +		out->esync_n_period = div64_u64(synth_freq,
> +						frequency * out->div);
> +		if (!out->esync_n_period)
> +			return -EINVAL;
>  	}

[Severity: Low]
Is the new comment accurate? It says the helper fails when the requested
frequency "does not divide the P-pin one". However, div64_u64() truncates
and only a zero quotient is rejected.

The new kernel-doc makes a similar promise:

    Return: 0 on success, -EINVAL if the frequency cannot be represented

For example, take synth 1 GHz and div 100 (P = 10 MHz):

  - A request for N = 4 MHz passes zl3073x_pin_check_freq(), because
    1e9 % 4e6 == 0.
  - esync_n_period becomes 1e9 / 4e8 = 2 (2.5 truncated), and the
    helper returns 0.
  - zl3073x_dev_output_pin_freq_get() then reports N = 5 MHz.

The non N-divided branch also returns 0 without checking new_div.

The truncation itself was already there. What this patch adds is the
comment and kernel-doc that don't match it.

Should there be a remainder check here, such as returning -EINVAL when
synth_freq % (frequency * out->div) is nonzero? Or should the comment
and kernel-doc be reworded to match the current behaviour?

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