From: Thor Thayer <thor.thayer@linux.intel.com>
To: Marek Vasut <marek.vasut@gmail.com>,
cyrille.pitchen@wedev4u.fr, dwmw2@infradead.org,
computersforpeace@gmail.com, boris.brezillon@bootlin.com,
richard@nod.at
Cc: linux-mtd@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] mtd: spi-nor: Fix Cadence QSPI page fault kernel panic
Date: Wed, 21 Mar 2018 10:15:14 -0500 [thread overview]
Message-ID: <f95ed3e9-00cb-3d7a-f25b-7915ac607ee1@linux.intel.com> (raw)
In-Reply-To: <cfadfbab-70bd-cb1c-c9a6-446cefd87e9e@gmail.com>
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.
next prev parent reply other threads:[~2018-03-21 15:15 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-03-19 18:45 [PATCH] mtd: spi-nor: Fix Cadence QSPI page fault kernel panic thor.thayer
2018-03-19 22:33 ` Marek Vasut
2018-03-21 15:15 ` Thor Thayer [this message]
2018-03-20 8:15 ` kbuild test robot
2018-03-20 10:31 ` kbuild test robot
2018-11-13 17:32 thor.thayer
2018-11-14 12:00 ` Vignesh R
2018-11-14 15:25 ` Thor Thayer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f95ed3e9-00cb-3d7a-f25b-7915ac607ee1@linux.intel.com \
--to=thor.thayer@linux.intel.com \
--cc=boris.brezillon@bootlin.com \
--cc=computersforpeace@gmail.com \
--cc=cyrille.pitchen@wedev4u.fr \
--cc=dwmw2@infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mtd@lists.infradead.org \
--cc=marek.vasut@gmail.com \
--cc=richard@nod.at \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).