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

Message ID 20170628233804.18412-3-cyril.bur@au1.ibm.com
State New
Headers show

Commit Message

Cyril Bur June 28, 2017, 11:38 p.m.
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@au1.ibm.com>
---
 core/flash.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

Comments

Alistair Popple June 29, 2017, 2:54 a.m. | #1
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@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?

- 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;
>  }
>  
>
Cyril Bur June 29, 2017, 3:41 a.m. | #2
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@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;
> >  }
> >  
> > 
> 
>
Alistair Popple June 29, 2017, 3:55 a.m. | #3
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@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@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;
> > >  }
> > >  
> > > 
> > 
> > 
>

Patch

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