| Message ID | 1386925978-23705-2-git-send-email-pekon@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Dec 13, 2013 at 02:42:57PM +0530, Pekon Gupta wrote: > This patch updates starting offset for free bytes in OOB which can be used by > file-systems to store their metadata (like clean-marker in case of JFFS2). This should be describing a regression fix, right? We don't just arbitrarily change the "OOB free" layout; we need a reason. So please describe it here. (It seems like an off-by-one, or off-by-<N> error, where the oobfree region was miscalculated.) Possibly you can paste an example intended ecclayout as well as an incorrect layout that was calculated before this fix. > Signed-off-by: Pekon Gupta <pekon@ti.com> > --- > drivers/mtd/nand/omap2.c | 17 ++++------------- > 1 file changed, 4 insertions(+), 13 deletions(-) > > diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c > index f777250..bbdb5e8 100644 > --- a/drivers/mtd/nand/omap2.c > +++ b/drivers/mtd/nand/omap2.c > @@ -1835,8 +1835,6 @@ static int omap_nand_probe(struct platform_device *pdev) > ecclayout->eccpos[0] = BADBLOCK_MARKER_LENGTH; > else > ecclayout->eccpos[0] = 1; > - ecclayout->oobfree->offset = ecclayout->eccpos[0] + > - ecclayout->eccbytes; > break; > > case OMAP_ECC_BCH4_CODE_HW_DETECTION_SW: > @@ -1854,8 +1852,6 @@ static int omap_nand_probe(struct platform_device *pdev) > (mtd->writesize / > nand_chip->ecc.size); > ecclayout->eccpos[0] = BADBLOCK_MARKER_LENGTH; > - ecclayout->oobfree->offset = ecclayout->eccpos[0] + > - ecclayout->eccbytes; > /* software bch library is used for locating errors */ > nand_chip->ecc.priv = nand_bch_init(mtd, > nand_chip->ecc.size, > @@ -1890,8 +1886,6 @@ static int omap_nand_probe(struct platform_device *pdev) > (mtd->writesize / > nand_chip->ecc.size); > ecclayout->eccpos[0] = BADBLOCK_MARKER_LENGTH; > - ecclayout->oobfree->offset = ecclayout->eccpos[0] + > - ecclayout->eccbytes; > /* This ECC scheme requires ELM H/W block */ > if (is_elm_present(info, pdata->elm_of_node, BCH4_ECC) < 0) { > pr_err("nand: error: could not initialize ELM\n"); > @@ -1920,8 +1914,6 @@ static int omap_nand_probe(struct platform_device *pdev) > (mtd->writesize / > nand_chip->ecc.size); > ecclayout->eccpos[0] = BADBLOCK_MARKER_LENGTH; > - ecclayout->oobfree->offset = ecclayout->eccpos[0] + > - ecclayout->eccbytes; > /* software bch library is used for locating errors */ > nand_chip->ecc.priv = nand_bch_init(mtd, > nand_chip->ecc.size, > @@ -1963,8 +1955,6 @@ static int omap_nand_probe(struct platform_device *pdev) > (mtd->writesize / > nand_chip->ecc.size); > ecclayout->eccpos[0] = BADBLOCK_MARKER_LENGTH; > - ecclayout->oobfree->offset = ecclayout->eccpos[0] + > - ecclayout->eccbytes; > break; > #else > pr_err("nand: error: CONFIG_MTD_NAND_OMAP_BCH not enabled\n"); > @@ -1978,9 +1968,6 @@ static int omap_nand_probe(struct platform_device *pdev) > goto return_error; > } > > - /* populate remaining ECC layout data */ > - ecclayout->oobfree->length = mtd->oobsize - (BADBLOCK_MARKER_LENGTH + > - ecclayout->eccbytes); > for (i = 1; i < ecclayout->eccbytes; i++) > ecclayout->eccpos[i] = ecclayout->eccpos[0] + i; > /* check if NAND device's OOB is enough to store ECC signatures */ > @@ -1990,6 +1977,10 @@ static int omap_nand_probe(struct platform_device *pdev) > err = -EINVAL; > goto return_error; > } > + /* populate remaining ECC layout data */ > + ecclayout->oobfree->offset = ecclayout->eccpos[ecclayout->eccbytes] + 1; Hmm, this line seems a little odd. The ecclayout->eccpos[] array and the value of ecclayout->eccbytes sould be related as follows: let N = ecclayout->eccbytes This means the eccpos[] array should have N entries, indexed 0 to N-1, and eccpos[N] is out of bounds. But you are accessing eccpos[N] above (i.e., eccpos[ecclayout->eccbytes]). Are you sure this is correct? It seems like it should be: ecclayout->oobfree->offset = ecclayout->eccpos[ecclayout->eccbytes - 1] + 1; > + ecclayout->oobfree->length = mtd->oobsize - (BADBLOCK_MARKER_LENGTH + > + ecclayout->eccbytes); > > /* second phase scan */ > if (nand_scan_tail(mtd)) { Brian -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Brian, >From: Brian Norris [mailto:computersforpeace@gmail.com] >>On Fri, Dec 13, 2013 at 02:42:57PM +0530, Pekon Gupta wrote: >> This patch updates starting offset for free bytes in OOB which can be used by >> file-systems to store their metadata (like clean-marker in case of JFFS2). > >This should be describing a regression fix, right? We don't just >arbitrarily change the "OOB free" layout; we need a reason. So please >describe it here. > >(It seems like an off-by-one, or off-by-<N> error, where the oobfree >region was miscalculated.) > >Possibly you can paste an example intended ecclayout as well as an >incorrect layout that was calculated before this fix. > >> Signed-off-by: Pekon Gupta <pekon@ti.com> >> --- >> drivers/mtd/nand/omap2.c | 17 ++++------------- >> 1 file changed, 4 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c [...] >> >> - /* populate remaining ECC layout data */ >> - ecclayout->oobfree->length = mtd->oobsize - (BADBLOCK_MARKER_LENGTH + >> - ecclayout->eccbytes); >> for (i = 1; i < ecclayout->eccbytes; i++) >> ecclayout->eccpos[i] = ecclayout->eccpos[0] + i; >> /* check if NAND device's OOB is enough to store ECC signatures */ >> @@ -1990,6 +1977,10 @@ static int omap_nand_probe(struct platform_device *pdev) >> err = -EINVAL; >> goto return_error; >> } >> + /* populate remaining ECC layout data */ >> + ecclayout->oobfree->offset = ecclayout->eccpos[ecclayout->eccbytes] + 1; > >Hmm, this line seems a little odd. The ecclayout->eccpos[] array and the >value of ecclayout->eccbytes sould be related as follows: > > let N = ecclayout->eccbytes > > This means the eccpos[] array should have N entries, indexed 0 to N-1, > and eccpos[N] is out of bounds. But you are accessing eccpos[N] above > (i.e., eccpos[ecclayout->eccbytes]). Are you sure this is correct? It > seems like it should be: > > ecclayout->oobfree->offset = ecclayout->eccpos[ecclayout->eccbytes - 1] + 1; > Thanks for this catch. Yes, you are correct. It's a typo. This wasn't caught as I had tested everything on UBIFS which does not use 'oobfree'. Also, as ecclayout->eccpos is defined as large static array, so this dint caused problems either. #define MTD_MAX_ECCPOS_ENTRIES_LARGE 640 struct nand_ecclayout { __u32 eccpos[MTD_MAX_ECCPOS_ENTRIES_LARGE]; But, I think mtd_tests.nand_oobtest would have caught this. I'll include this change in next version. <stripping down the CC list to avoid getting moderated by u-boot mailman> with regards, pekon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c index f777250..bbdb5e8 100644 --- a/drivers/mtd/nand/omap2.c +++ b/drivers/mtd/nand/omap2.c @@ -1835,8 +1835,6 @@ static int omap_nand_probe(struct platform_device *pdev) ecclayout->eccpos[0] = BADBLOCK_MARKER_LENGTH; else ecclayout->eccpos[0] = 1; - ecclayout->oobfree->offset = ecclayout->eccpos[0] + - ecclayout->eccbytes; break; case OMAP_ECC_BCH4_CODE_HW_DETECTION_SW: @@ -1854,8 +1852,6 @@ static int omap_nand_probe(struct platform_device *pdev) (mtd->writesize / nand_chip->ecc.size); ecclayout->eccpos[0] = BADBLOCK_MARKER_LENGTH; - ecclayout->oobfree->offset = ecclayout->eccpos[0] + - ecclayout->eccbytes; /* software bch library is used for locating errors */ nand_chip->ecc.priv = nand_bch_init(mtd, nand_chip->ecc.size, @@ -1890,8 +1886,6 @@ static int omap_nand_probe(struct platform_device *pdev) (mtd->writesize / nand_chip->ecc.size); ecclayout->eccpos[0] = BADBLOCK_MARKER_LENGTH; - ecclayout->oobfree->offset = ecclayout->eccpos[0] + - ecclayout->eccbytes; /* This ECC scheme requires ELM H/W block */ if (is_elm_present(info, pdata->elm_of_node, BCH4_ECC) < 0) { pr_err("nand: error: could not initialize ELM\n"); @@ -1920,8 +1914,6 @@ static int omap_nand_probe(struct platform_device *pdev) (mtd->writesize / nand_chip->ecc.size); ecclayout->eccpos[0] = BADBLOCK_MARKER_LENGTH; - ecclayout->oobfree->offset = ecclayout->eccpos[0] + - ecclayout->eccbytes; /* software bch library is used for locating errors */ nand_chip->ecc.priv = nand_bch_init(mtd, nand_chip->ecc.size, @@ -1963,8 +1955,6 @@ static int omap_nand_probe(struct platform_device *pdev) (mtd->writesize / nand_chip->ecc.size); ecclayout->eccpos[0] = BADBLOCK_MARKER_LENGTH; - ecclayout->oobfree->offset = ecclayout->eccpos[0] + - ecclayout->eccbytes; break; #else pr_err("nand: error: CONFIG_MTD_NAND_OMAP_BCH not enabled\n"); @@ -1978,9 +1968,6 @@ static int omap_nand_probe(struct platform_device *pdev) goto return_error; } - /* populate remaining ECC layout data */ - ecclayout->oobfree->length = mtd->oobsize - (BADBLOCK_MARKER_LENGTH + - ecclayout->eccbytes); for (i = 1; i < ecclayout->eccbytes; i++) ecclayout->eccpos[i] = ecclayout->eccpos[0] + i; /* check if NAND device's OOB is enough to store ECC signatures */ @@ -1990,6 +1977,10 @@ static int omap_nand_probe(struct platform_device *pdev) err = -EINVAL; goto return_error; } + /* populate remaining ECC layout data */ + ecclayout->oobfree->offset = ecclayout->eccpos[ecclayout->eccbytes] + 1; + ecclayout->oobfree->length = mtd->oobsize - (BADBLOCK_MARKER_LENGTH + + ecclayout->eccbytes); /* second phase scan */ if (nand_scan_tail(mtd)) {
This patch updates starting offset for free bytes in OOB which can be used by file-systems to store their metadata (like clean-marker in case of JFFS2). Signed-off-by: Pekon Gupta <pekon@ti.com> --- drivers/mtd/nand/omap2.c | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-)