Re: [PATCH 7/8] mtd: spi-nor: factor out write_enable() for erase commands

[Brian Norris <computersforpeace@gmail.com>]

On Wed, Sep 10, 2014 at 11:20:21PM +0800, Huang Shijie wrote:
> On Wed, Sep 10, 2014 at 12:05:37AM -0700, Brian Norris wrote:
> > On Tue, Aug 12, 2014 at 08:59:12AM +0800, Huang Shijie wrote:
> > >    For the spi_nor_erase(), the patch should like this:
> > > 
> > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> > > index c130bf7..26c48bc 100644
> > > --- a/drivers/mtd/spi-nor/spi-nor.c
> > > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > > @@ -324,6 +324,7 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
> > >  
> > >  	/* whole-chip erase? */
> > >  	if (len == mtd->size) {
> > > +		write_enable(nor);
> > >  		if (erase_chip(nor)) {
> > >  			ret = -EIO;
> > >  			goto erase_err;
> > > @@ -337,6 +338,7 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
> > >  	/* "sector"-at-a-time erase */
> > >  	} else {
> > >  		while (len) {
> > > +			write_enable(nor);
> The difference is here.

OK.

> you miss a write_enable for each sector's erase. 

But is that necessary? I thought 'write-enabled' was retained across
operations, so why would you have to perform it before each sector's
erase?

Or do you have a flash datasheet which says you must send WREN before
each sector erase?

I do see, now that I'm looking a little closer, that spi-nor doesn't
actually have any concurrency protection (!). So it looks like we could
potentially have other operations interleaved in this sequence of sector
erasures, potentially running a write_disable() in the midst of this loop.

I really hope I'm wrong about that last paragraph.

If I'm correct though, the solution to this is not, AIUI, to add more
write_enable() calls in this loop; the solution is to add some kind of
concurrency protections, a la nand_get_device().

> > >  			if (nor->erase(nor, addr)) {
> > >  				ret = -EIO;
> > >  				goto erase_err;
> > > 
> > 
> > How is your patch any different than mine? Mine has the exact same code,
> > except it covers both paths by putting the write_enable() outside the
> > conditional entirely.

Brian