Re: [PATCH] mtd: spi-nor: Fix Cadence QSPI page fault kernel panic

[Thor Thayer]

Hi Marek,

On 03/19/2018 05:33 PM, Marek Vasut wrote:
> On 03/19/2018 07:45 PM, thor.thayer@xxxxxxxxxxxxxxx wrote:
> > From: Thor Thayer <thor.thayer@xxxxxxxxxxxxxxx>
> >
> > The current Cadence QSPI driver caused a kernel panic when loading
> > a Root Filesystem from QSPI. The problem was caused by reading more
> > bytes than needed because the QSPI operated on 4 bytes at a time.
> > <snip>
> > [    7.947754] spi_nor_read[1048]:from 0x037cad74, len 1 [bfe07fff]
> > [    7.956247] cqspi_read[910]:offset 0x58502516, buffer=bfe07fff
> > [    7.956247]
> > [    7.966046] Unable to handle kernel paging request at virtual
> > address bfe08002
> > [    7.973239] pgd = eebfc000
> > [    7.975931] [bfe08002] *pgd=2fffb811, *pte=00000000, *ppte=00000000
> > </snip>
> > Notice above how only 1 byte needed to be read but by reading 4 bytes
> > into the end of a mapped page, a unrecoverable page fault occurred.
> >
> > This patch uses a temporary buffer to hold the 4 bytes read and then
> > copies only the bytes required into the buffer. A min() function is
> > used to limit the length to prevent buffer overflows.
> >
> > Similar changes were made for the write routine.
> >
> > Signed-off-by: Thor Thayer <thor.thayer@xxxxxxxxxxxxxxx>
> > ---
> >   drivers/mtd/spi-nor/cadence-quadspi.c | 39 ++++++++++++++++++++++++++++-------
> >   1 file changed, 31 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c
> > index 4b8e9183489a..b22ed982f896 100644
> > --- a/drivers/mtd/spi-nor/cadence-quadspi.c
> > +++ b/drivers/mtd/spi-nor/cadence-quadspi.c
> > @@ -501,7 +501,8 @@ static int cqspi_indirect_read_execute(struct spi_nor *nor, u8 *rxbuf,
> >   	void __iomem *reg_base = cqspi->iobase;
> >   	void __iomem *ahb_base = cqspi->ahb_base;
> >   	unsigned int remaining = n_rx;
> > -	unsigned int bytes_to_read = 0;
> > +	unsigned int mod_bytes, words_to_read, bytes_to_read = 0;
> > +	u8 *rxbuf_end = rxbuf + n_rx;
> >   	int ret = 0;
> >   
> >   	writel(from_addr, reg_base + CQSPI_REG_INDIRECTRDSTARTADDR);
> > @@ -533,9 +534,21 @@ static int cqspi_indirect_read_execute(struct spi_nor *nor, u8 *rxbuf,
> >   			bytes_to_read *= cqspi->fifo_width;
> >   			bytes_to_read = bytes_to_read > remaining ?
> >   					remaining : bytes_to_read;
> > -			ioread32_rep(ahb_base, rxbuf,
> > -				     DIV_ROUND_UP(bytes_to_read, 4));
> > -			rxbuf += bytes_to_read;
> > +			/* Read 4 byte chunks before using single byte mode */
> > +			words_to_read = bytes_to_read / 4;
> > +			mod_bytes = bytes_to_read % 4;
> > +			if (words_to_read) {
> > +				ioread32_rep(ahb_base, rxbuf, words_to_read);
> > +				rxbuf += (words_to_read * 4);
> > +			}
> > +			if (mod_bytes) {
> > +				unsigned int temp = ioread32(ahb_base);
> > +
> > +				memcpy(rxbuf, &temp, min((unsigned int)
> > +							 (rxbuf_end - rxbuf),
> > +							 mod_bytes));
> > +				rxbuf += mod_bytes;
> > +			}
>
> Wouldn't it make more sense to read in 4 byte increments all the time
> except for the one last read instead ? This code above where you always
> check for trailing bytes can make the next read cycle do ioreaad32 into
> unaligned memory address and thus cause slowdown. (consider the example
> where the controller first reports it has 5 bytes ready, then reports it
> has 7 bytes ready. If you read 4 bytes in the first cycle, wait a bit
> and then check how much data the controller has in the next cycle, it
> will be 8 bytes, all nicely aligned).
>
> Does it make sense ?
Ahh yes, Thanks for the example - I see your point. I'll look into that 
change. Thanks.