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

Fri Feb 9 05:35:44 PST 2024

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 at vger.kernel.org
>> Fixes: 003fe4b9545b ("mtd: rawnand: Support for sequential cache reads")
>> Signed-off-by: Miquel Raynal <miquel.raynal at 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/