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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <b1b385eb-15f7-8347-a5e2-0a711937c879@microchip.com>
Date:   Wed, 16 Dec 2020 08:25:04 +0000
From:   <Tudor.Ambarus@...rochip.com>
To:     <geert@...ux-m68k.org>, <broonie@...nel.org>
CC:     <linux-spi@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] spi: Limit the spi device max speed to controller's max
 speed

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@...nel.org> 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@...ux-m68k.org
> 
> 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
>