netdev - Re: [PATCH net-next] net: dsa: sja1105: adapt to a SPI controller with a limited max transfer size

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: <20210520142947.GC3962@sirena.org.uk>
Date:   Thu, 20 May 2021 15:29:47 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        linux-spi@...r.kernel.org, Guenter Roeck <linux@...ck-us.net>,
        Vladimir Oltean <vladimir.oltean@....com>
Subject: Re: [PATCH net-next] net: dsa: sja1105: adapt to a SPI controller
 with a limited max transfer size

On Thu, May 20, 2021 at 05:06:09PM +0300, Vladimir Oltean wrote:
> On Thu, May 20, 2021 at 02:56:15PM +0100, Mark Brown wrote:
> > On Thu, May 20, 2021 at 04:50:31PM +0300, Vladimir Oltean wrote:

> > > Only that certain SPI controllers, such as the spi-sc18is602 I2C-to-SPI
> > > bridge, cannot keep the chip select asserted for that long.
> > > The spi_max_transfer_size() and spi_max_message_size() functions are how
> > > the controller can impose its hardware limitations upon the SPI
> > > peripheral driver.

> > You should respect both, frankly I don't see any advantage to using
> > cs_change for something like this - just do a bunch of async SPI
> > transfers and you'll get the same effect in terms of being able to keep
> > the queue for the controller primed with more robust support since it's
> > not stressing edge cases.  cs_change is more for doing things that are
> > just very non-standard.

> Sorry, I don't really understand your comment: in which way would it be
> more robust for my use case to use spi_async()?

Your description sounds like the driver is just stitching a bunch of
messages together into a single big message with lots of cs_changes with
the goal of improving performance which is really not using the API at
all idiomatically.  That's at best asking for trouble (it'll certainly
work with fewer controllers), it may even be less performant as you're
less likely to get the benefit of framework enhancements.

> The cs_change logic was already there prior to this patch, I am just
> reiterating how it works. Given the way in which it works (which I think

It seems like you could avoid this issue and most likely other future
issues by making the way the driver uses the API more normal.

> is correct), the most natural way to limit the buffer length is to look
> for the max transfer len.

No, you really do need to pay attention to both - what makes you think
it is safe to just ignore one of them?

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