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

[Joakim Tjernlund]

Peter Wippich <pewi at 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 at gw-instruments.de> wrote on 03/08/2009 15:07:30:
> > > >
> > > >
> > > > Hi Jocke,
> > > >
> > > > On Mon, 3 Aug 2009, Joakim Tjernlund wrote:
> > > >
> > > > > linux-mtd-bounces at 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 at 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