Re: [PATCH 04/20] mtd: nand: ecc-bch: Stop exporting the private structure

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Adam,

Thank you very much for troubleshooting this, here is my proposal.

> > > I appear to have the NAND flash working with the following patch:
> > >
> > > @@ -247,11 +253,21 @@ int nand_ecc_sw_bch_init_ctx(struct nand_device *nand)
> > >         nand->ecc.ctx.priv = engine_conf;
> > >         nand->ecc.ctx.total = nsteps * code_size;
> > >
> > > +       struct nand_chip *chip = mtd_to_nand(mtd);
> > > +       chip->ecc.steps = nsteps;
> > > +       chip->ecc.size =  conf->step_size;

I was fearing that many boards would be affected by this issue but it
appears that the problem will only show up here because the OMAP driver
makes a strange use of the BCH library: it initializes it itself
because it only needs it for a single operation while usually, the core
is in charge of doing that. During the initialization, the OOB layout
is verified. Usually, the BCH driver is used with one of the generic OOB
layouts, while here the OMAP driver uses its own, which reads raw NAND
chip entries.

I recently moved the BCH driver to only use "generic" NAND bits, which
produced the bug because the entries derived by the layout helpers
have not been updated yet.

So using raw NAND bits in the BCH driver is not an option here.
Instead, I think the best way to address this is to export the
declaration of the BCH internal configuration structure to the OMAP
driver and use the right values, recently derived by the driver:

---8<---

Author: Miquel Raynal <miquel.raynal@bootlin.com>
Date:   Tue Jan 19 12:27:07 2021 +0100

    wip: fix omap
    
    Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

diff --git a/drivers/mtd/nand/raw/omap2.c b/drivers/mtd/nand/raw/omap2.c
index fbb9955f2467..2c3e65cb68f3 100644
--- a/drivers/mtd/nand/raw/omap2.c
+++ b/drivers/mtd/nand/raw/omap2.c
@@ -15,6 +15,7 @@
 #include <linux/jiffies.h>
 #include <linux/sched.h>
 #include <linux/mtd/mtd.h>
+#include <linux/mtd/nand-ecc-sw-bch.h>
 #include <linux/mtd/rawnand.h>
 #include <linux/mtd/partitions.h>
 #include <linux/omap-dma.h>
@@ -1866,18 +1867,19 @@ 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);
+       const struct nand_ecc_sw_bch_conf *engine_conf = nand->ecc.ctx.priv;
        int off = BADBLOCK_MARKER_LENGTH;
 
-       if (section >= chip->ecc.steps)
+       if (section >= engine_conf->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 * (engine_conf->code_size + 1));
+       oobregion->length = engine_conf->code_size;
 
        return 0;
 }
@@ -1885,7 +1887,8 @@ 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);
+       const struct nand_ecc_sw_bch_conf *engine_conf = nand->ecc.ctx.priv;
        int off = BADBLOCK_MARKER_LENGTH;
 
        if (section)
@@ -1895,7 +1898,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 += ((engine_conf->code_size + 1) * engine_conf->nsteps);
        if (off >= mtd->oobsize)
                return -ERANGE;
 
--->8---

Can you please try this patch and compare the values between your hack
and mine of:
- chip->ecc.bytes vs. engine_conf->code_size
- chip->ecc.steps vs. engine_conf->nsteps
The values should be the same, but I prefer to be sure.

Thanks,
Miquèl