Re: [PATCH v6 0/6] mtd: nand: omap: optimized chip->ecc.correct() for H/W ECC schemes

[Brian Norris <computersforpeace@gmail.com>]

Hi Pekon,

First of all, thanks a lot for the extra efforts to make your commits
easier to digest. I do have a few comments, now that I can understand
the pieces a little better.

On Sat, Jan 04, 2014 at 08:18:12AM +0530, Pekon Gupta wrote:
> This patch-series fixes following issues in omap_elm_correct_data():
> (1) Dependency on a specific reserved byte-position in OOB area
>     to differentiates between erased-pages v/s programmed-pages.
>     Problem: reserved byte-position cannot be accomodated in all ecc-schemes
>     Problem: reserved byte-position can itself be subjected upto 8 bit-flips
>              causing the 0xff to become 0x00, causing page to be
>              mis-recognized as erased-page.
> 
> (2) Bit-flips in erased-pages are detected by comparing each byte of Data & OOB
>     with 0xff in check_erased_page().
>     Problem: This is causes performance penalty when erased-pages are checked.

Is this performance penalty significant, though? Shouldn't we be
checking for an erased page only on uncorrectable errors? And aren't
those uncorrectable occasions rare? I ask because it seems like you're
trading some precision for performance, but I think the case you're
optimizing is an uncommon path anyway. But perhaps I'm wrong.

I'll comment with some specifics on the patch itslef.

> (3) Current code is not scalable for future ECC schemes due to presence of 
>     tweaks for BCH4_ECC and BCH8_ECC at multiple places.
> 
> (4) Currently, bit-flips are evaluated and fixed even when ELM reports them as
>     un-correctable bit-flips, this should not happen as 'number-of-error' field
>     in ELM_LOCATION_STATUS becomes invalid when un-correctable flag is set.
> 
> (5) Driver should return with error-code = '-EBADMSG' when
>      uncorrectable bit-flip is detected
>      bit-flip outside valid Data and OOB region is detected

Your other points look good.

Thanks,
Brian