Re: [PATCH v1 4/5] mtd: rawnand: meson: clear OOB buffer before read

[Arseniy Krasnov <avkrasnov@sberdevices.ru>]

On 02.05.2023 14:27, Miquel Raynal wrote:
> Hi Arseniy,
> 
> avkrasnov@sberdevices.ru wrote on Tue, 2 May 2023 13:11:38 +0300:
> 
>> On 02.05.2023 12:59, Miquel Raynal wrote:
>>> Hi Arseniy,
>>>
>>> avkrasnov@sberdevices.ru wrote on Wed, 26 Apr 2023 17:46:19 +0300:
>>>   
>>>> On 26.04.2023 16:51, Liang Yang wrote:  
>>>>> Hi Arseniy,
>>>>>
>>>>> On 2023/4/20 17:37, Arseniy Krasnov wrote:    
>>>>>> [ EXTERNAL EMAIL ]
>>>>>>
>>>>>> On 19.04.2023 09:41, Arseniy Krasnov wrote:    
>>>>>>>
>>>>>>>
>>>>>>> On 19.04.2023 06:05, Liang Yang wrote:    
>>>>>>>> Hi Arseniy,
>>>>>>>>
>>>>>>>> On 2023/4/18 22:57, Arseniy Krasnov wrote:    
>>>>>>>>> [ EXTERNAL EMAIL ]
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 18.04.2023 16:25, Miquel Raynal wrote:    
>>>>>>>>>> Hi Arseniy,
>>>>>>>>>>    
>>>>>>>>>>>>> Hello again @Liang @Miquel!
>>>>>>>>>>>>>
>>>>>>>>>>>>> One more question about OOB access, as I can see current driver uses the following
>>>>>>>>>>>>> callbacks:
>>>>>>>>>>>>>
>>>>>>>>>>>>>       nand->ecc.write_oob_raw = nand_write_oob_std;
>>>>>>>>>>>>>       nand->ecc.write_oob = nand_write_oob_std;
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Function 'nand_write_oob_std()' writes data to the end of the page. But as I
>>>>>>>>>>>>> can see by dumping 'data_buf' during read, physical layout of each page is the
>>>>>>>>>>>>> following (1KB ECC):
>>>>>>>>>>>>>
>>>>>>>>>>>>> 0x000: [         1 KB of data        ]
>>>>>>>>>>>>> 0x400: [ 2B user data] [ 14B ECC code]
>>>>>>>>>>>>> 0x410: [         1 KB of data        ]    (A)
>>>>>>>>>>>>> 0x810: [ 2B user data] [ 14B ECC code]
>>>>>>>>>>>>> 0x820: [        32B unused           ]
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> So, after 'nand_write_oob_std()' (let data be sequence from [0x0 ... 0x3f]),
>>>>>>>>>>>>> page will look like this:
>>>>>>>>>>>>>
>>>>>>>>>>>>> 0x000: [             0xFF            ]
>>>>>>>>>>>>> 0x400: [           ........          ]
>>>>>>>>>>>>> 0x7f0: [             0xFF            ]
>>>>>>>>>>>>> 0x800: [ 00 .......................  ]
>>>>>>>>>>>>> 0x830: [ ........................ 3f ]
>>>>>>>>>>>>>
>>>>>>>>>>>>> Here we have two problems:
>>>>>>>>>>>>> 1) Attempt to display raw data by 'nanddump' utility produces a little bit
>>>>>>>>>>>>>       invalid output, as driver relies on layout (A) from above. E.g. OOB data
>>>>>>>>>>>>>       is at 0x400 and 0x810. Here is an example (attempt to write 0x11 0x22 0x33 0x44):
>>>>>>>>>>>>>
>>>>>>>>>>>>> 0x000007f0: 11 22 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: 33 44 ff ff ff ff ff ff ff ff ff ff ff ff ff ff  |3D..............|
>>>>>>>>>>>>>      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  |................|
>>>>>>>>>>>>>    
>>>>>>>>>>>> Hi Arseniy,
>>>>>>>>>>>>
>>>>>>>>>>>> I realized the write_oob_raw() and write_oob() are wrong in meson_nand.c. I suggest both of them should be reworked and follow the format of meson nand controller. i.e. firstly format the data in Layout (A) and then write. reading is firstly reading the data of layout (A) and then compost the layout (B).    
>>>>>>>>>>>
>>>>>>>>>>> IIUC after such writing only OOB (e.g. user bytes) according layout (A), hw will also write ECC codes, so
>>>>>>>>>>> it will be impossible to write data to this page later, because we cannot update ECC codes properly for the newly
>>>>>>>>>>> written data (we can't update bits from 0 to 1).
>>>>>>>>>>>    
>>>>>>>>>>>>
>>>>>>>>>>>>    
>>>>>>>>>>>>>
>>>>>>>>>>>>> 2) Attempt to read data in ECC mode will fail, because IIUC page is in dirty
>>>>>>>>>>>>>       state (I mean was written at least once) and NAND controller tries to use
>>>>>>>>>>>>>       ECC codes at 0x400 and 0x810, which are obviously broken in this case. Thus    
>>>>>>>>>>>>
>>>>>>>>>>>> As i said above, write_oob_raw() and write_oob() should be reworked.
>>>>>>>>>>>> i don't know what do you mean page was written at least once. anyway the page should be written once, even just write_oob_raw().    
>>>>>>>>>>>
>>>>>>>>>>> Sorry, You mean that after OOB write, we cannot write to the data area (e.g. 0x0 .. 0x810) until page will be erased? For example
>>>>>>>>>>> JFFS2 writes to OOB own markers, then it tries to write to the data area of such page.    
>>>>>>>>>
>>>>>>>>> @Liang, I'll describe current test case in details:
>>>>>>>>> 1) I have erased page, I can read it in  both raw and ecc modes - no problem (it is full of 0xFF).
>>>>>>>>> 2) I (JFFS2 for example) want to write only OOB - let it be clean markers.
>>>>>>>>> 3) I use raw write to the needed page (please correct me if i'm wrong). Four bytes
>>>>>>>>>      at 0x400 and 0x810 are updated. All other bytes still 0xff.
>>>>>>>>> 4) Now, when i'm trying to read this page in ECC mode, I get ECC errors: IIUC this
>>>>>>>>>      happens because from controller point of view ECC codes are invalid for current
>>>>>>>>>      data (all ECCs are 0xff). Is this behaviour is ok?    
>>>>>>>>
>>>>>>>> Yes, it is exactly reported ECC errors.    
>>>>>>>
>>>>>>> I see, so if we write OOB (e.g. using raw mode), there is no way to read this page in ECC mode later? And the    
>>>>>
>>>>> Of course, there is no ECC parity bytes in it; or raw write the data with the ECC parity bytes per the layout (A) you describe above.
>>>>>     
>>>>
>>>> But don't it looks like strange? Just writing OOB makes page unreadable? May be it is better to move OOB data
>>>> out of ECC area as I suggested in v2?
>>>>  
>>>>>>> only way to make it readable is to write it in ECC mode, but before this write, we need to read it's
>>>>>>> user's byte (from previous OOB write) in raw mode, put it to info buf (as user's bytes) and write this page. In this
>>>>>>> case NAND controller will generate ECC codes including user's byte and page become readable in ECC mode
>>>>>>> again.    
>>>>>
>>>>> yes, you are right.
>>>>>     
>>>>>>>    
>>>>>>>>    
>>>>>>>>> 5) Ok, don't care on these ECC errors, let's go further.
>>>>>>>>> 6) I'm going to write same page in ECC mode - how to do it correctly? There is already
>>>>>>>>>      4 OOB bytes, considered to be covered by ECC (but in fact now - ECC area is FFed).    
>>>>>>>>
>>>>>>>> If step 4 has excuted "program" command at the page (nand_write_oob_std() does), it can't be written again before erasing the page(block). so we have to read the whole page in the ddr and change the content, erase block, write it again.
>>>>>>>>
>>>>>>>> I don't think Jffs2 has the same steps (1-6) as you said above. are you sure that happes on Jffs2 or just an example?    
>>>>>>
>>>>>>    
>>>>>>>
>>>>>>> I just checked JFFS2 mount/umount again, here is what i see:
>>>>>>> 0) First attempt to mount JFFS2.
>>>>>>> 1) It writes OOB to page N (i'm using raw write). It is cleanmarker value 0x85 0x19 0x03 0x20. Mount is done.
>>>>>>> 2) Umount JFFS2. Done.
>>>>>>> 3) Second attempt to mount JFFS2.
>>>>>>> 4) It reads OOB from page N (i'm using raw read). Value is 0x85 0x19 0x03 0x20. Done.
>>>>>>> 5) It reads page N in ECC mode, and i get:
>>>>>>>      jffs2: mtd->read(0x100 bytes from N) returned ECC error
>>>>>>> 6) Mount failed.
>>>>>>>
>>>>>>> We already had problem which looks like this on another device. Solution was to use OOB area which is
>>>>>>> not covered by ECC for JFFS2 cleanmarkers.    
>>>>>
>>>>> ok, so there is not ECC parity bytes and mtd->read() returns ECC error.
>>>>> does it have to use raw write/read on step 1) and 4)?
>>>>>     
>>>>
>>>> If i'm using non raw access to OOB, for example write OOB (user bytes) in ECC mode, then
>>>> steps 1) and 4) and 5) passes ok, but write to this page will be impossible (for example JFFS2
>>>> writes to such pages later) - we can't update ECC codes properly without erasing whole page.
>>>> Write operation will be done without problem, but read will trigger ECC errors due to broken
>>>> ECC codes.
>>>>
>>>> In general problem that we discuss is that in current implementation data and OOB conflicts
>>>> with each other by sharing same ECC codes, these ECC codes could be written only once (without
>>>> erasing), while data and OOB has different callbacks to access and thus supposed to work
>>>> separately.  
>>>
>>> The fact that there might be helpers just for writing OOB areas or just
>>> in-band areas are optimizations. NAND pages are meant to be written a
>>> single time, no matter what portion you write. In some cases, it is
>>> possible to perform subpage writes if the chip supports it. Pages may
>>> be split into several areas which cover a partial in-band area *and* a
>>> partial OOB area. If you write into the in-band *or* out-of-band areas
>>> of a given subpage, you *cannot* write the other part later without  
>>
>> Thanks for details! So in case of JFFS2 it looks like strange, that it tries
>> to write page after writing clean markers to it before? In the old vendor's
>> driver OOB write callback is suppressed by return 0 always and JFFS2 works
>> correctly.
> 
> Can you point the code you're mentioning? (both what JFFS2 which looks
> strange to you and the old vendor hack)

Here is version of the old vendor's driver:

https://github.com/kszaq/linux-amlogic/blob/master_new_amports/drivers/amlogic/nand/nand/aml_nand.c#L3260

In my version there is no BUG() there, but it is same driver for the same chip.

About JFFS2 - i didn't check its source code, but what I can see using printk(), is that it first
tries to write cleanmarker using OOB write callback. Then later it tries to write to this page, so
may be it is unexpected behaviour of JFFS2?

Thanks, Arseniy

> 
> 
> Thanks,
> Miquèl

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