Patchwork [v3,07/11] mtd: mtd_nandecctest: generalize injecting bit errors function for ecc code

login
register
mail settings
Submitter Akinobu Mita
Date Sept. 3, 2012, 1 p.m.
Message ID <1346677206-13621-8-git-send-email-akinobu.mita@gmail.com>
Download mbox | patch
Permalink /patch/181359/
State New
Headers show

Comments

Akinobu Mita - Sept. 3, 2012, 1 p.m.
In order to inject deliberate bit errors into the ecc code which only
has 3 bytes, this adjusts the bitops usage in inject_single_bit_error()
which is currently only used for injecting into the 256 or 512 bytes
data block.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: linux-mtd@lists.infradead.org
Cc: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
 drivers/mtd/tests/mtd_nandecctest.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)
Artem Bityutskiy - Sept. 4, 2012, 10:06 a.m.
On Mon, 2012-09-03 at 22:00 +0900, Akinobu Mita wrote:
> In order to inject deliberate bit errors into the ecc code which only
> has 3 bytes, this adjusts the bitops usage in inject_single_bit_error()
> which is currently only used for injecting into the 256 or 512 bytes
> data block.
> 
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: linux-mtd@lists.infradead.org
> Cc: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

I do not understand the commit message and the patch.
> ---
>  drivers/mtd/tests/mtd_nandecctest.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/tests/mtd_nandecctest.c b/drivers/mtd/tests/mtd_nandecctest.c
> index d3e8873..0274f73 100644
> --- a/drivers/mtd/tests/mtd_nandecctest.c
> +++ b/drivers/mtd/tests/mtd_nandecctest.c
> @@ -7,13 +7,22 @@
>  #include <linux/slab.h>
>  #include <linux/mtd/nand_ecc.h>
>  
> +#include <asm/byteorder.h>

I think you do not need to include this.

> +
>  #if defined(CONFIG_MTD_NAND) || defined(CONFIG_MTD_NAND_MODULE)
>  
> +#ifdef __LITTLE_ENDIAN
> +#define __change_bit_le(nr, addr) __change_bit(nr, addr)
> +#else
> +#define __change_bit_le(nr, addr) \
> +		__change_bit((nr) ^ ((BITS_PER_LONG - 1) & ~0x7), addr)
> +#endif

Why is this "#ifdef" needed? Can you please add a comment explaining
this? Which problem do you solve by adding '__change_bit_le()' ?
Akinobu Mita - Sept. 5, 2012, 10:42 a.m.
2012/9/4 Artem Bityutskiy <artem.bityutskiy@linux.intel.com>:
> On Mon, 2012-09-03 at 22:00 +0900, Akinobu Mita wrote:
>> In order to inject deliberate bit errors into the ecc code which only
>> has 3 bytes, this adjusts the bitops usage in inject_single_bit_error()
>> which is currently only used for injecting into the 256 or 512 bytes
>> data block.
>>
>> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
>> Cc: David Woodhouse <dwmw2@infradead.org>
>> Cc: linux-mtd@lists.infradead.org
>> Cc: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
>
> I do not understand the commit message and the patch.
>> ---
>>  drivers/mtd/tests/mtd_nandecctest.c | 13 +++++++++++--
>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mtd/tests/mtd_nandecctest.c b/drivers/mtd/tests/mtd_nandecctest.c
>> index d3e8873..0274f73 100644
>> --- a/drivers/mtd/tests/mtd_nandecctest.c
>> +++ b/drivers/mtd/tests/mtd_nandecctest.c
>> @@ -7,13 +7,22 @@
>>  #include <linux/slab.h>
>>  #include <linux/mtd/nand_ecc.h>
>>
>> +#include <asm/byteorder.h>
>
> I think you do not need to include this.

OK, I'll remove this.

>> +
>>  #if defined(CONFIG_MTD_NAND) || defined(CONFIG_MTD_NAND_MODULE)
>>
>> +#ifdef __LITTLE_ENDIAN
>> +#define __change_bit_le(nr, addr) __change_bit(nr, addr)
>> +#else
>> +#define __change_bit_le(nr, addr) \
>> +             __change_bit((nr) ^ ((BITS_PER_LONG - 1) & ~0x7), addr)
>> +#endif
>
> Why is this "#ifdef" needed? Can you please add a comment explaining
> this? Which problem do you solve by adding '__change_bit_le()' ?

Sorry for that imcomplete commit message.  I'll update with this:

mtd: mtd_nandecctest: support injecting bit error for ecc code

Currently inject_single_bit_error() is used to inject single bit error
into randomly selected bit position of the 256 or 512 bytes data block.

Later change will add tests which inject bit errors into the ecc code.
Unfortunately, inject_single_bit_error() doesn't work for the ecc code
which is not a multiple of sizeof(unsigned long).

Because bit fliping at random position is done by __change_bit().
For example, flipping bit position 0 by __change_bit(0, addr) modifies
3rd byte (32bit) or 7th byte (64bit) on big-endian systems.

Using little-endian version of bitops can fix this issue.  But
little-endian version of __change_bit is not yet available.
So this defines __change_bit_le() locally in a similar fashion to
asm-generic/bitops/le.h and use it.

...

Also I'll add this comment:

/*
 * The reason for this __change_bit_le() instead of __change_bit()
 * is to inject bit error properly within the region which is not a
 * multiple of sizeof(unsigned long) on big-endian systems
 */

Patch

diff --git a/drivers/mtd/tests/mtd_nandecctest.c b/drivers/mtd/tests/mtd_nandecctest.c
index d3e8873..0274f73 100644
--- a/drivers/mtd/tests/mtd_nandecctest.c
+++ b/drivers/mtd/tests/mtd_nandecctest.c
@@ -7,13 +7,22 @@ 
 #include <linux/slab.h>
 #include <linux/mtd/nand_ecc.h>
 
+#include <asm/byteorder.h>
+
 #if defined(CONFIG_MTD_NAND) || defined(CONFIG_MTD_NAND_MODULE)
 
+#ifdef __LITTLE_ENDIAN
+#define __change_bit_le(nr, addr) __change_bit(nr, addr)
+#else
+#define __change_bit_le(nr, addr) \
+		__change_bit((nr) ^ ((BITS_PER_LONG - 1) & ~0x7), addr)
+#endif
+
 static void inject_single_bit_error(void *data, size_t size)
 {
-	unsigned long offset = random32() % (size * BITS_PER_BYTE);
+	unsigned int offset = random32() % (size * BITS_PER_BYTE);
 
-	__change_bit(offset, data);
+	__change_bit_le(offset, data);
 }
 
 static void dump_data_ecc(void *error_data, void *error_ecc, void *correct_data,