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

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

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

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?