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

Thor Thayer
2018-03-21T15:12:40Z

Hi Marek,

On 03/19/2018 05:33 PM, Marek Vasut wrote:
> On 03/19/2018 07:45 PM, thor.thayer@linux.intel.com wrote:
>> From: Thor Thayer <thor.thayer@linux.intel.com>
>>
>> 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@linux.intel.com>
>> ---
>> 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.