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

[Ivan Vecera]

On 10/12/26 08:13, netdev-bot+sashiko@xxxxxxxxxx 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 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")

Maybe, does it make sense to make another respin? Paulo,Jakub?
> 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?
No. The current frequency is listed so that the pin always reports
a frequency from its supported list, even when the flash configuration
uses a value the driver itself would not set. Going back to such a
value can then be inexact or fail.

Thanks,
Ivan