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

[Vladimir Oltean <olteanv@xxxxxxxxx>]

On Thu, May 20, 2021 at 03:29:47PM +0100, Mark Brown wrote:
> 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.

Stitching a bunch of s/messages/transfers/, but yes, that is more or
less correct. I think cs_change is pretty well handled with the SPI
drivers I've had the pleasure so far. The spi-sc18is602.c driver has had
no changes in this area since its introduction in 2012 and it worked out
of the box (well, except for the maximum buffer length limit, which I
was expecting even before I had my hands on the hardware, since it is
explained here:
https://www.kernel.org/doc/html/latest/spi/spi-sc18is602.html).
SPI controllers that don't treat cs_change properly can always be
patched, although there is a sizable user base for this feature at the
moment from what I can see, so the semantics are pretty clear to me (and
the sja1105 is in line with them).

> > 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.

Does this piece of advice mean "don't use cs_change"? Why does it exist
then? I'm confused. Is it because the max_*_size properties are not well
defined in its presence? Isn't that a problem with the self-consistency
of the SPI API then?

> > 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?

I think the sja1105 is safe to just ignore the maximum message length
because "it knows what it's doing" (famous last words). The only real
question is "what does .max_message_size count when its containing
spi_transfers have cs_change set?", and while I can perfectly understand
why you'd like to avoid that question, I think my interpretation is the
sane one (it just counts the pieces with continuous CS), and I don't see
the problems that this interpretation can cause down the line.

If you want to, I can just resend the spi-sc18is602 patch without
master->max_message_size implemented, and voila, I'm not ignoring it any
longer :)
https://patchwork.kernel.org/project/spi-devel-general/patch/20210520131238.2903024-3-olteanv@xxxxxxxxx/