Re: [PATCH] spi: Limit the spi device max speed to controller's max speed

[Tudor.Ambarus]

On 12/15/20 4:24 PM, Geert Uytterhoeven wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi Mark, Tudor,

Hi, Geert,

> 
> On Fri, Dec 11, 2020 at 8:02 PM Mark Brown <broonie@xxxxxxxxxx> wrote:
>> On Wed, 9 Dec 2020 19:35:14 +0200, Tudor Ambarus wrote:
>>> Make sure the max_speed_hz of spi_device does not override
>>> the max_speed_hz of controller.
>>
>> Applied to
>>
>>    https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
>>
>> Thanks!
>>
>> [1/1] spi: Limit the spi device max speed to controller's max speed
>>       commit: 9326e4f1e5dd1a4410c429638d3c412b6fc17040
> 
>> -       if (!spi->max_speed_hz)
>> +       if (!spi->max_speed_hz ||
>> +           spi->max_speed_hz > spi->controller->max_speed_hz)
>>                 spi->max_speed_hz = spi->controller->max_speed_hz;
> 
> If spi->controller->max_speed_hz is zero, a non-zero spi->max_speed_hz
> will be overwritten by zero.
> 

oh, yes, you're right.

> Hence this broke spi-sh-msiof, which has the following check in
> sh_msiof_spi_set_clk_regs():
> 
>         if (!spi_hz || !parent_rate) {
>                 WARN(1, "Invalid clock rate parameters %lu and %u\n",
>                      parent_rate, spi_hz);
>                 return;
>         }
> 
> Without this, the driver would trigger a division-by-zero later...
> 
> Arguably all SPI controller drivers should fill in
> spi_controller.{min,max}_speed_hz, but as long as that is not the case,
> I think this patch should be reverted, or the check should be enhanced
> to make sure spi->controller->max_speed_hz is non-zero.
> 

I'm fine both ways, since this should be improved anyway. Will send a patch
with the non-zero check and add your Reported-by, Suggested-by tags, in case
Mark decides to keep it.

Cheers,
ta

> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
>