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

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

[Stephen Warren]

From: Stephen Warren <swarren@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.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 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(-)

diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
index a93810934ac9..af97234390c7 100644
--- a/drivers/dfu/dfu.c
+++ b/drivers/dfu/dfu.c
@@ -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);
 
@@ -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;
 
diff --git a/drivers/dfu/dfu_mmc.c b/drivers/dfu/dfu_mmc.c
index 63cc876612c9..aa077c052f39 100644
--- a/drivers/dfu/dfu_mmc.c
+++ b/drivers/dfu/dfu_mmc.c
@@ -196,6 +196,11 @@ int dfu_flush_medium_mmc(struct dfu_entity *dfu)
 	return ret;
 }
 
+long dfu_get_medium_size_mmc(struct dfu_entity *dfu)
+{
+	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;
diff --git a/drivers/dfu/dfu_nand.c b/drivers/dfu/dfu_nand.c
index ccdbef6b75f2..e1c9a8849246 100644
--- a/drivers/dfu/dfu_nand.c
+++ b/drivers/dfu/dfu_nand.c
@@ -114,6 +114,11 @@ static int dfu_write_medium_nand(struct dfu_entity *dfu,
 	return ret;
 }
 
+long dfu_get_medium_size_nand(struct dfu_entity *dfu)
+{
+	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;
diff --git a/drivers/dfu/dfu_ram.c b/drivers/dfu/dfu_ram.c
index 335a8e1f2491..b6c6e60c443c 100644
--- a/drivers/dfu/dfu_ram.c
+++ b/drivers/dfu/dfu_ram.c
@@ -41,14 +41,14 @@ static int dfu_write_medium_ram(struct dfu_entity *dfu, u64 offset,
 	return dfu_transfer_medium_ram(DFU_OP_WRITE, dfu, offset, buf, len);
 }
 
+long dfu_get_medium_size_ram(struct dfu_entity *dfu)
+{
+	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;
diff --git a/include/dfu.h b/include/dfu.h
index 26ffbc8e81d2..0cca601d47af 100644
--- a/include/dfu.h
+++ b/include/dfu.h
@@ -96,6 +96,8 @@ struct dfu_entity {
 		struct ram_internal_data ram;
 	} data;
 
+	long (*get_medium_size)(struct dfu_entity *dfu);
+
 	int (*read_medium)(struct dfu_entity *dfu,
 			u64 offset, void *buf, long *len);
 
-- 
1.8.1.5

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

[Lukasz Majewski]

Hi Stephen,

> From: Stephen Warren <swarren@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.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
>  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(-)
> 
> diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
> index a93810934ac9..af97234390c7 100644
> --- a/drivers/dfu/dfu.c
> +++ b/drivers/dfu/dfu.c
> @@ -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); 
> @@ -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;
>  
> diff --git a/drivers/dfu/dfu_mmc.c b/drivers/dfu/dfu_mmc.c
> index 63cc876612c9..aa077c052f39 100644
> --- a/drivers/dfu/dfu_mmc.c
> +++ b/drivers/dfu/dfu_mmc.c
> @@ -196,6 +196,11 @@ int dfu_flush_medium_mmc(struct dfu_entity *dfu)
>  	return ret;
>  }
>  
> +long dfu_get_medium_size_mmc(struct dfu_entity *dfu)

Those could be defined as static

> +{
> +	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;
> diff --git a/drivers/dfu/dfu_nand.c b/drivers/dfu/dfu_nand.c
> index ccdbef6b75f2..e1c9a8849246 100644
> --- a/drivers/dfu/dfu_nand.c
> +++ b/drivers/dfu/dfu_nand.c
> @@ -114,6 +114,11 @@ static int dfu_write_medium_nand(struct
> dfu_entity *dfu, return ret;
>  }
>  
> +long dfu_get_medium_size_nand(struct dfu_entity *dfu)

Those could be defined as static

> +{
> +	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;
> diff --git a/drivers/dfu/dfu_ram.c b/drivers/dfu/dfu_ram.c
> index 335a8e1f2491..b6c6e60c443c 100644
> --- a/drivers/dfu/dfu_ram.c
> +++ b/drivers/dfu/dfu_ram.c
> @@ -41,14 +41,14 @@ static int dfu_write_medium_ram(struct dfu_entity
> *dfu, u64 offset, return dfu_transfer_medium_ram(DFU_OP_WRITE, dfu,
> offset, buf, len); }
>  
> +long dfu_get_medium_size_ram(struct dfu_entity *dfu)

Those could be defined as static

> +{
> +	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;
> diff --git a/include/dfu.h b/include/dfu.h
> index 26ffbc8e81d2..0cca601d47af 100644
> --- a/include/dfu.h
> +++ b/include/dfu.h
> @@ -96,6 +96,8 @@ struct dfu_entity {
>  		struct ram_internal_data ram;
>  	} data;
>  
> +	long (*get_medium_size)(struct dfu_entity *dfu);
> +
>  	int (*read_medium)(struct dfu_entity *dfu,
>  			u64 offset, void *buf, long *len);
>  

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.

-- 
Best regards,

Lukasz Majewski

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

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

[Stephen Warren]

On 05/22/2014 04:20 AM, Lukasz Majewski wrote:
> Hi Stephen,
> 
>> From: Stephen Warren <swarren@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.

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.

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

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

[Stephen Warren]

On 05/30/2014 02:28 AM, Lukasz Majewski wrote:
...
> 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.

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.

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

[Lukasz Majewski]

Hi Stephen,

> On 05/30/2014 02:28 AM, Lukasz Majewski wrote:
> ...
> > 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.
> 
> 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?

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

This is really strange. I will look into that.

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

I will check this. Thanks for support.

> Thanks.



-- 
Best regards,

Lukasz Majewski

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

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

[Stephen Warren]

On 06/02/2014 12:14 AM, Lukasz Majewski wrote:
> Hi Stephen,
> 
>> On 05/30/2014 02:28 AM, Lukasz Majewski wrote:
>> ...
>>> 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.
>>
>> 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)

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

[Lukasz Majewski]

Hi Stephen,

> On 06/02/2014 12:14 AM, Lukasz Majewski wrote:
> > Hi Stephen,
> > 
> >> On 05/30/2014 02:28 AM, Lukasz Majewski wrote:
> >> ...
> >>> 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.
> >>
> >> 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)

I've updated the dfu-util to be the newest master.

commit fc81c6cc4eba30eaadf0010deb3d38f3be93ecd1
Author: Michael Grzeschik <mgr@pengutronix.de>
Date:   Tue May 13 23:55:21 2014 +0200

I still read only zero length file:

lukma at AMDC2363:~/work/u-boot-denx(master)$ dfu-util -a0 -U
u-boot-mmc.bin_read dfu-util 0.7

Copyright 2005-2008 Weston Schmidt, Harald Welte and OpenMoko Inc.
Copyright 2010-2012 Tormod Volden and Stefan Schmidt
This program is Free Software and has ABSOLUTELY NO WARRANTY
Please report bugs to dfu-util at lists.gnumonks.org

Opening DFU capable USB device...
ID 04e8:6601
Run-time device DFU version 0110
Claiming USB DFU Interface...
Setting Alternate Setting #0 ...
Determining device status: state = dfuIDLE, status = 0
dfuIDLE, continuing
DFU mode device DFU version 0110
Device returned transfer size 4096
Copying data from DFU device to PC
Upload  [                         ]   0%            0 bytes
Failed.

Regarding settings, I assume that they should not be changed, since
downloading the same data works.

I will look into that.

-- 
Best regards,

Lukasz Majewski

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

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

[Stephen Warren]

On 05/30/2014 02:28 AM, Lukasz Majewski wrote:
> Stephen Warren wrote:
...
>> 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?

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

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

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

FIXME: reword after updates to use "*size" command for fs...

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 <swarren@nvidia.com>
---
v2:
* Fix dfu_get_medium_size_mmc() to handle DFU_FS_FAT/EXT5 layouts too.
* Use "size" command to retrieve file size.
---
 drivers/dfu/dfu.c      | 19 +++++++++++-----
 drivers/dfu/dfu_mmc.c  | 59 ++++++++++++++++++++++++++++++++++++++++++--------
 drivers/dfu/dfu_nand.c |  7 +++++-
 drivers/dfu/dfu_ram.c  | 11 +++++-----
 include/dfu.h          |  3 +++
 5 files changed, 78 insertions(+), 21 deletions(-)

diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
index a93810934ac9..4b45dafc8442 100644
--- a/drivers/dfu/dfu.c
+++ b/drivers/dfu/dfu.c
@@ -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,10 +286,18 @@ 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);
+		if (dfu->r_left < 0)
+			return dfu->r_left;
+		switch (dfu->layout) {
+		case DFU_RAW_ADDR:
+		case DFU_RAM_ADDR:
+			break;
+		default:
+			if (dfu->r_left >= dfu_buf_size) {
+				printf("%s: File too big for buf\b", __func__);
+				return -EOVERFLOW;
+			}
 		}
 
 		debug("%s: %s %ld [B]\n", __func__, dfu->name, dfu->r_left);
@@ -300,7 +307,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;
 
diff --git a/drivers/dfu/dfu_mmc.c b/drivers/dfu/dfu_mmc.c
index 63cc876612c9..322bd8c5d2de 100644
--- a/drivers/dfu/dfu_mmc.c
+++ b/drivers/dfu/dfu_mmc.c
@@ -12,6 +12,8 @@
 #include <errno.h>
 #include <div64.h>
 #include <dfu.h>
+#include <ext4fs.h>
+#include <fat.h>
 #include <mmc.h>
 
 static unsigned char __aligned(CONFIG_SYS_CACHELINE_SIZE)
@@ -113,22 +115,17 @@ static int mmc_file_buffer(struct dfu_entity *dfu, void *buf, long *len)
 static int mmc_file_op(enum dfu_op op, struct dfu_entity *dfu,
 			void *buf, long *len)
 {
+	const char *fsname, *opname;
 	char cmd_buf[DFU_CMD_BUF_SIZE];
 	char *str_env;
 	int ret;
 
 	switch (dfu->layout) {
 	case DFU_FS_FAT:
-		sprintf(cmd_buf, "fat%s mmc %d:%d 0x%x %s",
-			op == DFU_OP_READ ? "load" : "write",
-			dfu->data.mmc.dev, dfu->data.mmc.part,
-			(unsigned int) buf, dfu->name);
+		fsname = "fat";
 		break;
 	case DFU_FS_EXT4:
-		sprintf(cmd_buf, "ext4%s mmc %d:%d 0x%x /%s",
-			op == DFU_OP_READ ? "load" : "write",
-			dfu->data.mmc.dev, dfu->data.mmc.part,
-			(unsigned int) buf, dfu->name);
+		fsname = "ext4";
 		break;
 	default:
 		printf("%s: Layout (%s) not (yet) supported!\n", __func__,
@@ -136,6 +133,28 @@ static int mmc_file_op(enum dfu_op op, struct dfu_entity *dfu,
 		return -1;
 	}
 
+	switch (op) {
+	case DFU_OP_READ:
+		opname = "load";
+		break;
+	case DFU_OP_WRITE:
+		opname = "write";
+		break;
+	case DFU_OP_SIZE:
+		opname = "size";
+		break;
+	default:
+		return -1;
+	}
+
+	sprintf(cmd_buf, "%s%s mmc %d:%d", fsname, opname,
+		dfu->data.mmc.dev, dfu->data.mmc.part);
+
+	if (op != DFU_OP_SIZE)
+		sprintf(cmd_buf + strlen(cmd_buf), " 0x%x", (unsigned int)buf);
+
+	sprintf(cmd_buf + strlen(cmd_buf), " %s", dfu->name);
+
 	if (op == DFU_OP_WRITE)
 		sprintf(cmd_buf + strlen(cmd_buf), " %lx", *len);
 
@@ -147,7 +166,7 @@ static int mmc_file_op(enum dfu_op op, struct dfu_entity *dfu,
 		return ret;
 	}
 
-	if (dfu->layout != DFU_RAW_ADDR && op == DFU_OP_READ) {
+	if (op != DFU_OP_WRITE) {
 		str_env = getenv("filesize");
 		if (str_env == NULL) {
 			puts("dfu: Wrong file size!\n");
@@ -196,6 +215,27 @@ int dfu_flush_medium_mmc(struct dfu_entity *dfu)
 	return ret;
 }
 
+long dfu_get_medium_size_mmc(struct dfu_entity *dfu)
+{
+	int ret;
+	long len;
+
+	switch (dfu->layout) {
+	case DFU_RAW_ADDR:
+		return dfu->data.mmc.lba_size * dfu->data.mmc.lba_blk_size;
+	case DFU_FS_FAT:
+	case DFU_FS_EXT4:
+		ret = mmc_file_op(DFU_OP_SIZE, dfu, NULL, &len);
+		if (ret < 0)
+			return ret;
+		return len;
+	default:
+		printf("%s: Layout (%s) not (yet) supported!\n", __func__,
+		       dfu_get_layout(dfu->layout));
+		return -1;
+	}
+}
+
 int dfu_read_medium_mmc(struct dfu_entity *dfu, u64 offset, void *buf,
 		long *len)
 {
@@ -316,6 +356,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;
diff --git a/drivers/dfu/dfu_nand.c b/drivers/dfu/dfu_nand.c
index ccdbef6b75f2..e1c9a8849246 100644
--- a/drivers/dfu/dfu_nand.c
+++ b/drivers/dfu/dfu_nand.c
@@ -114,6 +114,11 @@ static int dfu_write_medium_nand(struct dfu_entity *dfu,
 	return ret;
 }
 
+long dfu_get_medium_size_nand(struct dfu_entity *dfu)
+{
+	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;
diff --git a/drivers/dfu/dfu_ram.c b/drivers/dfu/dfu_ram.c
index 335a8e1f2491..b6c6e60c443c 100644
--- a/drivers/dfu/dfu_ram.c
+++ b/drivers/dfu/dfu_ram.c
@@ -41,14 +41,14 @@ static int dfu_write_medium_ram(struct dfu_entity *dfu, u64 offset,
 	return dfu_transfer_medium_ram(DFU_OP_WRITE, dfu, offset, buf, len);
 }
 
+long dfu_get_medium_size_ram(struct dfu_entity *dfu)
+{
+	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;
diff --git a/include/dfu.h b/include/dfu.h
index 26ffbc8e81d2..df720310f2cc 100644
--- a/include/dfu.h
+++ b/include/dfu.h
@@ -35,6 +35,7 @@ enum dfu_layout {
 enum dfu_op {
 	DFU_OP_READ = 1,
 	DFU_OP_WRITE,
+	DFU_OP_SIZE,
 };
 
 struct mmc_internal_data {
@@ -96,6 +97,8 @@ struct dfu_entity {
 		struct ram_internal_data ram;
 	} data;
 
+	long (*get_medium_size)(struct dfu_entity *dfu);
+
 	int (*read_medium)(struct dfu_entity *dfu,
 			u64 offset, void *buf, long *len);
 
-- 
1.8.1.5

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

[Stephen Warren]

On 06/10/2014 03:25 PM, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> FIXME: reword after updates to use "*size" command for fs...
> 
> 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.

Crap, ignore this. I passed the wrong commit ID to git format-patch and
didn't notice:-(

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

[Lukasz Majewski]

Hi Stephen,

> On 05/30/2014 02:28 AM, Lukasz Majewski wrote:
> > Stephen Warren wrote:
> ...
> >> 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?
> 
> 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

I think that the fs also need fs_device_id and fs_partition.

> 

DFU rework and as a result restructuring the dfu_alt_info is a pending
work for some time. 

Your idea sounds very appealing.

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

Yes, definitely.

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


-- 
Best regards,

Lukasz Majewski

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

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

[Stephen Warren]

On 06/12/2014 01:17 AM, Lukasz Majewski wrote:
> Stephen Warren wrote:
...
>> 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
> 
> I think that the fs also need fs_device_id and fs_partition.

I would expect fs_device_id to include the partition information. Shell
commands such as ls and load use the following format for the device ID:

controller_id[.hw_partition][:sw_partition]

(with support for the .hw_partition field being part of a patch I sent
to get_device_and_partition() within the last few weeks). Hence, we
don't need separate fields for this. This proposal would unify the way
ls/load/dfu specified devices.

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