| Message ID | 20170628233804.18412-2-cyril.bur@au1.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, 29 Jun 2017 09:38:02 AM Cyril Bur wrote: > Now that skiboot can have multiple flashes it doesn't make sense for > flash_reserve() and flash_release() to only operate on system_flash. > > New functions system_flash_reserve() and system_flash_release() preserve > the current functionality of flash_reserve() and flash_release() and > flash_reserve() and flash_releasei() can now be used to mark any flash > as busy. > > Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com> > --- > core/flash.c | 20 +++++++++++++++----- > hw/ipmi/ipmi-sel.c | 4 ++-- > include/skiboot.h | 4 ++-- > 3 files changed, 19 insertions(+), 9 deletions(-) > > diff --git a/core/flash.c b/core/flash.c > index f0390394..a905e986 100644 > --- a/core/flash.c > +++ b/core/flash.c > @@ -48,15 +48,15 @@ static struct lock flash_lock; > static struct flash *nvram_flash; > static u32 nvram_offset, nvram_size; > > -bool flash_reserve(void) > +static bool flash_reserve(struct flash *flash) > { > bool rc = false; > > if (!try_lock(&flash_lock)) So this is a global lock protecting all flash structures Skiboot knows about? Why wouldn't me make this more fine grained (ie. per struct)? > return false; > > - if (!system_flash->busy) { > - system_flash->busy = true; > + if (!flash->busy) { > + flash->busy = true; > rc = true; > } > unlock(&flash_lock); > @@ -64,13 +64,23 @@ bool flash_reserve(void) > return rc; > } > > -void flash_release(void) > +static void flash_release(struct flash *flash) > { > lock(&flash_lock); > - system_flash->busy = false; > + flash->busy = false; > unlock(&flash_lock); > } > > +bool system_flash_reserve(void) > +{ > + return flash_reserve(system_flash); > +} > + > +void system_flash_release(void) > +{ > + flash_release(system_flash); > +} > + > static int flash_nvram_info(uint32_t *total_size) > { > int rc; > diff --git a/hw/ipmi/ipmi-sel.c b/hw/ipmi/ipmi-sel.c > index 5c766472..14d10db2 100644 > --- a/hw/ipmi/ipmi-sel.c > +++ b/hw/ipmi/ipmi-sel.c > @@ -475,7 +475,7 @@ static void sel_pnor(uint8_t access) > break; > } > > - granted = flash_reserve(); > + granted = system_flash_reserve(); > if (granted) > occ_pnor_set_owner(PNOR_OWNER_EXTERNAL); > /* Ack the request */ > @@ -484,7 +484,7 @@ static void sel_pnor(uint8_t access) > break; > case RELEASE_PNOR: > prlog(PR_NOTICE, "PNOR access released\n"); > - flash_release(); > + system_flash_release(); > occ_pnor_set_owner(PNOR_OWNER_HOST); > break; > default: > diff --git a/include/skiboot.h b/include/skiboot.h > index 1a153b02..785986cf 100644 > --- a/include/skiboot.h > +++ b/include/skiboot.h > @@ -235,8 +235,8 @@ extern int flash_register(struct blocklevel_device *bl); > extern int flash_start_preload_resource(enum resource_id id, uint32_t subid, > void *buf, size_t *len); > extern int flash_resource_loaded(enum resource_id id, uint32_t idx); > -extern bool flash_reserve(void); > -extern void flash_release(void); > +extern bool system_flash_reserve(void); > +extern void system_flash_release(void); > #define FLASH_SUBPART_ALIGNMENT 0x1000 > #define FLASH_SUBPART_HEADER_SIZE FLASH_SUBPART_ALIGNMENT > extern int flash_subpart_info(void *part_header, uint32_t header_len, >
On Thu, 2017-06-29 at 11:23 +1000, Alistair Popple wrote: > On Thu, 29 Jun 2017 09:38:02 AM Cyril Bur wrote: > > Now that skiboot can have multiple flashes it doesn't make sense for > > flash_reserve() and flash_release() to only operate on system_flash. > > > > New functions system_flash_reserve() and system_flash_release() preserve > > the current functionality of flash_reserve() and flash_release() and > > flash_reserve() and flash_releasei() can now be used to mark any flash > > as busy. > > > > Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com> > > --- > > core/flash.c | 20 +++++++++++++++----- > > hw/ipmi/ipmi-sel.c | 4 ++-- > > include/skiboot.h | 4 ++-- > > 3 files changed, 19 insertions(+), 9 deletions(-) > > > > diff --git a/core/flash.c b/core/flash.c > > index f0390394..a905e986 100644 > > --- a/core/flash.c > > +++ b/core/flash.c > > @@ -48,15 +48,15 @@ static struct lock flash_lock; > > static struct flash *nvram_flash; > > static u32 nvram_offset, nvram_size; > > > > -bool flash_reserve(void) > > +static bool flash_reserve(struct flash *flash) > > { > > bool rc = false; > > > > if (!try_lock(&flash_lock)) > > So this is a global lock protecting all flash structures Skiboot knows about? > Why wouldn't me make this more fine grained (ie. per struct)? We totally could, my yak shaving razor is blunt. > > > return false; > > > > - if (!system_flash->busy) { > > - system_flash->busy = true; > > + if (!flash->busy) { > > + flash->busy = true; > > rc = true; > > } > > unlock(&flash_lock); > > @@ -64,13 +64,23 @@ bool flash_reserve(void) > > return rc; > > } > > > > -void flash_release(void) > > +static void flash_release(struct flash *flash) > > { > > lock(&flash_lock); > > - system_flash->busy = false; > > + flash->busy = false; > > unlock(&flash_lock); > > } > > > > +bool system_flash_reserve(void) > > +{ > > + return flash_reserve(system_flash); > > +} > > + > > +void system_flash_release(void) > > +{ > > + flash_release(system_flash); > > +} > > + > > static int flash_nvram_info(uint32_t *total_size) > > { > > int rc; > > diff --git a/hw/ipmi/ipmi-sel.c b/hw/ipmi/ipmi-sel.c > > index 5c766472..14d10db2 100644 > > --- a/hw/ipmi/ipmi-sel.c > > +++ b/hw/ipmi/ipmi-sel.c > > @@ -475,7 +475,7 @@ static void sel_pnor(uint8_t access) > > break; > > } > > > > - granted = flash_reserve(); > > + granted = system_flash_reserve(); > > if (granted) > > occ_pnor_set_owner(PNOR_OWNER_EXTERNAL); > > /* Ack the request */ > > @@ -484,7 +484,7 @@ static void sel_pnor(uint8_t access) > > break; > > case RELEASE_PNOR: > > prlog(PR_NOTICE, "PNOR access released\n"); > > - flash_release(); > > + system_flash_release(); > > occ_pnor_set_owner(PNOR_OWNER_HOST); > > break; > > default: > > diff --git a/include/skiboot.h b/include/skiboot.h > > index 1a153b02..785986cf 100644 > > --- a/include/skiboot.h > > +++ b/include/skiboot.h > > @@ -235,8 +235,8 @@ extern int flash_register(struct blocklevel_device *bl); > > extern int flash_start_preload_resource(enum resource_id id, uint32_t subid, > > void *buf, size_t *len); > > extern int flash_resource_loaded(enum resource_id id, uint32_t idx); > > -extern bool flash_reserve(void); > > -extern void flash_release(void); > > +extern bool system_flash_reserve(void); > > +extern void system_flash_release(void); > > #define FLASH_SUBPART_ALIGNMENT 0x1000 > > #define FLASH_SUBPART_HEADER_SIZE FLASH_SUBPART_ALIGNMENT > > extern int flash_subpart_info(void *part_header, uint32_t header_len, > > > >
On Thu, 29 Jun 2017 11:28:33 AM Cyril Bur wrote: > On Thu, 2017-06-29 at 11:23 +1000, Alistair Popple wrote: > > On Thu, 29 Jun 2017 09:38:02 AM Cyril Bur wrote: > > > Now that skiboot can have multiple flashes it doesn't make sense for > > > flash_reserve() and flash_release() to only operate on system_flash. > > > > > > New functions system_flash_reserve() and system_flash_release() preserve > > > the current functionality of flash_reserve() and flash_release() and > > > flash_reserve() and flash_releasei() can now be used to mark any flash > > > as busy. > > > > > > Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com> > > > --- > > > core/flash.c | 20 +++++++++++++++----- > > > hw/ipmi/ipmi-sel.c | 4 ++-- > > > include/skiboot.h | 4 ++-- > > > 3 files changed, 19 insertions(+), 9 deletions(-) > > > > > > diff --git a/core/flash.c b/core/flash.c > > > index f0390394..a905e986 100644 > > > --- a/core/flash.c > > > +++ b/core/flash.c > > > @@ -48,15 +48,15 @@ static struct lock flash_lock; > > > static struct flash *nvram_flash; > > > static u32 nvram_offset, nvram_size; > > > > > > -bool flash_reserve(void) > > > +static bool flash_reserve(struct flash *flash) > > > { > > > bool rc = false; > > > > > > if (!try_lock(&flash_lock)) > > > > So this is a global lock protecting all flash structures Skiboot knows about? > > Why wouldn't me make this more fine grained (ie. per struct)? > > We totally could, my yak shaving razor is blunt. Haha. Better get a grind stone out then! I don't think it's a big deal so long as it isn't preventing simultaneous access to different flash chips. Which I assume is what the next patch in the series fixes? > > > > > return false; > > > > > > - if (!system_flash->busy) { > > > - system_flash->busy = true; > > > + if (!flash->busy) { > > > + flash->busy = true; > > > rc = true; > > > } > > > unlock(&flash_lock); > > > @@ -64,13 +64,23 @@ bool flash_reserve(void) > > > return rc; > > > } > > > > > > -void flash_release(void) > > > +static void flash_release(struct flash *flash) > > > { > > > lock(&flash_lock); > > > - system_flash->busy = false; > > > + flash->busy = false; > > > unlock(&flash_lock); > > > } > > > > > > +bool system_flash_reserve(void) > > > +{ > > > + return flash_reserve(system_flash); > > > +} > > > + > > > +void system_flash_release(void) > > > +{ > > > + flash_release(system_flash); > > > +} > > > + > > > static int flash_nvram_info(uint32_t *total_size) > > > { > > > int rc; > > > diff --git a/hw/ipmi/ipmi-sel.c b/hw/ipmi/ipmi-sel.c > > > index 5c766472..14d10db2 100644 > > > --- a/hw/ipmi/ipmi-sel.c > > > +++ b/hw/ipmi/ipmi-sel.c > > > @@ -475,7 +475,7 @@ static void sel_pnor(uint8_t access) > > > break; > > > } > > > > > > - granted = flash_reserve(); > > > + granted = system_flash_reserve(); > > > if (granted) > > > occ_pnor_set_owner(PNOR_OWNER_EXTERNAL); > > > /* Ack the request */ > > > @@ -484,7 +484,7 @@ static void sel_pnor(uint8_t access) > > > break; > > > case RELEASE_PNOR: > > > prlog(PR_NOTICE, "PNOR access released\n"); > > > - flash_release(); > > > + system_flash_release(); > > > occ_pnor_set_owner(PNOR_OWNER_HOST); > > > break; > > > default: > > > diff --git a/include/skiboot.h b/include/skiboot.h > > > index 1a153b02..785986cf 100644 > > > --- a/include/skiboot.h > > > +++ b/include/skiboot.h > > > @@ -235,8 +235,8 @@ extern int flash_register(struct blocklevel_device *bl); > > > extern int flash_start_preload_resource(enum resource_id id, uint32_t subid, > > > void *buf, size_t *len); > > > extern int flash_resource_loaded(enum resource_id id, uint32_t idx); > > > -extern bool flash_reserve(void); > > > -extern void flash_release(void); > > > +extern bool system_flash_reserve(void); > > > +extern void system_flash_release(void); > > > #define FLASH_SUBPART_ALIGNMENT 0x1000 > > > #define FLASH_SUBPART_HEADER_SIZE FLASH_SUBPART_ALIGNMENT > > > extern int flash_subpart_info(void *part_header, uint32_t header_len, > > > > > > > >
On Thu, 2017-06-29 at 11:37 +1000, Alistair Popple wrote: > On Thu, 29 Jun 2017 11:28:33 AM Cyril Bur wrote: > > On Thu, 2017-06-29 at 11:23 +1000, Alistair Popple wrote: > > > On Thu, 29 Jun 2017 09:38:02 AM Cyril Bur wrote: > > > > Now that skiboot can have multiple flashes it doesn't make sense for > > > > flash_reserve() and flash_release() to only operate on system_flash. > > > > > > > > New functions system_flash_reserve() and system_flash_release() preserve > > > > the current functionality of flash_reserve() and flash_release() and > > > > flash_reserve() and flash_releasei() can now be used to mark any flash > > > > as busy. > > > > > > > > Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com> > > > > --- > > > > core/flash.c | 20 +++++++++++++++----- > > > > hw/ipmi/ipmi-sel.c | 4 ++-- > > > > include/skiboot.h | 4 ++-- > > > > 3 files changed, 19 insertions(+), 9 deletions(-) > > > > > > > > diff --git a/core/flash.c b/core/flash.c > > > > index f0390394..a905e986 100644 > > > > --- a/core/flash.c > > > > +++ b/core/flash.c > > > > @@ -48,15 +48,15 @@ static struct lock flash_lock; > > > > static struct flash *nvram_flash; > > > > static u32 nvram_offset, nvram_size; > > > > > > > > -bool flash_reserve(void) > > > > +static bool flash_reserve(struct flash *flash) > > > > { > > > > bool rc = false; > > > > > > > > if (!try_lock(&flash_lock)) > > > > > > So this is a global lock protecting all flash structures Skiboot knows about? > > > Why wouldn't me make this more fine grained (ie. per struct)? > > > > We totally could, my yak shaving razor is blunt. > > Haha. Better get a grind stone out then! I don't think it's a big deal so long > as it isn't preventing simultaneous access to different flash chips. Which I > assume is what the next patch in the series fixes? > The next patch does improve on the current situation yes. > > > > > > > return false; > > > > > > > > - if (!system_flash->busy) { > > > > - system_flash->busy = true; > > > > + if (!flash->busy) { > > > > + flash->busy = true; > > > > rc = true; > > > > } > > > > unlock(&flash_lock); > > > > @@ -64,13 +64,23 @@ bool flash_reserve(void) > > > > return rc; > > > > } > > > > > > > > -void flash_release(void) > > > > +static void flash_release(struct flash *flash) > > > > { > > > > lock(&flash_lock); > > > > - system_flash->busy = false; > > > > + flash->busy = false; > > > > unlock(&flash_lock); > > > > } > > > > > > > > +bool system_flash_reserve(void) > > > > +{ > > > > + return flash_reserve(system_flash); > > > > +} > > > > + > > > > +void system_flash_release(void) > > > > +{ > > > > + flash_release(system_flash); > > > > +} > > > > + > > > > static int flash_nvram_info(uint32_t *total_size) > > > > { > > > > int rc; > > > > diff --git a/hw/ipmi/ipmi-sel.c b/hw/ipmi/ipmi-sel.c > > > > index 5c766472..14d10db2 100644 > > > > --- a/hw/ipmi/ipmi-sel.c > > > > +++ b/hw/ipmi/ipmi-sel.c > > > > @@ -475,7 +475,7 @@ static void sel_pnor(uint8_t access) > > > > break; > > > > } > > > > > > > > - granted = flash_reserve(); > > > > + granted = system_flash_reserve(); > > > > if (granted) > > > > occ_pnor_set_owner(PNOR_OWNER_EXTERNAL); > > > > /* Ack the request */ > > > > @@ -484,7 +484,7 @@ static void sel_pnor(uint8_t access) > > > > break; > > > > case RELEASE_PNOR: > > > > prlog(PR_NOTICE, "PNOR access released\n"); > > > > - flash_release(); > > > > + system_flash_release(); > > > > occ_pnor_set_owner(PNOR_OWNER_HOST); > > > > break; > > > > default: > > > > diff --git a/include/skiboot.h b/include/skiboot.h > > > > index 1a153b02..785986cf 100644 > > > > --- a/include/skiboot.h > > > > +++ b/include/skiboot.h > > > > @@ -235,8 +235,8 @@ extern int flash_register(struct blocklevel_device *bl); > > > > extern int flash_start_preload_resource(enum resource_id id, uint32_t subid, > > > > void *buf, size_t *len); > > > > extern int flash_resource_loaded(enum resource_id id, uint32_t idx); > > > > -extern bool flash_reserve(void); > > > > -extern void flash_release(void); > > > > +extern bool system_flash_reserve(void); > > > > +extern void system_flash_release(void); > > > > #define FLASH_SUBPART_ALIGNMENT 0x1000 > > > > #define FLASH_SUBPART_HEADER_SIZE FLASH_SUBPART_ALIGNMENT > > > > extern int flash_subpart_info(void *part_header, uint32_t header_len, > > > > > > > > > > > >
On Thu, 29 Jun 2017 11:40:23 AM Cyril Bur wrote: > On Thu, 2017-06-29 at 11:37 +1000, Alistair Popple wrote: > > On Thu, 29 Jun 2017 11:28:33 AM Cyril Bur wrote: > > > On Thu, 2017-06-29 at 11:23 +1000, Alistair Popple wrote: > > > > On Thu, 29 Jun 2017 09:38:02 AM Cyril Bur wrote: > > > > > Now that skiboot can have multiple flashes it doesn't make sense for > > > > > flash_reserve() and flash_release() to only operate on system_flash. > > > > > > > > > > New functions system_flash_reserve() and system_flash_release() preserve > > > > > the current functionality of flash_reserve() and flash_release() and > > > > > flash_reserve() and flash_releasei() can now be used to mark any flash I didn't see the flash_releasei() function defined anywhere. > > > > > as busy. > > > > > > > > > > Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com> > > > > > --- > > > > > core/flash.c | 20 +++++++++++++++----- > > > > > hw/ipmi/ipmi-sel.c | 4 ++-- > > > > > include/skiboot.h | 4 ++-- > > > > > 3 files changed, 19 insertions(+), 9 deletions(-) > > > > > > > > > > diff --git a/core/flash.c b/core/flash.c > > > > > index f0390394..a905e986 100644 > > > > > --- a/core/flash.c > > > > > +++ b/core/flash.c > > > > > @@ -48,15 +48,15 @@ static struct lock flash_lock; > > > > > static struct flash *nvram_flash; > > > > > static u32 nvram_offset, nvram_size; > > > > > > > > > > -bool flash_reserve(void) > > > > > +static bool flash_reserve(struct flash *flash) > > > > > { > > > > > bool rc = false; > > > > > > > > > > if (!try_lock(&flash_lock)) > > > > > > > > So this is a global lock protecting all flash structures Skiboot knows about? > > > > Why wouldn't me make this more fine grained (ie. per struct)? > > > > > > We totally could, my yak shaving razor is blunt. > > > > Haha. Better get a grind stone out then! I don't think it's a big deal so long > > as it isn't preventing simultaneous access to different flash chips. Which I > > assume is what the next patch in the series fixes? > > > > The next patch does improve on the current situation yes. > > > > > > > > > > return false; > > > > > > > > > > - if (!system_flash->busy) { > > > > > - system_flash->busy = true; > > > > > + if (!flash->busy) { > > > > > + flash->busy = true; > > > > > rc = true; > > > > > } > > > > > unlock(&flash_lock); > > > > > @@ -64,13 +64,23 @@ bool flash_reserve(void) > > > > > return rc; > > > > > } > > > > > > > > > > -void flash_release(void) > > > > > +static void flash_release(struct flash *flash) Are you sure you didn't mean flash_releasei() here? > > > > > { > > > > > lock(&flash_lock); > > > > > - system_flash->busy = false; > > > > > + flash->busy = false; > > > > > unlock(&flash_lock); > > > > > } > > > > > > > > > > +bool system_flash_reserve(void) > > > > > +{ > > > > > + return flash_reserve(system_flash); > > > > > +} > > > > > + > > > > > +void system_flash_release(void) > > > > > +{ > > > > > + flash_release(system_flash); > > > > > +} > > > > > + > > > > > static int flash_nvram_info(uint32_t *total_size) > > > > > { > > > > > int rc; > > > > > diff --git a/hw/ipmi/ipmi-sel.c b/hw/ipmi/ipmi-sel.c > > > > > index 5c766472..14d10db2 100644 > > > > > --- a/hw/ipmi/ipmi-sel.c > > > > > +++ b/hw/ipmi/ipmi-sel.c > > > > > @@ -475,7 +475,7 @@ static void sel_pnor(uint8_t access) > > > > > break; > > > > > } > > > > > > > > > > - granted = flash_reserve(); > > > > > + granted = system_flash_reserve(); > > > > > if (granted) > > > > > occ_pnor_set_owner(PNOR_OWNER_EXTERNAL); > > > > > /* Ack the request */ > > > > > @@ -484,7 +484,7 @@ static void sel_pnor(uint8_t access) > > > > > break; > > > > > case RELEASE_PNOR: > > > > > prlog(PR_NOTICE, "PNOR access released\n"); > > > > > - flash_release(); > > > > > + system_flash_release(); > > > > > occ_pnor_set_owner(PNOR_OWNER_HOST); > > > > > break; > > > > > default: > > > > > diff --git a/include/skiboot.h b/include/skiboot.h > > > > > index 1a153b02..785986cf 100644 > > > > > --- a/include/skiboot.h > > > > > +++ b/include/skiboot.h > > > > > @@ -235,8 +235,8 @@ extern int flash_register(struct blocklevel_device *bl); > > > > > extern int flash_start_preload_resource(enum resource_id id, uint32_t subid, > > > > > void *buf, size_t *len); > > > > > extern int flash_resource_loaded(enum resource_id id, uint32_t idx); > > > > > -extern bool flash_reserve(void); > > > > > -extern void flash_release(void); > > > > > +extern bool system_flash_reserve(void); > > > > > +extern void system_flash_release(void); > > > > > #define FLASH_SUBPART_ALIGNMENT 0x1000 > > > > > #define FLASH_SUBPART_HEADER_SIZE FLASH_SUBPART_ALIGNMENT > > > > > extern int flash_subpart_info(void *part_header, uint32_t header_len, > > > > > > > > > > > > > > > > > >
On Thu, 2017-06-29 at 11:47 +1000, Alistair Popple wrote: > On Thu, 29 Jun 2017 11:40:23 AM Cyril Bur wrote: > > On Thu, 2017-06-29 at 11:37 +1000, Alistair Popple wrote: > > > On Thu, 29 Jun 2017 11:28:33 AM Cyril Bur wrote: > > > > On Thu, 2017-06-29 at 11:23 +1000, Alistair Popple wrote: > > > > > On Thu, 29 Jun 2017 09:38:02 AM Cyril Bur wrote: > > > > > > Now that skiboot can have multiple flashes it doesn't make sense for > > > > > > flash_reserve() and flash_release() to only operate on system_flash. > > > > > > > > > > > > New functions system_flash_reserve() and system_flash_release() preserve > > > > > > the current functionality of flash_reserve() and flash_release() and > > > > > > flash_reserve() and flash_releasei() can now be used to mark any flash > > I didn't see the flash_releasei() function defined anywhere. > Damn you win :( > > > > > > as busy. > > > > > > > > > > > > Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com> > > > > > > --- > > > > > > core/flash.c | 20 +++++++++++++++----- > > > > > > hw/ipmi/ipmi-sel.c | 4 ++-- > > > > > > include/skiboot.h | 4 ++-- > > > > > > 3 files changed, 19 insertions(+), 9 deletions(-) > > > > > > > > > > > > diff --git a/core/flash.c b/core/flash.c > > > > > > index f0390394..a905e986 100644 > > > > > > --- a/core/flash.c > > > > > > +++ b/core/flash.c > > > > > > @@ -48,15 +48,15 @@ static struct lock flash_lock; > > > > > > static struct flash *nvram_flash; > > > > > > static u32 nvram_offset, nvram_size; > > > > > > > > > > > > -bool flash_reserve(void) > > > > > > +static bool flash_reserve(struct flash *flash) > > > > > > { > > > > > > bool rc = false; > > > > > > > > > > > > if (!try_lock(&flash_lock)) > > > > > > > > > > So this is a global lock protecting all flash structures Skiboot knows about? > > > > > Why wouldn't me make this more fine grained (ie. per struct)? > > > > > > > > We totally could, my yak shaving razor is blunt. > > > > > > Haha. Better get a grind stone out then! I don't think it's a big deal so long > > > as it isn't preventing simultaneous access to different flash chips. Which I > > > assume is what the next patch in the series fixes? > > > > > > > The next patch does improve on the current situation yes. > > > > > > > > > > > > > return false; > > > > > > > > > > > > - if (!system_flash->busy) { > > > > > > - system_flash->busy = true; > > > > > > + if (!flash->busy) { > > > > > > + flash->busy = true; > > > > > > rc = true; > > > > > > } > > > > > > unlock(&flash_lock); > > > > > > @@ -64,13 +64,23 @@ bool flash_reserve(void) > > > > > > return rc; > > > > > > } > > > > > > > > > > > > -void flash_release(void) > > > > > > +static void flash_release(struct flash *flash) > > Are you sure you didn't mean flash_releasei() here? > Pretty sure, but since I didn't declare it... did I? who am I? > > > > > > { > > > > > > lock(&flash_lock); > > > > > > - system_flash->busy = false; > > > > > > + flash->busy = false; > > > > > > unlock(&flash_lock); > > > > > > } > > > > > > > > > > > > +bool system_flash_reserve(void) > > > > > > +{ > > > > > > + return flash_reserve(system_flash); > > > > > > +} > > > > > > + > > > > > > +void system_flash_release(void) > > > > > > +{ > > > > > > + flash_release(system_flash); > > > > > > +} > > > > > > + > > > > > > static int flash_nvram_info(uint32_t *total_size) > > > > > > { > > > > > > int rc; > > > > > > diff --git a/hw/ipmi/ipmi-sel.c b/hw/ipmi/ipmi-sel.c > > > > > > index 5c766472..14d10db2 100644 > > > > > > --- a/hw/ipmi/ipmi-sel.c > > > > > > +++ b/hw/ipmi/ipmi-sel.c > > > > > > @@ -475,7 +475,7 @@ static void sel_pnor(uint8_t access) > > > > > > break; > > > > > > } > > > > > > > > > > > > - granted = flash_reserve(); > > > > > > + granted = system_flash_reserve(); > > > > > > if (granted) > > > > > > occ_pnor_set_owner(PNOR_OWNER_EXTERNAL); > > > > > > /* Ack the request */ > > > > > > @@ -484,7 +484,7 @@ static void sel_pnor(uint8_t access) > > > > > > break; > > > > > > case RELEASE_PNOR: > > > > > > prlog(PR_NOTICE, "PNOR access released\n"); > > > > > > - flash_release(); > > > > > > + system_flash_release(); > > > > > > occ_pnor_set_owner(PNOR_OWNER_HOST); > > > > > > break; > > > > > > default: > > > > > > diff --git a/include/skiboot.h b/include/skiboot.h > > > > > > index 1a153b02..785986cf 100644 > > > > > > --- a/include/skiboot.h > > > > > > +++ b/include/skiboot.h > > > > > > @@ -235,8 +235,8 @@ extern int flash_register(struct blocklevel_device *bl); > > > > > > extern int flash_start_preload_resource(enum resource_id id, uint32_t subid, > > > > > > void *buf, size_t *len); > > > > > > extern int flash_resource_loaded(enum resource_id id, uint32_t idx); > > > > > > -extern bool flash_reserve(void); > > > > > > -extern void flash_release(void); > > > > > > +extern bool system_flash_reserve(void); > > > > > > +extern void system_flash_release(void); > > > > > > #define FLASH_SUBPART_ALIGNMENT 0x1000 > > > > > > #define FLASH_SUBPART_HEADER_SIZE FLASH_SUBPART_ALIGNMENT > > > > > > extern int flash_subpart_info(void *part_header, uint32_t header_len, > > > > > > > > > > > > > > > > > > > > > > > >
On Thu, 29 Jun 2017 11:51:02 AM Cyril Bur wrote: > On Thu, 2017-06-29 at 11:47 +1000, Alistair Popple wrote: > > On Thu, 29 Jun 2017 11:40:23 AM Cyril Bur wrote: > > > On Thu, 2017-06-29 at 11:37 +1000, Alistair Popple wrote: > > > > On Thu, 29 Jun 2017 11:28:33 AM Cyril Bur wrote: > > > > > On Thu, 2017-06-29 at 11:23 +1000, Alistair Popple wrote: > > > > > > On Thu, 29 Jun 2017 09:38:02 AM Cyril Bur wrote: > > > > > > > Now that skiboot can have multiple flashes it doesn't make sense for > > > > > > > flash_reserve() and flash_release() to only operate on system_flash. > > > > > > > > > > > > > > New functions system_flash_reserve() and system_flash_release() preserve > > > > > > > the current functionality of flash_reserve() and flash_release() and > > > > > > > flash_reserve() and flash_releasei() can now be used to mark any flash > > > > I didn't see the flash_releasei() function defined anywhere. > > > > Damn you win :( I'm pretty sure you can bribe maintainers to fix commit message typos up for you :-) Reviewed-by: Alistair Popple <alistair@popple.id.au> > > > > > > > as busy. > > > > > > > > > > > > > > Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com> > > > > > > > --- > > > > > > > core/flash.c | 20 +++++++++++++++----- > > > > > > > hw/ipmi/ipmi-sel.c | 4 ++-- > > > > > > > include/skiboot.h | 4 ++-- > > > > > > > 3 files changed, 19 insertions(+), 9 deletions(-) > > > > > > > > > > > > > > diff --git a/core/flash.c b/core/flash.c > > > > > > > index f0390394..a905e986 100644 > > > > > > > --- a/core/flash.c > > > > > > > +++ b/core/flash.c > > > > > > > @@ -48,15 +48,15 @@ static struct lock flash_lock; > > > > > > > static struct flash *nvram_flash; > > > > > > > static u32 nvram_offset, nvram_size; > > > > > > > > > > > > > > -bool flash_reserve(void) > > > > > > > +static bool flash_reserve(struct flash *flash) > > > > > > > { > > > > > > > bool rc = false; > > > > > > > > > > > > > > if (!try_lock(&flash_lock)) > > > > > > > > > > > > So this is a global lock protecting all flash structures Skiboot knows about? > > > > > > Why wouldn't me make this more fine grained (ie. per struct)? > > > > > > > > > > We totally could, my yak shaving razor is blunt. > > > > > > > > Haha. Better get a grind stone out then! I don't think it's a big deal so long > > > > as it isn't preventing simultaneous access to different flash chips. Which I > > > > assume is what the next patch in the series fixes? > > > > > > > > > > The next patch does improve on the current situation yes. > > > > > > > > > > > > > > > > return false; > > > > > > > > > > > > > > - if (!system_flash->busy) { > > > > > > > - system_flash->busy = true; > > > > > > > + if (!flash->busy) { > > > > > > > + flash->busy = true; > > > > > > > rc = true; > > > > > > > } > > > > > > > unlock(&flash_lock); > > > > > > > @@ -64,13 +64,23 @@ bool flash_reserve(void) > > > > > > > return rc; > > > > > > > } > > > > > > > > > > > > > > -void flash_release(void) > > > > > > > +static void flash_release(struct flash *flash) > > > > Are you sure you didn't mean flash_releasei() here? > > > > Pretty sure, but since I didn't declare it... did I? who am I? > > > > > > > > { > > > > > > > lock(&flash_lock); > > > > > > > - system_flash->busy = false; > > > > > > > + flash->busy = false; > > > > > > > unlock(&flash_lock); > > > > > > > } > > > > > > > > > > > > > > +bool system_flash_reserve(void) > > > > > > > +{ > > > > > > > + return flash_reserve(system_flash); > > > > > > > +} > > > > > > > + > > > > > > > +void system_flash_release(void) > > > > > > > +{ > > > > > > > + flash_release(system_flash); > > > > > > > +} > > > > > > > + > > > > > > > static int flash_nvram_info(uint32_t *total_size) > > > > > > > { > > > > > > > int rc; > > > > > > > diff --git a/hw/ipmi/ipmi-sel.c b/hw/ipmi/ipmi-sel.c > > > > > > > index 5c766472..14d10db2 100644 > > > > > > > --- a/hw/ipmi/ipmi-sel.c > > > > > > > +++ b/hw/ipmi/ipmi-sel.c > > > > > > > @@ -475,7 +475,7 @@ static void sel_pnor(uint8_t access) > > > > > > > break; > > > > > > > } > > > > > > > > > > > > > > - granted = flash_reserve(); > > > > > > > + granted = system_flash_reserve(); > > > > > > > if (granted) > > > > > > > occ_pnor_set_owner(PNOR_OWNER_EXTERNAL); > > > > > > > /* Ack the request */ > > > > > > > @@ -484,7 +484,7 @@ static void sel_pnor(uint8_t access) > > > > > > > break; > > > > > > > case RELEASE_PNOR: > > > > > > > prlog(PR_NOTICE, "PNOR access released\n"); > > > > > > > - flash_release(); > > > > > > > + system_flash_release(); > > > > > > > occ_pnor_set_owner(PNOR_OWNER_HOST); > > > > > > > break; > > > > > > > default: > > > > > > > diff --git a/include/skiboot.h b/include/skiboot.h > > > > > > > index 1a153b02..785986cf 100644 > > > > > > > --- a/include/skiboot.h > > > > > > > +++ b/include/skiboot.h > > > > > > > @@ -235,8 +235,8 @@ extern int flash_register(struct blocklevel_device *bl); > > > > > > > extern int flash_start_preload_resource(enum resource_id id, uint32_t subid, > > > > > > > void *buf, size_t *len); > > > > > > > extern int flash_resource_loaded(enum resource_id id, uint32_t idx); > > > > > > > -extern bool flash_reserve(void); > > > > > > > -extern void flash_release(void); > > > > > > > +extern bool system_flash_reserve(void); > > > > > > > +extern void system_flash_release(void); > > > > > > > #define FLASH_SUBPART_ALIGNMENT 0x1000 > > > > > > > #define FLASH_SUBPART_HEADER_SIZE FLASH_SUBPART_ALIGNMENT > > > > > > > extern int flash_subpart_info(void *part_header, uint32_t header_len, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
On Thu, 2017-06-29 at 11:53 +1000, Alistair Popple wrote: > On Thu, 29 Jun 2017 11:51:02 AM Cyril Bur wrote: > > On Thu, 2017-06-29 at 11:47 +1000, Alistair Popple wrote: > > > On Thu, 29 Jun 2017 11:40:23 AM Cyril Bur wrote: > > > > On Thu, 2017-06-29 at 11:37 +1000, Alistair Popple wrote: > > > > > On Thu, 29 Jun 2017 11:28:33 AM Cyril Bur wrote: > > > > > > On Thu, 2017-06-29 at 11:23 +1000, Alistair Popple wrote: > > > > > > > On Thu, 29 Jun 2017 09:38:02 AM Cyril Bur wrote: > > > > > > > > Now that skiboot can have multiple flashes it doesn't make sense for > > > > > > > > flash_reserve() and flash_release() to only operate on system_flash. > > > > > > > > > > > > > > > > New functions system_flash_reserve() and system_flash_release() preserve > > > > > > > > the current functionality of flash_reserve() and flash_release() and > > > > > > > > flash_reserve() and flash_releasei() can now be used to mark any flash > > > > > > I didn't see the flash_releasei() function defined anywhere. > > > > > > > Damn you win :( > > I'm pretty sure you can bribe maintainers to fix commit message typos up for you > :-) Beersi on me Stewart :) > > Reviewed-by: Alistair Popple <alistair@popple.id.au> > Thanks. > > > > > > > > as busy. > > > > > > > > > > > > > > > > Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com> > > > > > > > > --- > > > > > > > > core/flash.c | 20 +++++++++++++++----- > > > > > > > > hw/ipmi/ipmi-sel.c | 4 ++-- > > > > > > > > include/skiboot.h | 4 ++-- > > > > > > > > 3 files changed, 19 insertions(+), 9 deletions(-) > > > > > > > > > > > > > > > > diff --git a/core/flash.c b/core/flash.c > > > > > > > > index f0390394..a905e986 100644 > > > > > > > > --- a/core/flash.c > > > > > > > > +++ b/core/flash.c > > > > > > > > @@ -48,15 +48,15 @@ static struct lock flash_lock; > > > > > > > > static struct flash *nvram_flash; > > > > > > > > static u32 nvram_offset, nvram_size; > > > > > > > > > > > > > > > > -bool flash_reserve(void) > > > > > > > > +static bool flash_reserve(struct flash *flash) > > > > > > > > { > > > > > > > > bool rc = false; > > > > > > > > > > > > > > > > if (!try_lock(&flash_lock)) > > > > > > > > > > > > > > So this is a global lock protecting all flash structures Skiboot knows about? > > > > > > > Why wouldn't me make this more fine grained (ie. per struct)? > > > > > > > > > > > > We totally could, my yak shaving razor is blunt. > > > > > > > > > > Haha. Better get a grind stone out then! I don't think it's a big deal so long > > > > > as it isn't preventing simultaneous access to different flash chips. Which I > > > > > assume is what the next patch in the series fixes? > > > > > > > > > > > > > The next patch does improve on the current situation yes. > > > > > > > > > > > > > > > > > > > return false; > > > > > > > > > > > > > > > > - if (!system_flash->busy) { > > > > > > > > - system_flash->busy = true; > > > > > > > > + if (!flash->busy) { > > > > > > > > + flash->busy = true; > > > > > > > > rc = true; > > > > > > > > } > > > > > > > > unlock(&flash_lock); > > > > > > > > @@ -64,13 +64,23 @@ bool flash_reserve(void) > > > > > > > > return rc; > > > > > > > > } > > > > > > > > > > > > > > > > -void flash_release(void) > > > > > > > > +static void flash_release(struct flash *flash) > > > > > > Are you sure you didn't mean flash_releasei() here? > > > > > > > Pretty sure, but since I didn't declare it... did I? who am I? > > > > > > > > > > { > > > > > > > > lock(&flash_lock); > > > > > > > > - system_flash->busy = false; > > > > > > > > + flash->busy = false; > > > > > > > > unlock(&flash_lock); > > > > > > > > } > > > > > > > > > > > > > > > > +bool system_flash_reserve(void) > > > > > > > > +{ > > > > > > > > + return flash_reserve(system_flash); > > > > > > > > +} > > > > > > > > + > > > > > > > > +void system_flash_release(void) > > > > > > > > +{ > > > > > > > > + flash_release(system_flash); > > > > > > > > +} > > > > > > > > + > > > > > > > > static int flash_nvram_info(uint32_t *total_size) > > > > > > > > { > > > > > > > > int rc; > > > > > > > > diff --git a/hw/ipmi/ipmi-sel.c b/hw/ipmi/ipmi-sel.c > > > > > > > > index 5c766472..14d10db2 100644 > > > > > > > > --- a/hw/ipmi/ipmi-sel.c > > > > > > > > +++ b/hw/ipmi/ipmi-sel.c > > > > > > > > @@ -475,7 +475,7 @@ static void sel_pnor(uint8_t access) > > > > > > > > break; > > > > > > > > } > > > > > > > > > > > > > > > > - granted = flash_reserve(); > > > > > > > > + granted = system_flash_reserve(); > > > > > > > > if (granted) > > > > > > > > occ_pnor_set_owner(PNOR_OWNER_EXTERNAL); > > > > > > > > /* Ack the request */ > > > > > > > > @@ -484,7 +484,7 @@ static void sel_pnor(uint8_t access) > > > > > > > > break; > > > > > > > > case RELEASE_PNOR: > > > > > > > > prlog(PR_NOTICE, "PNOR access released\n"); > > > > > > > > - flash_release(); > > > > > > > > + system_flash_release(); > > > > > > > > occ_pnor_set_owner(PNOR_OWNER_HOST); > > > > > > > > break; > > > > > > > > default: > > > > > > > > diff --git a/include/skiboot.h b/include/skiboot.h > > > > > > > > index 1a153b02..785986cf 100644 > > > > > > > > --- a/include/skiboot.h > > > > > > > > +++ b/include/skiboot.h > > > > > > > > @@ -235,8 +235,8 @@ extern int flash_register(struct blocklevel_device *bl); > > > > > > > > extern int flash_start_preload_resource(enum resource_id id, uint32_t subid, > > > > > > > > void *buf, size_t *len); > > > > > > > > extern int flash_resource_loaded(enum resource_id id, uint32_t idx); > > > > > > > > -extern bool flash_reserve(void); > > > > > > > > -extern void flash_release(void); > > > > > > > > +extern bool system_flash_reserve(void); > > > > > > > > +extern void system_flash_release(void); > > > > > > > > #define FLASH_SUBPART_ALIGNMENT 0x1000 > > > > > > > > #define FLASH_SUBPART_HEADER_SIZE FLASH_SUBPART_ALIGNMENT > > > > > > > > extern int flash_subpart_info(void *part_header, uint32_t header_len, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
diff --git a/core/flash.c b/core/flash.c index f0390394..a905e986 100644 --- a/core/flash.c +++ b/core/flash.c @@ -48,15 +48,15 @@ static struct lock flash_lock; static struct flash *nvram_flash; static u32 nvram_offset, nvram_size; -bool flash_reserve(void) +static bool flash_reserve(struct flash *flash) { bool rc = false; if (!try_lock(&flash_lock)) return false; - if (!system_flash->busy) { - system_flash->busy = true; + if (!flash->busy) { + flash->busy = true; rc = true; } unlock(&flash_lock); @@ -64,13 +64,23 @@ bool flash_reserve(void) return rc; } -void flash_release(void) +static void flash_release(struct flash *flash) { lock(&flash_lock); - system_flash->busy = false; + flash->busy = false; unlock(&flash_lock); } +bool system_flash_reserve(void) +{ + return flash_reserve(system_flash); +} + +void system_flash_release(void) +{ + flash_release(system_flash); +} + static int flash_nvram_info(uint32_t *total_size) { int rc; diff --git a/hw/ipmi/ipmi-sel.c b/hw/ipmi/ipmi-sel.c index 5c766472..14d10db2 100644 --- a/hw/ipmi/ipmi-sel.c +++ b/hw/ipmi/ipmi-sel.c @@ -475,7 +475,7 @@ static void sel_pnor(uint8_t access) break; } - granted = flash_reserve(); + granted = system_flash_reserve(); if (granted) occ_pnor_set_owner(PNOR_OWNER_EXTERNAL); /* Ack the request */ @@ -484,7 +484,7 @@ static void sel_pnor(uint8_t access) break; case RELEASE_PNOR: prlog(PR_NOTICE, "PNOR access released\n"); - flash_release(); + system_flash_release(); occ_pnor_set_owner(PNOR_OWNER_HOST); break; default: diff --git a/include/skiboot.h b/include/skiboot.h index 1a153b02..785986cf 100644 --- a/include/skiboot.h +++ b/include/skiboot.h @@ -235,8 +235,8 @@ extern int flash_register(struct blocklevel_device *bl); extern int flash_start_preload_resource(enum resource_id id, uint32_t subid, void *buf, size_t *len); extern int flash_resource_loaded(enum resource_id id, uint32_t idx); -extern bool flash_reserve(void); -extern void flash_release(void); +extern bool system_flash_reserve(void); +extern void system_flash_release(void); #define FLASH_SUBPART_ALIGNMENT 0x1000 #define FLASH_SUBPART_HEADER_SIZE FLASH_SUBPART_ALIGNMENT extern int flash_subpart_info(void *part_header, uint32_t header_len,
Now that skiboot can have multiple flashes it doesn't make sense for flash_reserve() and flash_release() to only operate on system_flash. New functions system_flash_reserve() and system_flash_release() preserve the current functionality of flash_reserve() and flash_release() and flash_reserve() and flash_releasei() can now be used to mark any flash as busy. Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com> --- core/flash.c | 20 +++++++++++++++----- hw/ipmi/ipmi-sel.c | 4 ++-- include/skiboot.h | 4 ++-- 3 files changed, 19 insertions(+), 9 deletions(-)