[Skiboot] [PATCH 3/4] core/flash: Don't hold flash_lock for the entirety of an opal_flash_op()

[Cyril Bur]

On Thu, 2017-06-29 at 12:54 +1000, Alistair Popple wrote:
> On Thu, 29 Jun 2017 09:38:03 AM Cyril Bur wrote:
> > It doesn't make sense to hold the lock to the flash for an entire flash
> > op. The flash_lock provides mutual exclusion to the flashes structure
> > and each flash element has a busy boolean which ensures that mutual
> > exclusion on access of the flash.
> > 
> > Signed-off-by: Cyril Bur <cyril.bur at au1.ibm.com>
> > ---
> >  core/flash.c | 19 ++++++-------------
> >  1 file changed, 6 insertions(+), 13 deletions(-)
> > 
> > diff --git a/core/flash.c b/core/flash.c
> > index a905e986..177f7ae1 100644
> > --- a/core/flash.c
> > +++ b/core/flash.c
> > @@ -335,23 +335,16 @@ static int64_t opal_flash_op(enum flash_op op, uint64_t id, uint64_t offset,
> 
> I can't remember how we dealt with multiple "sides" on a single flash chip - did
> we create a flash struct for each side? Or do we only have one struct flash per
> physical chip? If not would this remove multual exclusion on the underlying
> hardware accesses?

Good question, I've had a quick look see, in platforms/astbmc/pnor.c is
where the pnor gets opened and flash_register() (core/flash.c) is
called, which creates one flash structure for the entire pnor. At the
moment we're safe and it doesn't look like the same physical hardware
can be pointed to by two flashes but definitely something to keep in
mind as this code now relies on that being true.


Cyril

> 
> - Alistair
> 
> >  	struct flash *flash = NULL;
> >  	int rc;
> >  
> > -	if (!try_lock(&flash_lock))
> > -		return OPAL_BUSY;
> > -
> >  	list_for_each(&flashes, flash, list)
> >  		if (flash->id == id)
> >  			break;
> >  
> > -	if (flash->id != id) {
> > +	if (flash->id != id)
> >  		/* Couldn't find the flash */
> > -		rc = OPAL_PARAMETER;
> > -		goto err;
> > -	}
> > +		return OPAL_PARAMETER;
> >  
> > -	if (flash->busy) {
> > -		rc = OPAL_BUSY;
> > -		goto err;
> > -	}
> > +	if (!flash_reserve(flash))
> > +		return OPAL_BUSY;
> >  
> >  	if (size > flash->size || offset >= flash->size
> >  			|| offset + size > flash->size) {
> > @@ -387,13 +380,13 @@ static int64_t opal_flash_op(enum flash_op op, uint64_t id, uint64_t offset,
> >  		goto err;
> >  	}
> >  
> > -	unlock(&flash_lock);
> > +	flash_release(flash);
> >  
> >  	opal_queue_msg(OPAL_MSG_ASYNC_COMP, NULL, NULL, token, rc);
> >  	return OPAL_ASYNC_COMPLETION;
> >  
> >  err:
> > -	unlock(&flash_lock);
> > +	flash_release(flash);
> >  	return rc;
> >  }
> >  
> > 
> 
>