diff mbox

[v1,1/2] mtd: nand: omap: fix ecclayout->oobfree->offset

Message ID 1386925978-23705-2-git-send-email-pekon@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

pekon gupta Dec. 13, 2013, 9:12 a.m. UTC
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(-)

Comments

Brian Norris Jan. 25, 2014, 8:56 a.m. UTC | #1
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
pekon gupta Feb. 12, 2014, 11:33 a.m. UTC | #2
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 mbox

Patch

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)) {