Hi Brian,
>From: Brian Norris [mailto:[email protected]]
>>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 <[email protected]>
>> ---
>> 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
_______________________________________________
U-Boot mailing list
[email protected]
http://lists.denx.de/mailman/listinfo/u-boot
