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

[Lukasz Majewski]

Hi Felipe,

> 
> Hi Lukasz,
> 
> Lukasz Majewski <lukma at 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 at linux.intel.com> wrote:
> >
> >  Hi, 
> >
> >  Marek Vasut <marex at denx.de> writes: 
> >  > On 02/10/2017 05:32 PM, Andy Shevchenko wrote: 
> >  >> From: Felipe Balbi <felipe.balbi at 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 at linux.intel.com> 
> >  >> Signed-off-by: Andy Shevchenko
> >  >> <andriy.shevchenko at 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 at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170213/b8a5fe80/attachment.sig>