| Message ID | 20170705065109.7738-1-miquel.raynal@free-electrons.com |
|---|---|
| State | Accepted |
| Delegated to: | Boris Brezillon |
| Headers | show |
Hi Miquel, On Wed, 5 Jul 2017 08:51:09 +0200 Miquel Raynal <miquel.raynal@free-electrons.com> wrote: > When using soft ecc, if no ooblayout is given, the core automatically > uses one of the nand_ooblayout_{sp,lp}*() functions to determine the > layout inside the out of band data. > > Until kernel version 4.6, struct nand_ecclayout was used for that > purpose. During the migration from 4.6 to 4.7, an error shown up in the > small page layout, in the case oob section is only 8 bytes long. > > The layout was using three bytes (0, 1, 2) for ecc, two bytes (3, 4) > as free bytes, one byte (5) for bad block marker and finally > two bytes (6, 7) as free bytes, as shown there: > > [linux-4.6] drivers/mtd/nand/nand_base.c:52 > static struct nand_ecclayout nand_oob_8 = { > .eccbytes = 3, > .eccpos = {0, 1, 2}, > .oobfree = { > {.offset = 3, > .length = 2}, > {.offset = 6, > .length = 2} } > }; > > This fixes the current implementation which is incoherent. It > references bit 3 at the same time as an ecc byte and a free byte. > > Furthermore, it is clear with the previous implementation that there > is only one ecc section with 8 bytes oob sections. We shall return > -ERANGE in the nand_ooblayout_ecc_sp() function when asked for the > second section. > > Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com> Looks good to me. BTW, it looks like a good candidate for stable. No need to send a v2, I'll add the following when applying: Fixes: 41b207a70d3a ("mtd: nand: implement the default mtd_ooblayout_ops") Cc: <stable@vger.kernel.org> Thanks, Boris > --- > drivers/mtd/nand/nand_base.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > index b1dd12729f19..c5221795a1e8 100644 > --- a/drivers/mtd/nand/nand_base.c > +++ b/drivers/mtd/nand/nand_base.c > @@ -65,8 +65,14 @@ static int nand_ooblayout_ecc_sp(struct mtd_info *mtd, int section, > > if (!section) { > oobregion->offset = 0; > - oobregion->length = 4; > + if (mtd->oobsize == 16) > + oobregion->length = 4; > + else > + oobregion->length = 3; > } else { > + if (mtd->oobsize == 8) > + return -ERANGE; > + > oobregion->offset = 6; > oobregion->length = ecc->total - 4; > }
On Wed, 5 Jul 2017 09:04:53 +0200 Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > Hi Miquel, > > On Wed, 5 Jul 2017 08:51:09 +0200 > Miquel Raynal <miquel.raynal@free-electrons.com> wrote: > > > When using soft ecc, if no ooblayout is given, the core automatically > > uses one of the nand_ooblayout_{sp,lp}*() functions to determine the > > layout inside the out of band data. > > > > Until kernel version 4.6, struct nand_ecclayout was used for that > > purpose. During the migration from 4.6 to 4.7, an error shown up in the > > small page layout, in the case oob section is only 8 bytes long. > > > > The layout was using three bytes (0, 1, 2) for ecc, two bytes (3, 4) > > as free bytes, one byte (5) for bad block marker and finally > > two bytes (6, 7) as free bytes, as shown there: > > > > [linux-4.6] drivers/mtd/nand/nand_base.c:52 > > static struct nand_ecclayout nand_oob_8 = { > > .eccbytes = 3, > > .eccpos = {0, 1, 2}, > > .oobfree = { > > {.offset = 3, > > .length = 2}, > > {.offset = 6, > > .length = 2} } > > }; > > > > This fixes the current implementation which is incoherent. It > > references bit 3 at the same time as an ecc byte and a free byte. > > > > Furthermore, it is clear with the previous implementation that there > > is only one ecc section with 8 bytes oob sections. We shall return > > -ERANGE in the nand_ooblayout_ecc_sp() function when asked for the > > second section. Applied to nand/fixes. Thanks, Boris > > > > Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com> > > Looks good to me. BTW, it looks like a good candidate for stable. > No need to send a v2, I'll add the following when applying: > > Fixes: 41b207a70d3a ("mtd: nand: implement the default mtd_ooblayout_ops") > Cc: <stable@vger.kernel.org> > > Thanks, > > Boris > > > --- > > drivers/mtd/nand/nand_base.c | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > > index b1dd12729f19..c5221795a1e8 100644 > > --- a/drivers/mtd/nand/nand_base.c > > +++ b/drivers/mtd/nand/nand_base.c > > @@ -65,8 +65,14 @@ static int nand_ooblayout_ecc_sp(struct mtd_info *mtd, int section, > > > > if (!section) { > > oobregion->offset = 0; > > - oobregion->length = 4; > > + if (mtd->oobsize == 16) > > + oobregion->length = 4; > > + else > > + oobregion->length = 3; > > } else { > > + if (mtd->oobsize == 8) > > + return -ERANGE; > > + > > oobregion->offset = 6; > > oobregion->length = ecc->total - 4; > > } >
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c index b1dd12729f19..c5221795a1e8 100644 --- a/drivers/mtd/nand/nand_base.c +++ b/drivers/mtd/nand/nand_base.c @@ -65,8 +65,14 @@ static int nand_ooblayout_ecc_sp(struct mtd_info *mtd, int section, if (!section) { oobregion->offset = 0; - oobregion->length = 4; + if (mtd->oobsize == 16) + oobregion->length = 4; + else + oobregion->length = 3; } else { + if (mtd->oobsize == 8) + return -ERANGE; + oobregion->offset = 6; oobregion->length = ecc->total - 4; }
When using soft ecc, if no ooblayout is given, the core automatically uses one of the nand_ooblayout_{sp,lp}*() functions to determine the layout inside the out of band data. Until kernel version 4.6, struct nand_ecclayout was used for that purpose. During the migration from 4.6 to 4.7, an error shown up in the small page layout, in the case oob section is only 8 bytes long. The layout was using three bytes (0, 1, 2) for ecc, two bytes (3, 4) as free bytes, one byte (5) for bad block marker and finally two bytes (6, 7) as free bytes, as shown there: [linux-4.6] drivers/mtd/nand/nand_base.c:52 static struct nand_ecclayout nand_oob_8 = { .eccbytes = 3, .eccpos = {0, 1, 2}, .oobfree = { {.offset = 3, .length = 2}, {.offset = 6, .length = 2} } }; This fixes the current implementation which is incoherent. It references bit 3 at the same time as an ecc byte and a free byte. Furthermore, it is clear with the previous implementation that there is only one ecc section with 8 bytes oob sections. We shall return -ERANGE in the nand_ooblayout_ecc_sp() function when asked for the second section. Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com> --- drivers/mtd/nand/nand_base.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)