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

[Thomas Petazzoni <thomas.petazzoni@free-electrons.com>]

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
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com