[U-Boot] [PATCH] dfu: add write error handling

[Lukasz Majewski]

Hi Stephen,

> From: Stephen Warren <swarren at nvidia.com>
> 
> Fix calls to dfu_write() and dfu_flush() to detect errors in the I/O
> itself. This could happen due to problems with the storage medium, or
> simply when trying to write a FAT/ext file that is larger than the
> buffer dfu_mmc.c maintains for this purpose.
> 
> Signal the error by switching the DFU state/status. This will be
> picked up by the DFU client when it sends the next DFU request. Note
> that errors can't simply be returned from e.g.
> dnload_request_complete(), since that function has no way to pass
> errors back to the DFU client; a call to dnload_request_complete()
> simply means that a USB OUT completed.
> 
> This error state/status needs to be cleared when the next DFU client
> connects. While there is a DFU_CLRSTATUS request, no DFU client seems
> to send this. Hence, clear this when selecting the USB alternate
> setting on the USB interface.
> 
> Finally, dfu.c relies on a call to dfu_flush() to clear up the
> internal state of the write transaction. Now that errors in
> dfu_write() are detected, dfu_flush() may no longer be called for
> every transaction. Separate out the cleanup code into a new function,
> and call it whenever dfu_write() fails, as well as from any call to
> dfu_flush().
> 
> Signed-off-by: Stephen Warren <swarren at nvidia.com>
> ---
> This probably depends on "dfu: fix some issues with reads/uploads",
> for context if nothing else.

Stephen, thanks for the patch.

Acked-by: Lukasz Majewski <l.majewski at samsung.com>

Test HW: Exynos4412 - Trats2

Tested-by: Lukasz Majewski <l.majewski at samsung.com>

> ---
>  drivers/dfu/dfu.c          | 46
> ++++++++++++++++++++++++++++------------------
> drivers/usb/gadget/f_dfu.c | 20 ++++++++++++++++---- 2 files changed,
> 44 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
> index dab5e7048ed5..1bf66d0613c9 100644
> --- a/drivers/dfu/dfu.c
> +++ b/drivers/dfu/dfu.c
> @@ -147,6 +147,19 @@ static int dfu_write_buffer_drain(struct
> dfu_entity *dfu) return ret;
>  }
>  
> +void dfu_write_transaction_cleanup(struct dfu_entity *dfu)
> +{
> +	/* clear everything */
> +	dfu_free_buf();
> +	dfu->crc = 0;
> +	dfu->offset = 0;
> +	dfu->i_blk_seq_num = 0;
> +	dfu->i_buf_start = dfu_buf;
> +	dfu->i_buf_end = dfu_buf;
> +	dfu->i_buf = dfu->i_buf_start;
> +	dfu->inited = 0;
> +}
> +
>  int dfu_flush(struct dfu_entity *dfu, void *buf, int size, int
> blk_seq_num) {
>  	int ret = 0;
> @@ -162,23 +175,14 @@ int dfu_flush(struct dfu_entity *dfu, void
> *buf, int size, int blk_seq_num) printf("\nDFU complete %s:
> 0x%08x\n", dfu_hash_algo->name, dfu->crc);
>  
> -	/* clear everything */
> -	dfu_free_buf();
> -	dfu->crc = 0;
> -	dfu->offset = 0;
> -	dfu->i_blk_seq_num = 0;
> -	dfu->i_buf_start = dfu_buf;
> -	dfu->i_buf_end = dfu_buf;
> -	dfu->i_buf = dfu->i_buf_start;
> -	dfu->inited = 0;
> +	dfu_write_transaction_cleanup(dfu);
>  
>  	return ret;
>  }
>  
>  int dfu_write(struct dfu_entity *dfu, void *buf, int size, int
> blk_seq_num) {
> -	int ret = 0;
> -	int tret;
> +	int ret;
>  
>  	debug("%s: name: %s buf: 0x%p size: 0x%x p_num: 0x%x offset:
> 0x%llx bufoffset: 0x%x\n", __func__, dfu->name, buf, size,
> blk_seq_num, dfu->offset, @@ -202,6 +206,7 @@ int dfu_write(struct
> dfu_entity *dfu, void *buf, int size, int blk_seq_num) if
> (dfu->i_blk_seq_num != blk_seq_num) { printf("%s: Wrong sequence
> number! [%d] [%d]\n", __func__, dfu->i_blk_seq_num, blk_seq_num);
> +		dfu_write_transaction_cleanup(dfu);
>  		return -1;
>  	}
>  
> @@ -223,15 +228,18 @@ int dfu_write(struct dfu_entity *dfu, void
> *buf, int size, int blk_seq_num) 
>  	/* flush buffer if overflow */
>  	if ((dfu->i_buf + size) > dfu->i_buf_end) {
> -		tret = dfu_write_buffer_drain(dfu);
> -		if (ret == 0)
> -			ret = tret;
> +		ret = dfu_write_buffer_drain(dfu);
> +		if (ret) {
> +			dfu_write_transaction_cleanup(dfu);
> +			return ret;
> +		}
>  	}
>  
>  	/* we should be in buffer now (if not then size too large) */
>  	if ((dfu->i_buf + size) > dfu->i_buf_end) {
>  		error("Buffer overflow! (0x%p + 0x%x > 0x%p)\n",
> dfu->i_buf, size, dfu->i_buf_end);
> +		dfu_write_transaction_cleanup(dfu);
>  		return -1;
>  	}
>  
> @@ -240,12 +248,14 @@ int dfu_write(struct dfu_entity *dfu, void
> *buf, int size, int blk_seq_num) 
>  	/* if end or if buffer full flush */
>  	if (size == 0 || (dfu->i_buf + size) > dfu->i_buf_end) {
> -		tret = dfu_write_buffer_drain(dfu);
> -		if (ret == 0)
> -			ret = tret;
> +		ret = dfu_write_buffer_drain(dfu);
> +		if (ret) {
> +			dfu_write_transaction_cleanup(dfu);
> +			return ret;
> +		}
>  	}
>  
> -	return ret;
> +	return 0;
>  }
>  
>  static int dfu_read_buffer_fill(struct dfu_entity *dfu, void *buf,
> int size) diff --git a/drivers/usb/gadget/f_dfu.c
> b/drivers/usb/gadget/f_dfu.c index 859fe828de74..9863dec44d99 100644
> --- a/drivers/usb/gadget/f_dfu.c
> +++ b/drivers/usb/gadget/f_dfu.c
> @@ -162,17 +162,27 @@ static void dfu_set_poll_timeout(struct
> dfu_status *dstat, unsigned int ms) static void
> dnload_request_complete(struct usb_ep *ep, struct usb_request *req) {
>  	struct f_dfu *f_dfu = req->context;
> +	int ret;
>  
> -	dfu_write(dfu_get_entity(f_dfu->altsetting), req->buf,
> -		  req->length, f_dfu->blk_seq_num);
> +	ret = dfu_write(dfu_get_entity(f_dfu->altsetting), req->buf,
> +			req->length, f_dfu->blk_seq_num);
> +	if (ret) {
> +		f_dfu->dfu_status = DFU_STATUS_errUNKNOWN;
> +		f_dfu->dfu_state = DFU_STATE_dfuERROR;
> +	}
>  }
>  
>  static void dnload_request_flush(struct usb_ep *ep, struct
> usb_request *req) {
>  	struct f_dfu *f_dfu = req->context;
> +	int ret;
>  
> -	dfu_flush(dfu_get_entity(f_dfu->altsetting), req->buf,
> -		  req->length, f_dfu->blk_seq_num);
> +	ret = dfu_flush(dfu_get_entity(f_dfu->altsetting), req->buf,
> +			req->length, f_dfu->blk_seq_num);
> +	if (ret) {
> +		f_dfu->dfu_status = DFU_STATUS_errUNKNOWN;
> +		f_dfu->dfu_state = DFU_STATE_dfuERROR;
> +	}
>  }
>  
>  static inline int dfu_get_manifest_timeout(struct dfu_entity *dfu)
> @@ -770,6 +780,8 @@ static int dfu_set_alt(struct usb_function *f,
> unsigned intf, unsigned alt) debug("%s: intf:%d alt:%d\n", __func__,
> intf, alt); 
>  	f_dfu->altsetting = alt;
> +	f_dfu->dfu_state = DFU_STATE_dfuIDLE;
> +	f_dfu->dfu_status = DFU_STATUS_OK;
>  
>  	return 0;
>  }



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group