[U-Boot] [PATCH] dfu: fix some issues with reads/uploads

[Lukasz Majewski]

Hi Stephen,

> On 05/22/2014 04:20 AM, Lukasz Majewski wrote:
> > Hi Stephen,
> > 
> >> From: Stephen Warren <swarren at nvidia.com>
> >>
> >> DFU read support appears to rely upon dfu->read_medium() updating
> >> the passed-by-reference len parameter to indicate the remaining
> >> size available for reading.
> >>
> >> dfu_read_medium_mmc() never does this, and the implementation of
> >> dfu_read_medium_nand() will only work if called just once; it
> >> hard-codes the value to the total size of the NAND device
> >> irrespective of read offset.
> >>
> >> I believe that overloading dfu->read_medium() is confusing. As
> >> such, this patch introduces a new function dfu->get_medium_size()
> >> which can be used to explicitly find out the medium size, and
> >> nothing else. dfu_read() is modified to use this function to set
> >> the initial value for dfu->r_left, rather than attempting to use
> >> the side-effects of dfu->read_medium() for this purpose.
> >>
> >> Due to this change, dfu_read() must initially set dfu->b_left to 0,
> >> since no data has been read.
> >>
> >> dfu_read_buffer_fill() must also be modified not to adjust
> >> dfu->r_left when simply copying data from dfu->i_buf_start to the
> >> upload request buffer. r_left represents the amount of data left
> >> to be read from HW. That value is not affected by the memcpy(),
> >> but only by calls to dfu->read_medium().
> >>
> >> After this change, I can read from either a 4MB or 1.5MB chunk of a
> >> 4MB eMMC boot partion with CONFIG_SYS_DFU_DATA_BUF_SIZE==1MB.
> >> Without this change, attempting to do that would result in DFU
> >> read returning no data at all due to r_left never being set.
> ...
> > I've tested it with trats2. It introduces regression with my tests
> > setup.
> > 
> > I think that it is the highest time to share my test setup for DFU.
> > Proper patches would be prepared ASAP.
> 
> Ah, your test scripts imply you're testing using files on a
> filesystem, whereas I've only tested with raw MMC partitions:
> 
> setenv dfu_alt_info boot0 raw 0 0x2000 mmcpart 1\;boot1 raw 0 0x2000
> mmcpart 2; dfu 0 mmc 0
> 
> (I test on alt info "boot1")
> 
> Can you re-run your tests on a raw partition and validate they work OK
> for you? That would at least prove the concept of the patches. I'm not
> convinced tests on raw partitions would work with or without my
> patches.

On the mmcparts I've got bootloders, so I would like to avoid erasing
them.

I've tested if raw u-boot can be downloaded and uploaded via DFU. The
u-boot size is 1MiB precisely. 

Corresponding dfu_alt_info entry: 
"u-boot raw 0x80 0x800;" \

SHA1 of the u-boot/master:
f23adc9f219977e603cf057a2704605349f02d36

1. Download (raw):
 
dfu-util -a0 -D u-boot-mmc.bin -> OK
I've double checked it with ums:
dd if=/dev/sdc of=u-boot-mmc.bin_ums skip=128 u-boot-mmc.bin_target
bs=512 count=2048

CRC match => downloading works.


2. Upload (raw):

dfu-util -a0 -U u-boot-mmc.bin_target
It exits immediately and I can only see the file size of 0.

So obviously we have regression here. However, since I didn't covered
this case in my tests I don't know when it was broken.


BTW: I definitely have to extend the test script to handle this case.

> 
> I can see why my patches made files rather than raw partitions fail; I
> only wrote dfu_get_medium_size_mmc() to support raw partitions, so it
> needs extra code to work for files. Solving the same problem for files
> rather than raw partitions might be painful; I guess I'll see!

The problem here is that raw files are far more larger than files to be
stored on the file system (as I've explained below). Moreover it
happens that raw files (like rootfs.img) are even bigger than available
RAM.

> 
> As an aside, mmc_file_op() doesn't seem to support files larger than
> the DFU buffer, size no offset information is ever passed to the
> fatload/fatwrite commands. Is that intentional?

Unfortunately it is, since U-boot's implementation of FAT or Ext4
doesn't support seek(). Due to that the whole file needs to fit into
the buffer before being written.

> 
> I'm also puzzled why the file IO code is part of dfu_mmc.c; commands
> like fat_load (or the internal U-Boot filesystem APIs) support more
> than just MMC.

Initially, the idea was to separate IO to different mediums - that is
why we have dfu_mmc.c, dfu_nand.c, dfu_ram.c. 

Afterwards we had to adjust dfu_mmc to be able to perform ram IO and
other file systems.

The DFU design was rapidly evolving when we started to use it. This
partially explains the current, rather ugly, state of the code.

> Wouldn't it be better to have the following separate
> "backends" to DFU:
> 
> * MMC (raw IO only)
> * NAND (raw IO only)
> * RAM (raw IO only)
> * Filesystem (anything that fs/fat/ext* load/write can support)
> 
> That way, DFU could be applied to e.g. USB mass storage, IDE,
> SATA, ... devices too.

I think that separating the IO even from dfu_mmc.c code would be a good
way to go. 

Could you however, present how would you like to fit it to the current
design?

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group