[PATCH] mtd: nandbiterrs: Support for NAND biterrors test on platforms without raw write

[PATCH] mtd: nandbiterrs: Support for NAND biterrors test on platforms without raw write

[Iwo Mergler]

Hi all,


I'm vaguely aware that there is an ongoing effort to move this
stuff to mtd-utils, but I was unable to find a source tree
with the work so far.

Below is a single-line patch for the kernel tests, feel free
to apply to the userspace source as well.


Best regards,

Iwo


Support for NAND biterrors test on platforms without raw write

While the default test mode relies on raw write (mtd_write_oob) to introduce
bit errors into a page, the rewrite test mode doesn't need it.

Some drivers use eldritch data/ECC arrangements in a NAND page and reshuffle
things on-the-fly, to present a normal page view to the kernel. Typically,
raw write / read is unsupported on such platforms. Examples are Freescale
MXS and Qualcomm MDM9 and probably many others.

Changed the overwrite test to use normal writes.

Signed-off-by: Iwo Mergler <Iwo.Mergler@netcommwireless.com>
---
  drivers/mtd/tests/nandbiterrs.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/tests/nandbiterrs.c 
b/drivers/mtd/tests/nandbiterrs.c
index 09a4cca..f26dec8 100644
--- a/drivers/mtd/tests/nandbiterrs.c
+++ b/drivers/mtd/tests/nandbiterrs.c
@@ -290,7 +290,7 @@ static int overwrite_test(void)

      while (opno < max_overwrite) {

-        err = rewrite_page(0);
+        err = write_page(0);
          if (err)
              break;

-- 
2.7.3

Re: [PATCH] mtd: nandbiterrs: Support for NAND biterrors test on platforms without raw write

[Boris Brezillon]

Hi Iwo,

On Fri, 6 May 2016 10:00:12 +1000
Iwo Mergler <iwo.mergler@netcommwireless.com> wrote:

> Hi all,
> 
> 
> I'm vaguely aware that there is an ongoing effort to move this
> stuff to mtd-utils, but I was unable to find a source tree
> with the work so far.
> 
> Below is a single-line patch for the kernel tests, feel free
> to apply to the userspace source as well.
> 
> 
> Best regards,
> 
> Iwo
> 
> 
> Support for NAND biterrors test on platforms without raw write
> 
> While the default test mode relies on raw write (mtd_write_oob) to introduce
> bit errors into a page, the rewrite test mode doesn't need it.
> 
> Some drivers use eldritch data/ECC arrangements in a NAND page and reshuffle
> things on-the-fly, to present a normal page view to the kernel. Typically,
> raw write / read is unsupported on such platforms. Examples are Freescale
> MXS and Qualcomm MDM9 and probably many others.

Sorry, but I think such platforms should unshuffle the data/ECC
sections to expose a standard in-band/out-of-band view to the upper
layer.

This is completely doable since ->read_page_raw()/->write_page_raw()
can be overloaded (see the GPMI implementation or the default
ECC_HW_SYNDROME raw implementation if you need examples).

So, it's a NACK on my side.

Best Regards,

Boris


> 
> Changed the overwrite test to use normal writes.
> 
> Signed-off-by: Iwo Mergler <Iwo.Mergler@netcommwireless.com>
> ---
>   drivers/mtd/tests/nandbiterrs.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/tests/nandbiterrs.c 
> b/drivers/mtd/tests/nandbiterrs.c
> index 09a4cca..f26dec8 100644
> --- a/drivers/mtd/tests/nandbiterrs.c
> +++ b/drivers/mtd/tests/nandbiterrs.c
> @@ -290,7 +290,7 @@ static int overwrite_test(void)
> 
>       while (opno < max_overwrite) {
> 
> -        err = rewrite_page(0);
> +        err = write_page(0);
>           if (err)
>               break;
> 



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH] mtd: nandbiterrs: Support for NAND biterrors test on platforms without raw write

[Boris Brezillon]

On Sun, 8 May 2016 18:44:51 +0200
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> Hi Iwo,
> 
> On Fri, 6 May 2016 10:00:12 +1000
> Iwo Mergler <iwo.mergler@netcommwireless.com> wrote:
> 
> > Hi all,
> > 
> > 
> > I'm vaguely aware that there is an ongoing effort to move this
> > stuff to mtd-utils, but I was unable to find a source tree
> > with the work so far.
> > 
> > Below is a single-line patch for the kernel tests, feel free
> > to apply to the userspace source as well.
> > 
> > 
> > Best regards,
> > 
> > Iwo
> > 
> > 
> > Support for NAND biterrors test on platforms without raw write
> > 
> > While the default test mode relies on raw write (mtd_write_oob) to introduce
> > bit errors into a page, the rewrite test mode doesn't need it.
> > 
> > Some drivers use eldritch data/ECC arrangements in a NAND page and reshuffle
> > things on-the-fly, to present a normal page view to the kernel. Typically,
> > raw write / read is unsupported on such platforms. Examples are Freescale
> > MXS and Qualcomm MDM9 and probably many others.  

Sorry, I didn't realize you were talking about freescale (i.e. GPMI
controller). I'm pretty sure raw access is properly supported on this
platform, and if it's not, you should fix the ->{read,write}_page_raw()
implementations.

> 
> Sorry, but I think such platforms should unshuffle the data/ECC
> sections to expose a standard in-band/out-of-band view to the upper
> layer.
> 
> This is completely doable since ->read_page_raw()/->write_page_raw()
> can be overloaded (see the GPMI implementation or the default
> ECC_HW_SYNDROME raw implementation if you need examples).
> 
> So, it's a NACK on my side.
> 
> Best Regards,
> 
> Boris
> 
> 
> > 
> > Changed the overwrite test to use normal writes.
> > 
> > Signed-off-by: Iwo Mergler <Iwo.Mergler@netcommwireless.com>
> > ---
> >   drivers/mtd/tests/nandbiterrs.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mtd/tests/nandbiterrs.c 
> > b/drivers/mtd/tests/nandbiterrs.c
> > index 09a4cca..f26dec8 100644
> > --- a/drivers/mtd/tests/nandbiterrs.c
> > +++ b/drivers/mtd/tests/nandbiterrs.c
> > @@ -290,7 +290,7 @@ static int overwrite_test(void)
> > 
> >       while (opno < max_overwrite) {
> > 
> > -        err = rewrite_page(0);
> > +        err = write_page(0);
> >           if (err)
> >               break;
> >   
> 
> 
> 



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH] mtd: nandbiterrs: Support for NAND biterrors test on platforms without raw write

[Iwo Mergler]

Hi Boris,


I have to admit that your NACK surprised me.

My patch removes an unnecessary use of raw write
from the test. It was only there because of my
original implementation, which I now consider
mistaken.

I fully agree with you that raw write should
be implemented, despite the impediments.
Although I have seen at least one NAND controller
that always computed and wrote ECC, with no way
for software to circumvent it.

Could you please elaborate a little why you
don't want a test module to work with incomplete
MTD drivers? Is that supposed to be motivating
driver writers for better implementations? ;-)

Would you accept the patch if I remove the comment
about data reshuffling drivers? It's not required
for the patch and, as you correctly pointed out,
now inaccurate.


Best regards,

Iwo


On 05/09/2016 02:44 AM, Boris Brezillon wrote:
> Hi Iwo,
>
> On Fri, 6 May 2016 10:00:12 +1000
> Iwo Mergler <iwo.mergler@netcommwireless.com> wrote:
>
>> Hi all,
>>
>>
>> I'm vaguely aware that there is an ongoing effort to move this
>> stuff to mtd-utils, but I was unable to find a source tree
>> with the work so far.
>>
>> Below is a single-line patch for the kernel tests, feel free
>> to apply to the userspace source as well.
>>
>>
>> Best regards,
>>
>> Iwo
>>
>>
>> Support for NAND biterrors test on platforms without raw write
>>
>> While the default test mode relies on raw write (mtd_write_oob) to introduce
>> bit errors into a page, the rewrite test mode doesn't need it.
>>
>> Some drivers use eldritch data/ECC arrangements in a NAND page and reshuffle
>> things on-the-fly, to present a normal page view to the kernel. Typically,
>> raw write / read is unsupported on such platforms. Examples are Freescale
>> MXS and Qualcomm MDM9 and probably many others.
> Sorry, but I think such platforms should unshuffle the data/ECC
> sections to expose a standard in-band/out-of-band view to the upper
> layer.
>
> This is completely doable since ->read_page_raw()/->write_page_raw()
> can be overloaded (see the GPMI implementation or the default
> ECC_HW_SYNDROME raw implementation if you need examples).
>
> So, it's a NACK on my side.
>
> Best Regards,
>
> Boris
>
>
>> Changed the overwrite test to use normal writes.
>>
>> Signed-off-by: Iwo Mergler <Iwo.Mergler@netcommwireless.com>
>> ---
>>    drivers/mtd/tests/nandbiterrs.c | 2 +-
>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/tests/nandbiterrs.c
>> b/drivers/mtd/tests/nandbiterrs.c
>> index 09a4cca..f26dec8 100644
>> --- a/drivers/mtd/tests/nandbiterrs.c
>> +++ b/drivers/mtd/tests/nandbiterrs.c
>> @@ -290,7 +290,7 @@ static int overwrite_test(void)
>>
>>        while (opno < max_overwrite) {
>>
>> -        err = rewrite_page(0);
>> +        err = write_page(0);
>>            if (err)
>>                break;
>>
>
>

Re: [PATCH] mtd: nandbiterrs: Support for NAND biterrors test on platforms without raw write

[Boris Brezillon]

Hi Iwo,

On Mon, 9 May 2016 14:09:46 +1000
Iwo Mergler <iwo.mergler@netcommwireless.com> wrote:

> Hi Boris,
> 
> 
> I have to admit that your NACK surprised me.
> 
> My patch removes an unnecessary use of raw write
> from the test. It was only there because of my
> original implementation, which I now consider
> mistaken.

Sorry, I didn't look at the diff itself, and focused on the commit
message :-/. Indeed, using normal write in the overwrite test should be
harmless, but I still think that all controller should properly
implement raw access functions, otherwise the "incremental errors"
test is irrelevant (you'll overwrite ECC bytes along with in-band
data, and will end up with more bitflips than you expected).

> 
> I fully agree with you that raw write should
> be implemented, despite the impediments.
> Although I have seen at least one NAND controller
> that always computed and wrote ECC, with no way
> for software to circumvent it.

Hm, I was told that so many times and each time I had a closer look it
appeared to be untrue, so I tend to be skeptical on these kind of
statement now. Could you tell me more about this controller?

> 
> Could you please elaborate a little why you
> don't want a test module to work with incomplete
> MTD drivers?

As I said, this test module will only work in overwrite mode when the
controller does not support raw accesses.

> Is that supposed to be motivating
> driver writers for better implementations? ;-)

Yes, partly, and also because it's really helpful when you need to debug
NAND stuff.

Honestly, I'd rather see NAND implementations return -ENOTSUPP when
they do not support raw accesses than pretending they are.

> 
> Would you accept the patch if I remove the comment
> about data reshuffling drivers? It's not required
> for the patch and, as you correctly pointed out,
> now inaccurate.

At least rework it to mention that you're only modifying the overwrite
test, and that writing in normal mode in this case is harmless.

Thanks,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH] mtd: nandbiterrs: Support for NAND biterrors test on platforms without raw write

[Iwo Mergler]

On 05/10/2016 06:48 PM, Boris Brezillon wrote:
> Hi Iwo,
>
> On Mon, 9 May 2016 14:09:46 +1000
> Iwo Mergler <iwo.mergler@netcommwireless.com> wrote:
>
>> Hi Boris,
>>
>>
>> I have to admit that your NACK surprised me.
>>
>> My patch removes an unnecessary use of raw write
>> from the test. It was only there because of my
>> original implementation, which I now consider
>> mistaken.
> Sorry, I didn't look at the diff itself, and focused on the commit
> message :-/. Indeed, using normal write in the overwrite test should be
> harmless, but I still think that all controller should properly
> implement raw access functions, otherwise the "incremental errors"
> test is irrelevant (you'll overwrite ECC bytes along with in-band
> data, and will end up with more bitflips than you expected).
Indeed. Fully agree.


>> I fully agree with you that raw write should
>> be implemented, despite the impediments.
>> Although I have seen at least one NAND controller
>> that always computed and wrote ECC, with no way
>> for software to circumvent it.
> Hm, I was told that so many times and each time I had a closer look it
> appeared to be untrue, so I tend to be skeptical on these kind of
> statement now. Could you tell me more about this controller?
It's been a while. Probably Philips/NXP mobile phone chip
set, PNX8000 or similar. Don't think it ever made it into the
market as such, but the earliest ARM9 'microcontrollers' (LPC3xxx)
have a high likelihood of being the same silicon.

DMA with no provision of software I/O was fashionable in
the early '00s. Patent US8060688 is a reflection of that pain,
despite the attempts at obfuscation by the attorneys. ;-)

>> Could you please elaborate a little why you
>> don't want a test module to work with incomplete
>> MTD drivers?
> As I said, this test module will only work in overwrite mode when the
> controller does not support raw accesses.
>> Is that supposed to be motivating
>> driver writers for better implementations? ;-)
> Yes, partly, and also because it's really helpful when you need to debug
> NAND stuff.
>
> Honestly, I'd rather see NAND implementations return -ENOTSUPP when
> they do not support raw accesses than pretending they are.
I think the Qualcomm driver does that, the original form
of the test fails with an error code on the raw write.

>> Would you accept the patch if I remove the comment
>> about data reshuffling drivers? It's not required
>> for the patch and, as you correctly pointed out,
>> now inaccurate.
> At least rework it to mention that you're only modifying the overwrite
> test, and that writing in normal mode in this case is harmless.
I'll do that. Patch follows.

Best regards,

Iwo

Re: [PATCH] mtd: nandbiterrs: Support for NAND biterrors test on platforms without raw write

[Iwo Mergler]

Support for NAND biterrors test on platforms without raw write

While the default test mode relies on raw write (mtd_write_oob) to introduce
bit errors into a page, the rewrite test mode doesn't need it.

Changed the overwrite test to use normal writes. The default test mode
is unaffected and still requires raw write as before.

Signed-off-by: Iwo Mergler <Iwo.Mergler@netcommwireless.com>
---
  drivers/mtd/tests/nandbiterrs.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/tests/nandbiterrs.c 
b/drivers/mtd/tests/nandbiterrs.c
index 09a4cca..f26dec8 100644
--- a/drivers/mtd/tests/nandbiterrs.c
+++ b/drivers/mtd/tests/nandbiterrs.c
@@ -290,7 +290,7 @@ static int overwrite_test(void)

      while (opno < max_overwrite) {

-        err = rewrite_page(0);
+        err = write_page(0);
          if (err)
              break;

-- 
2.7.3

Re: [PATCH] mtd: nandbiterrs: Support for NAND biterrors test on platforms without raw write

[Boris Brezillon]

On Wed, 11 May 2016 16:54:57 +1000
Iwo Mergler <iwo.mergler@netcommwireless.com> wrote:

> Support for NAND biterrors test on platforms without raw write
> 
> While the default test mode relies on raw write (mtd_write_oob) to introduce
> bit errors into a page, the rewrite test mode doesn't need it.
> 
> Changed the overwrite test to use normal writes. The default test mode
> is unaffected and still requires raw write as before.

Applied.

Thanks,

Boris

> 
> Signed-off-by: Iwo Mergler <Iwo.Mergler@netcommwireless.com>
> ---
>   drivers/mtd/tests/nandbiterrs.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/tests/nandbiterrs.c 
> b/drivers/mtd/tests/nandbiterrs.c
> index 09a4cca..f26dec8 100644
> --- a/drivers/mtd/tests/nandbiterrs.c
> +++ b/drivers/mtd/tests/nandbiterrs.c
> @@ -290,7 +290,7 @@ static int overwrite_test(void)
> 
>       while (opno < max_overwrite) {
> 
> -        err = rewrite_page(0);
> +        err = write_page(0);
>           if (err)
>               break;
>