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@xxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
