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

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

[Tudor Ambarus]

Make sure the max_speed_hz of spi_device does not override
the max_speed_hz of controller.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/spi/spi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index cd3c395b4e90..51d7c004fbab 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -3378,7 +3378,8 @@ int spi_setup(struct spi_device *spi)
 	if (status)
 		return status;
 
-	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;
 
 	mutex_lock(&spi->controller->io_mutex);
-- 
2.25.1

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

[Serge Semin]

Hello Tudor,

On Wed, Dec 09, 2020 at 07:35:14PM +0200, Tudor Ambarus wrote:
> Make sure the max_speed_hz of spi_device does not override
> the max_speed_hz of controller.

I have doubts that's right thing to do. It seems better to let
the controller driver to handle the speed clamping itself, while
to leave the SPI client device max_speed_hz field describing the
device speed capability. Moreover the SPI-transfers passed to the
controller will have a SPI-bus speed fixed in accordance with the
controller and client device capabilities anyway.
See the __spi_validate() method for details:
https://elixir.bootlin.com/linux/v5.10-rc7/source/drivers/spi/spi.c#L3570

-Sergey

> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
>  drivers/spi/spi.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index cd3c395b4e90..51d7c004fbab 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -3378,7 +3378,8 @@ int spi_setup(struct spi_device *spi)
>  	if (status)
>  		return status;
>  
> -	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;
>  
>  	mutex_lock(&spi->controller->io_mutex);
> -- 
> 2.25.1
> 

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 969 bytes --]

On Wed, Dec 09, 2020 at 10:46:36PM +0300, Serge Semin wrote:

> On Wed, Dec 09, 2020 at 07:35:14PM +0200, Tudor Ambarus wrote:

> > Make sure the max_speed_hz of spi_device does not override
> > the max_speed_hz of controller.

> I have doubts that's right thing to do. It seems better to let
> the controller driver to handle the speed clamping itself, while
> to leave the SPI client device max_speed_hz field describing the
> device speed capability. Moreover the SPI-transfers passed to the
> controller will have a SPI-bus speed fixed in accordance with the
> controller and client device capabilities anyway.
> See the __spi_validate() method for details:
> https://elixir.bootlin.com/linux/v5.10-rc7/source/drivers/spi/spi.c#L3570

Right, in general we aim to do this sort of fixup on the transfers
and messages rather than the devices, I guess we might be missing
validation in some of the flash acceleration paths or was this an issue
seen through inspection?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Serge Semin]

On Wed, Dec 09, 2020 at 07:54:20PM +0000, Mark Brown wrote:
> On Wed, Dec 09, 2020 at 10:46:36PM +0300, Serge Semin wrote:
> 
> > On Wed, Dec 09, 2020 at 07:35:14PM +0200, Tudor Ambarus wrote:
> 
> > > Make sure the max_speed_hz of spi_device does not override
> > > the max_speed_hz of controller.
> 
> > I have doubts that's right thing to do. It seems better to let
> > the controller driver to handle the speed clamping itself, while
> > to leave the SPI client device max_speed_hz field describing the
> > device speed capability. Moreover the SPI-transfers passed to the
> > controller will have a SPI-bus speed fixed in accordance with the
> > controller and client device capabilities anyway.
> > See the __spi_validate() method for details:
> > https://elixir.bootlin.com/linux/v5.10-rc7/source/drivers/spi/spi.c#L3570
> 

> Right, in general we aim to do this sort of fixup on the transfers
> and messages rather than the devices, I guess we might be missing
> validation in some of the flash acceleration paths or was this an issue
> seen through inspection?

In case of DW SPI driver we just make sure the SPI-client device
speed set in the max_speed_hz doesn't exceed the controller SPI-bus
clock frequency and clamp it if it does. So the driver is safe in that
matter.

-Sergey

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 674 bytes --]

On Wed, Dec 09, 2020 at 11:15:35PM +0300, Serge Semin wrote:
> On Wed, Dec 09, 2020 at 07:54:20PM +0000, Mark Brown wrote:

> > Right, in general we aim to do this sort of fixup on the transfers
> > and messages rather than the devices, I guess we might be missing
> > validation in some of the flash acceleration paths or was this an issue
> > seen through inspection?

> In case of DW SPI driver we just make sure the SPI-client device
> speed set in the max_speed_hz doesn't exceed the controller SPI-bus
> clock frequency and clamp it if it does. So the driver is safe in that
> matter.

Ideally the driver wouldn't have to check though (no harm in doing so of
course).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Serge Semin]

On Wed, Dec 09, 2020 at 08:25:52PM +0000, Mark Brown wrote:
> On Wed, Dec 09, 2020 at 11:15:35PM +0300, Serge Semin wrote:
> > On Wed, Dec 09, 2020 at 07:54:20PM +0000, Mark Brown wrote:
> 
> > > Right, in general we aim to do this sort of fixup on the transfers
> > > and messages rather than the devices, I guess we might be missing
> > > validation in some of the flash acceleration paths or was this an issue
> > > seen through inspection?
> 
> > In case of DW SPI driver we just make sure the SPI-client device
> > speed set in the max_speed_hz doesn't exceed the controller SPI-bus
> > clock frequency and clamp it if it does. So the driver is safe in that
> > matter.
> 

> Ideally the driver wouldn't have to check though (no harm in doing so of
> course).

If so then we'd need to have a dedicated speed-related field in the
spi_mem_op structure, which would be accordingly initialized by the
SPI-mem core.

-Sergey

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

[Tudor.Ambarus]

Hi, Serge, Mark,

On 12/9/20 10:30 PM, Serge Semin wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Wed, Dec 09, 2020 at 08:25:52PM +0000, Mark Brown wrote:
>> On Wed, Dec 09, 2020 at 11:15:35PM +0300, Serge Semin wrote:
>>> On Wed, Dec 09, 2020 at 07:54:20PM +0000, Mark Brown wrote:
>>
>>>> Right, in general we aim to do this sort of fixup on the transfers
>>>> and messages rather than the devices, I guess we might be missing
>>>> validation in some of the flash acceleration paths or was this an issue
>>>> seen through inspection?

We miss validation for the SPI controllers that provide the
spi_controller_mem_ops with its exec_op() method. In this case the SPI
core does not check if the max_speed_hz of spi_device overrides the
max_speed_hz of controller.

This was seen through inspection. There are octal SPI NOR flashes that
can run at 400 MHZ, and I've also seen SPI controllers that are limited
to 200 MHZ (microchip's sama7g5 octal SPI for example, which is not yet
in mainline).

>>
>>> In case of DW SPI driver we just make sure the SPI-client device
>>> speed set in the max_speed_hz doesn't exceed the controller SPI-bus
>>> clock frequency and clamp it if it does. So the driver is safe in that
>>> matter.
>>
> 
>> Ideally the driver wouldn't have to check though (no harm in doing so of
>> course).

Right. I thought of doing this in the SPI core, rather than doing in (each)
controller driver.

Serge,

I've looked at https://elixir.bootlin.com/linux/v5.10-rc7/source/drivers/spi/spi.c#L3570
That limits the xfer speed hz to the controller's max_speed_hz. SPI
controllers that implement the exec_op() optimized handler are not going
through this path and are not covered by the check in __spi_validate().

> 
> If so then we'd need to have a dedicated speed-related field in the
> spi_mem_op structure, which would be accordingly initialized by the
> SPI-mem core.
> 

We do need a max_speed_hz in the SPIMEM, but probably for other purposes:
NOR flashes, for example, can discover the maximum supported frequency
at run-time, when parsing the jesd216 SFDP tables. One may consider that
the run-time discovered max_speed_hz should have a higher precedence than
what we fill with the spi-max-frequency dt property, because the
manufacturers/jesd216 standard know/s better. Of course, if the
manufacturers put a wrong max_speed_hz in the sfdp table, that can be
updated at runtime with a fixup() hook, on a case by case basis. Other
thing to consider here, is the max_speed_hz supported by the PCB. For
example if the SPI flash supports 400 MHZ, the controller 200 MHZ, but
the PCB only 180 MHZ, then we'll have to synchronize all three. But this
seems like a discussion for other patch.

Let me know if you think that this patch is ok for now. I can update the
commit message if needed.

Cheers,
ta

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2650 bytes --]

On Thu, Dec 10, 2020 at 08:58:18AM +0000, Tudor.Ambarus@microchip.com wrote:
> On 12/9/20 10:30 PM, Serge Semin wrote:

> >>>> Right, in general we aim to do this sort of fixup on the transfers
> >>>> and messages rather than the devices, I guess we might be missing
> >>>> validation in some of the flash acceleration paths or was this an issue
> >>>> seen through inspection?

> We miss validation for the SPI controllers that provide the
> spi_controller_mem_ops with its exec_op() method. In this case the SPI
> core does not check if the max_speed_hz of spi_device overrides the
> max_speed_hz of controller.

> This was seen through inspection. There are octal SPI NOR flashes that
> can run at 400 MHZ, and I've also seen SPI controllers that are limited
> to 200 MHZ (microchip's sama7g5 octal SPI for example, which is not yet
> in mainline).

Right, that's the hole :/

> >> Ideally the driver wouldn't have to check though (no harm in doing so of
> >> course).

> Right. I thought of doing this in the SPI core, rather than doing in (each)
> controller driver.

Yes, we should just make sure things are OK in the core as much as we
can so there's less work for driver authors.

> > If so then we'd need to have a dedicated speed-related field in the
> > spi_mem_op structure, which would be accordingly initialized by the
> > SPI-mem core.

> We do need a max_speed_hz in the SPIMEM, but probably for other purposes:
> NOR flashes, for example, can discover the maximum supported frequency
> at run-time, when parsing the jesd216 SFDP tables. One may consider that
> the run-time discovered max_speed_hz should have a higher precedence than
> what we fill with the spi-max-frequency dt property, because the
> manufacturers/jesd216 standard know/s better. Of course, if the
> manufacturers put a wrong max_speed_hz in the sfdp table, that can be
> updated at runtime with a fixup() hook, on a case by case basis. Other
> thing to consider here, is the max_speed_hz supported by the PCB. For
> example if the SPI flash supports 400 MHZ, the controller 200 MHZ, but
> the PCB only 180 MHZ, then we'll have to synchronize all three. But this
> seems like a discussion for other patch.

The potential for board issues suggests that we should be taking the
minimum of what the board DT and runtime discovery say - if the board
limits things more than the parts we should assume that there's a system
integration limitation there.

> Let me know if you think that this patch is ok for now. I can update the
> commit message if needed.

It does work for now but it'd be nicer if we were doing this through
recording the decision on the transfer.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Tudor.Ambarus]

On 12/10/20 5:33 PM, Mark Brown wrote:
> On Thu, Dec 10, 2020 at 08:58:18AM +0000, Tudor.Ambarus@microchip.com wrote:
>> On 12/9/20 10:30 PM, Serge Semin wrote:
> 
>>>>>> Right, in general we aim to do this sort of fixup on the transfers
>>>>>> and messages rather than the devices, I guess we might be missing
>>>>>> validation in some of the flash acceleration paths or was this an issue
>>>>>> seen through inspection?
> 
>> We miss validation for the SPI controllers that provide the
>> spi_controller_mem_ops with its exec_op() method. In this case the SPI
>> core does not check if the max_speed_hz of spi_device overrides the
>> max_speed_hz of controller.
> 
>> This was seen through inspection. There are octal SPI NOR flashes that
>> can run at 400 MHZ, and I've also seen SPI controllers that are limited
>> to 200 MHZ (microchip's sama7g5 octal SPI for example, which is not yet
>> in mainline).
> 
> Right, that's the hole :/
> 
>>>> Ideally the driver wouldn't have to check though (no harm in doing so of
>>>> course).
> 
>> Right. I thought of doing this in the SPI core, rather than doing in (each)
>> controller driver.
> 
> Yes, we should just make sure things are OK in the core as much as we
> can so there's less work for driver authors.
> 
>>> If so then we'd need to have a dedicated speed-related field in the
>>> spi_mem_op structure, which would be accordingly initialized by the
>>> SPI-mem core.
> 
>> We do need a max_speed_hz in the SPIMEM, but probably for other purposes:
>> NOR flashes, for example, can discover the maximum supported frequency
>> at run-time, when parsing the jesd216 SFDP tables. One may consider that
>> the run-time discovered max_speed_hz should have a higher precedence than
>> what we fill with the spi-max-frequency dt property, because the
>> manufacturers/jesd216 standard know/s better. Of course, if the
>> manufacturers put a wrong max_speed_hz in the sfdp table, that can be
>> updated at runtime with a fixup() hook, on a case by case basis. Other
>> thing to consider here, is the max_speed_hz supported by the PCB. For
>> example if the SPI flash supports 400 MHZ, the controller 200 MHZ, but
>> the PCB only 180 MHZ, then we'll have to synchronize all three. But this
>> seems like a discussion for other patch.
> 
> The potential for board issues suggests that we should be taking the
> minimum of what the board DT and runtime discovery say - if the board
> limits things more than the parts we should assume that there's a system
> integration limitation there.
> 
>> Let me know if you think that this patch is ok for now. I can update the
>> commit message if needed.
> 
> It does work for now but it'd be nicer if we were doing this through
> recording the decision on the transfer.
> 

Ok, we can drop the patch, as nobody complained about this until now. I can
work on a better approach. Are you saying that we should calibrate/adjust the
maximum supported frequency on each operation/command? Most of the commands
can work at the same frequency. I know an exception: on SPI NOR flashes, the
jesd216 standard specifies that the READ SFDP command should be run at 50MHz,
even though the rest of the commands/opcodes can run at a higher frequency.
It is common that flashes can run this command at 50+ MHz, and nobody bothered
about adjusting the frequency at run-time until now. That being said, maybe we
can calibrate/adjust a generic max frequency for most of the commands and
treat the exceptions on a per operation basis.

Cheers,
ta

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1417 bytes --]

On Thu, Dec 10, 2020 at 04:32:02PM +0000, Tudor.Ambarus@microchip.com wrote:
> On 12/10/20 5:33 PM, Mark Brown wrote:

> > It does work for now but it'd be nicer if we were doing this through
> > recording the decision on the transfer.

> Ok, we can drop the patch, as nobody complained about this until now. I can

TBH I've actually got it queued to apply and test, we can make things
better incrementally but it seems fine for now and it's backportable if
someone does run into trouble.

> work on a better approach. Are you saying that we should calibrate/adjust the
> maximum supported frequency on each operation/command? Most of the commands
> can work at the same frequency. I know an exception: on SPI NOR flashes, the

It's the way that everything else works.  It's a bit inefficent though.

> jesd216 standard specifies that the READ SFDP command should be run at 50MHz,
> even though the rest of the commands/opcodes can run at a higher frequency.
> It is common that flashes can run this command at 50+ MHz, and nobody bothered
> about adjusting the frequency at run-time until now. That being said, maybe we
> can calibrate/adjust a generic max frequency for most of the commands and
> treat the exceptions on a per operation basis.

If the spec says 50MHz for that one op we should probably stick to that
by default - the chances of it making a difference seem low but better
to stay in spec if we can.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Mark Brown]

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

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

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

[Geert Uytterhoeven]

Hi Mark, Tudor,

On Fri, Dec 11, 2020 at 8:02 PM Mark Brown <broonie@kernel.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.

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.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-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

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@kernel.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@linux-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
>