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

[Miquel Raynal <miquel.raynal@bootlin.com>]

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/