[Skiboot] [PATCH 4/5] libflash/mbox-flash: Implement MARK_WRITE_ERASED mbox call

[Cyril Bur]

On Tue, 2017-05-02 at 14:28 +1000, Suraj Jitindar Singh wrote:
> On Mon, 2017-04-24 at 19:14 +1000, Cyril Bur wrote:
> > Version two of the mbox-flash protocol defines a new command:
> > MARK_WRITE_ERASED.
> > 
> > This command provides a simple way to mark a region of flash as all
> > 0xff
> > without the need to go and write all 0xff. This is an optimisation as
> > there is no need for an erase before a write, it is the
> > responsibility of
> > the BMC to deal with the flash correctly, however in v1 it was
> > ambiguous
> > what a client should do if the flash should be erased but not
> > actually
> > written to. This allows of a optimal path to resolve this problem.
> 
> nak-do-not-apply-by: Suraj Jitindar Singh <sjitindarsingh at gmail.com>
> 
> Yeah so this is wrong in the same way your dirty function was before
> you fixed it.
> I would recommend doing what I recommended in the previous patch which
> is reuse the dirty function to avoid bugs like this being duplicated in
> the future...
> 

Yeah I'll combine the two or rather, pull out the interesting bits.
Looks like patch 5/5 will need to grow!

Thanks.

> > 
> > Signed-off-by: Cyril Bur <cyril.bur at au1.ibm.com>
> > ---
> >  libflash/mbox-flash.c | 64
> > ++++++++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 61 insertions(+), 3 deletions(-)
> > 
> > diff --git a/libflash/mbox-flash.c b/libflash/mbox-flash.c
> > index 3dfcebe9..5d29215c 100644
> > --- a/libflash/mbox-flash.c
> > +++ b/libflash/mbox-flash.c
> > @@ -761,7 +761,64 @@ out:
> >  	return rc;
> >  }
> >  
> > -static int mbox_flash_erase(struct blocklevel_device *bl __unused,
> > +static int mbox_flash_erase_v2(struct blocklevel_device *bl,
> > uint64_t pos,
> > +		uint64_t len)
> > +{
> > +	uint64_t size;
> > +	struct bmc_mbox_msg *msg;
> > +	struct mbox_flash_data *mbox_flash;
> > +
> > +	mbox_flash = container_of(bl, struct mbox_flash_data, bl);
> > +
> > +	prlog(PR_TRACE, "Flash erase at 0x%08x for 0x%08x\n", (u32)
> > pos, (u32) len);
> > +	while (len > 0) {
> > +		int rc;
> > +
> > +		/* Move window and get a new size to erase */
> > +		rc = mbox_window_move(mbox_flash, &mbox_flash-
> > > write,
> > 
> > +				      MBOX_C_CREATE_WRITE_WINDOW,
> > pos, len, &size);
> > +		if (rc)
> > +			return rc;
> > +
> > +		msg = msg_alloc(mbox_flash,
> > MBOX_C_MARK_WRITE_ERASED);
> > +		if (!msg)
> > +			return FLASH_ERR_MALLOC_FAILED;
> > +
> > +		msg_put_u16(msg, 0, pos >> mbox_flash->shift);
> 
> Are you sure? Isn't pos here a flash index and we want a window index
> for the argument...

Correct, this should be as per the dirty logic

> 
> > +		msg_put_u16(msg, 2, len >> mbox_flash->shift);
> 
> Are you sure? What if len is less than block_size... (Also shouldn't we
> be using size anyway)
> 

Yeah I think this patch got neglected ...

> > +		rc = msg_send(mbox_flash, msg);
> > +		if (rc) {
> > +			prlog(PR_ERR, "Failed to enqueue/send BMC
> > MBOX message\n");
> > +			msg_free_memory(msg);
> > +			return rc;
> > +		}
> > +
> > +		rc = wait_for_bmc(mbox_flash, MBOX_DEFAULT_TIMEOUT);
> > +		if (rc) {
> > +			prlog(PR_ERR, "Error waiting for BMC\n");
> > +			msg_free_memory(msg);
> > +			return rc;
> > +		}
> > +
> > +		msg_free_memory(msg);
> > +
> > +		/*
> > +		 * Flush directly, don't mark that region dirty
> > otherwise it
> > +		 * isn't clear if a write happened there or not
> > +		 */
> > +
> > +		rc = mbox_flash_flush(mbox_flash);
> 
> Same as previous patch, can we just flush after we exit the while loop?
> 

Not as per my understanding of the spec.

> > +		if (rc)
> > +			return rc;
> > +
> > +		len -= size;
> > +		pos += size;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int mbox_flash_erase_v1(struct blocklevel_device *bl
> > __unused,
> >  		uint64_t pos __unused, uint64_t len __unused)
> >  {
> >         /*
> > @@ -859,7 +916,7 @@ static int protocol_init(struct mbox_flash_data
> > *mbox_flash)
> >  	/* Assume V2 */
> >  	mbox_flash->bl.read = &mbox_flash_read;
> >  	mbox_flash->bl.write = &mbox_flash_write;
> > -	mbox_flash->bl.erase = &mbox_flash_erase;
> > +	mbox_flash->bl.erase = &mbox_flash_erase_v2;
> >  	mbox_flash->bl.get_info = &mbox_flash_get_info;
> >  
> >  	/* Assume V2 */
> > @@ -913,6 +970,7 @@ static int protocol_init(struct mbox_flash_data
> > *mbox_flash)
> >  
> >  	prlog(PR_INFO, "Detected mbox protocol version %d\n",
> > mbox_flash->version);
> >  	if (mbox_flash->version == 1) {
> > +		mbox_flash->bl.erase = &mbox_flash_erase_v1;
> >  		/* Not all handlers differ, update those which do */
> >  		mbox_flash->handlers[MBOX_C_GET_MBOX_INFO] =
> > &mbox_flash_do_get_mbox_info;
> >  		mbox_flash->handlers[MBOX_C_GET_FLASH_INFO] =
> > &mbox_flash_do_get_flash_info_v1;
> > @@ -956,7 +1014,7 @@ int mbox_flash_init(struct blocklevel_device
> > **bl)
> >  	/* Assume V2 */
> >  	mbox_flash->bl.read = &mbox_flash_read;
> >  	mbox_flash->bl.write = &mbox_flash_write;
> > -	mbox_flash->bl.erase = &mbox_flash_erase;
> > +	mbox_flash->bl.erase = &mbox_flash_erase_v2;
> >  	mbox_flash->bl.get_info = &mbox_flash_get_info;
> >  
> >  	if (bmc_mbox_get_attn_reg() & MBOX_ATTN_BMC_REBOOT)
> 
>