linux-kernel - Re: [PATCH] mtd: spi-nor: honour max_data

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: <20180612092248.4e6a3e92@bbrezillon>
Date:   Tue, 12 Jun 2018 09:22:48 +0200
From:   Boris Brezillon <boris.brezillon@...tlin.com>
To:     NeilBrown <neil@...wn.name>
Cc:     Yogesh Gaur <yogeshnarayan.gaur@....com>,
        linux-mtd@...ts.infradead.org, boris.brezillon@...e-electrons.com,
        frieder.schrempf@...eet.de, computersforpeace@...il.com,
        david.wolfe@....com, han.xu@....com, festevam@...il.com,
        marek.vasut@...il.com, prabhakar.kushwaha@....com,
        linux-spi@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mtd: spi-nor: honour max_data_size for spi-nor writes

Hi Neil,

On Tue, 12 Jun 2018 08:05:13 +1000
NeilBrown <neil@...wn.name> wrote:

> On Mon, Jun 11 2018, Boris Brezillon wrote:
> >
> > Also, I'd prefer to have this patch split in 2:
> > 1/ one patch removing the check in spi_nor_write()
> > 2/ and the second patch removing the while() loop in m25p80_write()
> >
> > How about the following commit messages for those 2 patches:
> >
> > 1:
> > "
> > mtd: spi-nor: Support controllers with limited TX FIFO size
> >
> > Some SPI controllers can't write nor->page_size bytes in a single step
> > because their TX FIFO is too small.  
> 
> I no longer think this is good justification for changes to m25p80 or to
> anything outside the low-level SPI driver.  The size of the FIFO is not
> related to the maximum message size.
> 
> An SPI transaction involves:
>  - asserting chip-select
>  - sending/receiving a bunch of bits
>  - deasserting chip-select
> 
> That middle part of sending/receiving bits can be done:
>  - one bit at a time by bit-banging GPIOs
>  - 32 bytes at a time by filling a buffer, running the SPI engine, then
>     reading results out of the buffer (assuming a 32-byte buffer)
>  - with one single DMA operation if the SPI engine can DMA to/from
>    main memory
> 
> and there are probably other options.
> 
> If your SPI engine only handles (say) 32 bytes at a time, then call it
> repeatedly, while holding chip-select asserted the whole time.

Except you're not necessarily in control of the CS signal, and that's
most of the time the case with high-level (Q)SPI mem controllers like
the NXP/FSL QSPI controller (see the datasheet here if you want to check
[1]).

> The SPI
> clock is controlled by the host and in this case it will not have a
> stable frequency for the whole transaction (occasionally pauses), but
> that doesn't matter.  The slave just watches for the clock transitions
> and don't care when they come as long as they don't come too fast.
> 
> I recently modified the mt7621 spi driver (in drivers/staging) to work
> like this and got much better throughput when reading from spi-nor flash
> memory.  It handles writes of full pages (36 bytes at at time!) without
> problems.

Just because you managed to solve the problem in one driver does not
mean the problem does not exist for others. I read this datasheet [1]
several times and couldn't find a way to say 'I want to keep the CS
asserted between 2 transactions', so I think we really need this patch.

Regards,

Boris

[1]https://www.nxp.com/docs/en/reference-manual/VFXXXRM.pdf