[U-Boot] [PATCH v1] usb: gadget: f_dfu: write req->actual bytes

[Felipe Balbi]

Hi,

Lukasz Majewski <lukma at denx.de> writes:
>> Lukasz Majewski <lukma at denx.de> writes:
>> >> Lukasz Majewski <lukma at 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

-- 
balbi