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

* [U-Boot] [PATCH v1] usb: gadget: f_dfu: write req->actual bytes
       [not found] <de749db9-9d5c-4777-b4e9-2c0dd1840cb8@email.android.com>
@ 2017-02-13 11:42 ` Felipe Balbi
  2017-02-13 13:41   ` Lukasz Majewski
  0 siblings, 1 reply; 13+ messages in thread
From: Felipe Balbi @ 2017-02-13 11:42 UTC (permalink / raw)
  To: u-boot

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

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170213/233a9b20/attachment.sig>

^ permalink raw reply	[flat|nested] 13+ messages in thread