[1/5] mtd api changed to return bitflips on read operations

Proposed mtd api change.

Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
---
 include/linux/mtd/mtd.h |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

Le Mon, 28 Nov 2011 17:01:17 -0800,
Mike Dunn <mikedunn@newsguy.com> a écrit :

> +	/*
> +	 * max_bitflips returns to caller the greatest number of bit errors
> +	 * corrected on any one minimum i/o unit (e.g., nand page)
> +	 */
> +	int (*read) (struct mtd_info *mtd, loff_t from, size_t len,
> +		     size_t *retlen, u_char *buf, unsigned int *max_bitflips);
>  
> -	int (*read) (struct mtd_info *mtd, loff_t from, size_t len, size_t *retlen, u_char *buf);

Making this change in patch 1 will break the build if someone bisects
kernel changes between patch 1 and your other patches.

Also, seeing the large number of users that don't use/need the new
max_bitflips argument, wouldn't it be better to add a new, separate
->readext() operation (or another better name) ? This would probably
reduce the patch size quite a bit.

Also, another option is to allow max_bitflips to be NULL, which would
simplify things such as :

+	unsigned int max_bitflips;
 
-	ret = mtd->read(mtd, ptr, sizeof(fs), &sz, (u_char *) &fs);
+	ret = mtd->read(mtd, ptr, sizeof(fs), &sz, (u_char *) &fs,
+			&max_bitflips);


to

-	ret = mtd->read(mtd, ptr, sizeof(fs), &sz, (u_char *) &fs);
+	ret = mtd->read(mtd, ptr, sizeof(fs), &sz, (u_char *) &fs,
+			NULL);

and would therefore avoid the need for defining an useless variable.

Another question: is the max_bitflips information sufficient (i.e on a
large read with multiple pages, you will only get the value for the
worst page) ? Don't you need the bitflip count on a per-page basis ?

Regards,

Thomas

Hi,

thanks you, a nit-pick below.

On Mon, 2011-11-28 at 17:01 -0800, Mike Dunn wrote:
>   * @datbuf:	data buffer - if NULL only oob data are read/written
>   * @oobbuf:	oob data buffer
> + * @max_bitflips: for read operations, greatest number of bit errors corrected
> + *                on any one minimum i/o unit (e.g., nand page)
> + *                (value returned to caller by the driver)
>   *

Could you please make this change in the comment: s/greatest/maximum/ -
I find it easier to understand that way and it is more consistent with
the variable name.

Artem.

On Tue, 2011-11-29 at 14:40 +0100, Thomas Petazzoni wrote:
> Also, another option is to allow max_bitflips to be NULL, which would
> simplify things such as :

Yes, I vote for this solution.

> Another question: is the max_bitflips information sufficient (i.e on a
> large read with multiple pages, you will only get the value for the
> worst page) ? Don't you need the bitflip count on a per-page basis ?

I do not think we need accurate per-page information.

Artem.

On 12/01/2011 12:49 AM, Artem Bityutskiy wrote:
> Hi,
>
> thanks you, a nit-pick below.
>
> On Mon, 2011-11-28 at 17:01 -0800, Mike Dunn wrote:
>>   * @datbuf:	data buffer - if NULL only oob data are read/written
>>   * @oobbuf:	oob data buffer
>> + * @max_bitflips: for read operations, greatest number of bit errors corrected
>> + *                on any one minimum i/o unit (e.g., nand page)
>> + *                (value returned to caller by the driver)
>>   *
> Could you please make this change in the comment: s/greatest/maximum/ -
> I find it easier to understand that way and it is more consistent with
> the variable name.
>
>


Sure.  I'm a fan of clarity myself.

Thanks,
Mike

On 12/01/2011 01:08 AM, Artem Bityutskiy wrote:
> On Tue, 2011-11-29 at 14:40 +0100, Thomas Petazzoni wrote:
>> Also, another option is to allow max_bitflips to be NULL, which would
>> simplify things such as :
> Yes, I vote for this solution.


Yes, this was an dumb oversight on my part.  Will implement this as well.

I replied to Thomas' post, but the reply was flagged for human review by the ML
due to "suspicious header".  Probably because I had to read the original post
from the ML archives and paste it into my post because Thomas' post never made
it to my inbox for some reason.  Having email problems...


>> Another question: is the max_bitflips information sufficient (i.e on a
>> large read with multiple pages, you will only get the value for the
>> worst page) ? Don't you need the bitflip count on a per-page basis ?
> I do not think we need accurate per-page information.
>
>


My thought as well.

Should the patchset be combined into one patch?  Are separate, interdependent
patches ill-advised?

Thanks,
Mike

On Thu, 2011-12-01 at 03:22 -0800, Mike Dunn wrote:
> Should the patchset be combined into one patch?  Are separate, interdependent
> patches ill-advised?

I guess it is OK.

On 12/01/2011 03:38 AM, Artem Bityutskiy wrote:
> On Thu, 2011-12-01 at 03:22 -0800, Mike Dunn wrote:
>> Should the patchset be combined into one patch?  Are separate, interdependent
>> patches ill-advised?
> I guess it is OK.
>


OK, one patch it is.  I'll wait for Robert's fixed patch to get pushed to
l2-mtd-2.6.git and rebase with that before putting it together.

Thanks!
Mike