Re: [PATCH v2] MTD: modify mtd api to return bitflip info on read operations

[Mike Dunn <mikedunn@newsguy.com>]

On 12/04/2011 10:23 PM, Artem Bityutskiy wrote:
> On Sat, 2011-12-03 at 12:20 -0800, Mike Dunn wrote:
>> Hi,
>>
>> This patch proposes a change to the mtd API for the purpose of returning to
>> the caller information on the number of bit errors corrected by the ecc
>> facilities of the device during read operations.  The affected functions are
>> read() and read_oob().
> Mike,
>
> this is quite big patch, and I've realized that it is difficult to
> review it because of the size. I know I suggested one big patch, but now
> I think we should try to split it, if we can. This way it will also be
> easier to pass through dwmw2 because at the end he is the MTD
> maintainer.
>
> I can see the following parts in your patch:
>
> 1. Mechanical part - no much brains needed, just change prototypes, add
> few comments, add NULL arguments everywhere. This is the biggest part. 
>
> 2. Implementation part - should be much smaller - implements
> max_bitflips support in MTD.
>
> Part 2 is interesting to reveiw, and currently part 1 adds so much noise
> that the review becomes difficult. Can you split your patches like that?


OK.  But you may still be underestimating the quantity of code that warrants
review.  Since we're going back to multiple patches anyway, how about four patches:

1. api change to mtd.h (a few lines just to put the rest in context)
2. nand, onenand
3. mtd infrastructructure: mtdblock, mtdchar, mtdpart, mtdconcat, ...
4. rote changes

Patch 4 will contain nothing beyond function definition changes, and the addition of

if(max_bitflips != NULL)
    *max_bitflips = 0;

to the top of the read function.  Anything beyond that will go into patch 3. 
Patch 4 will include all drivers that are not nand or onenand, since these don't
have ecc (two exceptions being devices/docg3.c and nand/alauda.c, which are are
nand drivers that do not use the nand interface).

Please let me know if that sounds reasonable.


> I apologize for not suggesting this from the very beginning.


No problem.  I expected this would be an iterative process and resigned myself
to a lot of tedium :)


>
> P.S. As a side note, I am thinking that with your patch the -EUCLEAN
> return code may go away. It has always been a bit ugly interface anyway.
> What do you think? 


I agree.  And with the max_bitflips change, it become cruft.  The patch required
to accomplish this would not be very large, either.



> My thinking is that we can do this separately later.
> But you need to add assertions like:
>
> 	WARN_ON(err == -EUCLEAN && max_bitflips == 0);
>
> in interesting places.


OK, I'll include this with the next patch set.

Thanks,
Mike