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

[Ivan Vecera <ivecera@redhat.com>]

On 9/30/26 8:57 AM, 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] 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.

I will guard both by:

	if (!frequency)
		return -EINVAL;
	...
	new_div = synth_freq / (u32)frequency;
	if (!new_div)
		return -EINVAL;

!new_div also covers a synth_freq of 0 (and any request above synth_freq).

> 
> [ ... ]
> 
>>   	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?

I will fix together. The product will be computed in 64 bits and
the rescale is rejected unless it is exact:

	u64 product = (u64)out->esync_n_period * out->div;
	u32 rem;

	out->esync_n_period = div_u64_rem(product, new_div, &rem);
	if (rem || !out->esync_n_period)
		return -EINVAL;

So there is no u32 overflow, and an inexact rescale returns -EINVAL instead
of silently changing the N-pin frequency. That also settles the patch 1
question about notifying the N-pin on this path: with an exact rescale the
N-pin frequency is guaranteed unchanged, so no sibling notification is
needed.

> [ ... ]
> 
>>   	} 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?

With above the comment matches the code:

	u64 rem, divisor = frequency * out->div;

	out->esync_n_period = div64_u64_rem(synth_freq, divisor, &rem);
	if (rem || !out->esync_n_period)
		return -EINVAL;

A requested N-pin frequency that does not divide the P-pin one exactly now
returns -EINVAL.

Thanks,
Ivan

pw-bot: cr