[PATCH 0/4] mtd: rawnand: Fix sequential reads

[PATCH 0/4] mtd: rawnand: Fix sequential reads

[Miquel Raynal]

After a number of reports and hours of investigation, I could finally
identify the main issue that likely lead to all these issues, which is
addressed in the second patch. The three other patches are also flagged
for backporting because I think it is best to apply them as well as they
are supposed to prevent a few corner cases from happening.

Please test this series and let me know if you encounter other errors.

Thanks,
Miquèl

Miquel Raynal (4):
  mtd: rawnand: Prevent crossing LUN boundaries during sequential reads
  mtd: rawnand: Fix core interference with sequential reads
  mtd: rawnand: Prevent sequential reads with on-die ECC engines
  mtd: rawnand: Clarify conditions to enable continuous reads

 drivers/mtd/nand/raw/nand_base.c | 89 +++++++++++++++++++++++++++-----
 include/linux/mtd/rawnand.h      |  2 +
 2 files changed, 77 insertions(+), 14 deletions(-)

-- 
2.34.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 1/4] mtd: rawnand: Prevent crossing LUN boundaries during sequential reads

[Miquel Raynal]

The ONFI specification states that devices do not need to support
sequential reads across LUN boundaries. In order to prevent such event
from happening and possibly failing, let's introduce the concept of
"pause" in the sequential read to handle these cases. The first/last
pages remain the same but any time we cross a LUN boundary we will end
and restart (if relevant) the sequential read operation.

Cc: stable@vger.kernel.org
Fixes: 003fe4b9545b ("mtd: rawnand: Support for sequential cache reads")
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/nand/raw/nand_base.c | 43 +++++++++++++++++++++++++++-----
 include/linux/mtd/rawnand.h      |  2 ++
 2 files changed, 39 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 9e24bedffd89..04e80ace4182 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -1207,6 +1207,23 @@ static int nand_lp_exec_read_page_op(struct nand_chip *chip, unsigned int page,
 	return nand_exec_op(chip, &op);
 }
 
+static void rawnand_cap_cont_reads(struct nand_chip *chip)
+{
+	struct nand_memory_organization *memorg;
+	unsigned int pages_per_lun, first_lun, last_lun;
+
+	memorg = nanddev_get_memorg(&chip->base);
+	pages_per_lun = memorg->pages_per_eraseblock * memorg->eraseblocks_per_lun;
+	first_lun = chip->cont_read.first_page / pages_per_lun;
+	last_lun = chip->cont_read.last_page / pages_per_lun;
+
+	/* Prevent sequential cache reads across LUN boundaries */
+	if (first_lun != last_lun)
+		chip->cont_read.pause_page = first_lun * pages_per_lun + pages_per_lun - 1;
+	else
+		chip->cont_read.pause_page = chip->cont_read.last_page;
+}
+
 static int nand_lp_exec_cont_read_page_op(struct nand_chip *chip, unsigned int page,
 					  unsigned int offset_in_page, void *buf,
 					  unsigned int len, bool check_only)
@@ -1225,7 +1242,7 @@ static int nand_lp_exec_cont_read_page_op(struct nand_chip *chip, unsigned int p
 		NAND_OP_DATA_IN(len, buf, 0),
 	};
 	struct nand_op_instr cont_instrs[] = {
-		NAND_OP_CMD(page == chip->cont_read.last_page ?
+		NAND_OP_CMD(page == chip->cont_read.pause_page ?
 			    NAND_CMD_READCACHEEND : NAND_CMD_READCACHESEQ,
 			    NAND_COMMON_TIMING_NS(conf, tWB_max)),
 		NAND_OP_WAIT_RDY(NAND_COMMON_TIMING_MS(conf, tR_max),
@@ -1262,16 +1279,29 @@ static int nand_lp_exec_cont_read_page_op(struct nand_chip *chip, unsigned int p
 	}
 
 	if (page == chip->cont_read.first_page)
-		return nand_exec_op(chip, &start_op);
+		ret = nand_exec_op(chip, &start_op);
 	else
-		return nand_exec_op(chip, &cont_op);
+		ret = nand_exec_op(chip, &cont_op);
+	if (ret)
+		return ret;
+
+	if (!chip->cont_read.ongoing)
+		return 0;
+
+	if (page == chip->cont_read.pause_page &&
+	    page != chip->cont_read.last_page) {
+		chip->cont_read.first_page = chip->cont_read.pause_page + 1;
+		rawnand_cap_cont_reads(chip);
+	} else if (page == chip->cont_read.last_page) {
+		chip->cont_read.ongoing = false;
+	}
+
+	return 0;
 }
 
 static bool rawnand_cont_read_ongoing(struct nand_chip *chip, unsigned int page)
 {
-	return chip->cont_read.ongoing &&
-		page >= chip->cont_read.first_page &&
-		page <= chip->cont_read.last_page;
+	return chip->cont_read.ongoing && page >= chip->cont_read.first_page;
 }
 
 /**
@@ -3445,6 +3475,7 @@ static void rawnand_enable_cont_reads(struct nand_chip *chip, unsigned int page,
 	if (col)
 		chip->cont_read.first_page++;
 	chip->cont_read.last_page = page + ((readlen >> chip->page_shift) & chip->pagemask);
+	rawnand_cap_cont_reads(chip);
 }
 
 /**
diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
index c29ace15a053..9d0fc5109af6 100644
--- a/include/linux/mtd/rawnand.h
+++ b/include/linux/mtd/rawnand.h
@@ -1265,6 +1265,7 @@ struct nand_secure_region {
  * @cont_read: Sequential page read internals
  * @cont_read.ongoing: Whether a continuous read is ongoing or not
  * @cont_read.first_page: Start of the continuous read operation
+ * @cont_read.pause_page: End of the current sequential cache read operation
  * @cont_read.last_page: End of the continuous read operation
  * @controller: The hardware controller	structure which is shared among multiple
  *              independent devices
@@ -1321,6 +1322,7 @@ struct nand_chip {
 	struct {
 		bool ongoing;
 		unsigned int first_page;
+		unsigned int pause_page;
 		unsigned int last_page;
 	} cont_read;
 
-- 
2.34.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/4] mtd: rawnand: Prevent crossing LUN boundaries during sequential reads

[Miquel Raynal]

On Fri, 2023-12-15 at 12:32:05 UTC, Miquel Raynal wrote:
> The ONFI specification states that devices do not need to support
> sequential reads across LUN boundaries. In order to prevent such event
> from happening and possibly failing, let's introduce the concept of
> "pause" in the sequential read to handle these cases. The first/last
> pages remain the same but any time we cross a LUN boundary we will end
> and restart (if relevant) the sequential read operation.
> 
> Cc: stable@vger.kernel.org
> Fixes: 003fe4b9545b ("mtd: rawnand: Support for sequential cache reads")
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next.

Miquel

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 2/4] mtd: rawnand: Fix core interference with sequential reads

[Miquel Raynal]

A couple of reports pointed at some strange failures happening a bit
randomly since the introduction of sequential page reads support. After
investigation it turned out the most likely reason for these issues was
the fact that sometimes a (longer) read might happen, starting at the
same page that was read previously. This is optimized by the raw NAND
core, by not sending the READ_PAGE command to the NAND device and just
reading out the data in a local cache. When this page is also flagged as
being the starting point for a sequential read, it means the page right
next will be accessed without the right instructions. The NAND chip will
be confused and will not output correct data. In order to avoid such
situation from happening anymore, we can however handle this case with a
bit of additional logic, to postpone the initialization of the read
sequence by one page.

Reported-by: Alexander Shiyan <eagle.alexander923@gmail.com>
Closes: https://lore.kernel.org/linux-mtd/CAP1tNvS=NVAm-vfvYWbc3k9Cx9YxMc2uZZkmXk8h1NhGX877Zg@mail.gmail.com/
Reported-by: Måns Rullgård <mans@mansr.com>
Closes: https://lore.kernel.org/linux-mtd/yw1xfs6j4k6q.fsf@mansr.com/
Reported-by: Martin Hundebøll <martin@geanix.com>
Closes: https://lore.kernel.org/linux-mtd/9d0c42fcde79bfedfe5b05d6a4e9fdef71d3dd52.camel@geanix.com/
Fixes: 003fe4b9545b ("mtd: rawnand: Support for sequential cache reads")
Cc: stable@vger.kernel.org
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/nand/raw/nand_base.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 04e80ace4182..1b0a984d181d 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -3478,6 +3478,18 @@ static void rawnand_enable_cont_reads(struct nand_chip *chip, unsigned int page,
 	rawnand_cap_cont_reads(chip);
 }
 
+static void rawnand_cont_read_skip_first_page(struct nand_chip *chip, unsigned int page)
+{
+	if (!chip->cont_read.ongoing || page != chip->cont_read.first_page)
+		return;
+
+	chip->cont_read.first_page++;
+	if (chip->cont_read.first_page == chip->cont_read.pause_page)
+		chip->cont_read.first_page++;
+	if (chip->cont_read.first_page >= chip->cont_read.last_page)
+		chip->cont_read.ongoing = false;
+}
+
 /**
  * nand_setup_read_retry - [INTERN] Set the READ RETRY mode
  * @chip: NAND chip object
@@ -3652,6 +3664,8 @@ static int nand_do_read_ops(struct nand_chip *chip, loff_t from,
 			buf += bytes;
 			max_bitflips = max_t(unsigned int, max_bitflips,
 					     chip->pagecache.bitflips);
+
+			rawnand_cont_read_skip_first_page(chip, page);
 		}
 
 		readlen -= bytes;
-- 
2.34.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/4] mtd: rawnand: Fix core interference with sequential reads

[Miquel Raynal]

On Fri, 2023-12-15 at 12:32:06 UTC, Miquel Raynal wrote:
> A couple of reports pointed at some strange failures happening a bit
> randomly since the introduction of sequential page reads support. After
> investigation it turned out the most likely reason for these issues was
> the fact that sometimes a (longer) read might happen, starting at the
> same page that was read previously. This is optimized by the raw NAND
> core, by not sending the READ_PAGE command to the NAND device and just
> reading out the data in a local cache. When this page is also flagged as
> being the starting point for a sequential read, it means the page right
> next will be accessed without the right instructions. The NAND chip will
> be confused and will not output correct data. In order to avoid such
> situation from happening anymore, we can however handle this case with a
> bit of additional logic, to postpone the initialization of the read
> sequence by one page.
> 
> Reported-by: Alexander Shiyan <eagle.alexander923@gmail.com>
> Closes: https://lore.kernel.org/linux-mtd/CAP1tNvS=NVAm-vfvYWbc3k9Cx9YxMc2uZZkmXk8h1NhGX877Zg@mail.gmail.com/
> Reported-by: Måns Rullgård <mans@mansr.com>
> Closes: https://lore.kernel.org/linux-mtd/yw1xfs6j4k6q.fsf@mansr.com/
> Reported-by: Martin Hundebøll <martin@geanix.com>
> Closes: https://lore.kernel.org/linux-mtd/9d0c42fcde79bfedfe5b05d6a4e9fdef71d3dd52.camel@geanix.com/
> Fixes: 003fe4b9545b ("mtd: rawnand: Support for sequential cache reads")
> Cc: stable@vger.kernel.org
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next.

Miquel

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 3/4] mtd: rawnand: Prevent sequential reads with on-die ECC engines

[Miquel Raynal]

Some devices support sequential reads when using the on-die ECC engines,
some others do not. It is a bit hard to know which ones will break other
than experimentally, so in order to avoid such a difficult and painful
task, let's just pretend all devices should avoid using this
optimization when configured like this.

Cc: stable@vger.kernel.org
Fixes: 003fe4b9545b ("mtd: rawnand: Support for sequential cache reads")
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/nand/raw/nand_base.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 1b0a984d181d..139fdf3e58c0 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -5170,6 +5170,14 @@ static void rawnand_late_check_supported_ops(struct nand_chip *chip)
 	/* The supported_op fields should not be set by individual drivers */
 	WARN_ON_ONCE(chip->controller->supported_op.cont_read);
 
+	/*
+	 * Too many devices do not support sequential cached reads with on-die
+	 * ECC correction enabled, so in this case refuse to perform the
+	 * automation.
+	 */
+	if (chip->ecc.engine_type == NAND_ECC_ENGINE_TYPE_ON_DIE)
+		return;
+
 	if (!nand_has_exec_op(chip))
 		return;
 
-- 
2.34.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 3/4] mtd: rawnand: Prevent sequential reads with on-die ECC engines

[Miquel Raynal]

On Fri, 2023-12-15 at 12:32:07 UTC, Miquel Raynal wrote:
> Some devices support sequential reads when using the on-die ECC engines,
> some others do not. It is a bit hard to know which ones will break other
> than experimentally, so in order to avoid such a difficult and painful
> task, let's just pretend all devices should avoid using this
> optimization when configured like this.
> 
> Cc: stable@vger.kernel.org
> Fixes: 003fe4b9545b ("mtd: rawnand: Support for sequential cache reads")
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next.

Miquel

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 4/4] mtd: rawnand: Clarify conditions to enable continuous reads

[Miquel Raynal]

The current logic is probably fine but is a bit convoluted. Plus, we
don't want partial pages to be part of the sequential operation just in
case the core would optimize the page read with a subpage read (which
would break the sequence). This may happen on the first and last page
only, so if the start offset or the end offset is not aligned with a
page boundary, better avoid them to prevent any risk.

Cc: stable@vger.kernel.org
Fixes: 003fe4b9545b ("mtd: rawnand: Support for sequential cache reads")
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/nand/raw/nand_base.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 139fdf3e58c0..bbdcfbe643f3 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -3460,21 +3460,29 @@ static void rawnand_enable_cont_reads(struct nand_chip *chip, unsigned int page,
 				      u32 readlen, int col)
 {
 	struct mtd_info *mtd = nand_to_mtd(chip);
+	unsigned int end_page, end_col;
+
+	chip->cont_read.ongoing = false;
 
 	if (!chip->controller->supported_op.cont_read)
 		return;
 
-	if ((col && col + readlen < (3 * mtd->writesize)) ||
-	    (!col && readlen < (2 * mtd->writesize))) {
-		chip->cont_read.ongoing = false;
-		return;
-	}
+	end_page = DIV_ROUND_UP(col + readlen, mtd->writesize);
+	end_col = (col + readlen) % mtd->writesize;
 
-	chip->cont_read.ongoing = true;
-	chip->cont_read.first_page = page;
 	if (col)
-		chip->cont_read.first_page++;
-	chip->cont_read.last_page = page + ((readlen >> chip->page_shift) & chip->pagemask);
+		page++;
+
+	if (end_col && end_page)
+		end_page--;
+
+	if (page + 1 > end_page)
+		return;
+
+	chip->cont_read.first_page = page;
+	chip->cont_read.last_page = end_page;
+	chip->cont_read.ongoing = true;
+
 	rawnand_cap_cont_reads(chip);
 }
 
-- 
2.34.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 4/4] mtd: rawnand: Clarify conditions to enable continuous reads

[Miquel Raynal]

On Fri, 2023-12-15 at 12:32:08 UTC, Miquel Raynal wrote:
> The current logic is probably fine but is a bit convoluted. Plus, we
> don't want partial pages to be part of the sequential operation just in
> case the core would optimize the page read with a subpage read (which
> would break the sequence). This may happen on the first and last page
> only, so if the start offset or the end offset is not aligned with a
> page boundary, better avoid them to prevent any risk.
> 
> Cc: stable@vger.kernel.org
> Fixes: 003fe4b9545b ("mtd: rawnand: Support for sequential cache reads")
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next.

Miquel

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 4/4] mtd: rawnand: Clarify conditions to enable continuous reads

[Christophe Kerello]

Hi Miquel,

I am testing last nand/next branch with the MP1 board, and i get an 
issue since this patch was applied.

When I read the SLC NAND using nandump tool (reading page 0 and page 1), 
the OOB is not displayed at expected. For page 1, oob is displayed when 
for page 0 the first data of the page are displayed.

The nanddump command used is: nanddump -c -o -l 0x2000 /dev/mtd9

Page 0:
   OOB Data: 7f 45 4c 46 01 01 01 00 00 00 00 00 00 00 00 00 
|.ELF............|
   OOB Data: 03 00 28 00 01 00 00 00 a4 03 00 00 34 00 00 00 
|..(.........4...|
   OOB Data: 7c 11 00 00 00 04 00 05 34 00 20 00 06 00 28 00 
||.......4. ...(.|
   OOB Data: 1b 00 1a 00 01 00 00 00 00 00 00 00 00 00 00 00 
|................|
   OOB Data: 00 00 00 00 10 05 00 00 10 05 00 00 05 00 00 00 
|................|
   OOB Data: 00 00 01 00 01 00 00 00 e8 0e 00 00 e8 0e 01 00 
|................|
   OOB Data: e8 0e 01 00 44 01 00 00 48 01 00 00 06 00 00 00 
|....D...H.......|
   OOB Data: 00 00 01 00 02 00 00 00 f0 0e 00 00 f0 0e 01 00 
|................|
   OOB Data: f0 0e 01 00 10 01 00 00 10 01 00 00 06 00 00 00 
|................|
   OOB Data: 04 00 00 00 04 00 00 00 f4 00 00 00 f4 00 00 00 
|................|
   OOB Data: f4 00 00 00 44 00 00 00 44 00 00 00 04 00 00 00 
|....D...D.......|
   OOB Data: 04 00 00 00 51 e5 74 64 00 00 00 00 00 00 00 00 
|....Q.td........|
   OOB Data: 00 00 00 00 00 00 00 00 00 00 00 00 06 00 00 00 
|................|
   OOB Data: 10 00 00 00 52 e5 74 64 e8 0e 00 00 e8 0e 01 00 
|....R.td........|

Page 1:
   OOB Data: ff ff 94 25 8c 3c c7 44 e7 c0 b7 b0 92 5e 50 fb 
|...%.<.D.....^P.|
   OOB Data: 80 ca a3 de e2 73 b4 4e 58 39 fe b4 85 76 65 31 
|.....s.NX9...ve1|
   OOB Data: 48 86 91 f3 58 0b 59 df 2c 08 75 8b 6f 48 36 a6 
|H...X.Y.,.u.oH6.|
   OOB Data: bc 16 61 58 db 52 08 75 8b 6f 48 36 a6 bc 16 61 
|..aX.R.u.oH6...a|
   OOB Data: 58 db 52 08 75 8b 6f 48 36 a6 bc 16 61 58 db 52 
|X.R.u.oH6...aX.R|
   OOB Data: 08 75 8b 6f 48 36 a6 bc 16 61 58 db 52 08 75 8b 
|.u.oH6...aX.R.u.|
   OOB Data: 6f 48 36 a6 bc 16 61 58 db 52 ff ff ff ff ff ff 
|oH6...aX.R......|
   OOB Data: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff 
|................|
   OOB Data: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff 
|................|
   OOB Data: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff 
|................|
   OOB Data: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff 
|................|
   OOB Data: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff 
|................|
   OOB Data: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff 
|................|
   OOB Data: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff 
|................|

I have checked what is happening in rawnand_enable_cont_reads function,
and for page 0, con_read.ongoing = true when for page 1 con_read.ongoing 
= false

page 0:
[   51.785623] rawnand_enable_cont_reads: page=0, col=0, readlen=4096, 
mtd->writesize=4096
[   51.793751] rawnand_enable_cont_reads: end_page=1, end_col=0
[   51.799356] rawnand_enable_cont_reads: con_read.ongoing=1

page 1:
[   53.493337] rawnand_enable_cont_reads: page=1, col=0, readlen=4096, 
mtd->writesize=4096
[   53.501413] rawnand_enable_cont_reads: end_page=1, end_col=0
[   53.507013] rawnand_enable_cont_reads: con_read.ongoing=0

I do not expect con_read.ongoing set to true when we read one page.

I have also dumped what happened when we read the bad block table and it 
is also strange for me in particular the value end_page.

[    1.581940] nand: device found, Manufacturer ID: 0x2c, Chip ID: 0xd3
[    1.581966] nand: Micron MT29F8G08ABACAH4
[    1.581974] nand: 1024 MiB, SLC, erase size: 256 KiB, page size: 
4096, OOB size: 224
[    1.582379] rawnand_enable_cont_reads: page=262080, col=0, readlen=5, 
mtd->writesize=4096
[    1.582411] rawnand_enable_cont_reads: end_page=0, end_col=5
[    1.582419] rawnand_enable_cont_reads: con_read.ongoing=0
[    1.585817] Bad block table found at page 262080, version 0x01
[    1.585943] rawnand_enable_cont_reads: page=262080, col=0, readlen=5, 
mtd->writesize=4096
[    1.585960] rawnand_enable_cont_reads: end_page=0, end_col=5
[    1.585968] rawnand_enable_cont_reads: con_read.ongoing=0
[    1.586677] rawnand_enable_cont_reads: page=262016, col=0, readlen=5, 
mtd->writesize=4096
[    1.586700] rawnand_enable_cont_reads: end_page=0, end_col=5
[    1.586708] rawnand_enable_cont_reads: con_read.ongoing=0
[    1.587139] Bad block table found at page 262016, version 0x01
[    1.587168] rawnand_enable_cont_reads: page=262081, col=5, 
readlen=1019, mtd->writesize=4096
[    1.587181] rawnand_enable_cont_reads: end_page=0, end_col=1024
[    1.587189] rawnand_enable_cont_reads: con_read.ongoing=0
[    1.587672] rawnand_enable_cont_reads: page=262081, col=1024, 
readlen=5, mtd->writesize=4096
[    1.587692] rawnand_enable_cont_reads: end_page=0, end_col=1029
[    1.587700] rawnand_enable_cont_reads: con_read.ongoing=0

I currently do not understand the logic implemented but there is 
something suspect around end_page variable.

end_page = DIV_ROUND_UP(col + readlen, mtd->writesize);
=> So, if i have well understood, end_page is the number of pages we are 
going to read.

if (page + 1 > end_page) {
=> We are comparing the page that we are starting to read with the 
number of pages to read and not the last page to read

chip->cont_read.first_page = page;
chip->cont_read.last_page = end_page;
=> first_page is the first page to read and last page is the number of 
pages to read.

Before this patch,
chip->cont_read.last_page = page + ((readlen >> chip->page_shift) & 
chip->pagemask);
=> last page was the last page to read.

Regards,
Christophe Kerello.

On 12/22/23 12:37, Miquel Raynal wrote:
> On Fri, 2023-12-15 at 12:32:08 UTC, Miquel Raynal wrote:
>> The current logic is probably fine but is a bit convoluted. Plus, we
>> don't want partial pages to be part of the sequential operation just in
>> case the core would optimize the page read with a subpage read (which
>> would break the sequence). This may happen on the first and last page
>> only, so if the start offset or the end offset is not aligned with a
>> page boundary, better avoid them to prevent any risk.
>>
>> Cc: stable@vger.kernel.org
>> Fixes: 003fe4b9545b ("mtd: rawnand: Support for sequential cache reads")
>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> 
> Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next.
> 
> Miquel
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 4/4] mtd: rawnand: Clarify conditions to enable continuous reads

[Miquel Raynal]

Hi Christophe,

christophe.kerello@foss.st.com wrote on Fri, 9 Feb 2024 14:35:44 +0100:

> Hi Miquel,
> 
> I am testing last nand/next branch with the MP1 board, and i get an issue since this patch was applied.
> 
> When I read the SLC NAND using nandump tool (reading page 0 and page 1), the OOB is not displayed at expected. For page 1, oob is displayed when for page 0 the first data of the page are displayed.
> 
> The nanddump command used is: nanddump -c -o -l 0x2000 /dev/mtd9
> 
> Page 0:
>    OOB Data: 7f 45 4c 46 01 01 01 00 00 00 00 00 00 00 00 00 |.ELF............|
>    OOB Data: 03 00 28 00 01 00 00 00 a4 03 00 00 34 00 00 00 |..(.........4...|
>    OOB Data: 7c 11 00 00 00 04 00 05 34 00 20 00 06 00 28 00 ||.......4. ...(.|
>    OOB Data: 1b 00 1a 00 01 00 00 00 00 00 00 00 00 00 00 00 |................|
>    OOB Data: 00 00 00 00 10 05 00 00 10 05 00 00 05 00 00 00 |................|
>    OOB Data: 00 00 01 00 01 00 00 00 e8 0e 00 00 e8 0e 01 00 |................|
>    OOB Data: e8 0e 01 00 44 01 00 00 48 01 00 00 06 00 00 00 |....D...H.......|
>    OOB Data: 00 00 01 00 02 00 00 00 f0 0e 00 00 f0 0e 01 00 |................|
>    OOB Data: f0 0e 01 00 10 01 00 00 10 01 00 00 06 00 00 00 |................|
>    OOB Data: 04 00 00 00 04 00 00 00 f4 00 00 00 f4 00 00 00 |................|
>    OOB Data: f4 00 00 00 44 00 00 00 44 00 00 00 04 00 00 00 |....D...D.......|
>    OOB Data: 04 00 00 00 51 e5 74 64 00 00 00 00 00 00 00 00 |....Q.td........|
>    OOB Data: 00 00 00 00 00 00 00 00 00 00 00 00 06 00 00 00 |................|
>    OOB Data: 10 00 00 00 52 e5 74 64 e8 0e 00 00 e8 0e 01 00 |....R.td........|
> 
> Page 1:
>    OOB Data: ff ff 94 25 8c 3c c7 44 e7 c0 b7 b0 92 5e 50 fb |...%.<.D.....^P.|
>    OOB Data: 80 ca a3 de e2 73 b4 4e 58 39 fe b4 85 76 65 31 |.....s.NX9...ve1|
>    OOB Data: 48 86 91 f3 58 0b 59 df 2c 08 75 8b 6f 48 36 a6 |H...X.Y.,.u.oH6.|
>    OOB Data: bc 16 61 58 db 52 08 75 8b 6f 48 36 a6 bc 16 61 |..aX.R.u.oH6...a|
>    OOB Data: 58 db 52 08 75 8b 6f 48 36 a6 bc 16 61 58 db 52 |X.R.u.oH6...aX.R|
>    OOB Data: 08 75 8b 6f 48 36 a6 bc 16 61 58 db 52 08 75 8b |.u.oH6...aX.R.u.|
>    OOB Data: 6f 48 36 a6 bc 16 61 58 db 52 ff ff ff ff ff ff |oH6...aX.R......|
>    OOB Data: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
>    OOB Data: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
>    OOB Data: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
>    OOB Data: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
>    OOB Data: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
>    OOB Data: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
>    OOB Data: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
> 
> I have checked what is happening in rawnand_enable_cont_reads function,
> and for page 0, con_read.ongoing = true when for page 1 con_read.ongoing = false
> 
> page 0:
> [   51.785623] rawnand_enable_cont_reads: page=0, col=0, readlen=4096, mtd->writesize=4096
> [   51.793751] rawnand_enable_cont_reads: end_page=1, end_col=0
> [   51.799356] rawnand_enable_cont_reads: con_read.ongoing=1
> 
> page 1:
> [   53.493337] rawnand_enable_cont_reads: page=1, col=0, readlen=4096, mtd->writesize=4096
> [   53.501413] rawnand_enable_cont_reads: end_page=1, end_col=0
> [   53.507013] rawnand_enable_cont_reads: con_read.ongoing=0
> 
> I do not expect con_read.ongoing set to true when we read one page.
> 
> I have also dumped what happened when we read the bad block table and it is also strange for me in particular the value end_page.
> 
> [    1.581940] nand: device found, Manufacturer ID: 0x2c, Chip ID: 0xd3
> [    1.581966] nand: Micron MT29F8G08ABACAH4
> [    1.581974] nand: 1024 MiB, SLC, erase size: 256 KiB, page size: 4096, OOB size: 224
> [    1.582379] rawnand_enable_cont_reads: page=262080, col=0, readlen=5, mtd->writesize=4096
> [    1.582411] rawnand_enable_cont_reads: end_page=0, end_col=5
> [    1.582419] rawnand_enable_cont_reads: con_read.ongoing=0
> [    1.585817] Bad block table found at page 262080, version 0x01
> [    1.585943] rawnand_enable_cont_reads: page=262080, col=0, readlen=5, mtd->writesize=4096
> [    1.585960] rawnand_enable_cont_reads: end_page=0, end_col=5
> [    1.585968] rawnand_enable_cont_reads: con_read.ongoing=0
> [    1.586677] rawnand_enable_cont_reads: page=262016, col=0, readlen=5, mtd->writesize=4096
> [    1.586700] rawnand_enable_cont_reads: end_page=0, end_col=5
> [    1.586708] rawnand_enable_cont_reads: con_read.ongoing=0
> [    1.587139] Bad block table found at page 262016, version 0x01
> [    1.587168] rawnand_enable_cont_reads: page=262081, col=5, readlen=1019, mtd->writesize=4096
> [    1.587181] rawnand_enable_cont_reads: end_page=0, end_col=1024
> [    1.587189] rawnand_enable_cont_reads: con_read.ongoing=0
> [    1.587672] rawnand_enable_cont_reads: page=262081, col=1024, readlen=5, mtd->writesize=4096
> [    1.587692] rawnand_enable_cont_reads: end_page=0, end_col=1029
> [    1.587700] rawnand_enable_cont_reads: con_read.ongoing=0

Interesting, I played with those corner cases in earlier tests but
for this series I was focused on playing with filesystems and the fact
that sometimes continuous read was very sporadically breaking, so I
played with much more complex patterns but I don't remember checking
these two basic cases again...

Sorry for the breakage, I will have a look and keep you updated. I
believe the continuous read feature is fine per se, but the problem
here is that there is a mismatch between the actual operation and the
continuous read configuration on "top" of it, which should in these
cases not be enabled at all.

I am away this week, I will look into this when I'm back.

Thanks for the useful report,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 4/4] mtd: rawnand: Clarify conditions to enable continuous reads

[Miquel Raynal]

Hi Christophe,

christophe.kerello@foss.st.com wrote on Fri, 9 Feb 2024 14:35:44 +0100:

> Hi Miquel,
> 
> I am testing last nand/next branch with the MP1 board, and i get an issue since this patch was applied.
> 
> When I read the SLC NAND using nandump tool (reading page 0 and page 1), the OOB is not displayed at expected. For page 1, oob is displayed when for page 0 the first data of the page are displayed.
> 
> The nanddump command used is: nanddump -c -o -l 0x2000 /dev/mtd9

I believe the issue is not in the indexes but related to the OOB. I
currently test on a device on which I would prefer not to smash the
content, so this is just compile tested and not run time verified, but
could you tell me if this solves the issue:

--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -3577,7 +3577,8 @@ static int nand_do_read_ops(struct nand_chip *chip, loff_t from,
        oob = ops->oobbuf;
        oob_required = oob ? 1 : 0;
 
-       rawnand_enable_cont_reads(chip, page, readlen, col);
+       if (!oob_required)
+               rawnand_enable_cont_reads(chip, page, readlen, col);
 
        while (1) {
                struct mtd_ecc_stats ecc_stats = mtd->ecc_stats;


If that does not work, I'll destroy the content of the flash and
properly reproduce.

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 4/4] mtd: rawnand: Clarify conditions to enable continuous reads

[Christophe Kerello]

Hi Miquel,

On 2/21/24 12:20, Miquel Raynal wrote:
> Hi Christophe,
> 
> christophe.kerello@foss.st.com wrote on Fri, 9 Feb 2024 14:35:44 +0100:
> 
>> Hi Miquel,
>>
>> I am testing last nand/next branch with the MP1 board, and i get an issue since this patch was applied.
>>
>> When I read the SLC NAND using nandump tool (reading page 0 and page 1), the OOB is not displayed at expected. For page 1, oob is displayed when for page 0 the first data of the page are displayed.
>>
>> The nanddump command used is: nanddump -c -o -l 0x2000 /dev/mtd9
> 
> I believe the issue is not in the indexes but related to the OOB. I
> currently test on a device on which I would prefer not to smash the
> content, so this is just compile tested and not run time verified, but
> could you tell me if this solves the issue:
> 
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -3577,7 +3577,8 @@ static int nand_do_read_ops(struct nand_chip *chip, loff_t from,
>          oob = ops->oobbuf;
>          oob_required = oob ? 1 : 0;
>   
> -       rawnand_enable_cont_reads(chip, page, readlen, col);
> +       if (!oob_required)
> +               rawnand_enable_cont_reads(chip, page, readlen, col);

I am still able to reproduce the problem with the patch applied.
In fact, when nanddump reads the OOB, nand_do_read_ops is not called, 
but nand_read_oob_op is called, and as cont_read.ongoing=1, we are not 
dumping the oob but the first data of the page.

page 0:
[   57.642144] rawnand_enable_cont_reads: page=0, col=0, readlen=4096, 
mtd->writesize=4096
[   57.650210] rawnand_enable_cont_reads: end_page=1
[   57.654858] nand_do_read_ops: cont_read.ongoing=1
[   59.352562] nand_read_oob_op
page 1:
[   59.355966] rawnand_enable_cont_reads: page=1, col=0, readlen=4096, 
mtd->writesize=4096
[   59.364045] rawnand_enable_cont_reads: end_page=1
[   59.368757] nand_do_read_ops: cont_read.ongoing=0
[   61.390098] nand_read_oob_op

I have not currently bandwidth to work on this topic and I need to 
understand how continuous read is working, but I have made a patch and I 
do not have issues with it when I am using nanddump or mtd_debug tools.

I have not tested it on a file system, so it is just a proposal.

--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -3466,22 +3466,18 @@ static void rawnand_enable_cont_reads(struct 
nand_chip *chip, unsigned int page,
  				      u32 readlen, int col)
  {
  	struct mtd_info *mtd = nand_to_mtd(chip);
-	unsigned int end_page, end_col;
+	unsigned int end_page;

  	chip->cont_read.ongoing = false;

-	if (!chip->controller->supported_op.cont_read)
+	if (!chip->controller->supported_op.cont_read || col + readlen <= 
mtd->writesize)
  		return;

-	end_page = DIV_ROUND_UP(col + readlen, mtd->writesize);
-	end_col = (col + readlen) % mtd->writesize;
+	end_page = page + DIV_ROUND_UP(col + readlen, mtd->writesize) - 1;

  	if (col)
  		page++;

-	if (end_col && end_page)
-		end_page--;
-
  	if (page + 1 > end_page)
  		return;

Tell me if this patch is breaking the continuous read feature or if it 
can be pushed on the mailing list.

Regards,
Christophe Kerello.

>   
>          while (1) {
>                  struct mtd_ecc_stats ecc_stats = mtd->ecc_stats;
> 
> 
> If that does not work, I'll destroy the content of the flash and
> properly reproduce.
> 
> Thanks,
> Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 4/4] mtd: rawnand: Clarify conditions to enable continuous reads

[Miquel Raynal]

Hi Christophe,

christophe.kerello@foss.st.com wrote on Wed, 21 Feb 2024 17:29:45 +0100:

> Hi Miquel,
> 
> On 2/21/24 12:20, Miquel Raynal wrote:
> > Hi Christophe,
> > 
> > christophe.kerello@foss.st.com wrote on Fri, 9 Feb 2024 14:35:44 +0100:
> >   
> >> Hi Miquel,
> >>
> >> I am testing last nand/next branch with the MP1 board, and i get an issue since this patch was applied.
> >>
> >> When I read the SLC NAND using nandump tool (reading page 0 and page 1), the OOB is not displayed at expected. For page 1, oob is displayed when for page 0 the first data of the page are displayed.
> >>
> >> The nanddump command used is: nanddump -c -o -l 0x2000 /dev/mtd9  
> > 
> > I believe the issue is not in the indexes but related to the OOB. I
> > currently test on a device on which I would prefer not to smash the
> > content, so this is just compile tested and not run time verified, but
> > could you tell me if this solves the issue:
> > 
> > --- a/drivers/mtd/nand/raw/nand_base.c
> > +++ b/drivers/mtd/nand/raw/nand_base.c
> > @@ -3577,7 +3577,8 @@ static int nand_do_read_ops(struct nand_chip *chip, loff_t from,
> >          oob = ops->oobbuf;
> >          oob_required = oob ? 1 : 0;  
> >   > -       rawnand_enable_cont_reads(chip, page, readlen, col);  
> > +       if (!oob_required)
> > +               rawnand_enable_cont_reads(chip, page, readlen, col);  
> 
> I am still able to reproduce the problem with the patch applied.
> In fact, when nanddump reads the OOB, nand_do_read_ops is not called, but nand_read_oob_op is called, and as cont_read.ongoing=1, we are not dumping the oob but the first data of the page.
> 
> page 0:
> [   57.642144] rawnand_enable_cont_reads: page=0, col=0, readlen=4096, mtd->writesize=4096
> [   57.650210] rawnand_enable_cont_reads: end_page=1
> [   57.654858] nand_do_read_ops: cont_read.ongoing=1
> [   59.352562] nand_read_oob_op
> page 1:
> [   59.355966] rawnand_enable_cont_reads: page=1, col=0, readlen=4096, mtd->writesize=4096
> [   59.364045] rawnand_enable_cont_reads: end_page=1
> [   59.368757] nand_do_read_ops: cont_read.ongoing=0
> [   61.390098] nand_read_oob_op
> 
> I have not currently bandwidth to work on this topic and I need to understand how continuous read is working, but I have made a patch and I do not have issues with it when I am using nanddump or mtd_debug tools.

Actually since my previous answer I managed to reproduce the issue. I
was unable to do it because I was testing at the beginning of the
second partition, instead of the beginning of the device. I also
observe the same behavior.

> I have not tested it on a file system, so it is just a proposal.
>
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -3466,22 +3466,18 @@ static void rawnand_enable_cont_reads(struct nand_chip *chip, unsigned int page,
>   				      u32 readlen, int col)
>   {
>   	struct mtd_info *mtd = nand_to_mtd(chip);
> -	unsigned int end_page, end_col;
> +	unsigned int end_page;
> 
>   	chip->cont_read.ongoing = false;
> 
> -	if (!chip->controller->supported_op.cont_read)
> +	if (!chip->controller->supported_op.cont_read || col + readlen <= mtd->writesize)
>   		return;
> 
> -	end_page = DIV_ROUND_UP(col + readlen, mtd->writesize);
> +	end_page = page + DIV_ROUND_UP(col + readlen, mtd->writesize) - 1;

I had a similar change on my side so I believe this is needed.

> -	end_col = (col + readlen) % mtd->writesize;

We shall ensure we only enable continuous reads on full pages, to avoid
conflicts with the core trying to optimize things out. So I believe
this change won't fly, but I get the idea, there is definitely
something to fix there.

> 
>   	if (col)
>   		page++;
> 
> -	if (end_col && end_page)
> -		end_page--;
> -
>   	if (page + 1 > end_page)
>   		return;
> 
> Tell me if this patch is breaking the continuous read feature or if it can be pushed on the mailing list.

I'll have deeper look into this tomorrow and get back to you. Thanks a
lot for the proposal though, I will work on it.

> 
> Regards,
> Christophe Kerello.
> 
> >   >          while (1) {  
> >                  struct mtd_ecc_stats ecc_stats = mtd->ecc_stats;
> > 
> > 
> > If that does not work, I'll destroy the content of the flash and
> > properly reproduce.
> > 
> > Thanks,
> > Miquèl  


Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 0/4] mtd: rawnand: Fix sequential reads

[Martin Hundebøll]

Hi Miquel,

On Fri, 2023-12-15 at 13:32 +0100, Miquel Raynal wrote:
> After a number of reports and hours of investigation, I could finally
> identify the main issue that likely lead to all these issues, which
> is
> addressed in the second patch. The three other patches are also
> flagged
> for backporting because I think it is best to apply them as well as
> they
> are supposed to prevent a few corner cases from happening.
> 
> Please test this series and let me know if you encounter other
> errors.
> 
> Thanks,
> Miquèl
> 
> Miquel Raynal (4):
>   mtd: rawnand: Prevent crossing LUN boundaries during sequential
> reads
>   mtd: rawnand: Fix core interference with sequential reads
>   mtd: rawnand: Prevent sequential reads with on-die ECC engines
>   mtd: rawnand: Clarify conditions to enable continuous reads
> 
>  drivers/mtd/nand/raw/nand_base.c | 89 +++++++++++++++++++++++++++---
> --
>  include/linux/mtd/rawnand.h      |  2 +
>  2 files changed, 77 insertions(+), 14 deletions(-)
> 

Thank you for looking into this!

This series fixes the issue on our i.MX6ULL equipped with a Micron
MT29F4G08ABAFAWP nand flash, so for all four patches:

Tested-by: Martin Hundebøll <martin@geanix.com>

// Martin

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/