Re: [PATCH 12/13] mtd/docg3: add ECC correction code

[Robert Jarzmik <robert.jarzmik@free.fr>]

Mike Dunn <mikedunn@newsguy.com> writes:

> Hi Robert,
>
> A few comments (limited to the bch decode for now)...
> To avoid confusion with the terminology in Ivan's BCH algorithm, I wouldn't call
> it calc_ecc.  It's actually recv_ecc ^ calc_ecc, where recv_ecc is what the hw
> read from the ecc field of the oob data, and calc_ecc is what the hw calculated
> from the page data.  Maybe just hwecc or similiar.
Yep, point taken for V2, hwecc sounds good.

...zip...
>> +	numerrs = decode_bch(docg3_bch, NULL, DOC_ECC_BCH_COVERED_BYTES,
>> +			     NULL, ecc, NULL, errorpos);
> This looks right, with the redefined DOC_ECC_BCH_COVERED_BYTES (now 520).   
>
> Some commentary would be helpful (though maybe I'm too verbose)...
>
>     /*
>      * The hardware ecc unit produces oob_ecc ^ calc_ecc.  The kernel's bch
>      * algorithm is used to decode this.  However the hw operates on page
>      * data in a bit order that is the reverse of that of the bch alg,
>      * requiring that the bits be reversed on the result.  Thanks to Ivan
>      * Djelic for his analysis.
>      */
Right again. I will copy paste that explanation in the code.

> I recommend you test this.  One way would be to compile the bch algorithm in
> userspace and use it to generate the ecc for a 520  byte test vector.  Reverse
> the bits of this ecc, then flip a few bits in the test vector and write it to a
> page in flash, with your driver modified to write the calculated ecc instead of
> that served up by the hardware.  Then when you read the page, the above code
> should identify and correct the bits you flipped (assuming a genuine flash error
> did not occur while reading back the page).  I have the bch alg modified for
> userspace, if that would help.
I did test it in userspace.
And after Ivan's proposition, I tested it also with nandwrite/nanddump by
introducing some random errors. The error correction code works, a few
amendments (for V2) will be added for "bitflipped" data writes/reads.

But the conclusion is that Ivan and your work is indeed directly applicable in
the G3 case, and tested :)

>> +	if (numerrs < 0)
>> +		return numerrs;
>
> I recommend you check for the -EINVAL return value and issue a big fat error. 
> Maybe BUG_ON(numerrs == -EINVAL), at least for now.
Okay.

>
> Another explanatory comment here...
>     /* undo last step in BCH alg (modulo mirroring not needed) */
Is that the same as the function comment about bit reversing (the modulo
mirroring part) ? If so, the function comment might not be clear enough. If not,
could you explain a bit further please ?

Thanks for the review.

Cheers.

--
Robert