Hi Lukasz, Lukasz Majewski <lu...@denx.de> writes: > Hi Felipe, > > Thanks for the patch. > Please see my comments below. > > On 13 Feb 2017 11:42 am, Felipe Balbi <felipe.ba...@linux.intel.com> wrote: > > Hi, > > Marek Vasut <ma...@denx.de> writes: > > On 02/10/2017 05:32 PM, Andy Shevchenko wrote: > >> From: Felipe Balbi <felipe.ba...@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.ba...@linux.intel.com> > >> Signed-off-by: Andy Shevchenko <andriy.shevche...@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 -- balbi
signature.asc
Description: PGP signature
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot