Message-ID: <20230605114850.77cf9197@xps-13>
Date:   Mon, 5 Jun 2023 11:48:50 +0200
From:   Miquel Raynal <miquel.raynal@...tlin.com>
To:     Arseniy Krasnov <avkrasnov@...rdevices.ru>
Cc:     Liang Yang <liang.yang@...ogic.com>,
        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>,
        <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: [RFC PATCH v5 3/6] mtd: rawnand: meson: only expose unprotected
 user OOB bytes

Hi Arseniy,

avkrasnov@...rdevices.ru wrote on Fri, 2 Jun 2023 11:53:47 +0300:

> Hello Miquel, thanks for review!
> 
> On 01.06.2023 11:31, Miquel Raynal wrote:
> > Hi Arseniy,
> > 
> > AVKrasnov@...rdevices.ru wrote on Thu, 1 Jun 2023 09:18:46 +0300:
> >   
> >> This moves free bytes of OOB to non-protected ECC area. It is needed to  
> > 
> > As we discussed, I expect this commit to just change the OOB layout to
> > expose unprotected OOB bytes to the user, that is the only change this
> > commit should carry. If that does not work, you should add a
> > preparation patch.  
> 
> Ok, but I thought, if i change only OOB layout, e.g. update 'free' callback of
> mtd_ooblayout_ops, I also need to implement code which performs read/write
> according new layout (it must be done in a single patch)?

No, this is orthogonal.

The driver must read the the whole OOB area (and perhaps reorder the
data), but you should not make any decision regarding what bytes you
want or not want to expose.

Then, the user (no matter what "user" is here) will decide how to deal
with the data.

> Main thing is:
> 
> I guess that general confuse with this patch is that You consider
> that we change only OOB layout by moving user bytes out of ECC area, but at the same
> time I also increased size of OOB from 4 bytes (e.g. 2 x 2 bytes clean markers)
> to 32 bytes (e.g. tail of page after data and ECC codes), so if this
> assumption is correct, in the next version I won't change size of user area in
> OOB, thus this patch will be reduced as some comments from this review.

Exposing only 4 bytes was a mistake in the first place, please fix this
in a dedicated patch.

> >> make JFFS2 works correctly with this NAND controller. Problem fires when
> >> JFFS2 driver writes cleanmarker to some page and later it tries to write
> >> to this page - write will be done successfully, but after that such page
> >> becomes unreadable due to invalid ECC codes. This happens because second
> >> write needs to update ECC codes, but it is impossible to do it correctly
> >> without block erase. So idea of this patch is to use the unprotected OOB
> >> area to store the cleanmarkers, so that they can be written by the
> >> filesystem without caring much about the page being empty or not: the
> >> ECC codes will not be written anyway.
> >>
> >> JFFS2 is only useful on tiny NAND devices, where UBI does not fit, which
> >> are usually true SLC flashes, with the capability of writing a page with
> >> empty (0xFF) data, and still be able to write actual data to it later in
> >> a second write.
> >>
> >> Signed-off-by: Arseniy Krasnov <AVKrasnov@...rdevices.ru>
> >> ---
> >>  Changelog v4->v5:
> >>  * Drop cosmetic changes from this patch.
> >>  * Do not ignore ECC protected user bytes provided by hw. Even these
> >>    bytes are out of user area of OOB, its values are still read from
> >>    the provided OOB buffer and written by hardware. Same behaviour is
> >>    preserved for read access - such bytes are read from DMA buffer and
> >>    placed to OOB buffer.
> >>  * OOB read and write become more lightweight because I removed heavy
> >>    READ0 and PAGEPROG command from it (both commands are still sent
> >>    when OOB access is performed using OOB callbacks). In case of page
> >>    read/write OOB data is handled in the internal SRAM of the controller.
> >>  * Commit message updated.
> >>  * Temporary buffer for OOB read/write is removed. Seems everything
> >>    works correctly without it.
> >>
> >>  drivers/mtd/nand/raw/meson_nand.c | 134 ++++++++++++++++++++++++++----
> >>  1 file changed, 117 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
> >> index 82a629025adc..e42c28be02f3 100644
> >> --- a/drivers/mtd/nand/raw/meson_nand.c
> >> +++ b/drivers/mtd/nand/raw/meson_nand.c
> >> @@ -358,8 +358,11 @@ static u8 *meson_nfc_data_ptr(struct nand_chip *nand, int i)
> >>  static void meson_nfc_get_data_oob(struct nand_chip *nand,
> >>  				   u8 *buf, u8 *oobbuf)
> >>  {
> >> +	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
> >> +	struct mtd_info *mtd = nand_to_mtd(nand);
> >>  	int i, oob_len = 0;
> >>  	u8 *dsrc, *osrc;
> >> +	u8 *oobtail;
> >>  
> >>  	oob_len = nand->ecc.bytes + 2;
> >>  	for (i = 0; i < nand->ecc.steps; i++) {
> >> @@ -368,17 +371,27 @@ static void meson_nfc_get_data_oob(struct nand_chip *nand,
> >>  			memcpy(buf, dsrc, nand->ecc.size);
> >>  			buf += nand->ecc.size;
> >>  		}
> >> +
> >>  		osrc = meson_nfc_oob_ptr(nand, i);
> >>  		memcpy(oobbuf, osrc, oob_len);
> >>  		oobbuf += oob_len;
> >>  	}
> >> +
> >> +	oobtail = meson_chip->data_buf + nand->ecc.steps *
> >> +		  (nand->ecc.size + oob_len);
> >> +
> >> +	/* 'oobbuf' points to the start of user area. */
> >> +	memcpy(oobbuf, oobtail, mtd->oobsize - nand->ecc.steps * oob_len);
> >>  }
> >>  
> >>  static void meson_nfc_set_data_oob(struct nand_chip *nand,
> >>  				   const u8 *buf, u8 *oobbuf)
> >>  {
> >> +	struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
> >> +	struct mtd_info *mtd = nand_to_mtd(nand);
> >>  	int i, oob_len = 0;
> >>  	u8 *dsrc, *osrc;
> >> +	u8 *oobtail;
> >>  
> >>  	oob_len = nand->ecc.bytes + 2;
> >>  	for (i = 0; i < nand->ecc.steps; i++) {
> >> @@ -391,6 +404,12 @@ static void meson_nfc_set_data_oob(struct nand_chip *nand,
> >>  		memcpy(osrc, oobbuf, oob_len);
> >>  		oobbuf += oob_len;
> >>  	}
> >> +
> >> +	oobtail = meson_chip->data_buf + nand->ecc.steps *
> >> +		  (nand->ecc.size + oob_len);  
> > 
> > This is always targeting the same area, so it looks strange to me.
> >   
> >> +
> >> +	/* 'oobbuf' points to the start of user area. */
> >> +	memcpy(oobtail, oobbuf, mtd->oobsize - nand->ecc.steps * oob_len);  
> > 
> > TBH I don't get what you do here.  
> 
> This code works in raw mode and places OOB data from provided OOB buffer to DMA buffer.
> This is because number of user bytes is increased in this patch.
> 
> >   
> >>  }
> >>  
> >>  static int meson_nfc_queue_rb(struct nand_chip *nand, int timeout_ms)
> >> @@ -433,7 +452,7 @@ static void meson_nfc_set_user_byte(struct nand_chip *nand, u8 *oob_buf)
> >>  	__le64 *info;
> >>  	int i, count;
> >>  
> >> -	for (i = 0, count = 0; i < nand->ecc.steps; i++, count += 2) {
> >> +	for (i = 0, count = 0; i < nand->ecc.steps; i++, count += (nand->ecc.bytes + 2)) {
> >>  		info = &meson_chip->info_buf[i];
> >>  		*info |= oob_buf[count];
> >>  		*info |= oob_buf[count + 1] << 8;
> >> @@ -446,7 +465,7 @@ static void meson_nfc_get_user_byte(struct nand_chip *nand, u8 *oob_buf)
> >>  	__le64 *info;
> >>  	int i, count;
> >>  
> >> -	for (i = 0, count = 0; i < nand->ecc.steps; i++, count += 2) {
> >> +	for (i = 0, count = 0; i < nand->ecc.steps; i++, count += (nand->ecc.bytes + 2)) {
> >>  		info = &meson_chip->info_buf[i];
> >>  		oob_buf[count] = *info;
> >>  		oob_buf[count + 1] = *info >> 8;
> >> @@ -638,6 +657,84 @@ static int meson_nfc_rw_cmd_prepare_and_execute(struct nand_chip *nand,
> >>  	return 0;
> >>  }
> >>  
> >> +static u32 meson_nfc_oob_free_bytes(struct nand_chip *nand)
> >> +{
> >> +	struct mtd_info *mtd = nand_to_mtd(nand);
> >> +
> >> +	return mtd->oobsize - nand->ecc.steps * (nand->ecc.bytes + 2);  
> > 
> > This looks like a static value, just save it somewhere instead of
> > recomputing it?  
> 
> Ack
> 
> >   
> >> +}
> >> +
> >> +static int meson_nfc_write_oob(struct nand_chip *nand, int page)
> >> +{
> >> +	struct mtd_info *mtd = nand_to_mtd(nand);
> >> +	u32 page_size = mtd->writesize + mtd->oobsize;
> >> +	u32 oob_bytes = meson_nfc_oob_free_bytes(nand);
> >> +	u8 *oob_buf;
> >> +	int ret;
> >> +
> >> +	if (!oob_bytes)
> >> +		return 0;  
> > 
> > Can this happen?  
> 
> Ack, seems forget to remove it
> 
> >   
> >> +
> >> +	/* Called as OOB write helper, will send NAND_CMD_PAGEPROG. */
> >> +	if (page != -1) {
> >> +		ret = nand_prog_page_begin_op(nand, page, 0, NULL, 0);
> >> +		if (ret)
> >> +			return ret;
> >> +	}
> >> +
> >> +	oob_buf = nand->oob_poi;
> >> +
> >> +	ret = nand_change_write_column_op(nand, page_size - oob_bytes,
> >> +					  oob_buf + (mtd->oobsize - oob_bytes),
> >> +					  oob_bytes, false);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	return (page != -1) ? nand_prog_page_end_op(nand) : 0;
> >> +}
> >> +
> >> +static int meson_nfc_read_oob(struct nand_chip *nand, int page)
> >> +{
> >> +	struct mtd_info *mtd = nand_to_mtd(nand);
> >> +	u8 *oob_buf = nand->oob_poi;
> >> +	u32 oob_bytes;
> >> +	u32 page_size;
> >> +	int ret;
> >> +	int i;
> >> +
> >> +	/* Called as OOB read helper, send NAND_CMD_READ0. */
> >> +	if (page != -1) {  
> > 
> > I don't like this logic with the "-1", it really hides what the
> > controller needs to do, if you need a helper to send a command, then
> > create that helper and give it a decent name.  
> 
> I see, so I think I need to implement this in the following way:
> 1) For OOB callback it always sends NAND_CMD_READ0 (e.g. without any 'if')
> 2) For read OOB with data page we don't need to send NAND_CMD_READ0. (also without any 'if')
> 
> >   
> >> +		ret = nand_read_page_op(nand, page, 0, NULL, 0);
> >> +		if (ret)
> >> +			return ret;
> >> +	}
> >> +
> >> +	/* Read ECC codes and user bytes. */
> >> +	for (i = 0; i < nand->ecc.steps; i++) {
> >> +		u32 ecc_offs = nand->ecc.size * (i + 1) +
> >> +			       (nand->ecc.bytes + 2) * i;
> >> +
> >> +		ret = nand_change_read_column_op(nand, ecc_offs,
> >> +						 oob_buf + i * (nand->ecc.bytes + 2),
> >> +						 (nand->ecc.bytes + 2), false);
> >> +		if (ret)
> >> +			return ret;
> >> +	}
> >> +
> >> +	oob_bytes = meson_nfc_oob_free_bytes(nand);
> >> +
> >> +	if (!oob_bytes)
> >> +		return 0;
> >> +
> >> +	page_size = mtd->writesize + mtd->oobsize;
> >> +
> >> +	ret = nand_change_read_column_op(nand, page_size - oob_bytes,
> >> +					 oob_buf + (mtd->oobsize - oob_bytes),
> >> +					 oob_bytes, false);
> >> +
> >> +	return ret;
> >> +}
> >> +
> >>  static int meson_nfc_write_page_sub(struct nand_chip *nand,
> >>  				    int page, int raw)
> >>  {
> >> @@ -674,6 +771,12 @@ static int meson_nfc_write_page_sub(struct nand_chip *nand,
> >>  				     NFC_CMD_SCRAMBLER_DISABLE);
> >>  	}
> >>  
> >> +	if (!raw) {  
> > 
> > Why this check?
> > 
> > You should instead propagate the oob_required field and check that
> > value I believe.  
> 
> 
> This check is for ECC mode, because in this mode we write user bytes of OOB.
> ECC bytes of OOB are written by hardware.

Just provide the buffer. The ECC engine will smash data if there was
any there. Otherwise it will fill the holes. It's expected. Don't try
to be smarter than you should :)

> I think I made a mistake, because
> I need new callback to write OOB in raw mode - it will write both ECC and user
> parts,

There is no such thing as user and ECC part at the driver level. You
get a buffer, you need to write it to the flash.

The user expects:

| data | OOB |

The controller expects something like:

| data1 | OOB1 | data2 | OOB2 |

So just perform the reordering between data and OOB in the DMA buffer,
that is _all_.

> in current version I write only user part in raw mode.
> 
> >   
> >> +		ret = meson_nfc_write_oob(nand, -1);
> >> +		if (ret)
> >> +			return ret;
> >> +	}
> >> +
> >>  	cmd = nfc->param.chip_select | NFC_CMD_CLE | NAND_CMD_PAGEPROG;
> >>  	writel(cmd, nfc->reg_base + NFC_REG_CMD);
> >>  	meson_nfc_queue_rb(nand, PSEC_TO_MSEC(sdr->tPROG_max));
> >> @@ -834,17 +937,10 @@ static int meson_nfc_read_page_hwecc(struct nand_chip *nand, u8 *buf,
> >>  		memcpy(buf, meson_chip->data_buf, mtd->writesize);
> >>  	}
> >>  
> >> -	return bitflips;
> >> -}
> >> -
> >> -static int meson_nfc_read_oob_raw(struct nand_chip *nand, int page)
> >> -{
> >> -	return meson_nfc_read_page_raw(nand, NULL, 1, page);
> >> -}
> >> +	if (oob_required && ret)  
> > 
> > Unclear why you check ret here?

In general, if (ret) means there is an error.

Please consider using:

if (ret)
	goto error path;

do something else;

> >   
> 
> If read was successful, we read OOB. If not - there is no sense in it.
> 
> >> +		meson_nfc_read_oob(nand, -1);
> >>  
> >> -static int meson_nfc_read_oob(struct nand_chip *nand, int page)
> >> -{
> >> -	return meson_nfc_read_page_hwecc(nand, NULL, 1, page);
> >> +	return bitflips;
> >>  }
> >>  
> >>  static bool meson_nfc_is_buffer_dma_safe(const void *buffer)
> >> @@ -987,12 +1083,16 @@ static int meson_ooblayout_free(struct mtd_info *mtd, int section,
> >>  				struct mtd_oob_region *oobregion)
> >>  {
> >>  	struct nand_chip *nand = mtd_to_nand(mtd);
> >> +	u32 oob_bytes = meson_nfc_oob_free_bytes(nand);
> >>  
> >>  	if (section >= nand->ecc.steps)
> >>  		return -ERANGE;
> >>  
> >> -	oobregion->offset = section * (2 + nand->ecc.bytes);  
> > 
> > The first two bytes of OOB are reserved for the bad block markers. This
> > is not related to your controller.  
> 
> I think first two bytes (in fact there are 4 bytes at positions 0, 1, 16 and 17)
> is considered by hardware as user bytes covered by ECC.

The two first bytes should not be available. They are not "ECC" bytes,
they are not "free" bytes. None of these two callbacks should give
access to these two bytes reserved for bad block markers.

Just to be clear: "ECC bytes" as in "meson_ooblayout_ecc" do *not* mean
"these are the protected bytes". They mean "these are the bytes in OOB
the hardware ECC engine will use to place its own data to make the
recovery process work".

Thanks,
Miquèl

Powered by blists - more mailing lists