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

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

[thor.thayer]

From: Thor Thayer <thor.thayer@linux.intel.com>

The current Cadence QSPI driver caused a kernel panic sporadically
when writing to QSPI. The problem was caused by writing more bytes
than needed because the QSPI operated on 4 bytes at a time.
<snip>
[   11.202044] Unable to handle kernel paging request at virtual address bffd3000
[   11.209254] pgd = e463054d
[   11.211948] [bffd3000] *pgd=2fffb811, *pte=00000000, *ppte=00000000
[   11.218202] Internal error: Oops: 7 [#1] SMP ARM
[   11.222797] Modules linked in:
[   11.225844] CPU: 1 PID: 1317 Comm: systemd-hwdb Not tainted 4.17.7-d0c45cd44a8f
[   11.235796] Hardware name: Altera SOCFPGA Arria10
[   11.240487] PC is at __raw_writesl+0x70/0xd4
[   11.244741] LR is at cqspi_write+0x1a0/0x2cc
</snip>
On a page boundary limit the number of bytes copied from the tx buffer
to remain within the page.

This patch uses a temporary buffer to hold the 4 bytes to write and then
copies only the bytes required from the tx buffer. A min() function is
used to limit the length to prevent buffer indexing overflows.

Reported-by: Adrian Amborzewicz <adrian.ambrozewicz@intel.com>
Signed-off-by: Thor Thayer <thor.thayer@linux.intel.com>
---
 drivers/mtd/spi-nor/cadence-quadspi.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c
index d846428ef038..3659d94b4d2f 100644
--- a/drivers/mtd/spi-nor/cadence-quadspi.c
+++ b/drivers/mtd/spi-nor/cadence-quadspi.c
@@ -644,9 +644,23 @@ static int cqspi_indirect_write_execute(struct spi_nor *nor, loff_t to_addr,
 		ndelay(cqspi->wr_delay);
 
 	while (remaining > 0) {
+		size_t write_words, mod_bytes;
+
 		write_bytes = remaining > page_size ? page_size : remaining;
-		iowrite32_rep(cqspi->ahb_base, txbuf,
-			      DIV_ROUND_UP(write_bytes, 4));
+		write_words = write_bytes / 4;
+		mod_bytes = write_bytes % 4;
+		/* Write 4 bytes at a time then single bytes. */
+		if (write_words) {
+			iowrite32_rep(cqspi->ahb_base, txbuf, write_words);
+			txbuf += (write_words * 4);
+		}
+		if (mod_bytes) {
+			unsigned int temp = 0xFFFFFFFF;
+
+			memcpy(&temp, txbuf, min(sizeof(temp), mod_bytes));
+			iowrite32(temp, cqspi->ahb_base);
+			txbuf += mod_bytes;
+		}
 
 		if (!wait_for_completion_timeout(&cqspi->transfer_complete,
 					msecs_to_jiffies(CQSPI_TIMEOUT_MS))) {
@@ -655,7 +669,6 @@ static int cqspi_indirect_write_execute(struct spi_nor *nor, loff_t to_addr,
 			goto failwr;
 		}
 
-		txbuf += write_bytes;
 		remaining -= write_bytes;
 
 		if (remaining > 0)
-- 
2.7.4

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

[Vignesh R]

Hi,

On 13/11/18 11:02 PM, thor.thayer@linux.intel.com wrote:
> From: Thor Thayer <thor.thayer@linux.intel.com>
> 
> The current Cadence QSPI driver caused a kernel panic sporadically
> when writing to QSPI. The problem was caused by writing more bytes
> than needed because the QSPI operated on 4 bytes at a time.
> <snip>
> [   11.202044] Unable to handle kernel paging request at virtual address bffd3000
> [   11.209254] pgd = e463054d
> [   11.211948] [bffd3000] *pgd=2fffb811, *pte=00000000, *ppte=00000000
> [   11.218202] Internal error: Oops: 7 [#1] SMP ARM
> [   11.222797] Modules linked in:
> [   11.225844] CPU: 1 PID: 1317 Comm: systemd-hwdb Not tainted 4.17.7-d0c45cd44a8f
> [   11.235796] Hardware name: Altera SOCFPGA Arria10
> [   11.240487] PC is at __raw_writesl+0x70/0xd4
> [   11.244741] LR is at cqspi_write+0x1a0/0x2cc
> </snip>
> On a page boundary limit the number of bytes copied from the tx buffer
> to remain within the page.
> 
> This patch uses a temporary buffer to hold the 4 bytes to write and then
> copies only the bytes required from the tx buffer. A min() function is
> used to limit the length to prevent buffer indexing overflows.
> 
> Reported-by: Adrian Amborzewicz <adrian.ambrozewicz@intel.com>
> Signed-off-by: Thor Thayer <thor.thayer@linux.intel.com>
> ---
>  drivers/mtd/spi-nor/cadence-quadspi.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c
> index d846428ef038..3659d94b4d2f 100644
> --- a/drivers/mtd/spi-nor/cadence-quadspi.c
> +++ b/drivers/mtd/spi-nor/cadence-quadspi.c
> @@ -644,9 +644,23 @@ static int cqspi_indirect_write_execute(struct spi_nor *nor, loff_t to_addr,
>  		ndelay(cqspi->wr_delay);
>  
>  	while (remaining > 0) {
> +		size_t write_words, mod_bytes;
> +
>  		write_bytes = remaining > page_size ? page_size : remaining;
> -		iowrite32_rep(cqspi->ahb_base, txbuf,
> -			      DIV_ROUND_UP(write_bytes, 4));
> +		write_words = write_bytes / 4;
> +		mod_bytes = write_bytes % 4;
> +		/* Write 4 bytes at a time then single bytes. */
> +		if (write_words) {
> +			iowrite32_rep(cqspi->ahb_base, txbuf, write_words);
> +			txbuf += (write_words * 4);
> +		}
> +		if (mod_bytes) {
> +			unsigned int temp = 0xFFFFFFFF;
> +
> +			memcpy(&temp, txbuf, min(sizeof(temp), mod_bytes));

Sorry, I dont understand why min() is required here since:
	mod_bytes = write_bytes % 4;
Doesn't that ensure mod_bytes < sizeof(temp)?

> +			iowrite32(temp, cqspi->ahb_base);
> +			txbuf += mod_bytes;
> +		}
>  
>  		if (!wait_for_completion_timeout(&cqspi->transfer_complete,
>  					msecs_to_jiffies(CQSPI_TIMEOUT_MS))) {
> @@ -655,7 +669,6 @@ static int cqspi_indirect_write_execute(struct spi_nor *nor, loff_t to_addr,
>  			goto failwr;
>  		}
>  
> -		txbuf += write_bytes;
>  		remaining -= write_bytes;
>  
>  		if (remaining > 0)
> 

-- 
Regards
Vignesh

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

[Thor Thayer]

Hi Vignesh,

On 11/14/18 6:00 AM, Vignesh R wrote:
> Hi,
> 
> On 13/11/18 11:02 PM, thor.thayer@linux.intel.com wrote:
>> From: Thor Thayer <thor.thayer@linux.intel.com>
>>
>> The current Cadence QSPI driver caused a kernel panic sporadically
>> when writing to QSPI. The problem was caused by writing more bytes
>> than needed because the QSPI operated on 4 bytes at a time.
>> <snip>
>> [   11.202044] Unable to handle kernel paging request at virtual address bffd3000
>> [   11.209254] pgd = e463054d
>> [   11.211948] [bffd3000] *pgd=2fffb811, *pte=00000000, *ppte=00000000
>> [   11.218202] Internal error: Oops: 7 [#1] SMP ARM
>> [   11.222797] Modules linked in:
>> [   11.225844] CPU: 1 PID: 1317 Comm: systemd-hwdb Not tainted 4.17.7-d0c45cd44a8f
>> [   11.235796] Hardware name: Altera SOCFPGA Arria10
>> [   11.240487] PC is at __raw_writesl+0x70/0xd4
>> [   11.244741] LR is at cqspi_write+0x1a0/0x2cc
>> </snip>
>> On a page boundary limit the number of bytes copied from the tx buffer
>> to remain within the page.
>>
>> This patch uses a temporary buffer to hold the 4 bytes to write and then
>> copies only the bytes required from the tx buffer. A min() function is
>> used to limit the length to prevent buffer indexing overflows.
>>
>> Reported-by: Adrian Amborzewicz <adrian.ambrozewicz@intel.com>
>> Signed-off-by: Thor Thayer <thor.thayer@linux.intel.com>
>> ---
>>   drivers/mtd/spi-nor/cadence-quadspi.c | 19 ++++++++++++++++---
>>   1 file changed, 16 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c
>> index d846428ef038..3659d94b4d2f 100644
>> --- a/drivers/mtd/spi-nor/cadence-quadspi.c
>> +++ b/drivers/mtd/spi-nor/cadence-quadspi.c
>> @@ -644,9 +644,23 @@ static int cqspi_indirect_write_execute(struct spi_nor *nor, loff_t to_addr,
>>   		ndelay(cqspi->wr_delay);
>>   
>>   	while (remaining > 0) {
>> +		size_t write_words, mod_bytes;
>> +
>>   		write_bytes = remaining > page_size ? page_size : remaining;
>> -		iowrite32_rep(cqspi->ahb_base, txbuf,
>> -			      DIV_ROUND_UP(write_bytes, 4));
>> +		write_words = write_bytes / 4;
>> +		mod_bytes = write_bytes % 4;
>> +		/* Write 4 bytes at a time then single bytes. */
>> +		if (write_words) {
>> +			iowrite32_rep(cqspi->ahb_base, txbuf, write_words);
>> +			txbuf += (write_words * 4);
>> +		}
>> +		if (mod_bytes) {
>> +			unsigned int temp = 0xFFFFFFFF;
>> +
>> +			memcpy(&temp, txbuf, min(sizeof(temp), mod_bytes));
> 
> Sorry, I dont understand why min() is required here since:
> 	mod_bytes = write_bytes % 4;
> Doesn't that ensure mod_bytes < sizeof(temp)?
> 

Yes, good point. I was being overly cautious with the memcpy buffers. I 
will re-spin and re-submit. Thanks!

>> +			iowrite32(temp, cqspi->ahb_base);
>> +			txbuf += mod_bytes;
>> +		}
>>   
>>   		if (!wait_for_completion_timeout(&cqspi->transfer_complete,
>>   					msecs_to_jiffies(CQSPI_TIMEOUT_MS))) {
>> @@ -655,7 +669,6 @@ static int cqspi_indirect_write_execute(struct spi_nor *nor, loff_t to_addr,
>>   			goto failwr;
>>   		}
>>   
>> -		txbuf += write_bytes;
>>   		remaining -= write_bytes;
>>   
>>   		if (remaining > 0)
>>
> 

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

[thor.thayer]

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, an 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.

Request testing of this patch on other platforms. This was tested
on the Intel Arria10 SoCFPGA DevKit.

Fixes: 0cf1725676a97fc8 ("mtd: spi-nor: cqspi: Fix build on arches missing readsl/writesl")
Signed-off-by: Thor Thayer <thor.thayer@linux.intel.com>
Cc: <stable@vger.kernel.org>
Reviewed-by: Marek Vasut <marek.vasut@gmail.com>
---
v2  Changes to only write dangling bytes at end of transfer since
      previous patch may have multiple dangling byte transfers.
    Remove write patch since no errors reported and write timeout
      needs more investigation.
v3  Add Fixes tag Cc-stable tag.
---
 drivers/mtd/spi-nor/cadence-quadspi.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c
index 2f3a4d4232b3..c3f7aaa5d18f 100644
--- a/drivers/mtd/spi-nor/cadence-quadspi.c
+++ b/drivers/mtd/spi-nor/cadence-quadspi.c
@@ -507,7 +507,9 @@ 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 mod_bytes = n_rx % 4;
 	unsigned int bytes_to_read = 0;
+	u8 *rxbuf_end = rxbuf + n_rx;
 	int ret = 0;
 
 	writel(from_addr, reg_base + CQSPI_REG_INDIRECTRDSTARTADDR);
@@ -536,11 +538,24 @@ static int cqspi_indirect_read_execute(struct spi_nor *nor, u8 *rxbuf,
 		}
 
 		while (bytes_to_read != 0) {
+			unsigned int word_remain = round_down(remaining, 4);
+
 			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));
+			bytes_to_read = round_down(bytes_to_read, 4);
+			/* Read 4 byte word chunks then single bytes */
+			if (bytes_to_read) {
+				ioread32_rep(ahb_base, rxbuf,
+					     (bytes_to_read / 4));
+			} else if (!word_remain && mod_bytes) {
+				unsigned int temp = ioread32(ahb_base);
+
+				bytes_to_read = mod_bytes;
+				memcpy(rxbuf, &temp, min((unsigned int)
+							 (rxbuf_end - rxbuf),
+							 bytes_to_read));
+			}
 			rxbuf += bytes_to_read;
 			remaining -= bytes_to_read;
 			bytes_to_read = cqspi_get_rd_sram_level(cqspi);
-- 
2.7.4

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

[Thor Thayer]

On 04/23/2018 12:22 PM, thor.thayer@linux.intel.com wrote:
> From: Thor Thayer <thor.thayer@linux.intel.com>
> 

<snip>
Please disregard - the patch version is incorrect. I will fix and resubmit.

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

[thor.thayer]

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;
+			}
 			remaining -= bytes_to_read;
 			bytes_to_read = cqspi_get_rd_sram_level(cqspi);
 		}
@@ -599,7 +612,7 @@ static int cqspi_indirect_write_execute(struct spi_nor *nor, loff_t to_addr,
 	struct cqspi_st *cqspi = f_pdata->cqspi;
 	void __iomem *reg_base = cqspi->iobase;
 	unsigned int remaining = n_tx;
-	unsigned int write_bytes;
+	unsigned int mod_bytes, write_bytes, write_words;
 	int ret;
 
 	writel(to_addr, reg_base + CQSPI_REG_INDIRECTWRSTARTADDR);
@@ -625,9 +638,20 @@ static int cqspi_indirect_write_execute(struct spi_nor *nor, loff_t to_addr,
 
 	while (remaining > 0) {
 		write_bytes = remaining > page_size ? page_size : remaining;
-		iowrite32_rep(cqspi->ahb_base, txbuf,
-			      DIV_ROUND_UP(write_bytes, 4));
+		write_words = write_bytes / 4;
+		mod_bytes = write_bytes % 4;
+		/* Write 4 bytes at a time then single bytes. */
+		if (write_words) {
+			iowrite32_rep(cqspi->ahb_base, txbuf, write_words);
+			txbuf += (write_words * 4);
+		}
+		if (mod_bytes) {
+			unsigned int temp = 0xFFFFFFFF;
 
+			memcpy(&temp, txbuf, min(sizeof(temp), mod_bytes));
+			iowrite32(temp, cqspi->ahb_base);
+			txbuf += mod_bytes;
+		}
 		ret = wait_for_completion_timeout(&cqspi->transfer_complete,
 						  msecs_to_jiffies
 						  (CQSPI_TIMEOUT_MS));
@@ -637,7 +661,6 @@ static int cqspi_indirect_write_execute(struct spi_nor *nor, loff_t to_addr,
 			goto failwr;
 		}
 
-		txbuf += write_bytes;
 		remaining -= write_bytes;
 
 		if (remaining > 0)
-- 
2.7.4

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

[Marek Vasut]

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 ?

-- 
Best regards,
Marek Vasut

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@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.

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 4880 bytes --]

Hi Thor,

I love your patch! Perhaps something to improve:

[auto build test WARNING on v4.16-rc4]
[also build test WARNING on next-20180319]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/thor-thayer-linux-intel-com/mtd-spi-nor-Fix-Cadence-QSPI-page-fault-kernel-panic/20180320-133857
config: alpha-allmodconfig (attached as .config)
compiler: alpha-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=alpha 

All warnings (new ones prefixed by >>):

   In file included from include/linux/clk.h:16:0,
                    from drivers/mtd/spi-nor/cadence-quadspi.c:18:
   drivers/mtd/spi-nor/cadence-quadspi.c: In function 'cqspi_indirect_write_execute':
   include/linux/kernel.h:793:16: warning: comparison of distinct pointer types lacks a cast
     (void) (&min1 == &min2);   \
                   ^
   include/linux/kernel.h:802:2: note: in expansion of macro '__min'
     __min(typeof(x), typeof(y),   \
     ^~~~~
>> drivers/mtd/spi-nor/cadence-quadspi.c:651:25: note: in expansion of macro 'min'
       memcpy(&temp, txbuf, min(sizeof(temp), mod_bytes));
                            ^~~

vim +/min +651 drivers/mtd/spi-nor/cadence-quadspi.c

   606	
   607	static int cqspi_indirect_write_execute(struct spi_nor *nor, loff_t to_addr,
   608						const u8 *txbuf, const size_t n_tx)
   609	{
   610		const unsigned int page_size = nor->page_size;
   611		struct cqspi_flash_pdata *f_pdata = nor->priv;
   612		struct cqspi_st *cqspi = f_pdata->cqspi;
   613		void __iomem *reg_base = cqspi->iobase;
   614		unsigned int remaining = n_tx;
   615		unsigned int mod_bytes, write_bytes, write_words;
   616		int ret;
   617	
   618		writel(to_addr, reg_base + CQSPI_REG_INDIRECTWRSTARTADDR);
   619		writel(remaining, reg_base + CQSPI_REG_INDIRECTWRBYTES);
   620	
   621		/* Clear all interrupts. */
   622		writel(CQSPI_IRQ_STATUS_MASK, reg_base + CQSPI_REG_IRQSTATUS);
   623	
   624		writel(CQSPI_IRQ_MASK_WR, reg_base + CQSPI_REG_IRQMASK);
   625	
   626		reinit_completion(&cqspi->transfer_complete);
   627		writel(CQSPI_REG_INDIRECTWR_START_MASK,
   628		       reg_base + CQSPI_REG_INDIRECTWR);
   629		/*
   630		 * As per 66AK2G02 TRM SPRUHY8F section 11.15.5.3 Indirect Access
   631		 * Controller programming sequence, couple of cycles of
   632		 * QSPI_REF_CLK delay is required for the above bit to
   633		 * be internally synchronized by the QSPI module. Provide 5
   634		 * cycles of delay.
   635		 */
   636		if (cqspi->wr_delay)
   637			ndelay(cqspi->wr_delay);
   638	
   639		while (remaining > 0) {
   640			write_bytes = remaining > page_size ? page_size : remaining;
   641			write_words = write_bytes / 4;
   642			mod_bytes = write_bytes % 4;
   643			/* Write 4 bytes at a time then single bytes. */
   644			if (write_words) {
   645				iowrite32_rep(cqspi->ahb_base, txbuf, write_words);
   646				txbuf += (write_words * 4);
   647			}
   648			if (mod_bytes) {
   649				unsigned int temp = 0xFFFFFFFF;
   650	
 > 651				memcpy(&temp, txbuf, min(sizeof(temp), mod_bytes));
   652				iowrite32(temp, cqspi->ahb_base);
   653				txbuf += mod_bytes;
   654			}
   655			ret = wait_for_completion_timeout(&cqspi->transfer_complete,
   656							  msecs_to_jiffies
   657							  (CQSPI_TIMEOUT_MS));
   658			if (!ret) {
   659				dev_err(nor->dev, "Indirect write timeout\n");
   660				ret = -ETIMEDOUT;
   661				goto failwr;
   662			}
   663	
   664			remaining -= write_bytes;
   665	
   666			if (remaining > 0)
   667				reinit_completion(&cqspi->transfer_complete);
   668		}
   669	
   670		/* Check indirect done status */
   671		ret = cqspi_wait_for_bit(reg_base + CQSPI_REG_INDIRECTWR,
   672					 CQSPI_REG_INDIRECTWR_DONE_MASK, 0);
   673		if (ret) {
   674			dev_err(nor->dev,
   675				"Indirect write completion error (%i)\n", ret);
   676			goto failwr;
   677		}
   678	
   679		/* Disable interrupt. */
   680		writel(0, reg_base + CQSPI_REG_IRQMASK);
   681	
   682		/* Clear indirect completion status */
   683		writel(CQSPI_REG_INDIRECTWR_DONE_MASK, reg_base + CQSPI_REG_INDIRECTWR);
   684	
   685		cqspi_wait_idle(cqspi);
   686	
   687		return 0;
   688	
   689	failwr:
   690		/* Disable interrupt. */
   691		writel(0, reg_base + CQSPI_REG_IRQMASK);
   692	
   693		/* Cancel the indirect write */
   694		writel(CQSPI_REG_INDIRECTWR_CANCEL_MASK,
   695		       reg_base + CQSPI_REG_INDIRECTWR);
   696		return ret;
   697	}
   698	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 52486 bytes --]

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

[kbuild test robot]

Hi Thor,

I love your patch! Perhaps something to improve:

[auto build test WARNING on v4.16-rc4]
[also build test WARNING on next-20180319]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/thor-thayer-linux-intel-com/mtd-spi-nor-Fix-Cadence-QSPI-page-fault-kernel-panic/20180320-133857
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

>> drivers/mtd/spi-nor/cadence-quadspi.c:651:25: sparse: incompatible types in comparison expression (different type sizes)
   In file included from include/linux/clk.h:16:0,
                    from drivers/mtd/spi-nor/cadence-quadspi.c:18:
   drivers/mtd/spi-nor/cadence-quadspi.c: In function 'cqspi_indirect_write_execute':
   include/linux/kernel.h:793:16: warning: comparison of distinct pointer types lacks a cast
     (void) (&min1 == &min2);   7-                ^
   include/linux/kernel.h:802:2: note: in expansion of macro '__min'
     __min(typeof(x), typeof(y),   10-  ^~~~~
   drivers/mtd/spi-nor/cadence-quadspi.c:651:25: note: in expansion of macro 'min'
       memcpy(&temp, txbuf, min(sizeof(temp), mod_bytes));
                            ^~~

vim +651 drivers/mtd/spi-nor/cadence-quadspi.c

   606	
   607	static int cqspi_indirect_write_execute(struct spi_nor *nor, loff_t to_addr,
   608						const u8 *txbuf, const size_t n_tx)
   609	{
   610		const unsigned int page_size = nor->page_size;
   611		struct cqspi_flash_pdata *f_pdata = nor->priv;
   612		struct cqspi_st *cqspi = f_pdata->cqspi;
   613		void __iomem *reg_base = cqspi->iobase;
   614		unsigned int remaining = n_tx;
   615		unsigned int mod_bytes, write_bytes, write_words;
   616		int ret;
   617	
   618		writel(to_addr, reg_base + CQSPI_REG_INDIRECTWRSTARTADDR);
   619		writel(remaining, reg_base + CQSPI_REG_INDIRECTWRBYTES);
   620	
   621		/* Clear all interrupts. */
   622		writel(CQSPI_IRQ_STATUS_MASK, reg_base + CQSPI_REG_IRQSTATUS);
   623	
   624		writel(CQSPI_IRQ_MASK_WR, reg_base + CQSPI_REG_IRQMASK);
   625	
   626		reinit_completion(&cqspi->transfer_complete);
   627		writel(CQSPI_REG_INDIRECTWR_START_MASK,
   628		       reg_base + CQSPI_REG_INDIRECTWR);
   629		/*
   630		 * As per 66AK2G02 TRM SPRUHY8F section 11.15.5.3 Indirect Access
   631		 * Controller programming sequence, couple of cycles of
   632		 * QSPI_REF_CLK delay is required for the above bit to
   633		 * be internally synchronized by the QSPI module. Provide 5
   634		 * cycles of delay.
   635		 */
   636		if (cqspi->wr_delay)
   637			ndelay(cqspi->wr_delay);
   638	
   639		while (remaining > 0) {
   640			write_bytes = remaining > page_size ? page_size : remaining;
   641			write_words = write_bytes / 4;
   642			mod_bytes = write_bytes % 4;
   643			/* Write 4 bytes at a time then single bytes. */
   644			if (write_words) {
   645				iowrite32_rep(cqspi->ahb_base, txbuf, write_words);
   646				txbuf += (write_words * 4);
   647			}
   648			if (mod_bytes) {
   649				unsigned int temp = 0xFFFFFFFF;
   650	
 > 651				memcpy(&temp, txbuf, min(sizeof(temp), mod_bytes));
   652				iowrite32(temp, cqspi->ahb_base);
   653				txbuf += mod_bytes;
   654			}
   655			ret = wait_for_completion_timeout(&cqspi->transfer_complete,
   656							  msecs_to_jiffies
   657							  (CQSPI_TIMEOUT_MS));
   658			if (!ret) {
   659				dev_err(nor->dev, "Indirect write timeout\n");
   660				ret = -ETIMEDOUT;
   661				goto failwr;
   662			}
   663	
   664			remaining -= write_bytes;
   665	
   666			if (remaining > 0)
   667				reinit_completion(&cqspi->transfer_complete);
   668		}
   669	
   670		/* Check indirect done status */
   671		ret = cqspi_wait_for_bit(reg_base + CQSPI_REG_INDIRECTWR,
   672					 CQSPI_REG_INDIRECTWR_DONE_MASK, 0);
   673		if (ret) {
   674			dev_err(nor->dev,
   675				"Indirect write completion error (%i)\n", ret);
   676			goto failwr;
   677		}
   678	
   679		/* Disable interrupt. */
   680		writel(0, reg_base + CQSPI_REG_IRQMASK);
   681	
   682		/* Clear indirect completion status */
   683		writel(CQSPI_REG_INDIRECTWR_DONE_MASK, reg_base + CQSPI_REG_INDIRECTWR);
   684	
   685		cqspi_wait_idle(cqspi);
   686	
   687		return 0;
   688	
   689	failwr:
   690		/* Disable interrupt. */
   691		writel(0, reg_base + CQSPI_REG_IRQMASK);
   692	
   693		/* Cancel the indirect write */
   694		writel(CQSPI_REG_INDIRECTWR_CANCEL_MASK,
   695		       reg_base + CQSPI_REG_INDIRECTWR);
   696		return ret;
   697	}
   698	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation