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