All of lore.kernel.org
 help / color / mirror / Atom feed
From: Miquel Raynal <miquel.raynal@bootlin.com>
To: Adam Ford <aford173@gmail.com>
Cc: Richard Weinberger <richard@nod.at>,
	Vignesh Raghavendra <vigneshr@ti.com>,
	Tudor Ambarus <Tudor.Ambarus@microchip.com>,
	linux-mtd@lists.infradead.org, Julien Su <juliensu@mxic.com.tw>,
	ycllin@mxic.com.tw,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	Linux-OMAP <linux-omap@vger.kernel.org>
Subject: Re: [PATCH 04/20] mtd: nand: ecc-bch: Stop exporting the private structure
Date: Tue, 19 Jan 2021 12:56:34 +0100	[thread overview]
Message-ID: <20210119125634.6d4c5c1e@xps13> (raw)
In-Reply-To: <CAHCN7xKWBcuF0h5+brHndZCD5dFbbcxxZZxEPXRPVK7kZGbM1Q@mail.gmail.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

WARNING: multiple messages have this Message-ID
From: Miquel Raynal <miquel.raynal@bootlin.com>
To: Adam Ford <aford173@gmail.com>
Cc: Vignesh Raghavendra <vigneshr@ti.com>,
	Tudor Ambarus <Tudor.Ambarus@microchip.com>,
	Julien Su <juliensu@mxic.com.tw>,
	Richard Weinberger <richard@nod.at>,
	ycllin@mxic.com.tw, linux-mtd@lists.infradead.org,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	Linux-OMAP <linux-omap@vger.kernel.org>
Subject: Re: [PATCH 04/20] mtd: nand: ecc-bch: Stop exporting the private structure
Date: Tue, 19 Jan 2021 12:56:34 +0100	[thread overview]
Message-ID: <20210119125634.6d4c5c1e@xps13> (raw)
In-Reply-To: <CAHCN7xKWBcuF0h5+brHndZCD5dFbbcxxZZxEPXRPVK7kZGbM1Q@mail.gmail.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

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

  reply	other threads:[~2021-01-19 14:55 UTC|newest]

Thread overview: 69+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-29 23:01 [PATCH 00/20] Create generic software ECC engines Miquel Raynal
2020-09-29 23:01 ` [PATCH 01/20] mtd: nand: ecc: Add an I/O request tweaking mechanism Miquel Raynal
2020-09-30  7:53   ` Thomas Petazzoni
2020-09-30  8:16     ` Miquel Raynal
2020-10-30 17:30   ` Miquel Raynal
2020-09-29 23:01 ` [PATCH 02/20] mtd: nand: ecc-bch: Move BCH code to the generic NAND layer Miquel Raynal
2020-10-30 17:30   ` Miquel Raynal
2020-09-29 23:01 ` [PATCH 03/20] mtd: nand: ecc-bch: Cleanup and style fixes Miquel Raynal
2020-10-30 17:30   ` Miquel Raynal
2020-09-29 23:01 ` [PATCH 04/20] mtd: nand: ecc-bch: Stop exporting the private structure Miquel Raynal
2021-01-09 14:46   ` Adam Ford
2021-01-09 14:46     ` Adam Ford
2021-01-11 10:20     ` Miquel Raynal
2021-01-11 10:20       ` Miquel Raynal
2021-01-12 14:35       ` Miquel Raynal
2021-01-12 14:35         ` Miquel Raynal
2021-01-12 16:01         ` Adam Ford
2021-01-12 16:01           ` Adam Ford
2021-01-12 17:20           ` Adam Ford
2021-01-12 17:20             ` Adam Ford
2021-01-14 15:42             ` Miquel Raynal
2021-01-14 15:42               ` Miquel Raynal
2021-01-15 12:23               ` Adam Ford
2021-01-15 12:23                 ` Adam Ford
2021-01-15 16:06                 ` Adam Ford
2021-01-15 16:06                   ` Adam Ford
2021-01-15 16:17                   ` Miquel Raynal
2021-01-15 16:17                     ` Miquel Raynal
2021-01-15 16:28                     ` Adam Ford
2021-01-15 16:28                       ` Adam Ford
2021-01-19 11:56                       ` Miquel Raynal [this message]
2021-01-19 11:56                         ` Miquel Raynal
2021-01-19 14:21                         ` Adam Ford
2021-01-19 14:21                           ` Adam Ford
2021-01-19 14:36                           ` Miquel Raynal
2021-01-19 14:36                             ` Miquel Raynal
2021-01-19 15:49                             ` Adam Ford
2021-01-19 15:49                               ` Adam Ford
2021-01-19 15:53                               ` Miquel Raynal
2021-01-19 15:53                                 ` Miquel Raynal
2020-09-29 23:01 ` [PATCH 05/20] mtd: nand: ecc-bch: Return only valid error codes Miquel Raynal
2020-10-30 17:29   ` Miquel Raynal
2020-09-29 23:01 ` [PATCH 06/20] mtd: nand: ecc-bch: Drop mtd_nand_has_bch() Miquel Raynal
2020-10-30 17:29   ` Miquel Raynal
2020-09-29 23:01 ` [PATCH 07/20] mtd: nand: ecc-bch: Update the prototypes to be more generic Miquel Raynal
2020-10-30 17:29   ` Miquel Raynal
2020-09-29 23:01 ` [PATCH 08/20] mtd: nand: ecc-bch: Stop using raw NAND structures Miquel Raynal
2020-09-29 23:01 ` [PATCH 09/20] mtd: nand: ecc-bch: Create the software BCH engine Miquel Raynal
2020-10-30 17:29   ` Miquel Raynal
2020-09-29 23:01 ` [PATCH 10/20] mtd: rawnand: Get rid of chip->ecc.priv Miquel Raynal
2020-10-30 17:29   ` Miquel Raynal
2020-09-29 23:01 ` [PATCH 11/20] mtd: nand: ecc-hamming: Move Hamming code to the generic NAND layer Miquel Raynal
2020-09-29 23:01 ` [PATCH 12/20] mtd: nand: ecc-hamming: Clarify the driver descriptions Miquel Raynal
2020-10-30 17:29   ` Miquel Raynal
2020-09-29 23:01 ` [PATCH 13/20] mtd: nand: ecc-hamming: Drop/fix the kernel doc Miquel Raynal
2020-10-30 17:29   ` Miquel Raynal
2020-09-29 23:01 ` [PATCH 14/20] mtd: nand: ecc-hamming: Cleanup and style fixes Miquel Raynal
2020-10-30 17:29   ` Miquel Raynal
2020-09-29 23:01 ` [PATCH 15/20] mtd: nand: ecc-hamming: Rename the exported functions Miquel Raynal
2020-10-30 17:29   ` Miquel Raynal
2020-09-29 23:01 ` [PATCH 16/20] mtd: nand: ecc-hamming: Stop using raw NAND structures Miquel Raynal
2020-10-30 17:29   ` Miquel Raynal
2020-09-29 23:01 ` [PATCH 17/20] mtd: nand: ecc-hamming: Remove useless includes Miquel Raynal
2020-09-29 23:01 ` [PATCH 18/20] mtd: nand: ecc-hamming: Let the software Hamming ECC engine be unselected Miquel Raynal
2020-10-30 17:29   ` Miquel Raynal
2020-09-29 23:01 ` [PATCH 19/20] mtd: nand: ecc-hamming: Create the software Hamming engine Miquel Raynal
2020-10-30 17:28   ` Miquel Raynal
2020-09-29 23:01 ` [PATCH 20/20] mtd: nand: Let software ECC engines be retrieved from the NAND core Miquel Raynal
2020-10-30 17:28   ` Miquel Raynal

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210119125634.6d4c5c1e@xps13 \
    --to=miquel.raynal@bootlin.com \
    --cc=Tudor.Ambarus@microchip.com \
    --cc=aford173@gmail.com \
    --cc=juliensu@mxic.com.tw \
    --cc=linux-mtd@lists.infradead.org \
    --cc=linux-omap@vger.kernel.org \
    --cc=richard@nod.at \
    --cc=thomas.petazzoni@bootlin.com \
    --cc=vigneshr@ti.com \
    --cc=ycllin@mxic.com.tw \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.