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

[Ivan Vecera <ivecera@redhat.com>]

On 10/12/26 08:13, 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 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