Re: [U-Boot] [PATCH v1 1/2] mtd: nand: omap: fix ecclayout->oobfree->offset

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