Re: [U-Boot] [PATCH] dfu: add write error handling
Hi Stephen, > From: Stephen Warren > > 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 Applied to u-boot-dfu Thanks for the patch. -- Best regards, Lukasz Majewski Samsung R&D Institute Poland (SRPOL) | Linux Platform Group ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] dfu: add write error handling
Hi Stephen,
> From: Stephen Warren
>
> 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
> ---
> 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
Test HW: Exynos4412 - Trats2
Tested-by: Lukasz Majewski
> ---
> 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;
> }
>
[U-Boot] [PATCH] dfu: add write error handling
From: Stephen Warren
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
---
This probably depends on "dfu: fix some issues with reads/uploads",
for context if nothing else.
---
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_
