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

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

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

> >>>> -#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 :)

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

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

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

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

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

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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130606/9af3e6b5/attachment.pgp>