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

[Suraj Jitindar Singh]

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...

> 
> 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...

> +		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)

> +		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?

> +		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)