Re: [PATCH v2 2/2] mtd: rawnand: omap: Use ECC information from the generic structures

[Boris Brezillon <boris.brezillon@collabora.com>]

On Wed, 20 Jan 2021 11:17:50 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> The OMAP driver may leverage software BCH logic to locate errors while
> using its own hardware to detect the presence of errors. This is
> achieved with a "mixed" mode which initializes manually the software
> BCH internal logic while providing its own OOB layout.
> 
> The issue here comes from the fact that the BCH driver has been
> updated to only use generic NAND objects, and no longer depend on raw
> NAND structures as it is usable from SPI-NAND as well. However, at the
> end of the BCH context initialization, the driver checks the validity
> of the OOB layout. At this stage, the raw NAND fields have not been
> populated yet while being used by the layout helpers, leading to an
> invalid layout.
> 
> The solution here is to use the fields from the generic ECC structures
> which have already been updated and will always be valid instead of
> the raw NAND ones.
> 
> Note: I don't know which commit exactly triggered the error, but the
> entire migration to a generic BCH driver got merged in one go, so this
> should not be a problem for stable backports.
> 
> Reported-by: Adam Ford <aford173@gmail.com>
> Fixes: 80fe603160a4 ("mtd: nand: ecc-bch: Stop using raw NAND structures")
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/mtd/nand/raw/omap2.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/omap2.c b/drivers/mtd/nand/raw/omap2.c
> index fbb9955f2467..fd09b2a30abb 100644
> --- a/drivers/mtd/nand/raw/omap2.c
> +++ b/drivers/mtd/nand/raw/omap2.c
> @@ -1866,18 +1866,20 @@ static const struct mtd_ooblayout_ops omap_ooblayout_ops = {
>  static int omap_sw_ooblayout_ecc(struct mtd_info *mtd, int section,
>  				 struct mtd_oob_region *oobregion)
>  {
> -	struct nand_chip *chip = mtd_to_nand(mtd);
> +	struct nand_device *nand = mtd_to_nanddev(mtd);
> +	unsigned int nsteps = nanddev_get_ecc_nsteps(nand);
> +	unsigned int ecc_bytes = nand->ecc.ctx.total / nsteps;

I'd recommend adding a helper for that one too
(nanddev_ecc_bytes_per_step()?).

This being said, I think you're better off applying v1 (which is
self-contained) and keeping this refactor for the next release.

>  	int off = BADBLOCK_MARKER_LENGTH;
>  
> -	if (section >= chip->ecc.steps)
> +	if (section >= nsteps)
>  		return -ERANGE;
>  
>  	/*
>  	 * When SW correction is employed, one OMAP specific marker byte is
>  	 * reserved after each ECC step.
>  	 */
> -	oobregion->offset = off + (section * (chip->ecc.bytes + 1));
> -	oobregion->length = chip->ecc.bytes;
> +	oobregion->offset = off + (section * (ecc_bytes + 1));
> +	oobregion->length = ecc_bytes;
>  
>  	return 0;
>  }
> @@ -1885,7 +1887,9 @@ static int omap_sw_ooblayout_ecc(struct mtd_info *mtd, int section,
>  static int omap_sw_ooblayout_free(struct mtd_info *mtd, int section,
>  				  struct mtd_oob_region *oobregion)
>  {
> -	struct nand_chip *chip = mtd_to_nand(mtd);
> +	struct nand_device *nand = mtd_to_nanddev(mtd);
> +	unsigned int nsteps = nanddev_get_ecc_nsteps(nand);
> +	unsigned int ecc_bytes = nand->ecc.ctx.total / nsteps;
>  	int off = BADBLOCK_MARKER_LENGTH;
>  
>  	if (section)
> @@ -1895,7 +1899,7 @@ static int omap_sw_ooblayout_free(struct mtd_info *mtd, int section,
>  	 * When SW correction is employed, one OMAP specific marker byte is
>  	 * reserved after each ECC step.
>  	 */
> -	off += ((chip->ecc.bytes + 1) * chip->ecc.steps);
> +	off += ((ecc_bytes + 1) * nsteps);
>  	if (off >= mtd->oobsize)
>  		return -ERANGE;
>  


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