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

[Alistair Popple]

On Thu, 29 Jun 2017 01:41:19 PM Cyril Bur wrote:
> 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.

Good, in that case:

Reviewed-By: Alistair Popple <alistair at popple.id.au>

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