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

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

[Mike Dunn]

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(-)

diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index 9f5b312..67f4bbc 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -81,6 +81,9 @@ struct mtd_erase_region_info {
  *		mode = MTD_OPS_PLACE_OOB or MTD_OPS_RAW)
  * @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)
  *
  * Note, it is allowed to read more than one OOB area at one go, but not write.
  * The interface assumes that the OOB write requests program only one page's
@@ -95,6 +98,7 @@ struct mtd_oob_ops {
 	uint32_t	ooboffs;
 	uint8_t		*datbuf;
 	uint8_t		*oobbuf;
+	unsigned int	max_bitflips;
 };
 
 #define MTD_MAX_OOBFREE_ENTRIES_LARGE	32
@@ -201,8 +205,13 @@ struct mtd_info {
 	 */
 	struct backing_dev_info *backing_dev_info;
 
+	/*
+	 * 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);
 	int (*write) (struct mtd_info *mtd, loff_t to, size_t len, size_t *retlen, const u_char *buf);
 
 	/* In blackbox flight recorder like scenarios we want to make successful
-- 
1.7.3.4

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

[Thomas Petazzoni]

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

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

[Artem Bityutskiy]

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.

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

[Artem Bityutskiy]

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.

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

[Mike Dunn]

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

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

[Mike Dunn]

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

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

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 219 bytes --]

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.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Mike Dunn]

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