[U-Boot] dfu: make data buffer size configurable

[Heiko Schocher <hs@denx.de>]

Hello Tom,

Am 06.06.2013 17:55, schrieb Tom Rini:
> On Wed, Jun 05, 2013 at 04:04:46PM +0200, Heiko Schocher wrote:
> 
> [snip]
>>>> In current code CONFIG_SYS_DFU_MAX_FILE_SIZE is not used in dfu_nand.c,
>>>
>>> Nor anywhere else.  As I said in the DFU + UBI thread, there's a bug
>>> here :)
>>
>> CONFIG_SYS_DFU_MAX_FILE_SIZE is used in ./drivers/dfu/dfu_mmc.c ...
> 
> Ah yes, right.
> 
>>>> as if buffer is full, it is immediately flushed to nand.
>>>> Also default value from CONFIG_SYS_DFU_MAX_FILE_SIZE is smaller (4MiB)
>>>> as default value of CONFIG_SYS_DFU_DATA_BUF_SIZE (8MiB) ...
>>>
>>> Right, and the commit that did it was about increasing the size of the
>>> kernel that could be sent over.
>>
>> Hmm.. the CONFIG_SYS_DFU_DATA_BUF_SIZE limits not the size of
>> a file that could be loaded into a partition. It specifies
>> only the size of one chunk, that get burned into the raw nand ...
>>
>> And this should be a configurable size ...
> 
> Or a saner, smaller size?  I think we've got a few things being done in
> a confusing and/or incorrect manner, see below.

Yes, smaller size is maybe the way to go ... see below

>>>> I used on my upcoming board port a CONFIG_SYS_DFU_DATA_BUF_SIZE from
>>>> 1MiB and that worked perfectly, when transferring a file > 200MB.
>>>> The default value from 8MiB sometimes caused an error on the host:
>>>>
>>>> []# date;dfu-util -a rootfs -D dxr2-org/dxr2.xx-release-image-UNKNOWN-dxr2.ubi;date
>>>> Di 28. Mai 14:20:44 CEST 2013
>>>> dfu-util 0.5
>>>> [...]
>>>> Copying data from PC to DFU device
>>>> Starting download: [#############################################dfu_download: libusb_control_transfer returned -7
>>>> Error during download
>>>>
>>>> Why we have a buffersize from 8MiB for raw writes, but a max file size
>>>> from 4MiB only?
>>>
>>> Then we need to poke around the code here a bit more and see what's
>>> going on, and fix things so that we can both do larger (say, 8MiB)
>>> filesystem transfers and not have dfu-util get mad sometimes.
>>
>> Timeout in libusb_control_transfer while the target writes
>> the 8MiB into the nand ... ?
>>
>> I try to find out something ...
> 
> Well, it ought to be fine, but we're pushing some boundaries here, and
> I don't know if we have a good reason for it.  We aren't changing the
> size of the chunks being passed along.

My suspicion that the problem is a timeout was the right idea!

I tried following patch in the current dfu-util sources:
(git clone git://gitorious.org/dfu-util/dfu-util.git
 current head: f66634406e9397e67c34033c3c0c26dde486528c)

diff --git a/src/main.c b/src/main.c
index 2aea134..12f6f1d 100644
--- a/src/main.c
+++ b/src/main.c
@@ -290,7 +290,8 @@ int main(int argc, char **argv)
 		exit(0);
 	}

-	dfu_init(5000);
+	printf("Using 20 sec timeout\n");
+	dfu_init(20000);

 	num_devs = count_dfu_devices(ctx, dif);
 	if (num_devs == 0) {

and I see no longer the above error! So I see two solutions
for my problem:

- make DFU_DATA_BUF_SIZE in U-Boot smaller or configurable
- make the timeout in dfu-util bigger or configurable

>>>>>> -#define DFU_DATA_BUF_SIZE		(1024*1024*8)	/* 8 MiB */
>>>>>> +#ifndef CONFIG_SYS_DFU_DATA_BUF_SIZE
>>>>>> +#define CONFIG_SYS_DFU_DATA_BUF_SIZE		(1024*1024*8)	/* 8 MiB */
>>>>>> +#endif
>>>>>>  #ifndef CONFIG_SYS_DFU_MAX_FILE_SIZE
>>>>>>  #define CONFIG_SYS_DFU_MAX_FILE_SIZE	(4 << 20)	/* 4 MiB */
>>>>>>  #endif
>>>>>
>>>>> We use one variable for both spots.  Or is there some case I'm missing
>>>>> where we need to buffer 8MiB at a time for raw writes?  In which case we
>>>>> still need to make CONFIG_SYS_DFU_MAX_FILE_SIZE be used :)
>>>>
>>>> I do not really know, why we have 2 defines here!
>>>
>>> File size vs buffer size?  I'm not quite certain it was the right way to
>>> go either.
>>
>> Yeah, but why is the file size < buffer size as default?
> 
> A bug, I'm pretty sure.  The change that made buffer > file was with the
> comment to allow for bigger files.  I think it might not have been
> completely tested :)

Maybe. I don't know.

>> In dfu_mmc:
>> If raw partition, if dfu_buf (size of CONFIG_SYS_DFU_DATA_BUF_SIZE)
>> full -> write it to the mmc. Same for nand.
> 
> Right.  Since we want to buffer up some amount of data, flush, repeat
> until done.

Yep.

>> If FAT or EXT4 partition (mmc only), write the dfu_buffer through
>> mmc_file_buffer() to dfu_file_buf[CONFIG_SYS_DFU_MAX_FILE_SIZE] ...
>> this seems buggy to me, but maybe I oversee something, I could not
>> try it ... and if the hole file is transfered, the dfu_file_buf
>> gets flushed to the partition ...
> 
> The need here is that since we can't append to files, transfer the whole
> file, then write.  We will not be told the filesize ahead of time, so we
> have to transfer up to the buffer and if we would exceed, error out at
> that point, otherwise continue.  Now I'm pretty sure, but not 100% sure
> that there's a reason we can't use dfu_buf in both places.  That needs
> re-checking.

Ok.

>> The CONFIG_SYS_DFU_MAX_FILE_SIZE is only needed as we can only
>> write a complete image to FAT, EXT4 (also UBI) partitions, I think.
> 
> It'll be needed for any file write, rather than block write.  The
> question is, can it ever be valid for MAX_FILE_SIZE to be smaller than
> BUF_SIZE ?  Maybe.

I would say no ...

>> So we have in the dfu subsystem following problems:
>>
>> a) we have no common API to add image types. Currently
>>    all dfu_{device_type} has to program it.
> 
> We also have no common image types.

Then we should introduce it ... oh, maybe we have them:
drivers/dfu/dfu_mmc.c in dfu_write_medium_mmc()i

switch(dfu->layout)
DFU_RAW_ADDR:
DFU_FS_FAT:
DFU_FS_EXT4:

?
>> b) we have no possibility to write image types (FAT, EXT4 or
>>    UBI) in chunks -> CONFIG_SYS_DFU_MAX_FILE_SIZE introduced
> 
> Correct.
> 
>> c) CONFIG_SYS_DFU_DATA_BUF_SIZE > CONFIG_SYS_DFU_MAX_FILE_SIZE
>>    which is in my eyes buggy ...
> 
> Or at least very odd, given the current defaults for both.
> 
>> d) sporadic problems with CONFIG_SYS_DFU_DATA_BUF_SIZE = 8MiB
>>    Currently i get always an error ... try to find out why ...
> 
> Maybe we don't need to go so large for the buffer.

Yep, with 1 or 2 MiB I see no problems with current dfu-util.

> If you've got a beaglebone (and I know there's one in denx :)) or am335x
> gp evm, you can test filesystem writes on SD cards with DFU.

Ah! Ok.

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany