| Message ID | 572BDE8C.3020703@netcommwireless.com |
|---|---|
| State | Superseded |
| Headers | show |
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; >
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; > > > > >
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; >> > >
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
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
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;
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(-)