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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201210153351.GB4747@sirena.org.uk>
Date:   Thu, 10 Dec 2020 15:33:51 +0000
From:   Mark Brown <broonie@...nel.org>
To:     Tudor.Ambarus@...rochip.com
Cc:     fancer.lancer@...il.com, 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 Thu, Dec 10, 2020 at 08:58:18AM +0000, Tudor.Ambarus@...rochip.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.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)