dfu: fix some issues with reads/uploads

From: Stephen Warren <[email protected]>

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.

Signed-off-by Stephen Warren <[email protected]>

---
 drivers/dfu/dfu.c      |  9 ++-------
 drivers/dfu/dfu_mmc.c  |  6 ++++++
 drivers/dfu/dfu_nand.c |  7 ++++++-
 drivers/dfu/dfu_ram.c  | 11 ++++++-----
 include/dfu.h          |  2 ++
 5 files changed, 22 insertions(+), 13 deletions(-)

drivers/dfu/dfu.c +1 -6

index a93810934ac9..af97234390c7 100644
@@ -241,7 +241,6 @@ static int dfu_read_buffer_fill(struct dfu_entity
 *dfu, void *buf, int size) dfu->crc = crc32(dfu->crc, buf, chunk);
 			dfu->i_buf += chunk;
 			dfu->b_left -= chunk;
-			dfu->r_left -= chunk;
 			size -= chunk;
 			buf += chunk;
 			readn += chunk;

@@ -287,11 +286,7 @@ int dfu_read(struct dfu_entity *dfu, void *buf,
 int size, int blk_seq_num) if (dfu->i_buf_start == NULL)
 			return -ENOMEM;
 
-		ret = dfu->read_medium(dfu, 0, dfu->i_buf_start,
 &dfu->r_left);
-		if (ret != 0) {
-			debug("%s: failed to get r_left\n",
 __func__);
-			return ret;
-		}
+		dfu->r_left = dfu->get_medium_size(dfu);
 

debug("%s: %s %ld [B]\n", __func__, dfu->name,
dfu->r_left);

drivers/dfu/dfu.c +1 -1

@@ -300,7 +295,7 @@ int dfu_read(struct dfu_entity *dfu, void *buf,
 int size, int blk_seq_num) dfu->offset = 0;
 		dfu->i_buf_end = dfu_get_buf() + dfu_buf_size;
 		dfu->i_buf = dfu->i_buf_start;
-		dfu->b_left = min(dfu_buf_size, dfu->r_left);
+		dfu->b_left = 0;
 
 		dfu->bad_skip = 0;
 

drivers/dfu/dfu_mmc.c +0 -0

index 63cc876612c9..aa077c052f39 100644

drivers/dfu/dfu_mmc.c +5 -0

+{
+	return dfu->data.mmc.lba_size * dfu->data.mmc.lba_blk_size;
+}
+
 int dfu_read_medium_mmc(struct dfu_entity *dfu, u64 offset, void
 *buf, long *len)
 {

@@ -316,6 +321,7 @@ int dfu_fill_entity_mmc(struct dfu_entity *dfu,
 char *s) }
 
 	dfu->dev_type = DFU_DEV_MMC;
+	dfu->get_medium_size = dfu_get_medium_size_mmc;
 	dfu->read_medium = dfu_read_medium_mmc;
 	dfu->write_medium = dfu_write_medium_mmc;
 	dfu->flush_medium = dfu_flush_medium_mmc;

drivers/dfu/dfu_nand.c +0 -0

index ccdbef6b75f2..e1c9a8849246 100644

drivers/dfu/dfu_nand.c +5 -1

+{
+	return dfu->data.nand.size;
+}
+
 static int dfu_read_medium_nand(struct dfu_entity *dfu, u64 offset,
 void *buf, long *len)
 {

@@ -121,7 +126,6 @@ static int dfu_read_medium_nand(struct dfu_entity
 *dfu, u64 offset, void *buf, 
 	switch (dfu->layout) {
 	case DFU_RAW_ADDR:
-		*len = dfu->data.nand.size;
 		ret = nand_block_read(dfu, offset, buf, len);
 		break;
 	default:

@@ -220,6 +224,7 @@ int dfu_fill_entity_nand(struct dfu_entity *dfu,
 char *s) return -1;
 	}
 
+	dfu->get_medium_size = dfu_get_medium_size_nand;
 	dfu->read_medium = dfu_read_medium_nand;
 	dfu->write_medium = dfu_write_medium_nand;
 	dfu->flush_medium = dfu_flush_medium_nand;

drivers/dfu/dfu_ram.c +0 -0

index 335a8e1f2491..b6c6e60c443c 100644

drivers/dfu/dfu_ram.c +5 -5

+{
+	return dfu->data.ram.size;
+}
+
 static int dfu_read_medium_ram(struct dfu_entity *dfu, u64 offset,
 			       void *buf, long *len)
 {
-	if (!*len) {
-		*len = dfu->data.ram.size;
-		return 0;
-	}
-
 	return dfu_transfer_medium_ram(DFU_OP_READ, dfu, offset,
 buf, len); }
 

@@ -69,6 +69,7 @@ int dfu_fill_entity_ram(struct dfu_entity *dfu,
 char *s) dfu->data.ram.size = simple_strtoul(s, &s, 16);
 
 	dfu->write_medium = dfu_write_medium_ram;
+	dfu->get_medium_size = dfu_get_medium_size_ram;
 	dfu->read_medium = dfu_read_medium_ram;
 
 	dfu->inited = 0;

include/dfu.h +0 -0

index 26ffbc8e81d2..0cca601d47af 100644

Hi Stephen,

Quoted 31 lines

From: Stephen Warren <[email protected]>

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.

Quoted 33 lines

Hi Stephen,

Quoted 31 lines

From: Stephen Warren <[email protected]>

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.

...

Quoted 5 lines

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.

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!

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?

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.

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'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;" \

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.

...

Quoted 5 lines

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

...

Quoted 5 lines

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.

Quoted 2 lines

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.

Hmmm. I tested with that exact value of dfu_alt_info and got back an exactly 1MB file with either the dfu-util packaged in Ubuntu 12.10, or with the latest git tree of dfu-util.

Can you investigate why you're
getting back a 0-length file?

Perhaps there's some error condition, so
dfu-util exits before any data is downloaded, or perhaps when dfu_get_medium_size_mmc() is called, either lba_size or lba_blk_size is 0 (neither should be...)

Thanks.

Hi Stephen,

Quoted 16 lines · Lukasz Majewski

...

Quoted 5 lines

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

...

Quoted 7 lines

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.

Quoted 3 lines

Hmmm. I tested with that exact value of dfu_alt_info and got back an exactly 1MB file with either the dfu-util packaged in Ubuntu 12.10, or with the latest git tree of dfu-util.

Could you share the exact SHA1 of the dfu-util? Or is it the master newest branch?

Quoted 18 lines

Hi Stephen,

Quoted 16 lines · Lukasz Majewski

...

Quoted 5 lines

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

...

Quoted 7 lines

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.

Quoted 6 lines

Quoted 3 lines

Hmmm. I tested with that exact value of dfu_alt_info and got back an exactly 1MB file with either the dfu-util packaged in Ubuntu 12.10, or with the latest git tree of dfu-util.

Could you share the exact SHA1 of the dfu-util? Or is it the master newest branch?

Distro package:
dfu-util package 0.5-1 in Ubuntu 12.04.

In the git tree:
25c173e3fd0a dfu_prefix: Rewrite handling of prefix requirements (which was origin/master when I fetched it a week or two ago)

Stephen Warren wrote:

Quoted 8 lines

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)

Quoted 2 lines

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?

Quoted 1 line

Stephen Warren wrote:

...

Quoted 8 lines

Quoted 8 lines

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)

Quoted 8 lines

Quoted 2 lines

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?

Here's how I'd love to see the dfu command work. It would take a bit of refactoring of the code, and a change to the format of the dfu_alt_info variable and "dfu" command parameters. Perhaps we could have an option to the "dfu" command saying which cmdline style the user is using?

dfu_alt_info = setting [; setting]*

setting = mmc_format | nand_format | ram_format | fs_format

mmc_format = "mmc" device_id hwpart_num start_sector sector_count

nand_format = "nand" device_id start_sector sector_count

ram_format = "ram" start_address byte_count

fs_format = "fs" fs_device_type fs_device_id filename

All fs access would be through the generic filesystem API in fs/fs.c rather than by using run_command(). The "fs_device_type", "fs_device_id", and "filename" parameters in "fs_format" above would simply be passed directly to the fs.c APIs without any interpretation by the DFU code. This would allow automatic support of any memory device type that "load", "ls", ... support. The code also wouldn't have to construct command-lines and then re-parse them; calling functions directly is a lot simpler.

The "dfu" command wouldn't take any device type/ID parameters. Instead, all device IDs would be part of the dfu_alt_info variable. This would allow e.g. eMMC partitions and SPI flash devices to be exposed at the same time, rather than having to somehow co-ordinate executing one dfu command in U-Boot to update the SPI flash, then quitting that, then starting a different dfu command in U-Boot to update some data on eMMC. It's harder to automate that vs. just starting dfu once, then running a single script on the host which writes a bunch of dfu settings in sequence without the need for co-ordination.

I'm not sure how much time I have to actually work on converting the code to work that way though. Since we already have a working non-DFU flashing solution using U-Boot[1], my management sees any work on DFU as a bit redundant. Still, it would be nice to use standard protocols for this...

[1] https://github.com/NVIDIA/tegra-uboot-flasher-scripts

Stephen Warren wrote:

Quoted 12 lines

Here's how I'd love to see the dfu command work. It would take a bit of refactoring of the code, and a change to the format of the dfu_alt_info variable and "dfu" command parameters. Perhaps we could have an option to the "dfu" command saying which cmdline style the user is using?

dfu_alt_info = setting [; setting]*

setting = mmc_format | nand_format | ram_format | fs_format

mmc_format = "mmc" device_id hwpart_num start_sector sector_count

Quoted 5 lines

nand_format = "nand" device_id start_sector sector_count

ram_format = "ram" start_address byte_count

fs_format = "fs" fs_device_type fs_device_id filename

I think that the fs also need fs_device_id and fs_partition.

Hi Stephen,

Quoted 48 lines · Lukasz Majewski

Quoted 34 lines

Hi Stephen,

Quoted 32 lines

From: Stephen Warren <[email protected]>

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.

...

Quoted 5 lines

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

Quoted 6 lines

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

Quoted 7 lines

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.

Quoted 4 lines

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.

Quoted 4 lines

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.

Quoted 10 lines

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?