| Message ID | Pine.LNX.4.63.0908031237170.17084@redbean.intranet.gw-instruments.de |
|---|---|
| State | New, archived |
| Headers | show |
linux-mtd-bounces@lists.infradead.org wrote on 03/08/2009 12:50:44: > > > Hi, > > came around this problem while stress testing jffs2. From time to time the > block erase failed and the file system overflows. I don't know if there > are any Nor chips out there which allow a new erase to start when in erase > suspend. However, the chips on my board dont't. And even when it doesn't > make much sense to suspend an erase operation for another erase. > > Patch below fixes the problem for me. > > Have fun and take care, > > Peter > > Signed-off-by: Peter Wippich <pewi@gw-instruments.de> > --- > diff -Naur a/drivers/mtd/chips/cfi_cmdset_0002.c > b/drivers/mtd/chips/cfi_cmdset_0002.c > --- a/drivers/mtd/chips/cfi_cmdset_0002.c 2007-07-10 20:56:30.000000000 +0200 > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c 2009-08-02 19:55:34.000000000 +0200 > @@ -521,6 +521,10 @@ > case FL_ERASING: > if (mode == FL_WRITING) /* FIXME: Erase-suspend-program appears broken. */ > goto sleep; > + if (mode == FL_ERASING) { > + printk(KERN_INFO "attempt erase suspend with mode FL_ERASING\n") ; > + goto sleep; > + } > > if (!( mode == FL_READY > || mode == FL_POINT > This change looks bogus. You are not supposed to need the extra test. I do think this test is faulty though: if (!( mode == FL_READY || mode == FL_POINT || !cfip || (mode == FL_WRITING && (cfip->EraseSuspend & 0x2)) || (mode == FL_WRITING && (cfip->EraseSuspend & 0x1) ))) What happens if cfip is NULL? Jocke
> > > > > > > Hi, > > > > came around this problem while stress testing jffs2. From time to time the > > block erase failed and the file system overflows. I don't know if there > > are any Nor chips out there which allow a new erase to start when in erase > > suspend. However, the chips on my board dont't. And even when it doesn't > > make much sense to suspend an erase operation for another erase. > > > > Patch below fixes the problem for me. > > > > Have fun and take care, > > > > Peter > > > > Signed-off-by: Peter Wippich <pewi@gw-instruments.de> > > --- > > diff -Naur a/drivers/mtd/chips/cfi_cmdset_0002.c > > b/drivers/mtd/chips/cfi_cmdset_0002.c > > --- a/drivers/mtd/chips/cfi_cmdset_0002.c 2007-07-10 20:56:30.000000000 +0200 > > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c 2009-08-02 19:55:34.000000000 +0200 > > @@ -521,6 +521,10 @@ > > case FL_ERASING: > > if (mode == FL_WRITING) /* FIXME: Erase-suspend-program appears broken. */ > > goto sleep; > > + if (mode == FL_ERASING) { > > + printk(KERN_INFO "attempt erase suspend with mode FL_ERASING\n") ; > > + goto sleep; > > + } > > > > if (!( mode == FL_READY > > || mode == FL_POINT > > > > This change looks bogus. You are not supposed to need the extra test. > I do think this test is faulty though: > > if (!( mode == FL_READY > || mode == FL_POINT > || !cfip > || (mode == FL_WRITING && (cfip->EraseSuspend & 0x2)) > || (mode == FL_WRITING && (cfip->EraseSuspend & 0x1) > ))) > > What happens if cfip is NULL? eeh, and what if erase suspend isn't supported at all? Take a hint from cfi_cmdset_0001.c > > Jocke
On Mon, 3 Aug 2009, Joakim Tjernlund wrote: > > > > > > > > > > > > > Hi, > > > > > > came around this problem while stress testing jffs2. From time to time the > > > block erase failed and the file system overflows. I don't know if there > > > are any Nor chips out there which allow a new erase to start when in erase > > > suspend. However, the chips on my board dont't. And even when it doesn't > > > make much sense to suspend an erase operation for another erase. > > > > > > Patch below fixes the problem for me. > > > > > > Have fun and take care, > > > > > > Peter > > > > > > Signed-off-by: Peter Wippich <pewi@gw-instruments.de> > > > --- > > > diff -Naur a/drivers/mtd/chips/cfi_cmdset_0002.c > > > b/drivers/mtd/chips/cfi_cmdset_0002.c > > > --- a/drivers/mtd/chips/cfi_cmdset_0002.c 2007-07-10 20:56:30.000000000 +0200 > > > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c 2009-08-02 19:55:34.000000000 +0200 > > > @@ -521,6 +521,10 @@ > > > case FL_ERASING: > > > if (mode == FL_WRITING) /* FIXME: Erase-suspend-program appears broken. */ > > > goto sleep; > > > + if (mode == FL_ERASING) { > > > + printk(KERN_INFO "attempt erase suspend with mode FL_ERASING\n") ; > > > + goto sleep; > > > + } > > > > > > if (!( mode == FL_READY > > > || mode == FL_POINT > > > > > > > This change looks bogus. You are not supposed to need the extra test. > > I do think this test is faulty though: > > > > if (!( mode == FL_READY > > || mode == FL_POINT > > || !cfip > > || (mode == FL_WRITING && (cfip->EraseSuspend & 0x2)) > > || (mode == FL_WRITING && (cfip->EraseSuspend & 0x1) > > ))) > > > > What happens if cfip is NULL? > > eeh, and what if erase suspend isn't supported at all? > Take a hint from cfi_cmdset_0001.c Just double checked the data sheet (it's a SST39VF6401B). Erase suspend is supported. And the suspend seems to work fine. Just the subsequent second erase during suspend seems to fail. In chip_good() at the end of the erase it reads 0x1985 ..... instead of 0xffff. Actually this continues for all blocks until the suspended erase is resumend. After that everything works fine again. Will check the other issue you pointed out later on. Peter | Peter Wippich Voice: +49 30 46776411 | | G&W Instruments GmbH fax: +49 30 46776419 | | Gustav-Meyer-Allee 25, Geb. 12 Email: pewi@gw-instruments.de | | D-13355 Berlin / Germany |
> > > > > > > > > > > > > > Hi, > > > > > > came around this problem while stress testing jffs2. From time to time the > > > block erase failed and the file system overflows. I don't know if there > > > are any Nor chips out there which allow a new erase to start when in erase > > > suspend. However, the chips on my board dont't. And even when it doesn't > > > make much sense to suspend an erase operation for another erase. > > > > > > Patch below fixes the problem for me. > > > > > > Have fun and take care, > > > > > > Peter > > > > > > Signed-off-by: Peter Wippich <pewi@gw-instruments.de> > > > --- > > > diff -Naur a/drivers/mtd/chips/cfi_cmdset_0002.c > > > b/drivers/mtd/chips/cfi_cmdset_0002.c > > > --- a/drivers/mtd/chips/cfi_cmdset_0002.c 2007-07-10 20:56:30.000000000 +0200 > > > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c 2009-08-02 19:55:34.000000000 +0200 > > > @@ -521,6 +521,10 @@ > > > case FL_ERASING: > > > if (mode == FL_WRITING) /* FIXME: Erase-suspend-program appears broken. */ > > > goto sleep; > > > + if (mode == FL_ERASING) { > > > + printk(KERN_INFO "attempt erase suspend with mode FL_ERASING\n") ; > > > + goto sleep; > > > + } > > > > > > if (!( mode == FL_READY > > > || mode == FL_POINT > > > > > > > This change looks bogus. You are not supposed to need the extra test. > > I do think this test is faulty though: > > > > if (!( mode == FL_READY > > || mode == FL_POINT > > || !cfip > > || (mode == FL_WRITING && (cfip->EraseSuspend & 0x2)) > > || (mode == FL_WRITING && (cfip->EraseSuspend & 0x1) > > ))) > > > > What happens if cfip is NULL? > > eeh, and what if erase suspend isn't supported at all? > Take a hint from cfi_cmdset_0001.c gaah, cfip->EraseSuspend & 0x1 appears to mean Read only. No wonder there is a comment /* FIXME: Erase-suspend-program appears broken. */ Note, I don't use cmdset 0002 so I may be full of s... > > > > > Jocke
Peter Wippich <pewi@gw-instruments.de> wrote on 03/08/2009 14:20:27: > > On Mon, 3 Aug 2009, Joakim Tjernlund wrote: > > > > > > > > > > > > > > > > > > > > Hi, > > > > > > > > came around this problem while stress testing jffs2. From time to time the > > > > block erase failed and the file system overflows. I don't know if there > > > > are any Nor chips out there which allow a new erase to start when in erase > > > > suspend. However, the chips on my board dont't. And even when it doesn't > > > > make much sense to suspend an erase operation for another erase. > > > > > > > > Patch below fixes the problem for me. > > > > > > > > Have fun and take care, > > > > > > > > Peter > > > > > > > > Signed-off-by: Peter Wippich <pewi@gw-instruments.de> > > > > --- > > > > diff -Naur a/drivers/mtd/chips/cfi_cmdset_0002.c > > > > b/drivers/mtd/chips/cfi_cmdset_0002.c > > > > --- a/drivers/mtd/chips/cfi_cmdset_0002.c 2007-07-10 20:56:30.000000000 +0200 > > > > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c 2009-08-02 19:55:34.000000000 +0200 > > > > @@ -521,6 +521,10 @@ > > > > case FL_ERASING: > > > > if (mode == FL_WRITING) /* FIXME: Erase-suspend-program appears broken. */ > > > > goto sleep; > > > > + if (mode == FL_ERASING) { > > > > + printk(KERN_INFO "attempt erase suspend with mode FL_ERASING\n") ; > > > > + goto sleep; > > > > + } > > > > > > > > if (!( mode == FL_READY > > > > || mode == FL_POINT > > > > > > > > > > This change looks bogus. You are not supposed to need the extra test. > > > I do think this test is faulty though: > > > > > > if (!( mode == FL_READY > > > || mode == FL_POINT > > > || !cfip > > > || (mode == FL_WRITING && (cfip->EraseSuspend & 0x2)) > > > || (mode == FL_WRITING && (cfip->EraseSuspend & 0x1) > > > ))) > > > > > > What happens if cfip is NULL? > > > > eeh, and what if erase suspend isn't supported at all? > > Take a hint from cfi_cmdset_0001.c > > Just double checked the data sheet (it's a SST39VF6401B). Erase suspend is > supported. And the suspend seems to work fine. Just the subsequent second > erase during suspend seems to fail. In chip_good() at the end of the > erase it reads 0x1985 ..... instead of 0xffff. > > Actually this continues for all blocks until the suspended erase is > resumend. After that everything works fine again. Well, I think something else is going on then. if cfip is non NULL(you better check with a printk) that if should be true and you will end up in sleep, right? > > Will check the other issue you pointed out later on. > > Peter
Peter Wippich <pewi@gw-instruments.de> wrote on 03/08/2009 14:20:27: > > > > On Mon, 3 Aug 2009, Joakim Tjernlund wrote: > > > > > > > > > > > > > > > > > > > > Hi, > > > > > > > > came around this problem while stress testing jffs2. From time to time the > > > > block erase failed and the file system overflows. I don't know if there > > > > are any Nor chips out there which allow a new erase to start when in erase > > > > suspend. However, the chips on my board dont't. And even when it doesn't > > > > make much sense to suspend an erase operation for another erase. > > > > > > > > Patch below fixes the problem for me. > > > > > > > > Have fun and take care, > > > > > > > > Peter > > > > > > > > Signed-off-by: Peter Wippich <pewi@gw-instruments.de> > > > > --- > > > > diff -Naur a/drivers/mtd/chips/cfi_cmdset_0002.c > > > > b/drivers/mtd/chips/cfi_cmdset_0002.c > > > > --- a/drivers/mtd/chips/cfi_cmdset_0002.c 2007-07-10 20:56:30.000000000 +0200 > > > > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c 2009-08-02 19:55:34.000000000 +0200 > > > > @@ -521,6 +521,10 @@ > > > > case FL_ERASING: > > > > if (mode == FL_WRITING) /* FIXME: Erase-suspend-program appears broken. */ > > > > goto sleep; > > > > + if (mode == FL_ERASING) { > > > > + printk(KERN_INFO "attempt erase suspend with mode FL_ERASING\n") ; Try removing this printk, probably it is causing enough delay to cure your problem.
Hi Jocke, On Mon, 3 Aug 2009, Joakim Tjernlund wrote: > linux-mtd-bounces@lists.infradead.org wrote on 03/08/2009 12:50:44: > > > > > > > Hi, > > > > came around this problem while stress testing jffs2. From time to time the > > block erase failed and the file system overflows. I don't know if there > > are any Nor chips out there which allow a new erase to start when in erase > > suspend. However, the chips on my board dont't. And even when it doesn't > > make much sense to suspend an erase operation for another erase. > > > > Patch below fixes the problem for me. > > > > Have fun and take care, > > > > Peter > > > > Signed-off-by: Peter Wippich <pewi@gw-instruments.de> > > --- > > diff -Naur a/drivers/mtd/chips/cfi_cmdset_0002.c > > b/drivers/mtd/chips/cfi_cmdset_0002.c > > --- a/drivers/mtd/chips/cfi_cmdset_0002.c 2007-07-10 20:56:30.000000000 +0200 > > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c 2009-08-02 19:55:34.000000000 +0200 > > @@ -521,6 +521,10 @@ > > case FL_ERASING: > > if (mode == FL_WRITING) /* FIXME: Erase-suspend-program appears broken. */ > > goto sleep; > > + if (mode == FL_ERASING) { > > + printk(KERN_INFO "attempt erase suspend with mode FL_ERASING\n") ; > > + goto sleep; > > + } > > > > if (!( mode == FL_READY > > || mode == FL_POINT > > > > This change looks bogus. You are not supposed to need the extra test. > I do think this test is faulty though: > > if (!( mode == FL_READY > || mode == FL_POINT > || !cfip > || (mode == FL_WRITING && (cfip->EraseSuspend & 0x2)) > || (mode == FL_WRITING && (cfip->EraseSuspend & 0x1) > ))) > > What happens if cfip is NULL? Checked this one. Because the Chip is jedec probed (not cfi) and there is no fixup cfip should be NULL. Then the above will evaluate to : mode == FL_READY = FALSE mode == FL_POINT = FALSE !cfip = TRUE (next two skipped, hopefully ...... ;-) End ! TRUE = FALSE -> we will not go to sleep ! Actually, it is not quite clear to me what the intention of this expression was. But if the original intention was to only allow erase suspend when requested mode is FL_READY or FL_POINT it is obviously wrong..... Ciao, Peter | Peter Wippich Voice: +49 30 46776411 | | G&W Instruments GmbH fax: +49 30 46776419 | | Gustav-Meyer-Allee 25, Geb. 12 Email: pewi@gw-instruments.de | | D-13355 Berlin / Germany |
Peter Wippich <pewi@gw-instruments.de> wrote on 03/08/2009 15:07:30: > > > Hi Jocke, > > On Mon, 3 Aug 2009, Joakim Tjernlund wrote: > > > linux-mtd-bounces@lists.infradead.org wrote on 03/08/2009 12:50:44: > > > > > > > > > > > Hi, > > > > > > came around this problem while stress testing jffs2. From time to time the > > > block erase failed and the file system overflows. I don't know if there > > > are any Nor chips out there which allow a new erase to start when in erase > > > suspend. However, the chips on my board dont't. And even when it doesn't > > > make much sense to suspend an erase operation for another erase. > > > > > > Patch below fixes the problem for me. > > > > > > Have fun and take care, > > > > > > Peter > > > > > > Signed-off-by: Peter Wippich <pewi@gw-instruments.de> > > > --- > > > diff -Naur a/drivers/mtd/chips/cfi_cmdset_0002.c > > > b/drivers/mtd/chips/cfi_cmdset_0002.c > > > --- a/drivers/mtd/chips/cfi_cmdset_0002.c 2007-07-10 20:56:30.000000000 +0200 > > > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c 2009-08-02 19:55:34.000000000 +0200 > > > @@ -521,6 +521,10 @@ > > > case FL_ERASING: > > > if (mode == FL_WRITING) /* FIXME: Erase-suspend-program appears broken. */ > > > goto sleep; > > > + if (mode == FL_ERASING) { > > > + printk(KERN_INFO "attempt erase suspend with mode FL_ERASING\n") ; > > > + goto sleep; > > > + } > > > > > > if (!( mode == FL_READY > > > || mode == FL_POINT > > > > > > > This change looks bogus. You are not supposed to need the extra test. > > I do think this test is faulty though: > > > > if (!( mode == FL_READY > > || mode == FL_POINT > > || !cfip > > || (mode == FL_WRITING && (cfip->EraseSuspend & 0x2)) > > || (mode == FL_WRITING && (cfip->EraseSuspend & 0x1) > > ))) > > > > What happens if cfip is NULL? > > Checked this one. Because the Chip is jedec probed (not cfi) and there is > no fixup cfip should be NULL. Then the above will evaluate to : > mode == FL_READY = FALSE > mode == FL_POINT = FALSE > !cfip = TRUE > (next two skipped, hopefully ...... ;-) > > End ! TRUE = FALSE -> we will not go to sleep ! Actually, it is not quite > clear to me what the intention of this expression was. But if the original > intention was to only allow erase suspend when requested mode is FL_READY > or FL_POINT it is obviously wrong..... Probably: if (!cfip || !(cfip->EraseSuspend & (0x1|0x2)) || !(mode == FL_READY || mode == FL_POINT || (mode == FL_WRITING && (cfip->EraseSuspend & 0x2)))) Pehaps you can remove the if (mode == FL_WRITING) /* FIXME: Erase-suspend-program appears broken. */ goto sleep; too? ( in a separate patch though) Jocke
Joakim Tjernlund/Transmode wrote on 03/08/2009 15:25:10: > > Peter Wippich <pewi@gw-instruments.de> wrote on 03/08/2009 15:07:30: > > > > > > Hi Jocke, > > > > On Mon, 3 Aug 2009, Joakim Tjernlund wrote: > > > > > linux-mtd-bounces@lists.infradead.org wrote on 03/08/2009 12:50:44: > > > > > > > > > > > > > > > Hi, > > > > > > > > came around this problem while stress testing jffs2. From time to time the > > > > block erase failed and the file system overflows. I don't know if there > > > > are any Nor chips out there which allow a new erase to start when in erase > > > > suspend. However, the chips on my board dont't. And even when it doesn't > > > > make much sense to suspend an erase operation for another erase. > > > > > > > > Patch below fixes the problem for me. > > > > > > > > Have fun and take care, > > > > > > > > Peter > > > > > > > > Signed-off-by: Peter Wippich <pewi@gw-instruments.de> > > > > --- > > > > diff -Naur a/drivers/mtd/chips/cfi_cmdset_0002.c > > > > b/drivers/mtd/chips/cfi_cmdset_0002.c > > > > --- a/drivers/mtd/chips/cfi_cmdset_0002.c 2007-07-10 20:56:30.000000000 +0200 > > > > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c 2009-08-02 19:55:34.000000000 +0200 > > > > @@ -521,6 +521,10 @@ > > > > case FL_ERASING: > > > > if (mode == FL_WRITING) /* FIXME: Erase-suspend-program appears broken. */ > > > > goto sleep; > > > > + if (mode == FL_ERASING) { > > > > + printk(KERN_INFO "attempt erase suspend with mode FL_ERASING\n") ; > > > > + goto sleep; > > > > + } > > > > > > > > if (!( mode == FL_READY > > > > || mode == FL_POINT > > > > > > > > > > This change looks bogus. You are not supposed to need the extra test. > > > I do think this test is faulty though: > > > > > > if (!( mode == FL_READY > > > || mode == FL_POINT > > > || !cfip > > > || (mode == FL_WRITING && (cfip->EraseSuspend & 0x2)) > > > || (mode == FL_WRITING && (cfip->EraseSuspend & 0x1) > > > ))) > > > > > > What happens if cfip is NULL? > > > > Checked this one. Because the Chip is jedec probed (not cfi) and there is > > no fixup cfip should be NULL. Then the above will evaluate to : > > mode == FL_READY = FALSE > > mode == FL_POINT = FALSE > > !cfip = TRUE > > (next two skipped, hopefully ...... ;-) > > > > End ! TRUE = FALSE -> we will not go to sleep ! Actually, it is not quite > > clear to me what the intention of this expression was. But if the original > > intention was to only allow erase suspend when requested mode is FL_READY > > or FL_POINT it is obviously wrong..... > Probably: > if (!cfip || !(cfip->EraseSuspend & (0x1|0x2)) || > !(mode == FL_READY || mode == FL_POINT || > (mode == FL_WRITING && (cfip->EraseSuspend & 0x2)))) > > Pehaps you can remove the > if (mode == FL_WRITING) /* FIXME: Erase-suspend-program appears broken. */ > goto sleep; > too? ( in a separate patch though) Oh, you will have to fake/construct a cfip too for you JEDEC chip. Jocke
Hi Jocke, On Mon, 3 Aug 2009, Joakim Tjernlund wrote: > Joakim Tjernlund/Transmode wrote on 03/08/2009 15:25:10: > > > > Peter Wippich <pewi@gw-instruments.de> wrote on 03/08/2009 15:07:30: > > > > > > > > > Hi Jocke, > > > > > > On Mon, 3 Aug 2009, Joakim Tjernlund wrote: > > > > > > > linux-mtd-bounces@lists.infradead.org wrote on 03/08/2009 12:50:44: > > > > > > > > > > > > > > > > > > > Hi, > > > > > > > > > > came around this problem while stress testing jffs2. From time to time the > > > > > block erase failed and the file system overflows. I don't know if there > > > > > are any Nor chips out there which allow a new erase to start when in erase > > > > > suspend. However, the chips on my board dont't. And even when it doesn't > > > > > make much sense to suspend an erase operation for another erase. > > > > > > > > > > Patch below fixes the problem for me. > > > > > > > > > > Have fun and take care, > > > > > > > > > > Peter > > > > > > > > > > Signed-off-by: Peter Wippich <pewi@gw-instruments.de> > > > > > --- > > > > > diff -Naur a/drivers/mtd/chips/cfi_cmdset_0002.c > > > > > b/drivers/mtd/chips/cfi_cmdset_0002.c > > > > > --- a/drivers/mtd/chips/cfi_cmdset_0002.c 2007-07-10 20:56:30.000000000 +0200 > > > > > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c 2009-08-02 19:55:34.000000000 +0200 > > > > > @@ -521,6 +521,10 @@ > > > > > case FL_ERASING: > > > > > if (mode == FL_WRITING) /* FIXME: Erase-suspend-program appears broken. */ > > > > > goto sleep; > > > > > + if (mode == FL_ERASING) { > > > > > + printk(KERN_INFO "attempt erase suspend with mode FL_ERASING\n") ; > > > > > + goto sleep; > > > > > + } > > > > > > > > > > if (!( mode == FL_READY > > > > > || mode == FL_POINT > > > > > > > > > > > > > This change looks bogus. You are not supposed to need the extra test. > > > > I do think this test is faulty though: > > > > > > > > if (!( mode == FL_READY > > > > || mode == FL_POINT > > > > || !cfip > > > > || (mode == FL_WRITING && (cfip->EraseSuspend & 0x2)) > > > > || (mode == FL_WRITING && (cfip->EraseSuspend & 0x1) > > > > ))) > > > > > > > > What happens if cfip is NULL? > > > > > > Checked this one. Because the Chip is jedec probed (not cfi) and there is > > > no fixup cfip should be NULL. Then the above will evaluate to : > > > mode == FL_READY = FALSE > > > mode == FL_POINT = FALSE > > > !cfip = TRUE > > > (next two skipped, hopefully ...... ;-) > > > > > > End ! TRUE = FALSE -> we will not go to sleep ! Actually, it is not quite > > > clear to me what the intention of this expression was. But if the original > > > intention was to only allow erase suspend when requested mode is FL_READY > > > or FL_POINT it is obviously wrong..... > > > Probably: > > if (!cfip || !(cfip->EraseSuspend & (0x1|0x2)) || > > !(mode == FL_READY || mode == FL_POINT || > > (mode == FL_WRITING && (cfip->EraseSuspend & 0x2)))) > > > > Pehaps you can remove the > > if (mode == FL_WRITING) /* FIXME: Erase-suspend-program appears broken. */ > > goto sleep; > > too? ( in a separate patch though) > > Oh, you will have to fake/construct a cfip too for you JEDEC chip. > > Jocke I would prefer something more readable and less tricky. How about something like this. #define ERASE_SUSPEND_NONE 0 #define ERASE_SUSPEND_READ 1 #define ERASE_SUSPEND_RW 2 if( !( mode==FL_READY || mode==FL_POINT) || ( cfip && cfip->EraseSuspend==ERASE_SUSPEND_NONE) ) goto sleep ; For CFI probed chips or chips with fixups this will allow disabling erase suspend support by setting cfip->EraseSuspend an will assume at least erase suspend for reads on all others. Ciao, Peter | Peter Wippich Voice: +49 30 46776411 | | G&W Instruments GmbH fax: +49 30 46776419 | | Gustav-Meyer-Allee 25, Geb. 12 Email: pewi@gw-instruments.de | | D-13355 Berlin / Germany |
Peter Wippich <pewi@gw-instruments.de> wrote on 03/08/2009 16:02:18: > > Hi Jocke, > > On Mon, 3 Aug 2009, Joakim Tjernlund wrote: > > > Joakim Tjernlund/Transmode wrote on 03/08/2009 15:25:10: > > > > > > Peter Wippich <pewi@gw-instruments.de> wrote on 03/08/2009 15:07:30: > > > > > > > > > > > > Hi Jocke, > > > > > > > > On Mon, 3 Aug 2009, Joakim Tjernlund wrote: > > > > > > > > > linux-mtd-bounces@lists.infradead.org wrote on 03/08/2009 12:50:44: > > > > > > > > > > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > came around this problem while stress testing jffs2. From time to time the > > > > > > block erase failed and the file system overflows. I don't know if there > > > > > > are any Nor chips out there which allow a new erase to start when in erase > > > > > > suspend. However, the chips on my board dont't. And even when it doesn't > > > > > > make much sense to suspend an erase operation for another erase. > > > > > > > > > > > > Patch below fixes the problem for me. > > > > > > > > > > > > Have fun and take care, > > > > > > > > > > > > Peter > > > > > > > > > > > > Signed-off-by: Peter Wippich <pewi@gw-instruments.de> > > > > > > --- > > > > > > diff -Naur a/drivers/mtd/chips/cfi_cmdset_0002.c > > > > > > b/drivers/mtd/chips/cfi_cmdset_0002.c > > > > > > --- a/drivers/mtd/chips/cfi_cmdset_0002.c 2007-07-10 20:56:30. > 000000000 +0200 > > > > > > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c 2009-08-02 19:55:34. > 000000000 +0200 > > > > > > @@ -521,6 +521,10 @@ > > > > > > case FL_ERASING: > > > > > > if (mode == FL_WRITING) /* FIXME: Erase-suspend-program > appears broken. */ > > > > > > goto sleep; > > > > > > + if (mode == FL_ERASING) { > > > > > > + printk(KERN_INFO "attempt erase suspend with mode FL_ERASING\n") ; > > > > > > + goto sleep; > > > > > > + } > > > > > > > > > > > > if (!( mode == FL_READY > > > > > > || mode == FL_POINT > > > > > > > > > > > > > > > > This change looks bogus. You are not supposed to need the extra test. > > > > > I do think this test is faulty though: > > > > > > > > > > if (!( mode == FL_READY > > > > > || mode == FL_POINT > > > > > || !cfip > > > > > || (mode == FL_WRITING && (cfip->EraseSuspend & 0x2)) > > > > > || (mode == FL_WRITING && (cfip->EraseSuspend & 0x1) > > > > > ))) > > > > > > > > > > What happens if cfip is NULL? > > > > > > > > Checked this one. Because the Chip is jedec probed (not cfi) and there is > > > > no fixup cfip should be NULL. Then the above will evaluate to : > > > > mode == FL_READY = FALSE > > > > mode == FL_POINT = FALSE > > > > !cfip = TRUE > > > > (next two skipped, hopefully ...... ;-) > > > > > > > > End ! TRUE = FALSE -> we will not go to sleep ! Actually, it is not quite > > > > clear to me what the intention of this expression was. But if the original > > > > intention was to only allow erase suspend when requested mode is FL_READY > > > > or FL_POINT it is obviously wrong..... > > > > > Probably: > > > if (!cfip || !(cfip->EraseSuspend & (0x1|0x2)) || > > > !(mode == FL_READY || mode == FL_POINT || > > > (mode == FL_WRITING && (cfip->EraseSuspend & 0x2)))) > > > > > > Pehaps you can remove the > > > if (mode == FL_WRITING) /* FIXME: Erase-suspend-program appears broken. */ > > > goto sleep; > > > too? ( in a separate patch though) > > > > Oh, you will have to fake/construct a cfip too for you JEDEC chip. > > > > Jocke > > I would prefer something more readable and less tricky. How about > something like this. > > #define ERASE_SUSPEND_NONE 0 > #define ERASE_SUSPEND_READ 1 > #define ERASE_SUSPEND_RW 2 > > > if( !( mode==FL_READY || mode==FL_POINT) > || ( cfip && cfip->EraseSuspend==ERASE_SUSPEND_NONE) ) > goto sleep ; > > For CFI probed chips or chips with fixups this will allow disabling erase > suspend support by setting cfip->EraseSuspend an will assume at least > erase suspend for reads on all others. Hi again :) I think mine is better because: 1) It matches cmdset_0001 and the code in general better. 2) It can handle FL_WRITING too. But I am not the maintainer and I don't use 0002 myself, just trying to help out. I hope you will constuct a cfip too for your JEDEC chips. It is no fun to be without erase suspend :( Jocke
Hi Jocke, On Mon, 3 Aug 2009, Joakim Tjernlund wrote: > Hi again :) > > I think mine is better because: > 1) It matches cmdset_0001 and the code in general better. > 2) It can handle FL_WRITING too. > > But I am not the maintainer and I don't use 0002 myself, just trying to > help out. > > I hope you will constuct a cfip too for your JEDEC chips. It is no > fun to be without erase suspend :( > It's fine to have some discussion about that. From that at least we agree that something is wrong here. The main difference between the suggested patches is the change in behaviour from the current implementation. Currently the default behaviour is that erase suspend will work with jedec and cfi probed chips. I can not tell if this is by intention or by chance, but I think if we change something it should not break existing implementations. So at least a final fix shall: - use erase suspend for jedec chips for "read only" suspends were cfip == NULL - use erase suspend for all cfi probed chips for "read only" suspends if supported (as indicated by cfip->EraseSuspend) And for the write case, I can not tell why the erase suspend is viewed to be broken here. If we change this, as in your patch, severe testing is needed to make sure it is realy working. Currently I don't have the free resources to do this testing. Maybe we'll find some volunteer ?!....... I'll try to find a fix which will not break current implementations and allows updates to implement erase suspend for writes easily. Appriciate your comments. BTW: who's the maintainer and what are her/his thoughts. Ciao, Peter | Peter Wippich Voice: +49 30 46776411 | | G&W Instruments GmbH fax: +49 30 46776419 | | Gustav-Meyer-Allee 25, Geb. 12 Email: pewi@gw-instruments.de | | D-13355 Berlin / Germany |
Peter Wippich <pewi@gw-instruments.de> wrote on 03/08/2009 23:24:01: > > > Hi Jocke, > > On Mon, 3 Aug 2009, Joakim Tjernlund wrote: > > > Hi again :) > > > > I think mine is better because: > > 1) It matches cmdset_0001 and the code in general better. > > 2) It can handle FL_WRITING too. > > > > But I am not the maintainer and I don't use 0002 myself, just trying to > > help out. > > > > I hope you will constuct a cfip too for your JEDEC chips. It is no > > fun to be without erase suspend :( > > > > It's fine to have some discussion about that. From that at least we agree > that something is wrong here. The main difference between the suggested > patches is the change in behaviour from the current implementation. > Currently the default behaviour is that erase suspend will work with jedec > and cfi probed chips. I can not tell if this is by intention or by chance, > but I think if we change something it should not break existing > implementations. > So at least a final fix shall: > - use erase suspend for jedec chips for "read only" suspends were > cfip == NULL Are you sure all JEDEC chips support RO suspends? If not, I think this is a 3 step/pathes procedure: 1) Just fix the if stmt as I outlined. Then JEDEC chips wont bug, but won't have erase suspend either unless: 2) Add a fixup for your JEDEC flash that fills in the erase suspend bits you need. If all JEDEC chips support RO suspends you can do a JEDEC generic fixup that adds this capability to cfip. 3) remove the FIXME and test if it works for you. Send it to the list too and ask for some testing. > - use erase suspend for all cfi probed chips for "read only" suspends > if supported (as indicated by cfip->EraseSuspend) > > And for the write case, I can not tell why the erase suspend is viewed to > be broken here. If we change this, as in your patch, severe testing is > needed to make sure it is realy working. Currently I don't have the free > resources to do this testing. Maybe we'll find some volunteer ?!....... > > I'll try to find a fix which will not break current implementations and > allows updates to implement erase suspend for writes easily. Appriciate > your comments. > > BTW: who's the maintainer and what are her/his thoughts. Don't konw.
And Hi again, On Tue, 4 Aug 2009, Joakim Tjernlund wrote: > Peter Wippich <pewi@gw-instruments.de> wrote on 03/08/2009 23:24:01: > > > > > > Hi Jocke, > > > > On Mon, 3 Aug 2009, Joakim Tjernlund wrote: > > > > > Hi again :) > > > > > > I think mine is better because: > > > 1) It matches cmdset_0001 and the code in general better. > > > 2) It can handle FL_WRITING too. > > > > > > But I am not the maintainer and I don't use 0002 myself, just trying to > > > help out. > > > > > > I hope you will constuct a cfip too for your JEDEC chips. It is no > > > fun to be without erase suspend :( > > > > > > > It's fine to have some discussion about that. From that at least we agree > > that something is wrong here. The main difference between the suggested > > patches is the change in behaviour from the current implementation. > > Currently the default behaviour is that erase suspend will work with jedec > > and cfi probed chips. I can not tell if this is by intention or by chance, > > but I think if we change something it should not break existing > > implementations. > > So at least a final fix shall: > > - use erase suspend for jedec chips for "read only" suspends were > > cfip == NULL > > Are you sure all JEDEC chips support RO suspends? If not, > I think this is a 3 step/pathes procedure: > 1) Just fix the if stmt as I outlined. Then JEDEC chips wont bug, but won't have erase > suspend either unless: > > 2) Add a fixup for your JEDEC flash that fills in the erase suspend bits you need. If all > JEDEC chips support RO suspends you can do a JEDEC generic fixup that adds this capability > to cfip. > > 3) remove the FIXME and test if it works for you. Send it to the > list too and ask for some testing. > > > - use erase suspend for all cfi probed chips for "read only" suspends > > if supported (as indicated by cfip->EraseSuspend) > > > > And for the write case, I can not tell why the erase suspend is viewed to > > be broken here. If we change this, as in your patch, severe testing is > > needed to make sure it is realy working. Currently I don't have the free > > resources to do this testing. Maybe we'll find some volunteer ?!....... > > > > I'll try to find a fix which will not break current implementations and > > allows updates to implement erase suspend for writes easily. Appriciate > > your comments. > > > > BTW: who's the maintainer and what are her/his thoughts. > > Don't konw. Partly disagree. For Step 1 we shall leave erase suspend active for Jedec Chips because this is the current behaviour. If this behaviour is wrong we would have seen a lot of complains on the list (which aren't there). If we change the default behaviour then we have to add fxups for each and every Jedec chip which is used out there and supports erase suspend. And from cherry picking in the long list most chips which use cfi_cmdset_0002 support erase suspend. And BTW: I realy hate this "cfip->EraseSuspend & 0x1" stuff. Nobody knows what this is unless going through the CFI specification. It would make the code much better readable if we introduce some symbolic constants here. Ciao, Peter
Peter Wippich <pewi@gw-instruments.de> wrote on 04/08/2009 14:21:25: > > > And Hi again, > > On Tue, 4 Aug 2009, Joakim Tjernlund wrote: > > > Peter Wippich <pewi@gw-instruments.de> wrote on 03/08/2009 23:24:01: > > > > > > > > > Hi Jocke, > > > > > > On Mon, 3 Aug 2009, Joakim Tjernlund wrote: > > > > > > > Hi again :) > > > > > > > > I think mine is better because: > > > > 1) It matches cmdset_0001 and the code in general better. > > > > 2) It can handle FL_WRITING too. > > > > > > > > But I am not the maintainer and I don't use 0002 myself, just trying to > > > > help out. > > > > > > > > I hope you will constuct a cfip too for your JEDEC chips. It is no > > > > fun to be without erase suspend :( > > > > > > > > > > It's fine to have some discussion about that. From that at least we agree > > > that something is wrong here. The main difference between the suggested > > > patches is the change in behaviour from the current implementation. > > > Currently the default behaviour is that erase suspend will work with jedec > > > and cfi probed chips. I can not tell if this is by intention or by chance, > > > but I think if we change something it should not break existing > > > implementations. > > > So at least a final fix shall: > > > - use erase suspend for jedec chips for "read only" suspends were > > > cfip == NULL > > > > Are you sure all JEDEC chips support RO suspends? If not, > > I think this is a 3 step/pathes procedure: > > 1) Just fix the if stmt as I outlined. Then JEDEC chips wont bug, but won't have erase > > suspend either unless: > > > > 2) Add a fixup for your JEDEC flash that fills in the erase suspend bits you > need. If all > > JEDEC chips support RO suspends you can do a JEDEC generic fixup that > adds this capability > > to cfip. > > > > 3) remove the FIXME and test if it works for you. Send it to the > > list too and ask for some testing. > > > > > - use erase suspend for all cfi probed chips for "read only" suspends > > > if supported (as indicated by cfip->EraseSuspend) > > > > > > And for the write case, I can not tell why the erase suspend is viewed to > > > be broken here. If we change this, as in your patch, severe testing is > > > needed to make sure it is realy working. Currently I don't have the free > > > resources to do this testing. Maybe we'll find some volunteer ?!....... > > > > > > I'll try to find a fix which will not break current implementations and > > > allows updates to implement erase suspend for writes easily. Appriciate > > > your comments. > > > > > > BTW: who's the maintainer and what are her/his thoughts. > > > > Don't konw. > > Partly disagree. For Step 1 we shall leave erase suspend active for Jedec > Chips because this is the current behaviour. If this behaviour is wrong > we would have seen a lot of complains on the list (which aren't there). > If we change the default behaviour then we have to add fxups for each and > every Jedec chip which is used out there and supports erase suspend. And > from cherry picking in the long list most chips which use > cfi_cmdset_0002 support erase suspend. Well, I figured the fixups was already there, in jedec_probe.c, but your chip isn't listed I guess. Try adding your chip there and se what happens. I suspect that only chips that are in this list works or they are all broken if the do not set cfip. > > And BTW: I realy hate this "cfip->EraseSuspend & 0x1" stuff. Nobody knows > what this is unless going through the CFI specification. It would make > the code much better readable if we introduce some symbolic constants > here. Possibly, but that is another issue that will have to be addressed with a separate cleanup patch. The style in both 0001 and 0002 is the "cfip->EraseSuspend & 0x1" style so you better follow it with your fixes, you got a much better chance getting you stuff in the tree with as little changes as possible while keeping the current style. Anyhow, style should be discussed with David Woodhouse, the maintainer of MTD. I am done with this now and you need to find someone else that actually cares/uses this driver with JEDEC chips. Jocke
Hi Jocke, On Tue, 4 Aug 2009, Joakim Tjernlund wrote: > Peter Wippich <pewi@gw-instruments.de> wrote on 04/08/2009 14:21:25: > > > > > > And Hi again, > > > > On Tue, 4 Aug 2009, Joakim Tjernlund wrote: > > > > > Peter Wippich <pewi@gw-instruments.de> wrote on 03/08/2009 23:24:01: > > > > > > > > > > > > Hi Jocke, > > > > > > > > On Mon, 3 Aug 2009, Joakim Tjernlund wrote: > > > > > > > > > Hi again :) > > > > > > > > > > I think mine is better because: > > > > > 1) It matches cmdset_0001 and the code in general better. > > > > > 2) It can handle FL_WRITING too. > > > > > > > > > > But I am not the maintainer and I don't use 0002 myself, just trying to > > > > > help out. > > > > > > > > > > I hope you will constuct a cfip too for your JEDEC chips. It is no > > > > > fun to be without erase suspend :( > > > > > > > > > > > > > It's fine to have some discussion about that. From that at least we agree > > > > that something is wrong here. The main difference between the suggested > > > > patches is the change in behaviour from the current implementation. > > > > Currently the default behaviour is that erase suspend will work with jedec > > > > and cfi probed chips. I can not tell if this is by intention or by chance, > > > > but I think if we change something it should not break existing > > > > implementations. > > > > So at least a final fix shall: > > > > - use erase suspend for jedec chips for "read only" suspends were > > > > cfip == NULL > > > > > > Are you sure all JEDEC chips support RO suspends? If not, > > > I think this is a 3 step/pathes procedure: > > > 1) Just fix the if stmt as I outlined. Then JEDEC chips wont bug, but won't have erase > > > suspend either unless: > > > > > > 2) Add a fixup for your JEDEC flash that fills in the erase suspend bits you > > need. If all > > > JEDEC chips support RO suspends you can do a JEDEC generic fixup that > > adds this capability > > > to cfip. > > > > > > 3) remove the FIXME and test if it works for you. Send it to the > > > list too and ask for some testing. > > > > > > > - use erase suspend for all cfi probed chips for "read only" suspends > > > > if supported (as indicated by cfip->EraseSuspend) > > > > > > > > And for the write case, I can not tell why the erase suspend is viewed to > > > > be broken here. If we change this, as in your patch, severe testing is > > > > needed to make sure it is realy working. Currently I don't have the free > > > > resources to do this testing. Maybe we'll find some volunteer ?!....... > > > > > > > > I'll try to find a fix which will not break current implementations and > > > > allows updates to implement erase suspend for writes easily. Appriciate > > > > your comments. > > > > > > > > BTW: who's the maintainer and what are her/his thoughts. > > > > > > Don't konw. > > > > Partly disagree. For Step 1 we shall leave erase suspend active for Jedec > > Chips because this is the current behaviour. If this behaviour is wrong > > we would have seen a lot of complains on the list (which aren't there). > > If we change the default behaviour then we have to add fxups for each and > > every Jedec chip which is used out there and supports erase suspend. And > > from cherry picking in the long list most chips which use > > cfi_cmdset_0002 support erase suspend. > > Well, I figured the fixups was already there, in jedec_probe.c, but > your chip isn't > listed I guess. Try adding your chip there and se what happens. I suspect > that only chips that are in this list works or they are all broken if the > do not set cfip. Mmmhh, maybe I'm getting blind. Can not find anything here. The only reference to cmdset_priv in jedec_probe.c I can find is p_cfi->cmdset_priv = NULL; And from this it follows that cfip is NULL for all jedec probed chips in get_chip() unless it gets a fixup somewhere else, which I can not find either. Which concludes that there are some NULL pointer free()'s , but that should not hurt much. > > And BTW: I realy hate this "cfip->EraseSuspend & 0x1" stuff. Nobody knows > > what this is unless going through the CFI specification. It would make > > the code much better readable if we introduce some symbolic constants > > here. > > Possibly, but that is another issue that will have to be addressed with a > separate cleanup patch. The style in both 0001 and 0002 is the "cfip->EraseSuspend & 0x1" > style so you better follow it with your fixes, you got a much better chance getting > you stuff in the tree with as little changes as possible while keeping the current style. > > Anyhow, style should be discussed with David Woodhouse, the maintainer of MTD. agreed. > I am done with this now and you need to find someone else that actually > cares/uses this driver with JEDEC chips. Thank you for taking your time discussing that. Peter
diff -Naur a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c --- a/drivers/mtd/chips/cfi_cmdset_0002.c 2007-07-10 20:56:30.000000000 +0200 +++ b/drivers/mtd/chips/cfi_cmdset_0002.c 2009-08-02 19:55:34.000000000 +0200 @@ -521,6 +521,10 @@ case FL_ERASING: if (mode == FL_WRITING) /* FIXME: Erase-suspend-program appears broken. */ goto sleep; + if (mode == FL_ERASING) { + printk(KERN_INFO "attempt erase suspend with mode FL_ERASING\n") ; + goto sleep; + } if (!( mode == FL_READY || mode == FL_POINT
Hi, came around this problem while stress testing jffs2. From time to time the block erase failed and the file system overflows. I don't know if there are any Nor chips out there which allow a new erase to start when in erase suspend. However, the chips on my board dont't. And even when it doesn't make much sense to suspend an erase operation for another erase. Patch below fixes the problem for me. Have fun and take care, Peter Signed-off-by: Peter Wippich <pewi@gw-instruments.de> ---