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

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

[Andy Shevchenko]

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>
---
 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);
 	if (ret) {
 		f_dfu->dfu_status = DFU_STATUS_errUNKNOWN;
 		f_dfu->dfu_state = DFU_STATE_dfuERROR;
-- 
2.11.0

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

[Marek Vasut]

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.

> ---
>  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);
>  	if (ret) {
>  		f_dfu->dfu_status = DFU_STATUS_errUNKNOWN;
>  		f_dfu->dfu_state = DFU_STATE_dfuERROR;
> 


-- 
Best regards,
Marek Vasut

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

[Felipe Balbi]

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:

>>  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?

-- 
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/9bebe731/attachment.sig>

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

[Felipe Balbi]

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>

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

[Lukasz Majewski]

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 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>

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

[Andy Shevchenko]

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.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

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

[Felipe Balbi]

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

-- 
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/20170221/f5cf04fc/attachment.sig>

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

[Lukasz Majewski]

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 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/20170221/79a34d28/attachment.sig>

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

[Felipe Balbi]

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}"

-- 
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/20170221/ca59e706/attachment-0001.sig>

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

[Lukasz Majewski]

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 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/20170221/b74f4fc9/attachment.sig>

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

[Felipe Balbi]

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

-- 
balbi

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

[Lukasz Majewski]

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 at denx.de

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

[Felipe Balbi]

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.

-- 
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/20170222/7552f501/attachment.sig>