[Skiboot] [PATCH 2/4] core/flash: Make flash_reserve() and flash_release() use any flash

[Alistair Popple]

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 at popple.id.au>

> > > > > > > as busy.
> > > > > > > 
> > > > > > > Signed-off-by: Cyril Bur <cyril.bur at 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,
> > > > > > > 
> > > > > > 
> > > > > > 
> > > > 
> > > > 
> > 
> > 
>