Re: [PATCH v2 1/3] mtd: nand: omap: fix ecclayout->oobfree->offset

[Brian Norris <computersforpeace@gmail.com>]

Hi Pekon,

On Fri, Feb 14, 2014 at 10:31:46PM +0530, Pekon Gupta wrote:
> 1) In current implementation, ecclayout->oobfree->offset is calculated with
>  respect to ecclayout->eccpos[0] which is incorrect because ECC bytes may not
>  be stored contiguously in OOB.
>  So, this patch calculates ecclayout->oobfree->offset with respect to last
>  ECC byte-position 'eccpos[ecclayout->eccbytes-1]'.
> 
> 2) ECC layout of some ecc-schemes expects reserved-markers at specific eccpos[]
>  which should not be over-written by any file-system metadata.
>  So this patch aligns oobfree->offset taking into account of such markers.

This is a much better description, and this series is looking a lot more
straightforward now. Thanks. A quick comment below, and I'll spend some
more time looking later.

(BTW, can you mark these for -stable, version 3.13+ if/when you resend?
Or else I'll mark it myself, I think.)

> Tested-by: Enric Balletbo i Serra <eballetbo@gmail.com>
> Tested-by: Stefan Roese <sr@denx.de>
> Signed-off-by: Pekon Gupta <pekon@ti.com>
> ---
>  drivers/mtd/nand/omap2.c | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> index ef4190a..874fd9d 100644
> --- a/drivers/mtd/nand/omap2.c
> +++ b/drivers/mtd/nand/omap2.c
> @@ -1829,8 +1829,9 @@ 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;
> +		/* no reserved-marker in ecclayout for this ecc-scheme */
> +		ecclayout->oobfree->offset	=
> +				ecclayout->eccpos[ecclayout->eccbytes - 1] + 1;

Thanks for adjusting for 'eccbytes - 1'. This diff *would* be correct,
except that you're only initializing eccpos[1 .. eccbytes-1] in a for
loop after the switch-case block. It seems like this first patch
actually depends on patch 3, where you move the initialization of the
entire eccpos[] array into each 'case' block. e.g.:

@@ -1826,9 +1827,11 @@ static int omap_nand_probe(struct platform_device *pdev)
                                                        (mtd->writesize /
                                                        nand_chip->ecc.size);
                if (nand_chip->options & NAND_BUSWIDTH_16)
-                       ecclayout->eccpos[0]    = BADBLOCK_MARKER_LENGTH;
+                       oob_index               = BADBLOCK_MARKER_LENGTH;
                else
-                       ecclayout->eccpos[0]    = 1;
+                       oob_index               = 1;
+               for (i = 0; i < ecclayout->eccbytes; i++, oob_index++)
+                       ecclayout->eccpos[i]    = oob_index;

So, can you rearrange this series so patch 3 comes first?

> @@ -1848,8 +1849,9 @@ 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;
> +		/* include reserved-marker in ecclayout->oobfree calculation */
> +		ecclayout->oobfree->offset	= 1 +
> +				ecclayout->eccpos[ecclayout->eccbytes - 1] + 1;
>  		/* software bch library is used for locating errors */
>  		nand_chip->ecc.priv		= nand_bch_init(mtd,
>  							nand_chip->ecc.size,
> @@ -1884,8 +1886,9 @@ 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;
> +		/* reserved marker already included in ecclayout->eccbytes */
> +		ecclayout->oobfree->offset	=
> +				ecclayout->eccpos[ecclayout->eccbytes - 1] + 1;
>  		/* 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");
> @@ -1914,8 +1917,9 @@ 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;
> +		/* include reserved-marker in ecclayout->oobfree calculation */
> +		ecclayout->oobfree->offset	= 1 +
> +				ecclayout->eccpos[ecclayout->eccbytes - 1] + 1;
>  		/* software bch library is used for locating errors */
>  		nand_chip->ecc.priv		= nand_bch_init(mtd,
>  							nand_chip->ecc.size,
> @@ -1957,8 +1961,9 @@ 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;
> +		/* reserved marker already included in ecclayout->eccbytes */
> +		ecclayout->oobfree->offset	=
> +				ecclayout->eccpos[ecclayout->eccbytes - 1] + 1;
>  		break;
>  #else
>  		pr_err("nand: error: CONFIG_MTD_NAND_OMAP_BCH not enabled\n");

Brian