[GIT PULL] mtd: spi-nor: Changes for 4.11

[Brian Norris]

+ Mark, as the disagreement touches his area

Hi,

On Wed, Feb 08, 2017 at 11:36:10AM +0100, Cyrille Pitchen wrote:
> Le 08/02/2017 à 06:52, Heiner Kallweit a écrit :
> > Am 08.02.2017 um 02:09 schrieb Marek Vasut:
> >> On 02/07/2017 04:31 PM, Cyrille Pitchen wrote:
> [...]
> >>> Heiner Kallweit (1):
> >>>       mtd: m25p80: consider max message size in m25p80_read
> >>
> >> I cannot say I'm super-happy about this particular patch, still ...
> >>
> > W/o it at least the fsl-espi driver is broken for transfers >= 64kB.
> > 
> 
> We asked Mark Brown about the need to check the limits for the size of SPI
> transfers and messages and I've understood his answer correctly, he told us
> we should check those limits:
> 
> http://patchwork.ozlabs.org/patch/688176/
> 
> "> Does this imply that we have to fix every single driver this way ?"
> 
> "Ideally.  Any SPI driver that has an upper limit really ought to export
> it, and any driver that might bump into one of those limits should
> check.  A lot of drivers are either unlikely to be deployed in a system
> that has relevant limits or tend to only do short enough transfers
> though - if systems are currently working it's just neatening things up
> really"
> 
> Then the discussion ended there. I didn't see any comment after that so I
> just thought the discussion was over and everybody agreed.

I thought we more or less settled on this a while ago. But I could be
wrong.

> Besides, this patch doesn't introduce the check of SPI controller limits
> but only extend this check. What I mean is that the m25p80 driver already
> checks the maximum transfer size. This check was introduced by commit
> 
> 95193796256c ("mtd: m25p80: read in spi_max_transfer_size chunks")
> 
> Then if we already check the SPI max transfer size, I guess it make sense
> to also test other limits such as the SPI max message size.

Yes, if this patch is wrong, then I'd expect commit 95193796256c was
also wrong.

The central disagreement seems to be on the existence of upper limits
for transfer and/or message size. Marek seemed to suggest elsewhere that
there is usually no upper limit (or that "upper limits" are usually just
driver bugs that are being worked around). I don't disagree that often
these might just be driver bugs, but I *do* disagree that there are
never limits. I think Mark (? or somebody else, not reading too closely)
suggested that there are just few drivers that tickle the limits.

One example that shows both sides of this coin:

5185a81c02d4 "spi: rockchip: limit transfers to (64K - 1) bytes"

The hardware has a hard limit on transfers, at least, which can only be
64K. For some reason there's an off-by-one error (probably a driver
bug?), so I cheated and made it one smaller.

[...]

(snip the rest about trying to split spi_messages within the SPI driver;
I agree that would be awful.)

But one question: *why* exactly is the transfer or message limit so
fixed? If a SPI controller allows manual control over the CS line, then
why would anybody care if a sequence of data gets split up into multiple
HW sequences (e.g., for a long spi_transfer, why can't we split it into
2 sub-transfers, while keeping the CS asserted?).

For the above rockchip SPI controller, AIUI we could actually support
the "transfer limit" by doing this. And in fact, it could probably be
done within the SPI core code, instead either the SPI driver or the MTD
driver.

Now, in this case, *transfer* vs. *message* seems to have a difference,
in whether or not you have guarantees about the CS line. The CS must
stay asserted (barring the 'cs_change' flag) between transfers, but not
between messages.

So the one thing I can see that might support Marek's disagreement here:
does the FSL ESPI hardware support controlling the CS line? It seems
like currently, the driver just uses a few fixed settings for its CSMODE
register fields, whereas it might be able to support ->transfer_one()
(and flexible CS assertion, and splitting large transactions into
smaller chunks) if you handled that differently...

Heiner (or others), any thoughts on the above? I might be very mistaken,
especially since I'm not intimately familiar with a lot of SPI drivers.

Regardless, I think this patch is OK, given the established practice,
and we should take it. We might want to push back to handle this better
in the SPI layer, if at all possible.

One thing I do object to: why is this change marked for stable? It's not
our fault you broke your duct-taped-together SPI driver in 4.9. I don't
think backporting features is the way to satisfy the -stable
requirements. Maybe some reverts would be the right way...

Brian