[PATCH 0/3] mtd: rawnand: More continuous read fixes

[PATCH 0/3] mtd: rawnand: More continuous read fixes

[Miquel Raynal]

Hello,

Following Christophe report I manually inserted many different
conditions to test the logic enablingand configuring continuous reads in
the core, trying to clarify the core and hopefully fix it for real. I am
pretty confident regarding the first patch but a bit more in the fog for
the second/third. Even though I'm pretty sure they improve the situation
there might still be corner cases I don't have in mind.

Link: https://lore.kernel.org/linux-mtd/20240221175327.42f7076d@xps-13/T/#m399bacb10db8f58f6b1f0149a1df867ec086bb0a

Cheers,
Miquèl

Miquel Raynal (3):
  mtd: rawnand: Fix and simplify again the continuous read derivations
  mtd: rawnand: Add a helper for calculating a page index
  mtd: rawnand: Ensure all continuous terms are always in sync

 drivers/mtd/nand/raw/nand_base.c | 77 +++++++++++++++++++-------------
 1 file changed, 47 insertions(+), 30 deletions(-)

-- 
2.34.1


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

[PATCH 1/3] mtd: rawnand: Fix and simplify again the continuous read derivations

[Miquel Raynal]

We need to avoid the first page if we don't read it entirely.
We need to avoid the last page if we don't read it entirely.
While rather simple, this logic has been failed in the previous
fix. This time I wrote about 30 unit tests locally to check each
possible condition, hopefully I covered them all.

Reported-by: Christophe Kerello <christophe.kerello@foss.st.com>
Closes: https://lore.kernel.org/linux-mtd/20240221175327.42f7076d@xps-13/T/#m399bacb10db8f58f6b1f0149a1df867ec086bb0a
Suggested-by: Christophe Kerello <christophe.kerello@foss.st.com>
Fixes: 828f6df1bcba ("mtd: rawnand: Clarify conditions to enable continuous reads")
Cc: stable@vger.kernel.org
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/nand/raw/nand_base.c | 38 ++++++++++++++++++--------------
 1 file changed, 22 insertions(+), 16 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 3b3ce2926f5d..bcfd99a1699f 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -3466,30 +3466,36 @@ 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 first_page, last_page;
 
 	chip->cont_read.ongoing = false;
 
 	if (!chip->controller->supported_op.cont_read)
 		return;
 
-	end_page = DIV_ROUND_UP(col + readlen, mtd->writesize);
-	end_col = (col + readlen) % mtd->writesize;
+	/*
+	 * Don't bother making any calculations if the length is too small.
+	 * Side effect: avoids possible integer underflows below.
+	 */
+	if (readlen < (2 * mtd->writesize))
+		return;
 
+	/* Derive the page where continuous read should start (the first full page read) */
+	first_page = page;
 	if (col)
-		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);
+		first_page++;
+
+	/* Derive the page where continuous read should stop (the last full page read) */
+	last_page = page + ((col + readlen) / mtd->writesize) - 1;
+
+	/* Configure and enable continuous read when suitable */
+	if (first_page < last_page) {
+		chip->cont_read.first_page = first_page;
+		chip->cont_read.last_page = last_page;
+		chip->cont_read.ongoing = true;
+		/* May reset the ongoing flag */
+		rawnand_cap_cont_reads(chip);
+	}
 }
 
 static void rawnand_cont_read_skip_first_page(struct nand_chip *chip, unsigned int page)
-- 
2.34.1


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

Re: [PATCH 1/3] mtd: rawnand: Fix and simplify again the continuous read derivations

[Christophe Kerello]

Hi Miquel,

On 2/23/24 12:55, Miquel Raynal wrote:
> We need to avoid the first page if we don't read it entirely.
> We need to avoid the last page if we don't read it entirely.
> While rather simple, this logic has been failed in the previous
> fix. This time I wrote about 30 unit tests locally to check each
> possible condition, hopefully I covered them all.
> 
> Reported-by: Christophe Kerello <christophe.kerello@foss.st.com>
> Closes: https://lore.kernel.org/linux-mtd/20240221175327.42f7076d@xps-13/T/#m399bacb10db8f58f6b1f0149a1df867ec086bb0a
> Suggested-by: Christophe Kerello <christophe.kerello@foss.st.com>
> Fixes: 828f6df1bcba ("mtd: rawnand: Clarify conditions to enable continuous reads")
> Cc: stable@vger.kernel.org
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Tested-by: Christophe Kerello <christophe.kerello@foss.st.com>

Regards,
Christophe Kerello.

> ---
>   drivers/mtd/nand/raw/nand_base.c | 38 ++++++++++++++++++--------------
>   1 file changed, 22 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index 3b3ce2926f5d..bcfd99a1699f 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -3466,30 +3466,36 @@ 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 first_page, last_page;
>   
>   	chip->cont_read.ongoing = false;
>   
>   	if (!chip->controller->supported_op.cont_read)
>   		return;
>   
> -	end_page = DIV_ROUND_UP(col + readlen, mtd->writesize);
> -	end_col = (col + readlen) % mtd->writesize;
> +	/*
> +	 * Don't bother making any calculations if the length is too small.
> +	 * Side effect: avoids possible integer underflows below.
> +	 */
> +	if (readlen < (2 * mtd->writesize))
> +		return;
>   
> +	/* Derive the page where continuous read should start (the first full page read) */
> +	first_page = page;
>   	if (col)
> -		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);
> +		first_page++;
> +
> +	/* Derive the page where continuous read should stop (the last full page read) */
> +	last_page = page + ((col + readlen) / mtd->writesize) - 1;
> +
> +	/* Configure and enable continuous read when suitable */
> +	if (first_page < last_page) {
> +		chip->cont_read.first_page = first_page;
> +		chip->cont_read.last_page = last_page;
> +		chip->cont_read.ongoing = true;
> +		/* May reset the ongoing flag */
> +		rawnand_cap_cont_reads(chip);
> +	}
>   }
>   
>   static void rawnand_cont_read_skip_first_page(struct nand_chip *chip, unsigned int page)

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

Re: [PATCH 1/3] mtd: rawnand: Fix and simplify again the continuous read derivations

[Miquel Raynal]

On Fri, 2024-02-23 at 11:55:43 UTC, Miquel Raynal wrote:
> We need to avoid the first page if we don't read it entirely.
> We need to avoid the last page if we don't read it entirely.
> While rather simple, this logic has been failed in the previous
> fix. This time I wrote about 30 unit tests locally to check each
> possible condition, hopefully I covered them all.
> 
> Reported-by: Christophe Kerello <christophe.kerello@foss.st.com>
> Closes: https://lore.kernel.org/linux-mtd/20240221175327.42f7076d@xps-13/T/#m399bacb10db8f58f6b1f0149a1df867ec086bb0a
> Suggested-by: Christophe Kerello <christophe.kerello@foss.st.com>
> Fixes: 828f6df1bcba ("mtd: rawnand: Clarify conditions to enable continuous reads")
> Cc: stable@vger.kernel.org
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> Tested-by: Christophe Kerello <christophe.kerello@foss.st.com>

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

Miquel

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

[PATCH 2/3] mtd: rawnand: Add a helper for calculating a page index

[Miquel Raynal]

For LUN crossing boundaries, it is handy to know what is the index of
the last page in a LUN. This helper will soon be reused. At the same
time I rename page_per_lun to ppl in the calling function to clarify the
lines.

Cc: stable@vger.kernel.org
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
This is a dependency for the next patch, so I Cc'd stable on it as well.
---
 drivers/mtd/nand/raw/nand_base.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index bcfd99a1699f..d6a27e08b112 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -1211,19 +1211,25 @@ static int nand_lp_exec_read_page_op(struct nand_chip *chip, unsigned int page,
 	return nand_exec_op(chip, &op);
 }
 
+static unsigned int rawnand_last_page_of_lun(unsigned int pages_per_lun, unsigned int lun)
+{
+	/* lun is expected to be very small */
+	return (lun * pages_per_lun) + pages_per_lun - 1;
+}
+
 static void rawnand_cap_cont_reads(struct nand_chip *chip)
 {
 	struct nand_memory_organization *memorg;
-	unsigned int pages_per_lun, first_lun, last_lun;
+	unsigned int ppl, 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;
+	ppl = memorg->pages_per_eraseblock * memorg->eraseblocks_per_lun;
+	first_lun = chip->cont_read.first_page / ppl;
+	last_lun = chip->cont_read.last_page / ppl;
 
 	/* 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;
+		chip->cont_read.pause_page = rawnand_last_page_of_lun(ppl, first_lun);
 	else
 		chip->cont_read.pause_page = chip->cont_read.last_page;
 }
-- 
2.34.1


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

[PATCH 3/3] mtd: rawnand: Ensure all continuous terms are always in sync

[Miquel Raynal]

While crossing a LUN boundary, it is probably safer (and clearer) to
keep all members of the continuous read structure aligned, including the
pause page (which is the last page of the lun or the last page of the
continuous read). Once these members properly in sync, we can use the
rawnand_cap_cont_reads() helper everywhere to "prepare" the next
continuous read if there is one.

Fixes: bbcd80f53a5e ("mtd: rawnand: Prevent crossing LUN boundaries during sequential reads")
Cc: stable@vger.kernel.org
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
This is not 100% a fix but I believe it is worth backporting as there
may be corner cases which were not identified with the initial
implementation.
---
 drivers/mtd/nand/raw/nand_base.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index d6a27e08b112..4d5a663e4e05 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -1232,6 +1232,15 @@ static void rawnand_cap_cont_reads(struct nand_chip *chip)
 		chip->cont_read.pause_page = rawnand_last_page_of_lun(ppl, first_lun);
 	else
 		chip->cont_read.pause_page = chip->cont_read.last_page;
+
+	if (chip->cont_read.first_page == chip->cont_read.pause_page) {
+		chip->cont_read.first_page++;
+		chip->cont_read.pause_page = min(chip->cont_read.last_page,
+						 rawnand_last_page_of_lun(ppl, first_lun + 1));
+	}
+
+	if (chip->cont_read.first_page >= chip->cont_read.last_page)
+		chip->cont_read.ongoing = false;
 }
 
 static int nand_lp_exec_cont_read_page_op(struct nand_chip *chip, unsigned int page,
@@ -1298,12 +1307,11 @@ static int nand_lp_exec_cont_read_page_op(struct nand_chip *chip, unsigned int p
 	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) {
+	if (page == chip->cont_read.last_page) {
 		chip->cont_read.ongoing = false;
+	} else if (page == chip->cont_read.pause_page) {
+		chip->cont_read.first_page++;
+		rawnand_cap_cont_reads(chip);
 	}
 
 	return 0;
@@ -3510,10 +3518,7 @@ static void rawnand_cont_read_skip_first_page(struct nand_chip *chip, unsigned i
 		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;
+	rawnand_cap_cont_reads(chip);
 }
 
 /**
-- 
2.34.1


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

Re: [PATCH 3/3] mtd: rawnand: Ensure all continuous terms are always in sync

[Miquel Raynal]

On Fri, 2024-02-23 at 11:55:45 UTC, Miquel Raynal wrote:
> While crossing a LUN boundary, it is probably safer (and clearer) to
> keep all members of the continuous read structure aligned, including the
> pause page (which is the last page of the lun or the last page of the
> continuous read). Once these members properly in sync, we can use the
> rawnand_cap_cont_reads() helper everywhere to "prepare" the next
> continuous read if there is one.
> 
> Fixes: bbcd80f53a5e ("mtd: rawnand: Prevent crossing LUN boundaries during sequential 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 nand/next.

Miquel

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

Re: [PATCH 0/3] mtd: rawnand: More continuous read fixes

[Christophe Kerello]

Hi Miquel,

On 2/23/24 12:55, Miquel Raynal wrote:
> Hello,
> 
> Following Christophe report I manually inserted many different
> conditions to test the logic enablingand configuring continuous reads in
> the core, trying to clarify the core and hopefully fix it for real. I am
> pretty confident regarding the first patch but a bit more in the fog for
> the second/third. Even though I'm pretty sure they improve the situation
> there might still be corner cases I don't have in mind.

I have tested the patchset and the issue is fixed, so I will send a 
tested-by on patch 1.

But, I think that there is still an issue using FMC2 and probably others 
drivers.

FMC2 driver has 2 modes: polling mode that will called 
nand_read_page_op and the sequencer mode that is defining its own HW 
read algorithm.

The FMC2 sequencer do not support continuous read feature.

I have added basic logs in nand_do_read_ops. My understanding is that 
the continuous read feature should be disabled at the end of this function.

FMC2 polling mode:
root@stm32mp1:~# mtd_debug read /dev/mtd9 0 0x2000 /tmp/read.hex
[   41.083132] nand_do_read_ops starts: cont_read.ongoing=1
[   41.086410] nand_do_read_ops starts: cont_read.first_page=0, 
cont_read.last_page=1
[   41.094797] nand_do_read_ops ends: cont_read.ongoing=0
[   41.098111] nand_do_read_ops ends: cont_read.first_page=0, 
cont_read.last_page=1
Copied 8192 bytes from address 0x00000000 in flash to /tmp/read.hex

It is OK. In polling mode, con_read.ongoing is set to false before 
leaving nand_do_read_ops function.

FMC2 sequencer:
root@stm32mp1:~# mtd_debug read /dev/mtd9 0 0x2000 /tmp/read.hex
[   57.143059] nand_do_read_ops starts: cont_read.ongoing=1
[   57.146370] nand_do_read_ops starts: cont_read.first_page=0, 
cont_read.last_page=1
[   57.154469] nand_do_read_ops ends: cont_read.ongoing=1
[   57.158020] nand_do_read_ops ends: cont_read.first_page=0, 
cont_read.last_page=1
Copied 8192 bytes from address 0x00000000 in flash to /tmp/read.hex

KO, con_read.ongoing is set to true before leaving nand_do_read_ops 
function. That means that read_oob can returned wrong data (similar 
issue as the initial reported issue).

So, I see 2 ways to fix this issue.

On framework side by adding a callback (rawnand_disable_cont_reads) that 
will set to false con_read.ongoing before leaving nand_do_read_ops function.

Or

On FMC2 driver side by disabling the continuous read feature in case of 
the sequencer is used like it is done in nandsim.c driver.
Something like:
if (check_only) {
		for (op_id = 0; op_id < op->ninstrs; op_id++) {
			instr = &op->instrs[op_id];
			if (instr->type == NAND_OP_CMD_INSTR &&
			    (instr->ctx.cmd.opcode == NAND_CMD_READCACHEEND ||
			     instr->ctx.cmd.opcode == NAND_CMD_READCACHESEQ))
				return -EOPNOTSUPP;
		}

		return 0;
	}

So, should it be fixed on driver side or framework side?

Regards,
Christophe Kerello.

> 
> Link: https://lore.kernel.org/linux-mtd/20240221175327.42f7076d@xps-13/T/#m399bacb10db8f58f6b1f0149a1df867ec086bb0a
> 
> Cheers,
> Miquèl
> 
> Miquel Raynal (3):
>    mtd: rawnand: Fix and simplify again the continuous read derivations
>    mtd: rawnand: Add a helper for calculating a page index
>    mtd: rawnand: Ensure all continuous terms are always in sync
> 
>   drivers/mtd/nand/raw/nand_base.c | 77 +++++++++++++++++++-------------
>   1 file changed, 47 insertions(+), 30 deletions(-)
> 

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

Re: [PATCH 0/3] mtd: rawnand: More continuous read fixes

[Miquel Raynal]

Hi Christophe,

christophe.kerello@foss.st.com wrote on Mon, 26 Feb 2024 15:28:59 +0100:

> Hi Miquel,
> 
> On 2/23/24 12:55, Miquel Raynal wrote:
> > Hello,
> > 
> > Following Christophe report I manually inserted many different
> > conditions to test the logic enablingand configuring continuous reads in
> > the core, trying to clarify the core and hopefully fix it for real. I am
> > pretty confident regarding the first patch but a bit more in the fog for
> > the second/third. Even though I'm pretty sure they improve the situation
> > there might still be corner cases I don't have in mind.  
> 
> I have tested the patchset and the issue is fixed, so I will send a tested-by on patch 1.

Great! Thanks!

For now I could not get my hands on a chip with more than one LUN. If
by chance yours has two LUNs (or more), could you please run some
experiments when crossing the LUN boundary?

Anyhow, your Tested-by will be welcome.

> But, I think that there is still an issue using FMC2 and probably others drivers.
> 
> FMC2 driver has 2 modes: polling mode that will called nand_read_page_op and the sequencer mode that is defining its own HW read algorithm.
> 
> The FMC2 sequencer do not support continuous read feature.
> 
> I have added basic logs in nand_do_read_ops. My understanding is that the continuous read feature should be disabled at the end of this function.
> 
> FMC2 polling mode:
> root@stm32mp1:~# mtd_debug read /dev/mtd9 0 0x2000 /tmp/read.hex
> [   41.083132] nand_do_read_ops starts: cont_read.ongoing=1
> [   41.086410] nand_do_read_ops starts: cont_read.first_page=0, cont_read.last_page=1
> [   41.094797] nand_do_read_ops ends: cont_read.ongoing=0
> [   41.098111] nand_do_read_ops ends: cont_read.first_page=0, cont_read.last_page=1
> Copied 8192 bytes from address 0x00000000 in flash to /tmp/read.hex
> 
> It is OK. In polling mode, con_read.ongoing is set to false before leaving nand_do_read_ops function.
> 
> FMC2 sequencer:
> root@stm32mp1:~# mtd_debug read /dev/mtd9 0 0x2000 /tmp/read.hex
> [   57.143059] nand_do_read_ops starts: cont_read.ongoing=1
> [   57.146370] nand_do_read_ops starts: cont_read.first_page=0, cont_read.last_page=1
> [   57.154469] nand_do_read_ops ends: cont_read.ongoing=1
> [   57.158020] nand_do_read_ops ends: cont_read.first_page=0, cont_read.last_page=1
> Copied 8192 bytes from address 0x00000000 in flash to /tmp/read.hex
> 
> KO, con_read.ongoing is set to true before leaving nand_do_read_ops function. That means that read_oob can returned wrong data (similar issue as the initial reported issue).
> 
> So, I see 2 ways to fix this issue.
> 
> On framework side by adding a callback (rawnand_disable_cont_reads) that will set to false con_read.ongoing before leaving nand_do_read_ops function.

Can you please trace what happens here? Upon what specific condition
nand_do_read_ops does return with ongoing set to true? This is probably
what needs fixing, but I don't feel like a driver hook is the right
approach.

I was expecting the ongoing boolean to always be reset at the end of
nand_do_read_ops(), I probably missed a scenario. In any case what is
probably needed for the sequencer to work is:

+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -3726,6 +3726,7 @@ static int nand_do_read_ops(struct nand_chip *chip, loff_t from,
                }
        }
        nand_deselect_target(chip);
+       chip->cont_read.ongoing = false;
 
        ops->retlen = ops->len - (size_t) readlen;
        if (oob)

> 
> Or
> 
> On FMC2 driver side by disabling the continuous read feature in case of the sequencer is used like it is done in nandsim.c driver.
> Something like:
> if (check_only) {
> 		for (op_id = 0; op_id < op->ninstrs; op_id++) {
> 			instr = &op->instrs[op_id];
> 			if (instr->type == NAND_OP_CMD_INSTR &&
> 			    (instr->ctx.cmd.opcode == NAND_CMD_READCACHEEND ||
> 			     instr->ctx.cmd.opcode == NAND_CMD_READCACHESEQ))
> 				return -EOPNOTSUPP;
> 		}
> 
> 		return 0;
> 	}

This is a second possible choice if the first one does not give
interesting results. Not my favorite in your case.

Thanks for your help,
Miquèl

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

Re: [PATCH 0/3] mtd: rawnand: More continuous read fixes

[Christophe Kerello]

Hi Miquel,

On 2/29/24 11:46, Miquel Raynal wrote:
> Hi Christophe,
> 
> christophe.kerello@foss.st.com wrote on Mon, 26 Feb 2024 15:28:59 +0100:
> 
>> Hi Miquel,
>>
>> On 2/23/24 12:55, Miquel Raynal wrote:
>>> Hello,
>>>
>>> Following Christophe report I manually inserted many different
>>> conditions to test the logic enablingand configuring continuous reads in
>>> the core, trying to clarify the core and hopefully fix it for real. I am
>>> pretty confident regarding the first patch but a bit more in the fog for
>>> the second/third. Even though I'm pretty sure they improve the situation
>>> there might still be corner cases I don't have in mind.
>>
>> I have tested the patchset and the issue is fixed, so I will send a tested-by on patch 1.
> 
> Great! Thanks!
> 
> For now I could not get my hands on a chip with more than one LUN. If
> by chance yours has two LUNs (or more), could you please run some
> experiments when crossing the LUN boundary?

Sorry, I have one LUN.

> 
> Anyhow, your Tested-by will be welcome.
> 
>> But, I think that there is still an issue using FMC2 and probably others drivers.
>>
>> FMC2 driver has 2 modes: polling mode that will called nand_read_page_op and the sequencer mode that is defining its own HW read algorithm.
>>
>> The FMC2 sequencer do not support continuous read feature.
>>
>> I have added basic logs in nand_do_read_ops. My understanding is that the continuous read feature should be disabled at the end of this function.
>>
>> FMC2 polling mode:
>> root@stm32mp1:~# mtd_debug read /dev/mtd9 0 0x2000 /tmp/read.hex
>> [   41.083132] nand_do_read_ops starts: cont_read.ongoing=1
>> [   41.086410] nand_do_read_ops starts: cont_read.first_page=0, cont_read.last_page=1
>> [   41.094797] nand_do_read_ops ends: cont_read.ongoing=0
>> [   41.098111] nand_do_read_ops ends: cont_read.first_page=0, cont_read.last_page=1
>> Copied 8192 bytes from address 0x00000000 in flash to /tmp/read.hex
>>
>> It is OK. In polling mode, con_read.ongoing is set to false before leaving nand_do_read_ops function.
>>
>> FMC2 sequencer:
>> root@stm32mp1:~# mtd_debug read /dev/mtd9 0 0x2000 /tmp/read.hex
>> [   57.143059] nand_do_read_ops starts: cont_read.ongoing=1
>> [   57.146370] nand_do_read_ops starts: cont_read.first_page=0, cont_read.last_page=1
>> [   57.154469] nand_do_read_ops ends: cont_read.ongoing=1
>> [   57.158020] nand_do_read_ops ends: cont_read.first_page=0, cont_read.last_page=1
>> Copied 8192 bytes from address 0x00000000 in flash to /tmp/read.hex
>>
>> KO, con_read.ongoing is set to true before leaving nand_do_read_ops function. That means that read_oob can returned wrong data (similar issue as the initial reported issue).
>>
>> So, I see 2 ways to fix this issue.
>>
>> On framework side by adding a callback (rawnand_disable_cont_reads) that will set to false con_read.ongoing before leaving nand_do_read_ops function.
> 
> Can you please trace what happens here? Upon what specific condition
> nand_do_read_ops does return with ongoing set to true? This is probably
> what needs fixing, but I don't feel like a driver hook is the right
> approach.

The continuous read feature is disabled in
nand_lp_exec_cont_read_page_op function. In case of the sequencer is
used, this function is never called as the HW will read the page using
its own algorithm.

I have done the traces in polling and in sequencer mode.

sequencer:
root@stm32mp1:~# mtd_debug read /dev/mtd9 0 0x3000 /tmp/read.hex
[   70.867763] rawnand_enable_cont_reads
[   70.870033] rawnand_cap_cont_reads
[   70.873463] rawnand_cap_cont_reads: ppl=262144 luns_per_target=1
[   70.879649] nand_do_read_ops before while loop: cont_read.ongoing=1
[   70.885705] nand_do_read_ops before while loop: 
cont_read.first_page=0, cont_read.last_page=2
[   70.894335] stm32_fmc2_nfc_seq_read_page
[   70.898637] stm32_fmc2_nfc_seq_read_page
[   70.902440] stm32_fmc2_nfc_seq_read_page
[   70.906471] nand_do_read_ops after while loop: cont_read.ongoing=1
[   70.912293] nand_do_read_ops after while loop: 
cont_read.first_page=0, cont_read.last_page=2
Copied 12288 bytes from address 0x00000000 in flash to /tmp/read.hex

polling:
root@stm32mp1:~# mtd_debug read /dev/mtd9 0 0x3000 /tmp/read.hex
[   59.712206] rawnand_enable_cont_reads
[   59.714478] rawnand_cap_cont_reads
[   59.718060] rawnand_cap_cont_reads: ppl=262144 luns_per_target=1
[   59.723951] nand_do_read_ops before while loop: cont_read.ongoing=1
[   59.730273] nand_do_read_ops before while loop: 
cont_read.first_page=0, cont_read.last_page=2
[   59.738793] stm32_fmc2_nfc_read_page
[   59.742335] nand_read_page_op
[   59.745264] rawnand_cont_read_ongoing
[   59.748942] nand_lp_exec_cont_read_page_op
[   59.753596] stm32_fmc2_nfc_read_page
[   59.756716] nand_read_page_op
[   59.759549] rawnand_cont_read_ongoing
[   59.763184] nand_lp_exec_cont_read_page_op
[   59.768329] stm32_fmc2_nfc_read_page
[   59.770879] nand_read_page_op
[   59.773907] rawnand_cont_read_ongoing
[   59.777615] nand_lp_exec_cont_read_page_op
[   59.782217] nand_do_read_ops after while loop: cont_read.ongoing=0
[   59.787818] nand_do_read_ops after while loop: 
cont_read.first_page=0, cont_read.last_page=2
Copied 12288 bytes from address 0x00000000 in flash to /tmp/read.hex

> 
> I was expecting the ongoing boolean to always be reset at the end of
> nand_do_read_ops(), I probably missed a scenario. In any case what is
> probably needed for the sequencer to work is:
> 
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -3726,6 +3726,7 @@ static int nand_do_read_ops(struct nand_chip *chip, loff_t from,
>                  }
>          }
>          nand_deselect_target(chip);
> +       chip->cont_read.ongoing = false;

With this patch, it is OK, the continuous read feature will be disabled
as expected in all cases.

Regards,
Christophe Kerello.

>   
>          ops->retlen = ops->len - (size_t) readlen;
>          if (oob)
> 
>>
>> Or
>>
>> On FMC2 driver side by disabling the continuous read feature in case of the sequencer is used like it is done in nandsim.c driver.
>> Something like:
>> if (check_only) {
>> 		for (op_id = 0; op_id < op->ninstrs; op_id++) {
>> 			instr = &op->instrs[op_id];
>> 			if (instr->type == NAND_OP_CMD_INSTR &&
>> 			    (instr->ctx.cmd.opcode == NAND_CMD_READCACHEEND ||
>> 			     instr->ctx.cmd.opcode == NAND_CMD_READCACHESEQ))
>> 				return -EOPNOTSUPP;
>> 		}
>>
>> 		return 0;
>> 	}
> 
> This is a second possible choice if the first one does not give
> interesting results. Not my favorite in your case.
> 
> Thanks for your help,
> Miquèl

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

Re: [PATCH 0/3] mtd: rawnand: More continuous read fixes

[Miquel Raynal]

Hi Christophe,

> > I was expecting the ongoing boolean to always be reset at the end of
> > nand_do_read_ops(), I probably missed a scenario. In any case what is
> > probably needed for the sequencer to work is:
> > 
> > +++ b/drivers/mtd/nand/raw/nand_base.c
> > @@ -3726,6 +3726,7 @@ static int nand_do_read_ops(struct nand_chip *chip, loff_t from,
> >                  }
> >          }
> >          nand_deselect_target(chip);
> > +       chip->cont_read.ongoing = false;  
> 
> With this patch, it is OK, the continuous read feature will be disabled
> as expected in all cases.

Thanks to your feedback, I've thought again about this case. The fix
presented here will most likely work but does not sound ideal as we
still enable the continuous read internal flag without using it. So I
am drafting something else that should ensure we are always enabling
continuous read only when we can use it.

As a second step, I will reset the flag like above, but with a
WARN_ON_ONCE(). This way:
- If it happens that we missed another path where this is not disabled,
  there will be no behavioral bug (hopefully).
- We will still be notified by the warning.

Thanks for all your valuable feedback and testing. Let me know what
you think of this new series.

Thanks,
Miquèl

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