Re: [PATCH 2/3] mtd: rawnand: Export nand_read_page_hwecc_oob_first()

[Paul Cercueil <paul@xxxxxxxxxxxxxxx>]

Hi Miquel,

Le ven., oct. 15 2021 at 08:13:13 +0200, Miquel Raynal 
<miquel.raynal@xxxxxxxxxxx> a écrit :
> Hi Paul,
>
> paul@xxxxxxxxxxxxxxx wrote on Sat,  9 Oct 2021 20:49:51 +0200:
>
> >  Move the function nand_read_page_hwecc_oob_first() (previously
> >  nand_davinci_read_page_hwecc_oob_first()) to nand_base.c, and 
> > export it
> >  as a GPL symbol, so that it can be used by more modules.
> >
> >  Cc: <stable@xxxxxxxxxxxxxxx> # v5.2
> >  Fixes: a0ac778eb82c ("mtd: rawnand: ingenic: Add support for the 
> > JZ4740")
> >  Signed-off-by: Paul Cercueil <paul@xxxxxxxxxxxxxxx>
> >  ---
> >   drivers/mtd/nand/raw/davinci_nand.c | 70 
> > +----------------------------
> >   drivers/mtd/nand/raw/nand_base.c    | 69 
> > ++++++++++++++++++++++++++++
> >   include/linux/mtd/rawnand.h         |  2 +
> >   3 files changed, 72 insertions(+), 69 deletions(-)
> >
> >  diff --git a/drivers/mtd/nand/raw/davinci_nand.c 
> > b/drivers/mtd/nand/raw/davinci_nand.c
> >  index 89de24d3bb7a..45fec8c192ab 100644
> >  --- a/drivers/mtd/nand/raw/davinci_nand.c
> >  +++ b/drivers/mtd/nand/raw/davinci_nand.c
> >  @@ -371,74 +371,6 @@ static int nand_davinci_correct_4bit(struct 
> > nand_chip *chip, u_char *data,
> >   	return corrected;
> >   }
> >
> >  -/**
> >  - * nand_read_page_hwecc_oob_first - hw ecc, read oob first
> >  - * @chip: nand chip info structure
> >  - * @buf: buffer to store read data
> >  - * @oob_required: caller requires OOB data read to chip->oob_poi
> >  - * @page: page number to read
> >  - *
> >  - * Hardware ECC for large page chips, require OOB to be read 
> > first. For this
> >  - * ECC mode, the write_page method is re-used from ECC_HW. These 
> > methods
> >  - * read/write ECC from the OOB area, unlike the ECC_HW_SYNDROME 
> > support with
> >  - * multiple ECC steps, follows the "infix ECC" scheme and 
> > reads/writes ECC from
> >  - * the data area, by overwriting the NAND manufacturer bad block 
> > markings.
> >  - */
> >  -static int nand_davinci_read_page_hwecc_oob_first(struct nand_chip 
> > *chip,
> >  -						  uint8_t *buf,
> >  -						  int oob_required, int page)
> >  -{
> >  -	struct mtd_info *mtd = nand_to_mtd(chip);
> >  -	int i, eccsize = chip->ecc.size, ret;
> >  -	int eccbytes = chip->ecc.bytes;
> >  -	int eccsteps = chip->ecc.steps;
> >  -	uint8_t *p = buf;
> >  -	uint8_t *ecc_code = chip->ecc.code_buf;
> >  -	unsigned int max_bitflips = 0;
> >  -
> >  -	/* Read the OOB area first */
> >  -	ret = nand_read_oob_op(chip, page, 0, chip->oob_poi, 
> > mtd->oobsize);
> >  -	if (ret)
> >  -		return ret;
> >  -
> >  -	ret = nand_read_page_op(chip, page, 0, NULL, 0);
> >  -	if (ret)
> >  -		return ret;
> >  -
> >  -	ret = mtd_ooblayout_get_eccbytes(mtd, ecc_code, chip->oob_poi, 0,
> >  -					 chip->ecc.total);
> >  -	if (ret)
> >  -		return ret;
> >  -
> >  -	for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) {
> >  -		int stat;
> >  -
> >  -		chip->ecc.hwctl(chip, NAND_ECC_READ);
> >  -
> >  -		ret = nand_read_data_op(chip, p, eccsize, false, false);
> >  -		if (ret)
> >  -			return ret;
> >  -
> >  -		stat = chip->ecc.correct(chip, p, &ecc_code[i], NULL);
> >  -		if (stat == -EBADMSG &&
> >  -		    (chip->ecc.options & NAND_ECC_GENERIC_ERASED_CHECK)) {
> >  -			/* check for empty pages with bitflips */
> >  -			stat = nand_check_erased_ecc_chunk(p, eccsize,
> >  -							   &ecc_code[i],
> >  -							   eccbytes, NULL, 0,
> >  -							   chip->ecc.strength);
> >  -		}
> >  -
> >  -		if (stat < 0) {
> >  -			mtd->ecc_stats.failed++;
> >  -		} else {
> >  -			mtd->ecc_stats.corrected += stat;
> >  -			max_bitflips = max_t(unsigned int, max_bitflips, stat);
> >  -		}
> >  -	}
> >  -	return max_bitflips;
> >  -}
> >  -
> >   
> > /*----------------------------------------------------------------------*/
> >
> >   /* An ECC layout for using 4-bit ECC with small-page flash, storing
> >  @@ -648,7 +580,7 @@ static int davinci_nand_attach_chip(struct 
> > nand_chip *chip)
> >   			} else if (chunks == 4 || chunks == 8) {
> >   				mtd_set_ooblayout(mtd,
> >   						  nand_get_large_page_ooblayout());
> >  -				chip->ecc.read_page = nand_davinci_read_page_hwecc_oob_first;
> >  +				chip->ecc.read_page = nand_read_page_hwecc_oob_first;
> >   			} else {
> >   				return -EIO;
> >   			}
> >  diff --git a/drivers/mtd/nand/raw/nand_base.c 
> > b/drivers/mtd/nand/raw/nand_base.c
> >  index 3d6c6e880520..cb5f343b9fa2 100644
> >  --- a/drivers/mtd/nand/raw/nand_base.c
> >  +++ b/drivers/mtd/nand/raw/nand_base.c
> >  @@ -3160,6 +3160,75 @@ static int nand_read_page_hwecc(struct 
> > nand_chip *chip, uint8_t *buf,
> >   	return max_bitflips;
> >   }
> >
> >  +/**
> >  + * nand_read_page_hwecc_oob_first - Hardware ECC page read with ECC
> >  + *                                  data read from OOB area
> >  + * @chip: nand chip info structure
> >  + * @buf: buffer to store read data
> >  + * @oob_required: caller requires OOB data read to chip->oob_poi
> >  + * @page: page number to read
> >  + *
> >  + * Hardware ECC for large page chips, require OOB to be read 
> > first. For this
>
> requires
>
> With this ECC configuration?
>
> >  + * ECC mode, the write_page method is re-used from ECC_HW. These 
> > methods
>
> I do not understand this sentence nor the next one about syndrome. I
> believe it is related to your engine and should not leak into the 
> core.
>
> >  + * read/write ECC from the OOB area, unlike the ECC_HW_SYNDROME 
> > support with
> >  + * multiple ECC steps, follows the "infix ECC" scheme and 
> > reads/writes ECC from
> >  + * the data area, by overwriting the NAND manufacturer bad block 
> > markings.
>
> That's a sentence I don't like. What do you mean exactly?
>
> What "Infix ECC" scheme is?
>
> Do you mean that unlike the syndrome  mode it *does not* overwrite the
> BBM ?

I don't mean anything. I did not write that comment. I just moved the 
function verbatim with no changes. If something needs to be fixed, then 
it needs to be fixed before/after this patch.

> >  + */
> >  +int nand_read_page_hwecc_oob_first(struct nand_chip *chip, uint8_t 
> > *buf,
> >  +				   int oob_required, int page)
> >  +{
> >  +	struct mtd_info *mtd = nand_to_mtd(chip);
> >  +	int i, eccsize = chip->ecc.size, ret;
> >  +	int eccbytes = chip->ecc.bytes;
> >  +	int eccsteps = chip->ecc.steps;
> >  +	uint8_t *p = buf;
> >  +	uint8_t *ecc_code = chip->ecc.code_buf;
> >  +	unsigned int max_bitflips = 0;
> >  +
> >  +	/* Read the OOB area first */
> >  +	ret = nand_read_oob_op(chip, page, 0, chip->oob_poi, 
> > mtd->oobsize);
> >  +	if (ret)
> >  +		return ret;
> >  +
> >  +	ret = nand_read_page_op(chip, page, 0, NULL, 0);
>
> Definitely not, your are requesting the chip to do the read_page
> operation twice. You only need a nand_change_read_column I believe.

Again, this code is just being moved around - don't shoot the messenger 
:)

> >  +	if (ret)
> >  +		return ret;
> >  +
> >  +	ret = mtd_ooblayout_get_eccbytes(mtd, ecc_code, chip->oob_poi, 0,
> >  +					 chip->ecc.total);
> >  +	if (ret)
> >  +		return ret;
> >  +
> >  +	for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) {
> >  +		int stat;
> >  +
> >  +		chip->ecc.hwctl(chip, NAND_ECC_READ);
> >  +
> >  +		ret = nand_read_data_op(chip, p, eccsize, false, false);
> >  +		if (ret)
> >  +			return ret;
> >  +
> >  +		stat = chip->ecc.correct(chip, p, &ecc_code[i], NULL);
> >  +		if (stat == -EBADMSG &&
> >  +		    (chip->ecc.options & NAND_ECC_GENERIC_ERASED_CHECK)) {
> >  +			/* check for empty pages with bitflips */
> >  +			stat = nand_check_erased_ecc_chunk(p, eccsize,
> >  +							   &ecc_code[i],
> >  +							   eccbytes, NULL, 0,
> >  +							   chip->ecc.strength);
> >  +		}
> >  +
> >  +		if (stat < 0) {
> >  +			mtd->ecc_stats.failed++;
> >  +		} else {
> >  +			mtd->ecc_stats.corrected += stat;
> >  +			max_bitflips = max_t(unsigned int, max_bitflips, stat);
> >  +		}
> >  +	}
> >  +	return max_bitflips;
> >  +}
> >  +EXPORT_SYMBOL_GPL(nand_read_page_hwecc_oob_first);
> >  +
> >   /**
> >    * nand_read_page_syndrome - [REPLACEABLE] hardware ECC syndrome 
> > based page read
> >    * @chip: nand chip info structure
> >  diff --git a/include/linux/mtd/rawnand.h 
> > b/include/linux/mtd/rawnand.h
> >  index b2f9dd3cbd69..5b88cd51fadb 100644
> >  --- a/include/linux/mtd/rawnand.h
> >  +++ b/include/linux/mtd/rawnand.h
> >  @@ -1539,6 +1539,8 @@ int nand_read_data_op(struct nand_chip *chip, 
> > void *buf, unsigned int len,
> >   		      bool force_8bit, bool check_only);
> >   int nand_write_data_op(struct nand_chip *chip, const void *buf,
> >   		       unsigned int len, bool force_8bit);
> >  +int nand_read_page_hwecc_oob_first(struct nand_chip *chip, uint8_t 
> > *buf,
> >  +				   int oob_required, int page);
>
> You certainly want to add this symbol closer to the other read/write
> page helpers?

Where would that be? The other read/write page helpers are all "static" 
so they don't appear in any header.

Cheers,
-Paul

> >   /* Scan and identify a NAND device */
> >   int nand_scan_with_ids(struct nand_chip *chip, unsigned int 
> > max_chips,
>
>
> Thanks,
> Miquèl