[Patch 1/1] don't suspend erase for erase in cfi_cmdset_0002

[Patch 1/1] don't suspend erase for erase in cfi_cmdset_0002

[Peter Wippich]

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

Re: [Patch 1/1] don't suspend erase for erase in cfi_cmdset_0002

[Joakim Tjernlund]

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

Re: [Patch 1/1] don't suspend erase for erase in cfi_cmdset_0002

[Joakim Tjernlund]

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

Re: [Patch 1/1] don't suspend erase for erase in cfi_cmdset_0002

[Peter Wippich]

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                                       |

Re: [Patch 1/1] don't suspend erase for erase in cfi_cmdset_0002

[Joakim Tjernlund]

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

Re: [Patch 1/1] don't suspend erase for erase in cfi_cmdset_0002

[Joakim Tjernlund]

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

Re: [Patch 1/1] don't suspend erase for erase in cfi_cmdset_0002

[Joakim Tjernlund]

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.

Re: [Patch 1/1] don't suspend erase for erase in cfi_cmdset_0002

[Peter Wippich]

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                                       |

Re: [Patch 1/1] don't suspend erase for erase in cfi_cmdset_0002

[Joakim Tjernlund]

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

Re: [Patch 1/1] don't suspend erase for erase in cfi_cmdset_0002

[Joakim Tjernlund]

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

Re: [Patch 1/1] don't suspend erase for erase in cfi_cmdset_0002

[Peter Wippich]

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                                       |

Re: [Patch 1/1] don't suspend erase for erase in cfi_cmdset_0002

[Joakim Tjernlund]

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

Re: [Patch 1/1] don't suspend erase for erase in cfi_cmdset_0002

[Peter Wippich]

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                                       |

Re: [Patch 1/1] don't suspend erase for erase in cfi_cmdset_0002

[Joakim Tjernlund]

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.

Re: [Patch 1/1] don't suspend erase for erase in cfi_cmdset_0002

[Peter Wippich]

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 

Re: [Patch 1/1] don't suspend erase for erase in cfi_cmdset_0002

[Joakim Tjernlund]

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

Re: [Patch 1/1] don't suspend erase for erase in cfi_cmdset_0002

[Peter Wippich]

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