| Message ID | 20170628233804.18412-3-cyril.bur@au1.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
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; > } > >
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; > > } > > > > > >
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; > > > } > > > > > > > > > > >
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; }
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(-)