Re: [U-Boot] [PATCH v1] usb: gadget: f_dfu: write req->actual bytes
Hi Lukasz,
Lukasz Majewski lukma@denx.de writes:
Hi Felipe,
Thanks for the patch. Please see my comments below.
On 13 Feb 2017 11:42 am, Felipe Balbi felipe.balbi@linux.intel.com wrote:
Hi,
Marek Vasut marex@denx.de writes:
On 02/10/2017 05:32 PM, Andy Shevchenko wrote:
From: Felipe Balbi felipe.balbi@linux.intel.com
If last packet is short, we shouldn't write req->length bytes to non-volatile media, we should write only what's available to us, which is held in req->actual.
Signed-off-by: Felipe Balbi felipe.balbi@linux.intel.com Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com
Since I have no clue about DFU internals, I will wait for Lukasz's Ack.
you don't need to have any clues about DFU internals to realise that this fixes an actual bug, see below:
I don't know your exact use case. Please keep in mind that we most
eMMC :-)
work on NAND and eMMC, which require the whole block write.
Well, then the file should have been padded already before sending it over USB, right? :-)
You shouldn't write req->length if you don't receive req->length as you are, potentially, writing garbage to the storage medium :-)
However, I will setup test environment (after changing the job it was gone), test your patch and then let you know.
cool
drivers/usb/gadget/f_dfu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/f_dfu.c b/drivers/usb/gadget/f_dfu.c index 8e7c981657..64cdfa7c98 100644 --- a/drivers/usb/gadget/f_dfu.c +++ b/drivers/usb/gadget/f_dfu.c @@ -159,7 +159,7 @@ static void dnload_request_complete(struct usb_ep *ep, struct usb_request *req) int ret;
ret = dfu_write(dfu_get_entity(f_dfu->altsetting), req->buf,
- req->length, f_dfu->blk_seq_num);
- req->actual, f_dfu->blk_seq_num);
DFU driver queues a request to USB controller. Per the gadget API req->length contains maximum amount of data to be transmitted. req->actual is written by USB controller with the actual amount of data that we transmitted.
In the case of IN (TX), upon completion req->length and req->actual should always be equal (unless errors show up, etc)
In the case of OUT (RX), upon completion req->actual MAY BE less than req->length and that's not an error. Say host sent us a short packet which causes early termination of transfer.
With that in mind, let's consider the situation where we're receiving data from host using DFU. Let's assume that we have a 4096 byte buffer for transfers and we're receiving a binary that's 7679 bytes in size.
Here's what we will do (pseudo-code):
int remaining = 7679; char buf[4096];
while (remaining) { req->length = 4096; req->buf = buf; usb_ep_queue(req);
/* wait for completion */
remaining -= req->actual;
dfu_write(buf, req->length); /* this is the error */ }
Can you see here that in the last packet we will write 4096 bytes when we should write only 3583?
In principle you are right. I need to check if this change will not introduce regressions.
Can you share your use case?
Intel Edison running v2017.03-rc1 + patches (see [1]), flashing u-boot.bin over DFU (see [2] for details). Without $subject, image has to be aligned to 4096 bytes as below:
$ dd if=u-boot.bin of=u-boot-4k.bin bs=4k seek=1 && truncate -s %4096 u-boot-4k.bin
With $subject, I don't need truncate. We still need the 4096 byte of zeroes in the beginning of the image for other reasons (which I really don't know why at this point).
[1] https://github.com/andy-shev/u-boot/tree/edison [2] https://communities.intel.com/message/435516#435516
Hi Felipe,
Hi Lukasz,
Lukasz Majewski lukma@denx.de writes:
Hi Felipe,
Thanks for the patch. Please see my comments below.
On 13 Feb 2017 11:42 am, Felipe Balbi felipe.balbi@linux.intel.com wrote:
Hi,
Marek Vasut marex@denx.de writes:
On 02/10/2017 05:32 PM, Andy Shevchenko wrote:
From: Felipe Balbi felipe.balbi@linux.intel.com
If last packet is short, we shouldn't write req->length bytes to non-volatile media, we should write only what's available to us, which is held in req->actual.
Signed-off-by: Felipe Balbi felipe.balbi@linux.intel.com Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com
Since I have no clue about DFU internals, I will wait for Lukasz's Ack.
you don't need to have any clues about DFU internals to realise that this fixes an actual bug, see below:
I don't know your exact use case. Please keep in mind that we most
eMMC :-)
Ok.
work on NAND and eMMC, which require the whole block write.
Well, then the file should have been padded already before sending it over USB, right? :-)
You shouldn't write req->length if you don't receive req->length as you are, potentially, writing garbage to the storage medium :-)
Yes. Correct....
However, I will setup test environment (after changing the job it was gone), test your patch and then let you know.
cool
drivers/usb/gadget/f_dfu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/f_dfu.c b/drivers/usb/gadget/f_dfu.c index 8e7c981657..64cdfa7c98 100644 --- a/drivers/usb/gadget/f_dfu.c +++ b/drivers/usb/gadget/f_dfu.c @@ -159,7 +159,7 @@ static void dnload_request_complete(struct usb_ep *ep, struct usb_request *req) int ret;
ret = dfu_write(dfu_get_entity(f_dfu->altsetting), req->buf,
- req->length, f_dfu->blk_seq_num);
- req->actual, f_dfu->blk_seq_num);
DFU driver queues a request to USB controller. Per the gadget API req->length contains maximum amount of data to be transmitted. req->actual is written by USB controller with the actual amount of data that we transmitted.
In the case of IN (TX), upon completion req->length and req->actual should always be equal (unless errors show up, etc)
In the case of OUT (RX), upon completion req->actual MAY BE less than req->length and that's not an error. Say host sent us a short packet which causes early termination of transfer.
With that in mind, let's consider the situation where we're receiving data from host using DFU. Let's assume that we have a 4096 byte buffer for transfers and we're receiving a binary that's 7679 bytes in size.
Here's what we will do (pseudo-code):
int remaining = 7679; char buf[4096];
while (remaining) { req->length = 4096; req->buf = buf; usb_ep_queue(req);
/* wait for completion */
remaining -= req->actual;
dfu_write(buf, req->length); /* this is the error */ }
Can you see here that in the last packet we will write 4096 bytes when we should write only 3583?
In principle you are right. I need to check if this change will not introduce regressions.
Can you share your use case?
Intel Edison running v2017.03-rc1 + patches (see [1]), flashing u-boot.bin over DFU (see [2] for details). Without $subject, image has to be aligned to 4096 bytes as below:
$ dd if=u-boot.bin of=u-boot-4k.bin bs=4k seek=1 && truncate -s %4096 u-boot-4k.bin
With $subject, I don't need truncate. We still need the 4096 byte of zeroes in the beginning of the image for other reasons (which I really don't know why at this point).
[1] https://github.com/andy-shev/u-boot/tree/edison [2] https://communities.intel.com/message/435516#435516
Ok. I will check this. Thanks for pointing out :-)
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
On Mon, 2017-02-13 at 14:41 +0100, Lukasz Majewski wrote:
[1] https://github.com/andy-shev/u-boot/tree/edison [2] https://communities.intel.com/message/435516#435516
Ok. I will check this. Thanks for pointing out :-)
Just in case, mentioned branch is occasionally rebased, so, for "stable" code I do tags with "edison" prefix.
Hi Lukasz,
Lukasz Majewski lukma@denx.de writes:
drivers/usb/gadget/f_dfu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/f_dfu.c b/drivers/usb/gadget/f_dfu.c index 8e7c981657..64cdfa7c98 100644 --- a/drivers/usb/gadget/f_dfu.c +++ b/drivers/usb/gadget/f_dfu.c @@ -159,7 +159,7 @@ static void dnload_request_complete(struct usb_ep *ep, struct usb_request *req) int ret;
ret = dfu_write(dfu_get_entity(f_dfu->altsetting), req->buf,
- req->length, f_dfu->blk_seq_num);
- req->actual, f_dfu->blk_seq_num);
DFU driver queues a request to USB controller. Per the gadget API req->length contains maximum amount of data to be transmitted. req->actual is written by USB controller with the actual amount of data that we transmitted.
In the case of IN (TX), upon completion req->length and req->actual should always be equal (unless errors show up, etc)
In the case of OUT (RX), upon completion req->actual MAY BE less than req->length and that's not an error. Say host sent us a short packet which causes early termination of transfer.
With that in mind, let's consider the situation where we're receiving data from host using DFU. Let's assume that we have a 4096 byte buffer for transfers and we're receiving a binary that's 7679 bytes in size.
Here's what we will do (pseudo-code):
int remaining = 7679; char buf[4096];
while (remaining) { req->length = 4096; req->buf = buf; usb_ep_queue(req);
/* wait for completion */
remaining -= req->actual;
dfu_write(buf, req->length); /* this is the error */ }
Can you see here that in the last packet we will write 4096 bytes when we should write only 3583?
In principle you are right. I need to check if this change will not introduce regressions.
Can you share your use case?
Intel Edison running v2017.03-rc1 + patches (see [1]), flashing u-boot.bin over DFU (see [2] for details). Without $subject, image has to be aligned to 4096 bytes as below:
$ dd if=u-boot.bin of=u-boot-4k.bin bs=4k seek=1 && truncate -s %4096 u-boot-4k.bin
With $subject, I don't need truncate. We still need the 4096 byte of zeroes in the beginning of the image for other reasons (which I really don't know why at this point).
[1] https://github.com/andy-shev/u-boot/tree/edison [2] https://communities.intel.com/message/435516#435516
Ok. I will check this. Thanks for pointing out :-)
Any updates here? I'd like to send Tangier Soc and Intel Edison Board support but I kinda depend on this patch making upstream. I can resend as part of the "add intel edison" series.
Let me know
Hi Felipe,
Hi Lukasz,
Lukasz Majewski lukma@denx.de writes:
drivers/usb/gadget/f_dfu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/f_dfu.c b/drivers/usb/gadget/f_dfu.c index 8e7c981657..64cdfa7c98 100644 --- a/drivers/usb/gadget/f_dfu.c +++ b/drivers/usb/gadget/f_dfu.c @@ -159,7 +159,7 @@ static void dnload_request_complete(struct usb_ep *ep, struct usb_request *req) int ret;
ret = dfu_write(dfu_get_entity(f_dfu->altsetting), req->buf,
- req->length, f_dfu->blk_seq_num);
- req->actual, f_dfu->blk_seq_num);
DFU driver queues a request to USB controller. Per the gadget API req->length contains maximum amount of data to be transmitted. req->actual is written by USB controller with the actual amount of data that we transmitted.
In the case of IN (TX), upon completion req->length and req->actual should always be equal (unless errors show up, etc)
In the case of OUT (RX), upon completion req->actual MAY BE less than req->length and that's not an error. Say host sent us a short packet which causes early termination of transfer.
With that in mind, let's consider the situation where we're receiving data from host using DFU. Let's assume that we have a 4096 byte buffer for transfers and we're receiving a binary that's 7679 bytes in size.
Here's what we will do (pseudo-code):
int remaining = 7679; char buf[4096];
while (remaining) { req->length = 4096; req->buf = buf; usb_ep_queue(req);
/* wait for completion */
remaining -= req->actual;
dfu_write(buf, req->length); /* this is the error */ }
Can you see here that in the last packet we will write 4096 bytes when we should write only 3583?
In principle you are right. I need to check if this change will not introduce regressions.
Can you share your use case?
Intel Edison running v2017.03-rc1 + patches (see [1]), flashing u-boot.bin over DFU (see [2] for details). Without $subject, image has to be aligned to 4096 bytes as below:
$ dd if=u-boot.bin of=u-boot-4k.bin bs=4k seek=1 && truncate -s %4096 u-boot-4k.bin
With $subject, I don't need truncate. We still need the 4096 byte of zeroes in the beginning of the image for other reasons (which I really don't know why at this point).
[1] https://github.com/andy-shev/u-boot/tree/edison [2] https://communities.intel.com/message/435516#435516
Ok. I will check this. Thanks for pointing out :-)
Any updates here? I'd like to send Tangier Soc and Intel Edison Board support but I kinda depend on this patch making upstream. I can resend as part of the "add intel edison" series.
Let me know
I'm setting up /test/py/dfu now on BBB. I will let you know by EOD.
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
Hi,
Lukasz Majewski lukma@denx.de writes:
Lukasz Majewski lukma@denx.de writes:
> drivers/usb/gadget/f_dfu.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/usb/gadget/f_dfu.c > b/drivers/usb/gadget/f_dfu.c index 8e7c981657..64cdfa7c98 > 100644 --- a/drivers/usb/gadget/f_dfu.c > +++ b/drivers/usb/gadget/f_dfu.c > @@ -159,7 +159,7 @@ static void > dnload_request_complete(struct usb_ep *ep, struct > usb_request *req) int ret; > > ret = dfu_write(dfu_get_entity(f_dfu->altsetting), req->buf, > - req->length, f_dfu->blk_seq_num); > + req->actual, f_dfu->blk_seq_num);
DFU driver queues a request to USB controller. Per the gadget API req->length contains maximum amount of data to be transmitted. req->actual is written by USB controller with the actual amount of data that we transmitted.
In the case of IN (TX), upon completion req->length and req->actual should always be equal (unless errors show up, etc)
In the case of OUT (RX), upon completion req->actual MAY BE less than req->length and that's not an error. Say host sent us a short packet which causes early termination of transfer.
With that in mind, let's consider the situation where we're receiving data from host using DFU. Let's assume that we have a 4096 byte buffer for transfers and we're receiving a binary that's 7679 bytes in size.
Here's what we will do (pseudo-code):
int remaining = 7679; char buf[4096];
while (remaining) { req->length = 4096; req->buf = buf; usb_ep_queue(req);
/* wait for completion */
remaining -= req->actual;
dfu_write(buf, req->length); /* this is the error */ }
Can you see here that in the last packet we will write 4096 bytes when we should write only 3583?
In principle you are right. I need to check if this change will not introduce regressions.
Can you share your use case?
Intel Edison running v2017.03-rc1 + patches (see [1]), flashing u-boot.bin over DFU (see [2] for details). Without $subject, image has to be aligned to 4096 bytes as below:
$ dd if=u-boot.bin of=u-boot-4k.bin bs=4k seek=1 && truncate -s %4096 u-boot-4k.bin
With $subject, I don't need truncate. We still need the 4096 byte of zeroes in the beginning of the image for other reasons (which I really don't know why at this point).
[1] https://github.com/andy-shev/u-boot/tree/edison [2] https://communities.intel.com/message/435516#435516
Ok. I will check this. Thanks for pointing out :-)
Any updates here? I'd like to send Tangier Soc and Intel Edison Board support but I kinda depend on this patch making upstream. I can resend as part of the "add intel edison" series.
Let me know
I'm setting up /test/py/dfu now on BBB. I will let you know by EOD.
Here's what I used for testing:
#!/usr/bin/env ruby
$errors = 0 $success = 0 $total = 0
$initial_size = 64 $max_size = 8 * 1024 * 1024 # 8MiB $current_size = $initial_size
def create_file(size) `dd if=/dev/zero of=file.bin bs=#{size} count=1 > /dev/null 2>&1` end
def send_file(size) `dfu-util -v -d 8087:0a99 -D file.bin > /dev/null 2>&1` end
def transmit(size) create_file(size) send_file(size) end
def transmit_and_check(size) transmit(size)
if $?.success? $success += 1 print "." else $errors += 1 print "F" end
$total += 1 end
while $current_size <= $max_size do transmit_and_check($current_size - 1) transmit_and_check($current_size) transmit_and_check($current_size + 1)
$current_size *= 2 end
puts "\n" puts "Test Done" puts "Successes: #{$success}" puts "Failures: #{$errors}" puts "Total: #{$total}"
Hi.
Hi,
Lukasz Majewski lukma@denx.de writes:
Lukasz Majewski lukma@denx.de writes:
>> drivers/usb/gadget/f_dfu.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/usb/gadget/f_dfu.c >> b/drivers/usb/gadget/f_dfu.c index 8e7c981657..64cdfa7c98 >> 100644 --- a/drivers/usb/gadget/f_dfu.c >> +++ b/drivers/usb/gadget/f_dfu.c >> @@ -159,7 +159,7 @@ static void >> dnload_request_complete(struct usb_ep *ep, struct >> usb_request *req) int ret; >> >> ret = dfu_write(dfu_get_entity(f_dfu->altsetting), >> req->buf, >> - req->length, f_dfu->blk_seq_num); >> + req->actual, f_dfu->blk_seq_num);
DFU driver queues a request to USB controller. Per the gadget API req->length contains maximum amount of data to be transmitted. req->actual is written by USB controller with the actual amount of data that we transmitted.
In the case of IN (TX), upon completion req->length and req->actual should always be equal (unless errors show up, etc)
In the case of OUT (RX), upon completion req->actual MAY BE less than req->length and that's not an error. Say host sent us a short packet which causes early termination of transfer.
With that in mind, let's consider the situation where we're receiving data from host using DFU. Let's assume that we have a 4096 byte buffer for transfers and we're receiving a binary that's 7679 bytes in size.
Here's what we will do (pseudo-code):
int remaining = 7679; char buf[4096];
while (remaining) { req->length = 4096; req->buf = buf; usb_ep_queue(req);
/* wait for completion */
remaining -= req->actual;
dfu_write(buf, req->length); /* this is the error */ }
Can you see here that in the last packet we will write 4096 bytes when we should write only 3583?
In principle you are right. I need to check if this change will not introduce regressions.
Can you share your use case?
Intel Edison running v2017.03-rc1 + patches (see [1]), flashing u-boot.bin over DFU (see [2] for details). Without $subject, image has to be aligned to 4096 bytes as below:
$ dd if=u-boot.bin of=u-boot-4k.bin bs=4k seek=1 && truncate -s %4096 u-boot-4k.bin
With $subject, I don't need truncate. We still need the 4096 byte of zeroes in the beginning of the image for other reasons (which I really don't know why at this point).
[1] https://github.com/andy-shev/u-boot/tree/edison [2] https://communities.intel.com/message/435516#435516
Ok. I will check this. Thanks for pointing out :-)
Any updates here? I'd like to send Tangier Soc and Intel Edison Board support but I kinda depend on this patch making upstream. I can resend as part of the "add intel edison" series.
Let me know
I'm setting up /test/py/dfu now on BBB. I will let you know by EOD.
Here's what I used for testing:
I do appreciate that you tested it - even better, that with different approach.
However, some time ago Stephen Warren has rewritten tests for DFU, UMS to use some python infrastructure. Those tests (especially DFU, test corner cases - ZLP, +/-1B to packet size, etc).
If you want, you can add your board to them.
More info ./test/py {dfu, ums} and ./Documentation.
(Or you can wait a bit, so I will share my "fresh" BBB setup).
Best regards, Łukasz
#!/usr/bin/env ruby
$errors = 0 $success = 0 $total = 0
$initial_size = 64 $max_size = 8 * 1024 * 1024 # 8MiB $current_size = $initial_size
def create_file(size) `dd if=/dev/zero of=file.bin bs=#{size} count=1 > /dev/null 2>&1` end
def send_file(size) `dfu-util -v -d 8087:0a99 -D file.bin > /dev/null 2>&1` end
def transmit(size) create_file(size) send_file(size) end
def transmit_and_check(size) transmit(size)
if $?.success? $success += 1 print "." else $errors += 1 print "F" end
$total += 1 end
while $current_size <= $max_size do transmit_and_check($current_size - 1) transmit_and_check($current_size) transmit_and_check($current_size + 1)
$current_size *= 2 end
puts "\n" puts "Test Done" puts "Successes: #{$success}" puts "Failures: #{$errors}" puts "Total: #{$total}"
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
Hi,
Lukasz Majewski lukma@denx.de writes:
Lukasz Majewski lukma@denx.de writes:
Lukasz Majewski lukma@denx.de writes:
> >> drivers/usb/gadget/f_dfu.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/usb/gadget/f_dfu.c > >> b/drivers/usb/gadget/f_dfu.c index 8e7c981657..64cdfa7c98 > >> 100644 --- a/drivers/usb/gadget/f_dfu.c > >> +++ b/drivers/usb/gadget/f_dfu.c > >> @@ -159,7 +159,7 @@ static void > >> dnload_request_complete(struct usb_ep *ep, struct > >> usb_request *req) int ret; > >> > >> ret = dfu_write(dfu_get_entity(f_dfu->altsetting), > >> req->buf, > >> - req->length, f_dfu->blk_seq_num); > >> + req->actual, f_dfu->blk_seq_num); > > DFU driver queues a request to USB controller. Per the gadget > API req->length contains maximum amount of data to be > transmitted. req->actual is written by USB controller with > the actual amount of data that we transmitted. > > In the case of IN (TX), upon completion req->length and > req->actual should always be equal (unless errors show up, > etc) > > In the case of OUT (RX), upon completion req->actual MAY BE > less than req->length and that's not an error. Say host sent > us a short packet which causes early termination of transfer. > > With that in mind, let's consider the situation where we're > receiving data from host using DFU. Let's assume that we have > a 4096 byte buffer for transfers and we're receiving a binary > that's 7679 bytes in size. > > Here's what we will do (pseudo-code): > > int remaining = 7679; > char buf[4096]; > > while (remaining) { > req->length = 4096; > req->buf = buf; > usb_ep_queue(req); > > /* wait for completion */ > > remaining -= req->actual; > > dfu_write(buf, req->length); /* this is the error */ > } > > Can you see here that in the last packet we will write 4096 > bytes when we should write only 3583? > > In principle you are right. I need to check if this change > will not introduce regressions. > > Can you share your use case?
Intel Edison running v2017.03-rc1 + patches (see [1]), flashing u-boot.bin over DFU (see [2] for details). Without $subject, image has to be aligned to 4096 bytes as below:
$ dd if=u-boot.bin of=u-boot-4k.bin bs=4k seek=1 && truncate -s %4096 u-boot-4k.bin
With $subject, I don't need truncate. We still need the 4096 byte of zeroes in the beginning of the image for other reasons (which I really don't know why at this point).
[1] https://github.com/andy-shev/u-boot/tree/edison [2] https://communities.intel.com/message/435516#435516
Ok. I will check this. Thanks for pointing out :-)
Any updates here? I'd like to send Tangier Soc and Intel Edison Board support but I kinda depend on this patch making upstream. I can resend as part of the "add intel edison" series.
Let me know
I'm setting up /test/py/dfu now on BBB. I will let you know by EOD.
Here's what I used for testing:
I do appreciate that you tested it - even better, that with different approach.
However, some time ago Stephen Warren has rewritten tests for DFU, UMS to use some python infrastructure. Those tests (especially DFU, test corner cases - ZLP, +/-1B to packet size, etc).
that's exactly what I tested :-)
If you want, you can add your board to them.
I'll see how to do that and maybe add to the TODO list
Hi Felipe,
Hi,
Lukasz Majewski lukma@denx.de writes:
Lukasz Majewski lukma@denx.de writes:
Lukasz Majewski lukma@denx.de writes:
> > >> drivers/usb/gadget/f_dfu.c | 2 +- > > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > >> > > >> diff --git a/drivers/usb/gadget/f_dfu.c > > >> b/drivers/usb/gadget/f_dfu.c index > > >> 8e7c981657..64cdfa7c98 100644 --- > > >> a/drivers/usb/gadget/f_dfu.c +++ > > >> b/drivers/usb/gadget/f_dfu.c @@ -159,7 +159,7 @@ > > >> static void dnload_request_complete(struct usb_ep *ep, > > >> struct usb_request *req) int ret; > > >> > > >> ret = dfu_write(dfu_get_entity(f_dfu->altsetting), > > >> req->buf, > > >> - req->length, f_dfu->blk_seq_num); > > >> + req->actual, f_dfu->blk_seq_num); > > > > DFU driver queues a request to USB controller. Per the > > gadget API req->length contains maximum amount of data to > > be transmitted. req->actual is written by USB controller > > with the actual amount of data that we transmitted. > > > > In the case of IN (TX), upon completion req->length and > > req->actual should always be equal (unless errors show up, > > etc) > > > > In the case of OUT (RX), upon completion req->actual MAY > > BE less than req->length and that's not an error. Say host > > sent us a short packet which causes early termination of > > transfer. > > > > With that in mind, let's consider the situation where > > we're receiving data from host using DFU. Let's assume > > that we have a 4096 byte buffer for transfers and we're > > receiving a binary that's 7679 bytes in size. > > > > Here's what we will do (pseudo-code): > > > > int remaining = 7679; > > char buf[4096]; > > > > while (remaining) { > > req->length = 4096; > > req->buf = buf; > > usb_ep_queue(req); > > > > /* wait for completion */ > > > > remaining -= req->actual; > > > > dfu_write(buf, req->length); /* this is the error */ > > } > > > > Can you see here that in the last packet we will write > > 4096 bytes when we should write only 3583? > > > > In principle you are right. I need to check if this change > > will not introduce regressions. > > > > Can you share your use case? > > Intel Edison running v2017.03-rc1 + patches (see [1]), > flashing u-boot.bin over DFU (see [2] for details). Without > $subject, image has to be aligned to 4096 bytes as below: > > $ dd if=u-boot.bin of=u-boot-4k.bin bs=4k seek=1 && truncate > -s %4096 u-boot-4k.bin > > With $subject, I don't need truncate. We still need the 4096 > byte of zeroes in the beginning of the image for other > reasons (which I really don't know why at this point). > > [1] https://github.com/andy-shev/u-boot/tree/edison > [2] https://communities.intel.com/message/435516#435516 >
Ok. I will check this. Thanks for pointing out :-)
Any updates here? I'd like to send Tangier Soc and Intel Edison Board support but I kinda depend on this patch making upstream. I can resend as part of the "add intel edison" series.
Let me know
I'm setting up /test/py/dfu now on BBB. I will let you know by EOD.
Here's what I used for testing:
I do appreciate that you tested it - even better, that with different approach.
However, some time ago Stephen Warren has rewritten tests for DFU, UMS to use some python infrastructure. Those tests (especially DFU, test corner cases - ZLP, +/-1B to packet size, etc).
that's exactly what I tested :-)
If you want, you can add your board to them.
I'll see how to do that and maybe add to the TODO list
Acked-by: Lukasz Majewski lukma@denx.de
Tested-by: Lukasz Majewski lukma@denx.de
Test HW: BBB (am335x) - with tests/py/dfu
I've added your patch to u-boot-dfu tree. I will send PR tomorrow.
You may also want to look into patches from Patrick Delaunay (those also will be added to PR):
http://patchwork.ozlabs.org/patch/704131/ http://patchwork.ozlabs.org/patch/706492/ http://patchwork.ozlabs.org/patch/706493/
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
Hi,
Lukasz Majewski lukma@denx.de writes:
[snip]
I do appreciate that you tested it - even better, that with different approach.
However, some time ago Stephen Warren has rewritten tests for DFU, UMS to use some python infrastructure. Those tests (especially DFU, test corner cases - ZLP, +/-1B to packet size, etc).
that's exactly what I tested :-)
If you want, you can add your board to them.
I'll see how to do that and maybe add to the TODO list
Acked-by: Lukasz Majewski lukma@denx.de
Tested-by: Lukasz Majewski lukma@denx.de
Test HW: BBB (am335x) - with tests/py/dfu
I've added your patch to u-boot-dfu tree. I will send PR tomorrow.
yeah, cool thanks ;-)
You may also want to look into patches from Patrick Delaunay (those also will be added to PR):
http://patchwork.ozlabs.org/patch/704131/ http://patchwork.ozlabs.org/patch/706492/ http://patchwork.ozlabs.org/patch/706493/
they look fine to my eyes. I have two extra patches (coming shortly) to make USB Command Verifier tool (from USB-IF) happy with our DFU implementation.
participants (3)
-
Andy Shevchenko -
Felipe Balbi -
Lukasz Majewski