Message-ID: <20230413140314.qioyvmzvmcv43kzk@CAB-WSD-L081021>
Date:   Thu, 13 Apr 2023 17:03:14 +0300
From:   Dmitry Rokosov <ddrokosov@...rdevices.ru>
To:     Miquel Raynal <miquel.raynal@...tlin.com>
CC:     Liang Yang <liang.yang@...ogic.com>,
        Arseniy Krasnov <avkrasnov@...rdevices.ru>,
        Richard Weinberger <richard@....at>,
        Vignesh Raghavendra <vigneshr@...com>,
        Neil Armstrong <neil.armstrong@...aro.org>,
        Kevin Hilman <khilman@...libre.com>,
        Jerome Brunet <jbrunet@...libre.com>,
        Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
        Jianxin Pan <jianxin.pan@...ogic.com>,
        Yixun Lan <yixun.lan@...ogic.com>, <oxffffaa@...il.com>,
        <kernel@...rdevices.ru>, <linux-mtd@...ts.infradead.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-amlogic@...ts.infradead.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1 4/5] mtd: rawnand: meson: clear OOB buffer before read

On Thu, Apr 13, 2023 at 12:29:16PM +0200, Miquel Raynal wrote:
> Hi Dmitry,
> 
> ddrokosov@...rdevices.ru wrote on Thu, 13 Apr 2023 12:27:06 +0300:
> 
> > On Wed, Apr 12, 2023 at 10:56:03PM +0200, Miquel Raynal wrote:
> > > Hi Dmitry,
> > > 
> > > ddrokosov@...rdevices.ru wrote on Wed, 12 Apr 2023 22:15:48 +0300:
> > >   
> > > > On Wed, Apr 12, 2023 at 10:04:28PM +0800, Liang Yang wrote:  
> > > > > Hi Miquel and Arseniy,
> > > > > 
> > > > > On 2023/4/12 20:57, Miquel Raynal wrote:    
> > > > > > [ EXTERNAL EMAIL ]
> > > > > > 
> > > > > > Hi Arseniy,
> > > > > > 
> > > > > > avkrasnov@...rdevices.ru wrote on Wed, 12 Apr 2023 15:22:26 +0300:
> > > > > >     
> > > > > > > On 12.04.2023 15:18, Miquel Raynal wrote:    
> > > > > > > > Hi Arseniy,
> > > > > > > > 
> > > > > > > > avkrasnov@...rdevices.ru wrote on Wed, 12 Apr 2023 13:14:52 +0300:    
> > > > > > > > > On 12.04.2023 12:36, Miquel Raynal wrote:    
> > > > > > > > > > Hi Arseniy,
> > > > > > > > > > 
> > > > > > > > > > avkrasnov@...rdevices.ru wrote on Wed, 12 Apr 2023 12:20:55 +0300:    
> > > > > > > > > > > On 12.04.2023 10:44, Miquel Raynal wrote:    
> > > > > > > > > > > > Hi Arseniy,
> > > > > > > > > > > > 
> > > > > > > > > > > > AVKrasnov@...rdevices.ru wrote on Wed, 12 Apr 2023 09:16:58 +0300:    
> > > > > > > > > > > > > This NAND reads only few user's bytes in ECC mode (not full OOB), so    
> > > > > > > > > > > > 
> > > > > > > > > > > > "This NAND reads" does not look right, do you mean "Subpage reads do
> > > > > > > > > > > > not retrieve all the OOB bytes,"?    
> > > > > > > > > > > > > fill OOB buffer with zeroes to not return garbage from previous reads
> > > > > > > > > > > > > to user.
> > > > > > > > > > > > > Otherwise 'nanddump' utility prints something like this for just erased
> > > > > > > > > > > > > page:
> > > > > > > > > > > > > 
> > > > > > > > > > > > > ...
> > > > > > > > > > > > > 0x000007f0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
> > > > > > > > > > > > >    OOB Data: ff ff ff ff 00 00 ff ff 80 cf 22 99 cb ad d3 be
> > > > > > > > > > > > >    OOB Data: 63 27 ae 06 16 0a 2f eb bb dd 46 74 41 8e 88 6e
> > > > > > > > > > > > >    OOB Data: 38 a1 2d e6 77 d4 05 06 f2 a5 7e 25 eb 34 7c ff
> > > > > > > > > > > > >    OOB Data: 38 ea de 14 10 de 9b 40 33 16 6a cc 9d aa 2f 5e
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Signed-off-by: Arseniy Krasnov <AVKrasnov@...rdevices.ru>
> > > > > > > > > > > > > ---
> > > > > > > > > > > > >   drivers/mtd/nand/raw/meson_nand.c | 5 +++++
> > > > > > > > > > > > >   1 file changed, 5 insertions(+)
> > > > > > > > > > > > > 
> > > > > > > > > > > > > diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
> > > > > > > > > > > > > index f84a10238e4d..f2f2472cb511 100644
> > > > > > > > > > > > > --- a/drivers/mtd/nand/raw/meson_nand.c
> > > > > > > > > > > > > +++ b/drivers/mtd/nand/raw/meson_nand.c
> > > > > > > > > > > > > @@ -858,9 +858,12 @@ static int meson_nfc_read_page_sub(struct nand_chip *nand,
> > > > > > > > > > > > >   static int meson_nfc_read_page_raw(struct nand_chip *nand, u8 *buf,
> > > > > > > > > > > > >   				   int oob_required, int page)
> > > > > > > > > > > > >   {
> > > > > > > > > > > > > +	struct mtd_info *mtd = nand_to_mtd(nand);
> > > > > > > > > > > > >   	u8 *oob_buf = nand->oob_poi;
> > > > > > > > > > > > >   	int ret;
> > > > > > > > > > > > > +	memset(oob_buf, 0, mtd->oobsize);    
> > > > > > > > > > > > 
> > > > > > > > > > > > I'm surprised raw reads do not read the entire OOB?    
> > > > > > > > > > > 
> > > > > > > > > > > Yes! Seems in case of raw access (what i see in this driver) number of OOB bytes read
> > > > > > > > > > > still depends on ECC parameters: for each portion of data covered with ECC code we can
> > > > > > > > > > > read it's ECC code and "user bytes" from OOB - it is what i see by dumping DMA buffer by
> > > > > > > > > > > printk(). For example I'm working with 2K NAND pages, each page has 2 x 1K ECC blocks.
> > > > > > > > > > > For each ECC block I have 16 OOB bytes which I can access by read/write. Each 16 bytes
> > > > > > > > > > > contains 2 bytes of user's data and 14 bytes ECC codes. So when I read page in raw mode
> > > > > > > > > > > controller returns 32 bytes (2 x (2 + 14)) of OOB. While OOB is reported as 64 bytes.    
> > > > > > > > > > 
> > > > > > > > > > In all modes, when you read OOB, you should get the full OOB. The fact
> > > > > > > > > > that ECC correction is enabled or disabled does not matter. If the NAND
> > > > > > > > > > features OOB sections of 64 bytes, you should get the 64 bytes.
> > > > > > > > > > 
> > > > > > > > > > What happens sometimes, is that some of the bytes are not protected
> > > > > > > > > > against bitflips, but the policy is to return the full buffer.    
> > > > > > > > > 
> > > > > > > > > Ok, so to clarify case for this NAND controller:
> > > > > > > > > 1) In both ECC and raw modes i need to return the same raw OOB data (e.g. user bytes
> > > > > > > > >     + ECC codes)?    
> > > > > > > > 
> > > > > > > > Well, you need to cover the same amount of data, yes. But in the ECC
> > > > > > > > case the data won't be raw (at least not all of it).    
> > > > > > > 
> > > > > > > So "same amount of data", in ECC mode current implementation returns only user OOB bytes (e.g.
> > > > > > > OOB data excluding ECC codes), in raw it returns user bytes + ECC codes. IIUC correct
> > > > > > > behaviour is to always return user bytes + ECC codes as OOB data even in ECC mode ?    
> > > > > > 
> > > > > > If the page are 2k+64B you should read 2k+64B when OOB are requested.
> > > > > > 
> > > > > > If the controller only returns 2k+32B, then perform a random read to
> > > > > > just move the read pointer to mtd->size + mtd->oobsize - 32 and
> > > > > > retrieve the missing 32 bytes?    
> > > > > 
> > > > > 1) raw read can read out the whole page data 2k+64B, decided by the len in
> > > > > the controller raw read command:
> > > > > 	cmd = (len & GENMASK(5, 0)) | scrambler | DMA_DIR(dir);
> > > > > after that, the missing oob bytes(not used) can be copied from
> > > > > meson_chip->data_buf. so the implementation of meson_nfc_read_page_raw() is
> > > > > like this if need.
> > > > > 	{
> > > > > 		......
> > > > > 		meson_nfc_read_page_sub(nand, page, 1);
> > > > > 		meson_nfc_get_data_oob(nand, buf, oob_buf);
> > > > > 		oob_len = (nand->ecc.bytes + 2) * nand->ecc.steps;
> > > > > 		memcpy(oob_buf + oob_len, meson_chip->data_buf + oob_len, mtd->oobsize -
> > > > > oob_len);
> > > > > 
> > > > > 	}
> > > > > 2) In ECC mode, the controller can't bring back the missing OOB bytes. it
> > > > > can read out the user bytes and ecc bytes per meson_ooblayout_ops define.
> > > > >     
> > > > 
> > > > How does the Meson controller know the actual NAND flash layout when the
> > > > OOB is split into protected and unprotected areas, such as Free and ECC
> > > > areas? If the controller has a static OOB layout, where is the mapping
> > > > located?  
> > > 
> > > It's usually a set of values hardcoded in the driver. It's a per
> > > geometry set.
> > >   
> > 
> > Sorry, I'm still confused. Before I developed spinand drivers, the OOB layout
> > was located on the flash driver side.
> 
> The spinand subsystem is different, most of the chips have on-die ECC,
> which means the ECC layout is most of the time per-chip.
> 
> In the raw NAND layer, most of the time the ECC engine is merged with
> the NAND controller and thus the list of available layouts depend on
> the controller. But these layouts often adapt to the NAND chip
> geometry, so you can only now which layout to use after identifying the
> chip (there is an ->attach_chip() hook to handle things related to
> geometry after controller base initialization.
> 
> > Do you mean if the OOB geometry in the rawnand subsystem is under the
> > responsibility of the controller driver?
> 
> The geometry is NAND chip specific.
> The OOB layout depends on the chip geometry, the ECC engine
> capabilities and configuration (like the strength).
> The discovery of the geometry is performed by the core (using raw NAND
> controller driver callbacks of course).
> If on-host ECC engine is picked (the default in the raw NAND
> subsystem), then the controller driver picks the correct OOB layout and
> refuses to probe otherwise.
> 
> Hope its clearer now :)

Yeah, pretty clear!

I wanted to express my special thanks to you for sharing such detailed
information about the raw NAND layer! Your insights will be incredibly
helpful for me and will provide valuable context that I would not have
otherwise had.

Thank you so much for taking the time to share your expertise with me!

-- 
Thank you,
Dmitry

Powered by blists - more mailing lists