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

[Lukasz Majewski]

Hi Lukasz,

> 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've tested this again with the newest u-boot-dfu branch and the above 
senario works.

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