Message-ID: <20230601103111.6840acc0@xps-13>
Date:   Thu, 1 Jun 2023 10:31:11 +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 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.

> 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.

>  }
>  
>  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?

> +}
> +
> +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?

> +
> +	/* 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.

> +		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.

> +		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?

> +		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.

> -	oobregion->length = 2;
> +	/* Split rest of OOB area (not covered by ECC engine) per each
> +	 * ECC section. This will be OOB data available to user.
> +	 */
> +	oobregion->offset = (section + nand->ecc.steps) * (2 + nand->ecc.bytes);

This is not possible, see above.

> +	oobregion->length = oob_bytes / nand->ecc.steps;
>  
>  	return 0;
>  }
> @@ -1220,12 +1320,12 @@ static int meson_nand_attach_chip(struct nand_chip *nand)
>  	nand->ecc.engine_type = NAND_ECC_ENGINE_TYPE_ON_HOST;
>  	nand->ecc.write_page_raw = meson_nfc_write_page_raw;
>  	nand->ecc.write_page = meson_nfc_write_page_hwecc;
> -	nand->ecc.write_oob_raw = nand_write_oob_std;
> -	nand->ecc.write_oob = nand_write_oob_std;
>  
> +	nand->ecc.write_oob_raw = meson_nfc_write_oob;
> +	nand->ecc.write_oob = meson_nfc_write_oob;
>  	nand->ecc.read_page_raw = meson_nfc_read_page_raw;
>  	nand->ecc.read_page = meson_nfc_read_page_hwecc;
> -	nand->ecc.read_oob_raw = meson_nfc_read_oob_raw;
> +	nand->ecc.read_oob_raw = meson_nfc_read_oob;
>  	nand->ecc.read_oob = meson_nfc_read_oob;
>  
>  	if (nand->options & NAND_BUSWIDTH_16) {


Thanks,
Miquèl

Powered by blists - more mailing lists