dfu: Several enhancements for dfu subsystem

Before this patch it was only possible to access only the default eMMC partition. By partition selection I mean the access to eMMC via ext_csd[179] register programming.

It sometimes happens that it is necessary to write to other partitions. This patch adds extra attributes to "mmc" sub type of the dfu_alt_info variable (e.g. boot-mmc.bin mmc 0 200 mmcpart 1;)

It saves the original boot value and restores it after storing the file.

Such definition will not impact other definitions of the "mmc" dfu partitions specifier.

Change-Id I34069510eb27aa80794189d2d13cdb97e54ba83d

Signed-off-by Lukasz Majewski <[email protected]>

Quoted 11 lines

Before this patch it was only possible to access only the default eMMC partition. By partition selection I mean the access to eMMC via ext_csd[179] register programming.

It sometimes happens that it is necessary to write to other partitions. This patch adds extra attributes to "mmc" sub type of the dfu_alt_info variable (e.g. boot-mmc.bin mmc 0 200 mmcpart 1;)

It saves the original boot value and restores it after storing the file.

Quoted 5 lines

Such definition will not impact other definitions of the "mmc" dfu partitions specifier.

Change-Id I34069510eb27aa80794189d2d13cdb97e54ba83d

Signed-off-by Lukasz Majewski <[email protected]>

Please strip the Change-Id .

Best regards,
Marek Vasut

Before this patch it was only possible to access the default eMMC HW partition. By partition selection I mean the access to eMMC via the ext_csd[179] register programming.

It sometimes happens that it is necessary to write to other partitions. This patch adds extra attribute to "raw" sub type of the dfu_alt_info environment variable (e.g. boot-mmc.bin raw 0x0 0x200 mmcpart 1;)

It saves the original boot value and restores it after storing the file.

From: Przemyslaw Marczak <[email protected]>

Thanks to this multiple calls of function dfu_config_entities() give continuous dfu alt numbering until call dfu_free_entities().

This allows to store dfu entities in multiple env variables.

Change-Id Ibca7e785bfca9f53b64d3dff0490185b06841b54

Signed-off-by Przemyslaw Marczak <[email protected]>

---
 drivers/dfu/dfu.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

drivers/dfu/dfu.c +0 -0

index f94c412..e15f959 100644

Quoted 13 lines

From: Przemyslaw Marczak <[email protected]>

Thanks to this multiple calls of function dfu_config_entities() give continuous dfu alt numbering until call dfu_free_entities().

This allows to store dfu entities in multiple env variables.

Change-Id Ibca7e785bfca9f53b64d3dff0490185b06841b54

Signed-off-by Przemyslaw Marczak <[email protected]>

---
 drivers/dfu/dfu.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

drivers/dfu/dfu.c line 22

Quoted 9 lines

drivers/dfu/dfu.c +1 -0

index f94c412..e15f959 100644
@@ -19,6 +19,7 @@
 static bool dfu_reset_request;
 static LIST_HEAD(dfu_list);
 static int dfu_alt_num;
+static int alt_num_count;

Can the variable name here be any less consistent with the rest ... ? ;-)

[...]

Best regards,
Marek Vasut

Hi Marek,

Quoted 14 lines · Lukasz Majewski

Quoted 13 lines

From: Przemyslaw Marczak <[email protected]>

Thanks to this multiple calls of function dfu_config_entities() give continuous dfu alt numbering until call dfu_free_entities().

This allows to store dfu entities in multiple env variables.

Change-Id Ibca7e785bfca9f53b64d3dff0490185b06841b54

Signed-off-by Przemyslaw Marczak <[email protected]>

---
  drivers/dfu/dfu.c |    6 +++++-
  1 file changed, 5 insertions(+), 1 deletion(-)

Quoted 12 lines

drivers/dfu/dfu.c line 22

Quoted 9 lines

drivers/dfu/dfu.c +1 -0

index f94c412..e15f959 100644
@@ -19,6 +19,7 @@
  static bool dfu_reset_request;
  static LIST_HEAD(dfu_list);
  static int dfu_alt_num;
+static int alt_num_count;

Can the variable name here be any less consistent with the rest ... ? ;-)

I think, that something like dfu_alt_num_cnt would fit better there.

Quoted 5 lines

[...]

Best regards,
Marek Vasut

Hello,
This code was applied to u-boot-samsung few weeks ago.

Quoted 32 lines

Hi Marek,

Quoted 29 lines · Lukasz Majewski

Quoted 19 lines

From: Przemyslaw Marczak <[email protected]>

Thanks to this multiple calls of function dfu_config_entities() give continuous dfu alt numbering until call dfu_free_entities().

This allows to store dfu entities in multiple env variables.

Change-Id Ibca7e785bfca9f53b64d3dff0490185b06841b54

Signed-off-by Przemyslaw Marczak <[email protected]>

---

  drivers/dfu/dfu.c |    6 +++++-
  1 file changed, 5 insertions(+), 1 deletion(-)

drivers/dfu/dfu.c +0 -0

index f94c412..e15f959 100644

drivers/dfu/dfu.c line 24

Quoted 6 lines

drivers/dfu/dfu.c +1 -0

@@ -19,6 +19,7 @@
 
  static bool dfu_reset_request;
  static LIST_HEAD(dfu_list);
  static int dfu_alt_num;
 
+static int alt_num_count;

Can the variable name here be any less consistent with the rest ... ? ;-)

Quoted 6 lines

I think, that something like dfu_alt_num_cnt would fit better there.

Quoted 4 lines

[...]

Best regards,
Marek Vasut

Thanks

Quoted 2 lines

Hello,
This code was applied to u-boot-samsung few weeks ago.

Please do NOT top-post. Why did it arrive in the ML yesterday ?

Quoted 38 lines · Lukasz Majewski

Quoted 36 lines

Hi Marek,

Quoted 22 lines · Lukasz Majewski

Quoted 21 lines

From: Przemyslaw Marczak <[email protected]>

Thanks to this multiple calls of function dfu_config_entities() give continuous dfu alt numbering until call dfu_free_entities().

This allows to store dfu entities in multiple env variables.

Change-Id Ibca7e785bfca9f53b64d3dff0490185b06841b54

Signed-off-by Przemyslaw Marczak <[email protected]>

---

  drivers/dfu/dfu.c |    6 +++++-
  1 file changed, 5 insertions(+), 1 deletion(-)

drivers/dfu/dfu.c +0 -0

index f94c412..e15f959 100644
@@ -19,6 +19,7 @@
 

Quoted 8 lines

drivers/dfu/dfu.c

Quoted 5 lines

drivers/dfu/dfu.c +1 -0

  static bool dfu_reset_request;
  static LIST_HEAD(dfu_list);
  static int dfu_alt_num;
 
+static int alt_num_count;

Can the variable name here be any less consistent with the rest ... ? ;-)

I think, that something like dfu_alt_num_cnt would fit better there.

Quoted 6 lines

Quoted 4 lines

Quoted 4 lines

[...]

Best regards,
Marek Vasut

Thanks

Best regards,
Marek Vasut

Up till now the CRC32 of received data was calculated unconditionally. The standard crc32 implementation causes long delays when large images were uploaded.

Quoted 3 lines

Up till now the CRC32 of received data was calculated unconditionally. The standard crc32 implementation causes long delays when large images were uploaded.

You might want to check common/cmd_hash.c and include/hash.h for the hash_command() call. It does the resolution of the hash algorithm from it's name and you can operate also SHA1 and SHA256 with it. It would be nice if you could just extend it a bit and use that instead of adding another ad-hoc mechanism.

Do you think it'd be possible to reuse it please ?

Best regards,
Marek Vasut

Hi Marek,

Quoted 5 lines · Lukasz Majewski

Quoted 3 lines

Up till now the CRC32 of received data was calculated unconditionally. The standard crc32 implementation causes long delays when large images were uploaded.

Quoted 7 lines

You might want to check common/cmd_hash.c and include/hash.h for the hash_command() call. It does the resolution of the hash algorithm from it's name and you can operate also SHA1 and SHA256 with it. It would be nice if you could just extend it a bit and use that instead of adding another ad-hoc mechanism.

Do you think it'd be possible to reuse it please ?

I think, that crc32 shall be calculated when needed. That is why I've added a dfu_ckecksum_method variable.

With its help it is now possible to use different algorithms for checking - not only crc32 (which in u-boot is the default and painfully slow implementation).

In the future the code:
if (dfu_checksum_method == DFU_CRC32)

	crc32 calculation;

will be changed to:

switch (dfu_checksum_method) {
	case CRC32:
		crc32 calculation;
		break;
	case SHA1:
		sha1 calculation;
		break;
	case MD5:
		md5 calculation;
		break;
}

Moreover it is possible to dynamically change the checksum method (between invoking dfu command) via adjusting "dfu_checksum_method" variable.

The default approach is to not calculate anything.

Quoted 44 lines

Hi Marek,

Quoted 5 lines · Lukasz Majewski

Quoted 3 lines

Up till now the CRC32 of received data was calculated unconditionally. The standard crc32 implementation causes long delays when large images were uploaded.

Quoted 7 lines

You might want to check common/cmd_hash.c and include/hash.h for the hash_command() call. It does the resolution of the hash algorithm from it's name and you can operate also SHA1 and SHA256 with it. It would be nice if you could just extend it a bit and use that instead of adding another ad-hoc mechanism.

Do you think it'd be possible to reuse it please ?

I think, that crc32 shall be calculated when needed. That is why I've added a dfu_ckecksum_method variable.

With its help it is now possible to use different algorithms for checking - not only crc32 (which in u-boot is the default and painfully slow implementation).

In the future the code:
if (dfu_checksum_method == DFU_CRC32)

	crc32 calculation;

will be changed to:

switch (dfu_checksum_method) {
	case CRC32:
		crc32 calculation;
		break;
	case SHA1:
		sha1 calculation;
		break;
	case MD5:
		md5 calculation;
		break;
}

Moreover it is possible to dynamically change the checksum method (between invoking dfu command) via adjusting "dfu_checksum_method" variable.

Quoted 1 line

The default approach is to not calculate anything.

I get it, but the direct calling of crc32() function can be abstracted already with the hash_command() now. You won't need to the above switch in such case. Also, you can implement a NULL hash algo, which would effectivelly model the case where you want to disable hashing.

Best regards,
Marek Vasut

Up till now the CRC32 of received data was calculated unconditionally. The standard crc32 implementation causes long delays when large images were uploaded.

The "dfu_checksum_method" environment variable gives the opportunity to enable on demand (when e.g. debugging) the crc32 calculation. It can be done without need to recompile the u-boot binary.

By default the crc32 is not calculated.

Tests results:
400 MiB ums.img file
With crc32 calculation: 65 sec [avg 6.29 MB/s]
Without crc32 calculation: 25 sec [avg 16.17 MB/s]

Signed-off-by Lukasz Majewski <[email protected]>

---
drivers/dfu/dfu.c | 34 ++++++++++++++++++++++++++++++----
include/dfu.h | 5 +++++

2 files changed, 35 insertions(+), 4 deletions(-)

drivers/dfu/dfu.c +30 -4

index e15f959..5d50b47 100644
@@ -20,6 +20,7 @@ static bool dfu_reset_request;
 static LIST_HEAD(dfu_list);
 static int dfu_alt_num;
 static int alt_num_count;
+static int dfu_checksum_method;
 
 bool dfu_reset(void)
 {

@@ -99,6 +100,23 @@ unsigned char *dfu_get_buf(void)
 	return dfu_buf;
 }
 
+static int dfu_get_checksum_method(void)
+{
+	char *s;
+
+	s = getenv("dfu_checksum_method");
+	if (!s)
+		return DFU_NO_CHECKSUM;
+
+	if (!strcmp(s, "crc32")) {
+		debug("%s: DFU checksum method: %s\n", __func__, s);
+		return DFU_CRC32;
+	} else {
+		error("DFU checksum method: %s not supported!\n", s);
+		return -EINVAL;
+	}
+}
+
 static int dfu_write_buffer_drain(struct dfu_entity *dfu)
 {
 	long w_size;

@@ -109,8 +127,8 @@ static int dfu_write_buffer_drain(struct dfu_entity *dfu)
 	if (w_size == 0)
 		return 0;
 
-	/* update CRC32 */
-	dfu->crc = crc32(dfu->crc, dfu->i_buf_start, w_size);
+	if (dfu_checksum_method == DFU_CRC32)
+		dfu->crc = crc32(dfu->crc, dfu->i_buf_start, w_size);
 
 	ret = dfu->write_medium(dfu, dfu->offset, dfu->i_buf_start, &w_size);
 	if (ret)

@@ -234,7 +252,8 @@ static int dfu_read_buffer_fill(struct dfu_entity *dfu, void *buf, int size)
 		/* consume */
 		if (chunk > 0) {
 			memcpy(buf, dfu->i_buf, chunk);
-			dfu->crc = crc32(dfu->crc, buf, chunk);
+			if (dfu_checksum_method == DFU_CRC32)
+				dfu->crc = crc32(dfu->crc, buf, chunk);
 			dfu->i_buf += chunk;
 			dfu->b_left -= chunk;
 			dfu->r_left -= chunk;

@@ -318,7 +337,9 @@ int dfu_read(struct dfu_entity *dfu, void *buf, int size, int blk_seq_num)
 	}
 
 	if (ret < size) {
-		debug("%s: %s CRC32: 0x%x\n", __func__, dfu->name, dfu->crc);
+		if (dfu_checksum_method == DFU_CRC32)
+			debug("%s: %s CRC32: 0x%x\n", __func__, dfu->name,
+			      dfu->crc);
 		puts("\nUPLOAD ... done\nCtrl+C to exit ...\n");
 
 		dfu_free_buf();

@@ -393,6 +414,11 @@ int dfu_config_entities(char *env, char *interface, int num)
 	dfu_alt_num = dfu_find_alt_num(env);
 	debug("%s: dfu_alt_num=%d\n", __func__, dfu_alt_num);
 
+	ret = dfu_get_checksum_method();
+	if (ret < 0)
+		return ret;
+	dfu_checksum_method = ret;
+
 	dfu = calloc(sizeof(*dfu), dfu_alt_num);
 	if (!dfu)
 		return -1;

include/dfu.h +5 -0

index 751f0fd..855d6dc 100644
@@ -37,6 +37,11 @@ enum dfu_op {
 	DFU_OP_WRITE,
 };
 
+enum dfu_checksum {
+	DFU_NO_CHECKSUM = 0,
+	DFU_CRC32,
+};
+
 #define DFU_NOT_SUPPORTED -1
 
 struct mmc_internal_data {

Signature · 1 line

1.7.10.4

Hi Lukasz,

Quoted 9 lines · Lukasz Majewski

Up till now the CRC32 of received data was calculated unconditionally. The standard crc32 implementation causes long delays when large images were uploaded.

The "dfu_checksum_method" environment variable gives the opportunity to enable on demand (when e.g. debugging) the crc32 calculation. It can be done without need to recompile the u-boot binary.

Quoted 7 lines

By default the crc32 is not calculated.

Tests results:
400 MiB ums.img file
With crc32 calculation: 65 sec [avg 6.29 MB/s]
Without crc32 calculation: 25 sec [avg 16.17 MB/s]

That's interesting; I'm surprised that there's so much difference. Can we get some info about the environment? I.e. board, whether cache is enabled etc.

The crc table is per byte and I guess lookups maybe expensive.

Regards

-- Pantelis

Quoted 110 lines

Signed-off-by Lukasz Majewski <[email protected]>

---
drivers/dfu/dfu.c | 34 ++++++++++++++++++++++++++++++----
include/dfu.h | 5 +++++

2 files changed, 35 insertions(+), 4 deletions(-)

drivers/dfu/dfu.c +18 -0

index e15f959..5d50b47 100644
@@ -20,6 +20,7 @@ static bool dfu_reset_request;
 static LIST_HEAD(dfu_list);
 static int dfu_alt_num;
 static int alt_num_count;
+static int dfu_checksum_method;
 
 bool dfu_reset(void)
 {

@@ -99,6 +100,23 @@ unsigned char *dfu_get_buf(void)
 	return dfu_buf;
 }
 
+static int dfu_get_checksum_method(void)
+{
+	char *s;
+
+	s = getenv("dfu_checksum_method");
+	if (!s)
+		return DFU_NO_CHECKSUM;
+
+	if (!strcmp(s, "crc32")) {
+		debug("%s: DFU checksum method: %s\n", __func__,
 s);
+		return DFU_CRC32;
+	} else {
+		error("DFU checksum method: %s not supported!\n",
 s);
+		return -EINVAL;
+	}
+}
+
 static int dfu_write_buffer_drain(struct dfu_entity *dfu)

{
	long w_size;

drivers/dfu/dfu.c +2 -2

@@ -109,8 +127,8 @@ static int dfu_write_buffer_drain(struct
 dfu_entity *dfu) if (w_size == 0)
 		return 0;
 
-	/* update CRC32 */
-	dfu->crc = crc32(dfu->crc, dfu->i_buf_start, w_size);
+	if (dfu_checksum_method == DFU_CRC32)
+		dfu->crc = crc32(dfu->crc, dfu->i_buf_start,
 w_size);
 
 	ret = dfu->write_medium(dfu, dfu->offset, dfu->i_buf_start,

&w_size); if (ret)

drivers/dfu/dfu.c +0 -0

@@ -234,7 +252,8 @@ static int dfu_read_buffer_fill(struct

dfu_entity *dfu, void *buf, int size) /* consume */

		if (chunk > 0) {
			memcpy(buf, dfu->i_buf, chunk);

drivers/dfu/dfu.c +2 -1

-			dfu->crc = crc32(dfu->crc, buf, chunk);
+			if (dfu_checksum_method == DFU_CRC32)
+				dfu->crc = crc32(dfu->crc, buf,

chunk); dfu->i_buf += chunk;
			dfu->b_left -= chunk;
			dfu->r_left -= chunk;

drivers/dfu/dfu.c +3 -1

@@ -318,7 +337,9 @@ int dfu_read(struct dfu_entity *dfu, void *buf,
 int size, int blk_seq_num) }
 
 	if (ret < size) {
-		debug("%s: %s CRC32: 0x%x\n", __func__, dfu->name,
 dfu->crc);
+		if (dfu_checksum_method == DFU_CRC32)
+			debug("%s: %s CRC32: 0x%x\n", __func__,
 dfu->name,
+			      dfu->crc);
 		puts("\nUPLOAD ... done\nCtrl+C to exit ...\n");

		dfu_free_buf();

drivers/dfu/dfu.c +5 -0

@@ -393,6 +414,11 @@ int dfu_config_entities(char *env, char
 *interface, int num) dfu_alt_num = dfu_find_alt_num(env);
 	debug("%s: dfu_alt_num=%d\n", __func__, dfu_alt_num);
 
+	ret = dfu_get_checksum_method();
+	if (ret < 0)
+		return ret;
+	dfu_checksum_method = ret;
+
 	dfu = calloc(sizeof(*dfu), dfu_alt_num);
 	if (!dfu)
 		return -1;

include/dfu.h +5 -0

index 751f0fd..855d6dc 100644
@@ -37,6 +37,11 @@ enum dfu_op {
 	DFU_OP_WRITE,
 };
 
+enum dfu_checksum {
+	DFU_NO_CHECKSUM = 0,
+	DFU_CRC32,
+};
+
 #define DFU_NOT_SUPPORTED -1
 
 struct mmc_internal_data {

Signature · 1 line

1.7.10.4

Hi Pantelis,

Quoted 21 lines

Hi Lukasz,

Quoted 9 lines · Lukasz Majewski

Up till now the CRC32 of received data was calculated unconditionally. The standard crc32 implementation causes long delays when large images were uploaded.

The "dfu_checksum_method" environment variable gives the opportunity to enable on demand (when e.g. debugging) the crc32 calculation. It can be done without need to recompile the u-boot binary.

Quoted 7 lines

By default the crc32 is not calculated.

Tests results:
400 MiB ums.img file
With crc32 calculation: 65 sec [avg 6.29 MB/s]
Without crc32 calculation: 25 sec [avg 16.17 MB/s]

Quoted 3 lines

That's interesting; I'm surprised that there's so much difference. Can we get some info about the environment? I.e. board, whether cache is enabled etc.

Board Exynos4412 - Trats2. Cache L1 enabled, cache L2 disabled.

Crc32 is calculated for 32 MiB chunks of data in the received buffer. I'm using the standard software crc32 u-boot's implementation. It is the same as the one for perl-crc32 debian package.

Quoted 3 lines

The crc table is per byte and I guess lookups maybe expensive.

It seems so. Moreover the Exynos4412 has HW crypto engine which supports SHA1, MD5 and other algorithms. Unfortunately it doesn't provide speedup for crc32.

Quoted 116 lines

Regards

-- Pantelis

Quoted 110 lines

Signed-off-by Lukasz Majewski <[email protected]>

---
drivers/dfu/dfu.c | 34 ++++++++++++++++++++++++++++++----
include/dfu.h | 5 +++++

2 files changed, 35 insertions(+), 4 deletions(-)

drivers/dfu/dfu.c +18 -0

index e15f959..5d50b47 100644
@@ -20,6 +20,7 @@ static bool dfu_reset_request;
 static LIST_HEAD(dfu_list);
 static int dfu_alt_num;
 static int alt_num_count;
+static int dfu_checksum_method;
 
 bool dfu_reset(void)
 {

@@ -99,6 +100,23 @@ unsigned char *dfu_get_buf(void)
 	return dfu_buf;
 }
 
+static int dfu_get_checksum_method(void)
+{
+	char *s;
+
+	s = getenv("dfu_checksum_method");
+	if (!s)
+		return DFU_NO_CHECKSUM;
+
+	if (!strcmp(s, "crc32")) {
+		debug("%s: DFU checksum method: %s\n", __func__,
 s);
+		return DFU_CRC32;
+	} else {
+		error("DFU checksum method: %s not supported!\n",
 s);
+		return -EINVAL;
+	}
+}
+
 static int dfu_write_buffer_drain(struct dfu_entity *dfu)

{
	long w_size;

drivers/dfu/dfu.c +2 -2

@@ -109,8 +127,8 @@ static int dfu_write_buffer_drain(struct
 dfu_entity *dfu) if (w_size == 0)
 		return 0;
 
-	/* update CRC32 */
-	dfu->crc = crc32(dfu->crc, dfu->i_buf_start, w_size);
+	if (dfu_checksum_method == DFU_CRC32)
+		dfu->crc = crc32(dfu->crc, dfu->i_buf_start,
 w_size);
 
 	ret = dfu->write_medium(dfu, dfu->offset, dfu->i_buf_start,

&w_size); if (ret)

drivers/dfu/dfu.c +0 -0

@@ -234,7 +252,8 @@ static int dfu_read_buffer_fill(struct

dfu_entity *dfu, void *buf, int size) /* consume */

		if (chunk > 0) {
			memcpy(buf, dfu->i_buf, chunk);

drivers/dfu/dfu.c +2 -1

-			dfu->crc = crc32(dfu->crc, buf, chunk);
+			if (dfu_checksum_method == DFU_CRC32)
+				dfu->crc = crc32(dfu->crc, buf,

chunk); dfu->i_buf += chunk;
			dfu->b_left -= chunk;
			dfu->r_left -= chunk;

drivers/dfu/dfu.c +3 -1

@@ -318,7 +337,9 @@ int dfu_read(struct dfu_entity *dfu, void *buf,
 int size, int blk_seq_num) }
 
 	if (ret < size) {
-		debug("%s: %s CRC32: 0x%x\n", __func__, dfu->name,
 dfu->crc);
+		if (dfu_checksum_method == DFU_CRC32)
+			debug("%s: %s CRC32: 0x%x\n", __func__,
 dfu->name,
+			      dfu->crc);
 		puts("\nUPLOAD ... done\nCtrl+C to exit ...\n");

		dfu_free_buf();

drivers/dfu/dfu.c +5 -0

@@ -393,6 +414,11 @@ int dfu_config_entities(char *env, char
 *interface, int num) dfu_alt_num = dfu_find_alt_num(env);
 	debug("%s: dfu_alt_num=%d\n", __func__, dfu_alt_num);
 
+	ret = dfu_get_checksum_method();
+	if (ret < 0)
+		return ret;
+	dfu_checksum_method = ret;
+
 	dfu = calloc(sizeof(*dfu), dfu_alt_num);
 	if (!dfu)
 		return -1;

include/dfu.h +5 -0

index 751f0fd..855d6dc 100644
@@ -37,6 +37,11 @@ enum dfu_op {
 	DFU_OP_WRITE,
 };
 
+enum dfu_checksum {
+	DFU_NO_CHECKSUM = 0,
+	DFU_CRC32,
+};
+
 #define DFU_NOT_SUPPORTED -1
 
 struct mmc_internal_data {

Signature · 1 line

1.7.10.4

Signature · 5 lines

Best regards,

Lukasz Majewski

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

Hi Pantelis,

Quoted 21 lines

Hi Lukasz,

Quoted 9 lines · Lukasz Majewski

Up till now the CRC32 of received data was calculated unconditionally. The standard crc32 implementation causes long delays when large images were uploaded.

The "dfu_checksum_method" environment variable gives the opportunity to enable on demand (when e.g. debugging) the crc32 calculation. It can be done without need to recompile the u-boot binary.

Quoted 7 lines

By default the crc32 is not calculated.

Tests results:
400 MiB ums.img file
With crc32 calculation: 65 sec [avg 6.29 MB/s]
Without crc32 calculation: 25 sec [avg 16.17 MB/s]

Quoted 3 lines

That's interesting; I'm surprised that there's so much difference. Can we get some info about the environment? I.e. board, whether cache is enabled etc.

Board Exynos4412 - Trats2. Cache L1 enabled, cache L2 disabled.

Crc32 is calculated for 32 MiB chunks of data in the received buffer. I'm using the standard software crc32 u-boot's implementation. It is the same as the one for perl-crc32 debian package.

Quoted 2 lines

The crc table is per byte and I guess lookups maybe expensive.

It seems so. Moreover the Exynos4412 has HW crypto engine which supports SHA1, MD5 and other algorithms. Unfortunately it doesn't provide speedup for crc32.

Quoted 116 lines

Regards

-- Pantelis

Quoted 110 lines

Signed-off-by Lukasz Majewski <[email protected]>

---
drivers/dfu/dfu.c | 34 ++++++++++++++++++++++++++++++----
include/dfu.h | 5 +++++

2 files changed, 35 insertions(+), 4 deletions(-)

drivers/dfu/dfu.c +18 -0

index e15f959..5d50b47 100644
@@ -20,6 +20,7 @@ static bool dfu_reset_request;
 static LIST_HEAD(dfu_list);
 static int dfu_alt_num;
 static int alt_num_count;
+static int dfu_checksum_method;
 
 bool dfu_reset(void)
 {

@@ -99,6 +100,23 @@ unsigned char *dfu_get_buf(void)
 	return dfu_buf;
 }
 
+static int dfu_get_checksum_method(void)
+{
+	char *s;
+
+	s = getenv("dfu_checksum_method");
+	if (!s)
+		return DFU_NO_CHECKSUM;
+
+	if (!strcmp(s, "crc32")) {
+		debug("%s: DFU checksum method: %s\n", __func__,
 s);
+		return DFU_CRC32;
+	} else {
+		error("DFU checksum method: %s not supported!\n",
 s);
+		return -EINVAL;
+	}
+}
+
 static int dfu_write_buffer_drain(struct dfu_entity *dfu)

{
	long w_size;

drivers/dfu/dfu.c +2 -2

@@ -109,8 +127,8 @@ static int dfu_write_buffer_drain(struct
 dfu_entity *dfu) if (w_size == 0)
 		return 0;
 
-	/* update CRC32 */
-	dfu->crc = crc32(dfu->crc, dfu->i_buf_start, w_size);
+	if (dfu_checksum_method == DFU_CRC32)
+		dfu->crc = crc32(dfu->crc, dfu->i_buf_start,
 w_size);
 
 	ret = dfu->write_medium(dfu, dfu->offset, dfu->i_buf_start,

&w_size); if (ret)

drivers/dfu/dfu.c +0 -0

@@ -234,7 +252,8 @@ static int dfu_read_buffer_fill(struct

dfu_entity *dfu, void *buf, int size) /* consume */

		if (chunk > 0) {
			memcpy(buf, dfu->i_buf, chunk);

drivers/dfu/dfu.c +2 -1

-			dfu->crc = crc32(dfu->crc, buf, chunk);
+			if (dfu_checksum_method == DFU_CRC32)
+				dfu->crc = crc32(dfu->crc, buf,

chunk); dfu->i_buf += chunk;
			dfu->b_left -= chunk;
			dfu->r_left -= chunk;

drivers/dfu/dfu.c +3 -1

@@ -318,7 +337,9 @@ int dfu_read(struct dfu_entity *dfu, void *buf,
 int size, int blk_seq_num) }
 
 	if (ret < size) {
-		debug("%s: %s CRC32: 0x%x\n", __func__, dfu->name,
 dfu->crc);
+		if (dfu_checksum_method == DFU_CRC32)
+			debug("%s: %s CRC32: 0x%x\n", __func__,
 dfu->name,
+			      dfu->crc);
 		puts("\nUPLOAD ... done\nCtrl+C to exit ...\n");

		dfu_free_buf();

drivers/dfu/dfu.c +5 -0

@@ -393,6 +414,11 @@ int dfu_config_entities(char *env, char
 *interface, int num) dfu_alt_num = dfu_find_alt_num(env);
 	debug("%s: dfu_alt_num=%d\n", __func__, dfu_alt_num);
 
+	ret = dfu_get_checksum_method();
+	if (ret < 0)
+		return ret;
+	dfu_checksum_method = ret;
+
 	dfu = calloc(sizeof(*dfu), dfu_alt_num);
 	if (!dfu)
 		return -1;

include/dfu.h +5 -0

index 751f0fd..855d6dc 100644
@@ -37,6 +37,11 @@ enum dfu_op {
 	DFU_OP_WRITE,
 };
 
+enum dfu_checksum {
+	DFU_NO_CHECKSUM = 0,
+	DFU_CRC32,
+};
+
 #define DFU_NOT_SUPPORTED -1
 
 struct mmc_internal_data {

Signature · 1 line

1.7.10.4

Signature · 5 lines

Best regards,

Lukasz Majewski

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

Up till now the CRC32 of received data was calculated unconditionally. The standard crc32 implementation causes long delays when large images were uploaded.

The "dfu_checksum_method" environment variable gives the opportunity to enable on demand (when e.g. debugging) the crc32 calculation. It can be done without need to recompile the u-boot binary.

By default the crc32 is not calculated.

Tests results:
400 MiB ums.img file
With crc32 calculation: 65 sec [avg 6.29 MB/s]
Without crc32 calculation: 25 sec [avg 16.17 MB/s]

Signed-off-by Lukasz Majewski <[email protected]>

Quoted 15 lines · Lukasz Majewski

Up till now the CRC32 of received data was calculated unconditionally. The standard crc32 implementation causes long delays when large images were uploaded.

The "dfu_checksum_method" environment variable gives the opportunity to enable on demand (when e.g. debugging) the crc32 calculation. It can be done without need to recompile the u-boot binary.

By default the crc32 is not calculated.

Tests results:
400 MiB ums.img file
With crc32 calculation: 65 sec [avg 6.29 MB/s]
Without crc32 calculation: 25 sec [avg 16.17 MB/s]

Quoted 1 line

Signed-off-by Lukasz Majewski <[email protected]>

OK, so, protocol question. What's going on in the background here such that it's a good and safe idea to not do this checksum and we won't end up in the case where data was corrupted and we just bricked a board in update mode?

Quoted 18 lines · Tom Rini

Quoted 16 lines · Lukasz Majewski

Up till now the CRC32 of received data was calculated unconditionally. The standard crc32 implementation causes long delays when large images were uploaded.

The "dfu_checksum_method" environment variable gives the opportunity to enable on demand (when e.g. debugging) the crc32 calculation. It can be done without need to recompile the u-boot binary.

By default the crc32 is not calculated.

Tests results:
400 MiB ums.img file
With crc32 calculation: 65 sec [avg 6.29 MB/s]
Without crc32 calculation: 25 sec [avg 16.17 MB/s]

Signed-off-by Lukasz Majewski <[email protected]>

Quoted 4 lines

OK, so, protocol question. What's going on in the background here such that it's a good and safe idea to not do this checksum and we won't end up in the case where data was corrupted and we just bricked a board in update mode?

This is just a convenience measure for people who do a lot of updates and thus the time matters, it's not a production-feature.

Quoted 11 lines · Lukasz Majewski

Up till now the CRC32 of received data was calculated unconditionally. The standard crc32 implementation causes long delays when large images were uploaded.

The "dfu_checksum_method" environment variable gives the opportunity to enable on demand (when e.g. debugging) the crc32 calculation. It can be done without need to recompile the u-boot binary.

By default the crc32 is not calculated.

Quoted 6 lines

Tests results:
400 MiB ums.img file
With crc32 calculation: 65 sec [avg 6.29 MB/s]
Without crc32 calculation: 25 sec [avg 16.17 MB/s]

Signed-off-by Lukasz Majewski <[email protected]>

OK, so, protocol question.

What's going on in the background here
such that it's a good and safe idea to not do this checksum and we won't end up in the case where data was corrupted and we just bricked a board in update mode?

Quoted 20 lines · On Mon, 31 Mar 2014 14:05:17 -0400 Tom Rini <[email protected]> wrote:

Quoted 17 lines · Lukasz Majewski

Up till now the CRC32 of received data was calculated unconditionally. The standard crc32 implementation causes long delays when large images were uploaded.

The "dfu_checksum_method" environment variable gives the opportunity to enable on demand (when e.g. debugging) the crc32 calculation. It can be done without need to recompile the u-boot binary.

By default the crc32 is not calculated.

Tests results:
400 MiB ums.img file
With crc32 calculation: 65 sec [avg 6.29 MB/s]
Without crc32 calculation: 25 sec [avg 16.17 MB/s]

Signed-off-by Lukasz Majewski <[email protected]>

Quoted 1 line

OK, so, protocol question.

The DFU 1.1 standard in its appendinx B specifies the DFU suffix. It has the crc32 for the whole file, vendorID, device ID and other handy fields.

Unfortunately, this part of the standard is not supported neither at dfu implementation in u-boot nor dfu-util (v.0.5 - debian).

When we use CRC in such a way, we should be able to decide which tool (algorithm) use for debug. SHA1, MD5, etc are widely available on each linux box. To have the same crc32 algorithm, which is in u-boot, implemented as linux command line tool you need to search a bit (libarchive-zip-perl package for debian).

TO SUM UP:

1. Handling of the DFU suffix shall be implemented and utilized in both u-boot and dfu-util with critical files (bootloaders, kernel).

2. There should be freedom to use different checksum algorithms for providing debugging information.

3. The current CRC32 calculation at DFU should be optimized.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 29 lines · Lukasz Majewski

Quoted 13 lines · On Mon, 31 Mar 2014 14:05:17 -0400 Tom Rini <[email protected]> wrote:

Quoted 11 lines · Lukasz Majewski

Up till now the CRC32 of received data was calculated unconditionally. The standard crc32 implementation causes long delays when large images were uploaded.

The "dfu_checksum_method" environment variable gives the opportunity to enable on demand (when e.g. debugging) the crc32 calculation. It can be done without need to recompile the u-boot binary.

By default the crc32 is not calculated.

Quoted 8 lines

Quoted 6 lines

Tests results:
400 MiB ums.img file
With crc32 calculation: 65 sec [avg 6.29 MB/s]
Without crc32 calculation: 25 sec [avg 16.17 MB/s]

Signed-off-by Lukasz Majewski <[email protected]>

OK, so, protocol question.

The DFU 1.1 standard in its appendinx B specifies the DFU suffix. It has the crc32 for the whole file, vendorID, device ID and other handy fields.

Quoted 2 lines

Unfortunately, this part of the standard is not supported neither at dfu implementation in u-boot nor dfu-util (v.0.5 - debian).

OK, so this is the important part. We're doing a crc32 on stuff that's not required by spec, just handy for verification, manually.

[snip]

Quoted 5 lines

When we use CRC in such a way, we should be able to decide which tool (algorithm) use for debug. SHA1, MD5, etc are widely available on each linux box. To have the same crc32 algorithm, which is in u-boot, implemented as linux command line tool you need to search a bit (libarchive-zip-perl package for debian).

I take your point, but I use rhash -C to get matching crc32 and it was the first thing I stumbled upon when going "oh right, what does crc32?".

[snip]

Quoted 5 lines

TO SUM UP:

1. Handling of the DFU suffix shall be implemented and utilized in both u-boot and dfu-util with critical files (bootloaders, kernel).

Quoted 4 lines

2. There should be freedom to use different checksum algorithms for providing debugging information.

3. The current CRC32 calculation at DFU should be optimized.

Well, is there work being done on the dfu-util side currently or planned in the near future to do #1 per the spec?

But, with that, I'm happier to default to no crc32'ing the data for today.

- --
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJTOdhmAAoJENk4IS6UOR1WDsYP/2eg3v+K5nCUZ22eYnrY4s14 f8KUan8My7Ifr/to9qbAIFsSuw5mlLPvYy5JNnrbmmipDH2bQIO20R1t94/Mm8Ut Hoj1nbGZ3JvMsoj86D+9pFz2AchVgbpvs+boiJGw2s1TZ3xKoNlJ1O4WJ8ttZRZS 1B3FC50PKYJK6lgWgfvds2AevLIAcF1QyePVsOLVKV2USilFiZ1LVb8qUFp5l6Ja LX3wfQjhPq4gQq8bX7LW6zNbDkXuZjxLlKT/kUxzl2qclpHj4+8rXVVRf1mLaLvU Dvx50V/JCncIivRBhfvK2BoQ/LOntmPwGfO95AY57naP9+nCzzE9vdKv/Ki5vpju /q/CjlXbkS4iwru+91neyMdfeCiiALV2yW0GBORgph7kCpUk343S753epl/MFmAW rOQ0xBjw4q5KeXijtQG5bdevynPkB09soKKhQX5XRe7i8olXe32+khQVwqpomjkG v0YbKvUTuCZ1NNZqEey/zO4gJPR2Tq4QiNFpfFPvcYOqlqoC/C84HfO+M8ocKiUh SUgdaUQI+uP7LSQ7Yv5N149ZD4aWAfsyU5YCtyC0gI9aXRF5YqwWU96eym8PUVM8 amo+srsp1uK5l6XRO0cqtM9cmLedPRNAEKhah4/GgZa3S6lTH3oD7JBEPNyazTsc FpOnIzGk5JJnVpeobQv2
=HfXm
-----END PGP SIGNATURE-----

The DFU 1.1 standard in its appendinx B specifies the DFU suffix. It has the crc32 for the whole file, vendorID, device ID and other handy fields.

Unfortunately, this part of the standard is not supported neither at dfu implementation in u-boot nor dfu-util (v.0.5 - debian).

It would be handy for small files (like bootloaders, kernels) where we download all the data at once. For critical files it should be definitely implemented. From my glimpse observation the dfu-util would require some extension to support the DFU suffix (Tormod, please correct me if I'm wrong).

Quoted 7 lines

The DFU 1.1 standard in its appendinx B specifies the DFU suffix. It has the crc32 for the whole file, vendorID, device ID and other handy fields.

Unfortunately, this part of the standard is not supported neither at dfu implementation in u-boot nor dfu-util (v.0.5 - debian).

Quoted 5 lines

It would be handy for small files (like bootloaders, kernels) where we download all the data at once. For critical files it should be definitely implemented. From my glimpse observation the dfu-util would require some extension to support the DFU suffix (Tormod, please correct me if I'm wrong).

You are wrong :) Please don't use what's available in Debian (stable?) as a reference.

I don't know what their maintainer is up to. dfu-util supports the DFU suffix according to the DFU standard. That means it checks the CRC after reading the file, and also checks that vendor/product values match, then sends the firmware to the device after stripping off the suffix.

Are you wanting to do some CRC checking at the device side? That would be outside the DFU standard.

But you can always put whatever you want
in the "firmware", including proprietary headers or suffices.

We
already support some of those, please see the dfu-util (and dfu-suffx/dfu-prefix) code at dfu-util.gnumonks.org.

Regards,
Tormod

Hi Tormod,

Quoted 14 lines · Lukasz Majewski

Quoted 12 lines

The DFU 1.1 standard in its appendinx B specifies the DFU suffix. It has the crc32 for the whole file, vendorID, device ID and other handy fields.

Unfortunately, this part of the standard is not supported neither at dfu implementation in u-boot nor dfu-util (v.0.5 - debian).

It would be handy for small files (like bootloaders, kernels) where we download all the data at once. For critical files it should be definitely implemented. From my glimpse observation the dfu-util would require some extension to support the DFU suffix (Tormod, please correct me if I'm wrong).

Quoted 2 lines

You are wrong :) Please don't use what's available in Debian (stable?) as a reference.

I'm regarding this as a reference since 80% of developers will download the dfu-util with apt-get on debian.

Quoted 5 lines

I don't know what their maintainer is up to. dfu-util supports the DFU suffix according to the DFU standard. That means it checks the CRC after reading the file, and also checks that vendor/product values match, then sends the firmware to the device after stripping off the suffix.

So this means that:
1. The file before reading by dfu-util has to be equipped with suffix.
2. The dfu-util reads it and then if matching sends data (with sufix stripped) to target. This means that we are "protected" from downloading wrong firmware to device, however
3. The target doesn't have any means to check if the downloaded data is consistent - the information about CRC is stripped at dfu-util.

Quoted 3 lines

Are you wanting to do some CRC checking at the device side? That would be outside the DFU standard.

I hoped that the suffix is appended by dfu-util and then sent with the binary to target. As a result we would be able to perform some integrity tests.

Quoted 2 lines

But you can always put whatever you want
in the "firmware", including proprietary headers or suffices.

I think that this would be finally required for updating small (crucial) files - like bootloaders, kernels.

Quoted 3 lines

We
already support some of those, please see the dfu-util (and dfu-suffx/dfu-prefix) code at dfu-util.gnumonks.org.

Ok, I see. This would probably require extension of ./src/prefix.c file. In this way u-boot community would impose some kind of standard prefix/suffix only for u-boot. It's a pity, that integrity checking is not standardized in the DFU.

Hello.

Quoted 17 lines · Lukasz Majewski

Hi Tormod,

Quoted 15 lines · Lukasz Majewski

Quoted 13 lines

The DFU 1.1 standard in its appendinx B specifies the DFU suffix. It has the crc32 for the whole file, vendorID, device ID and other handy fields.

Unfortunately, this part of the standard is not supported neither at dfu implementation in u-boot nor dfu-util (v.0.5 - debian).

It would be handy for small files (like bootloaders, kernels) where we download all the data at once. For critical files it should be definitely implemented. From my glimpse observation the dfu-util would require some extension to support the DFU suffix (Tormod, please correct me if I'm wrong).

Quoted 5 lines

Quoted 2 lines

You are wrong :) Please don't use what's available in Debian (stable?) as a reference.

I'm regarding this as a reference since 80% of developers will download the dfu-util with apt-get on debian.

You really believe that 80% of all developers are using Debian?

If
they ship an old version there is nothing Tormod can do about it. I implemented the dfu suffix feature one or two years ago and made a release after it. If distros are not picking it up you have to fill a bug for them to update.

Quoted 7 lines

Quoted 6 lines

I don't know what their maintainer is up to. dfu-util supports the DFU suffix according to the DFU standard. That means it checks the CRC after reading the file, and also checks that vendor/product values match, then sends the firmware to the device after stripping off the suffix.

Quoted 8 lines

So this means that:
1. The file before reading by dfu-util has to be equipped with suffix. 2. The dfu-util reads it and then if matching sends data (with sufix stripped) to target. This means that we are "protected" from downloading wrong firmware to device, however
3. The target doesn't have any means to check if the downloaded data is consistent - the information about CRC is stripped at dfu-util.

Correct. That is how the DFU spec defines it.

Quoted 8 lines

Quoted 4 lines

Are you wanting to do some CRC checking at the device side? That would be outside the DFU standard.

I hoped that the suffix is appended by dfu-util and then sent with the binary to target. As a result we would be able to perform some integrity tests.

The spec requires to remove it. I also found that odd when implementing it but the spec is clear on this.

Quoted 10 lines

Quoted 2 lines

But you can always put whatever you want
in the "firmware", including proprietary headers or suffices.

I think that this would be finally required for updating small (crucial) files - like bootloaders, kernels.

Quoted 3 lines

We
already support some of those, please see the dfu-util (and dfu-suffx/dfu-prefix) code at dfu-util.gnumonks.org.

Quoted 4 lines

Ok, I see. This would probably require extension of ./src/prefix.c file. In this way u-boot community would impose some kind of standard prefix/suffix only for u-boot. It's a pity, that integrity checking is not standardized in the DFU.

It all depends on how much you want to be compatible with the DFU spec.

regards
Stefan Schmidt

Up till now the CRC32 of received data was calculated unconditionally. The standard crc32 implementation causes long delays when large images were uploaded.

The "dfu_checksum_method" environment variable gives the opportunity to enable on demand (when e.g. debugging) the crc32 calculation. It can be done without need to recompile the u-boot binary.

By default the crc32 is not calculated.

Tests results:
400 MiB ums.img file
With crc32 calculation: 65 sec [avg 6.29 MB/s]
Without crc32 calculation: 25 sec [avg 16.17 MB/s]

Signed-off-by Lukasz Majewski <[email protected]>

Cc Marek Vasut <[email protected]>

Up till now the CRC32 of received data was calculated unconditionally. The standard crc32 implementation causes long delay when large images were uploaded.

The "dfu_hash_algo" environment variable gives the opportunity to enable on demand (when e.g. debugging) the hash (crc32) calculation. It can be done without need to recompile the u-boot binary and reuses the generic hash framework.

By default the crc32 is NOT calculated anymore.

Tests results:
400 MiB ums.img file
With crc32 calculation: 65 sec [avg 6.29 MB/s]
Without crc32 calculation: 25 sec [avg 16.17 MB/s]

Signed-off-by Lukasz Majewski <[email protected]>

Cc Marek Vasut <[email protected]>

Up till now the CRC32 of received data was calculated unconditionally. The standard crc32 implementation causes long delay when large images were uploaded.

The "dfu_hash_algo" environment variable gives the opportunity to enable on demand (when e.g. debugging) the hash (crc32) calculation. It can be done without need to recompile the u-boot binary and reuses the generic hash framework.

By default the crc32 is NOT calculated anymore.

Dear Lukasz Majewski,

In message <[email protected]>

Quoted 9 lines · you

Up till now the CRC32 of received data was calculated unconditionally. The standard crc32 implementation causes long delay when large images were uploaded.

The "dfu_hash_algo" environment variable gives the opportunity to enable on demand (when e.g. debugging) the hash (crc32) calculation. It can be done without need to recompile the u-boot binary and reuses the generic hash framework.

Quoted 1 line

By default the crc32 is NOT calculated anymore.

I consider this a VARY BAD idea, as it causes a significant decrease of reliability and robustness of the systems. Please do not do this.

In any case, if you introduce this, the behaviour should be documented, and the default setting should be such as to keep the previous behaviour, i. e. CRC checking should remain on by default. then people who are willing to trade reliability for a little speed

can still switch it off, but the unawarerest of the users will not suffer.

Best regards,

Wolfgang Denk

For automated tests I use MD5 and compare this value before sending data to target via DFU and after I read it. This testing is done purely on HOST machine.

Participants have agreed, that we shall optionally enable crc32 (or other algorithm) calculation.

2. The current crc32 implementation is painfully slow (although I have only L1 enabled on my target).

3. With large files (like rootfs images) we store data (to medium) with 32 MiB chunks, which means that when we calculate complete crc32 the image is already written to its final destination.

4. This patch also allows some flexibility: by setting the env variable we can decide which algorithm to use (crc32, sha1, etc). It is appealing since we use the hash_* code anyway.

Quoted 4 lines

In any case, if you introduce this, the behaviour should be documented, and the default setting should be such as to keep the previous behaviour, i. e. CRC checking should remain on by default. then people who are willing to trade reliability for a little speed

I would not touch the code if the speedup wouldn't be so significant. Reducing flashing time of 400 MiB file from 65 s to 25 s is worth its effort.

As I've stated previously the crc32 in the current dfu implementation is only informative.

To take the full advantage of it, we would need to modify the dfu-util to wrap the sent file to some kind of header or locally write some script to do that. However, this is not specified by the standard and would be u-boot's extension of the DFU.

Even more important issue is that it would work only for small files (like uImage).

Dear Lukasz,

Quoted 4 lines · In message <20140509085203.31133238@amdc2363> you wrote:

For automated tests I use MD5 and compare this value before sending data to target via DFU and after I read it. This testing is done purely on HOST machine.

This is unsufficient. You should always verify the image on the target after the download has completed.

Quoted 3 lines

Participants have agreed, that we shall optionally enable crc32 (or other algorithm) calculation.

If this is the default now, it should remain the default.

Quoted 2 lines

2. The current crc32 implementation is painfully slow (although I have only L1 enabled on my target).

This is an unrelated problem then, which should excluded from this discussion here.

Quoted 3 lines

3. With large files (like rootfs images) we store data (to medium) with 32 MiB chunks, which means that when we calculate complete crc32 the image is already written to its final destination.

You can still detect if the download was corrupted, report a proper error and initiate a re-download. This would at least give you a chance to react to corrupted data.

Just closing the eyes and hoping
no errors will ever happen has always been a bad strategy.

Quoted 4 lines

4. This patch also allows some flexibility: by setting the env variable we can decide which algorithm to use (crc32, sha1, etc). It is appealing since we use the hash_* code anyway.

Agreed. This was not my point.

What I complained about is the change in behaviour. I asked to make the existing behaviour the default, so unaware users will not be affected. Only if you intentionally want some other behaviour you can then enable this by setting the env variable.

Quoted 10 lines

Quoted 6 lines

In any case, if you introduce this, the behaviour should be documented, and the default setting should be such as to keep the previous behaviour, i. e. CRC checking should remain on by default. then people who are willing to trade reliability for a little speed

I would not touch the code if the speedup wouldn't be so significant. Reducing flashing time of 400 MiB file from 65 s to 25 s is worth its effort.

I disagree, if you pay for the speed by reduced reliability, and if you don't even inform the user about this new behaviour.

Also, I feel it might be worth to investigate why the checksumming is slow on your system.

Quoted 2 lines

As I've stated previously the crc32 in the current dfu implementation is only informative.

It is pretty useful information, isn't it?

Quoted 5 lines

To take the full advantage of it, we would need to modify the dfu-util to wrap the sent file to some kind of header or locally write some script to do that. However, this is not specified by the standard and would be u-boot's extension of the DFU.

Ok, add this to the many deficientcies of DFU :-(

Quoted 3 lines

Even more important issue is that it would work only for small files (like uImage).

Why so? Can we not calculate CRC even when the transfer is broken down into several chunks?

Best regards,

Wolfgang Denk

Dear Lukasz,

Quoted 4 lines · In message <20140509085203.31133238@amdc2363> you wrote:

For automated tests I use MD5 and compare this value before sending data to target via DFU and after I read it. This testing is done purely on HOST machine.

This is unsufficient. You should always verify the image on the target after the download has completed.

Quoted 2 lines

Participants have agreed, that we shall optionally enable crc32 (or other algorithm) calculation.

If this is the default now, it should remain the default.

Quoted 2 lines

2. The current crc32 implementation is painfully slow (although I have only L1 enabled on my target).

This is an unrelated problem then, which should excluded from this discussion here.

Quoted 3 lines

3. With large files (like rootfs images) we store data (to medium) with 32 MiB chunks, which means that when we calculate complete crc32 the image is already written to its final destination.

You can still detect if the download was corrupted, report a proper error and initiate a re-download. This would at least give you a chance to react to corrupted data. Just closing the eyes and hoping no errors will ever happen has always been a bad strategy.

Quoted 3 lines

4. This patch also allows some flexibility: by setting the env variable we can decide which algorithm to use (crc32, sha1, etc). It is appealing since we use the hash_* code anyway.

Agreed. This was not my point.

What I complained about is the change in behaviour. I asked to make the existing behaviour the default, so unaware users will not be affected. Only if you intentionally want some other behaviour you can then enable this by setting the env variable.

Quoted 10 lines

Dear Lukasz,

Quoted 4 lines · In message <20140509085203.31133238@amdc2363> you wrote:

For automated tests I use MD5 and compare this value before sending data to target via DFU and after I read it. This testing is done purely on HOST machine.

This is unsufficient. You should always verify the image on the target after the download has completed.

True. But this patch doesn't really change what you would have to do, and arguably make it easier.

Quoted 4 lines

Quoted 2 lines

Participants have agreed, that we shall optionally enable crc32 (or other algorithm) calculation.

If this is the default now, it should remain the default.

Keep in mind what this current default is. We say "here was the CRC32". We do not compare it with an expected value nor do we have the ability to since we're not passed from the host what the value was.

Quoted 5 lines

Quoted 2 lines

2. The current crc32 implementation is painfully slow (although I have only L1 enabled on my target).

This is an unrelated problem then, which should excluded from this discussion here.

Agreed.

Quoted 9 lines

Quoted 4 lines

3. With large files (like rootfs images) we store data (to medium) with 32 MiB chunks, which means that when we calculate complete crc32 the image is already written to its final destination.

You can still detect if the download was corrupted, report a proper error and initiate a re-download. This would at least give you a chance to react to corrupted data. Just closing the eyes and hoping no errors will ever happen has always been a bad strategy.

Before and after this change, only if the console is being monitored by some script. We do not nor are we given an expected hash so we cannot say data was corrupted.

Quoted 10 lines

Quoted 3 lines

4. This patch also allows some flexibility: by setting the env variable we can decide which algorithm to use (crc32, sha1, etc). It is appealing since we use the hash_* code anyway.

Agreed. This was not my point.

What I complained about is the change in behaviour. I asked to make the existing behaviour the default, so unaware users will not be affected. Only if you intentionally want some other behaviour you can then enable this by setting the env variable.

Well, looking at mainline usage of DFU, Lukasz is speaking for about half of the users / implementors. Since Denx is working with the other half, can you shed some light on actual use vs theoretical possibilities?

Hi Tom, Wolfgang,

Quoted 58 lines · Wolfgang Denk

Quoted 10 lines

Dear Lukasz,

Quoted 4 lines · In message<20140509085203.31133238@amdc2363> you wrote:

For automated tests I use MD5 and compare this value before sending data to target via DFU and after I read it. This testing is done purely on HOST machine.

This is unsufficient. You should always verify the image on the target after the download has completed.

True. But this patch doesn't really change what you would have to do, and arguably make it easier.

Quoted 4 lines

Quoted 2 lines

Participants have agreed, that we shall optionally enable crc32 (or other algorithm) calculation.

If this is the default now, it should remain the default.

Keep in mind what this current default is. We say "here was the CRC32". We do not compare it with an expected value nor do we have the ability to since we're not passed from the host what the value was.

Quoted 5 lines

Quoted 2 lines

2. The current crc32 implementation is painfully slow (although I have only L1 enabled on my target).

This is an unrelated problem then, which should excluded from this discussion here.

Agreed.

Quoted 9 lines

Quoted 4 lines

3. With large files (like rootfs images) we store data (to medium) with 32 MiB chunks, which means that when we calculate complete crc32 the image is already written to its final destination.

You can still detect if the download was corrupted, report a proper error and initiate a re-download. This would at least give you a chance to react to corrupted data. Just closing the eyes and hoping no errors will ever happen has always been a bad strategy.

Before and after this change, only if the console is being monitored by some script. We do not nor are we given an expected hash so we cannot say data was corrupted.

Quoted 10 lines

Quoted 3 lines

4. This patch also allows some flexibility: by setting the env variable we can decide which algorithm to use (crc32, sha1, etc). It is appealing since we use the hash_* code anyway.

Agreed. This was not my point.

What I complained about is the change in behaviour. I asked to make the existing behaviour the default, so unaware users will not be affected. Only if you intentionally want some other behaviour you can then enable this by setting the env variable.

Quoted 5 lines

Well, looking at mainline usage of DFU, Lukasz is speaking for about half of the users / implementors. Since Denx is working with the other half, can you shed some light on actual use vs theoretical possibilities?

I don't want to urge anybody on making any conclusion here :-), but I would be very grateful if we could come up with an agreement.

As I've stated previously, my opinion is similar to the one presented by Tom in this message.

For me it would be best to not calculate any checksum on default and only enable it when needed.

Quoted 10 lines

Quoted 5 lines

What I complained about is the change in behaviour. I asked to make the existing behaviour the default, so unaware users will not be affected. Only if you intentionally want some other behaviour you can then enable this by setting the env variable.

Well, looking at mainline usage of DFU, Lukasz is speaking for about half of the users / implementors. Since Denx is working with the other half, can you shed some light on actual use vs theoretical possibilities?

I don't want to urge anybody on making any conclusion here :-), but I would be very grateful if we could come up with an agreement.

As I've stated previously, my opinion is similar to the one presented by Tom in this message.

For me it would be best to not calculate any checksum on default and only enable it when needed.

Dear Lukasz,

Quoted 17 lines · In message <20140515090904.32f1d13d@amdc2363> you wrote:

Quoted 10 lines

Quoted 5 lines

What I complained about is the change in behaviour. I asked to make the existing behaviour the default, so unaware users will not be affected. Only if you intentionally want some other behaviour you can then enable this by setting the env variable.

Well, looking at mainline usage of DFU, Lukasz is speaking for about half of the users / implementors. Since Denx is working with the other half, can you shed some light on actual use vs theoretical possibilities?

I don't want to urge anybody on making any conclusion here :-), but I would be very grateful if we could come up with an agreement.

As I've stated previously, my opinion is similar to the one presented by Tom in this message.

Quoted 2 lines

For me it would be best to not calculate any checksum on default and only enable it when needed.

I asked Heiko to run some actual tests on the boards where he has to maintain DFU for. For a 288 MiB image he did not measure any difference - with your patch applied, both with and without CRC enabled, we would get the same (slow) 1:54 minutes download time.

This reinforces my speculation that you are actually addressing the wrong problem. Instead of adding new code and environment variables and making the system even more complex, we should just leave everything as is,

and you should try to find out why the CRC
calculation is so low for you. Checking if caches are enabled is probably among the things that should be done first.

Regarding the checksumming topic in general: the fact that the DFU standard defines a method to verify the checksum of the image (dwCRC field in the DFU File Suffix), but does not transmit this vital data to the target so the actual file download and storage procedure on the target is completely unprotected is IMO a serious design flaw of the DFU protocl. Do you think it would be possible to have this augmented / fixed?

Best regards,

Wolfgang Denk

Quoted 5 lines

This reinforces my speculation that you are actually addressing the wrong problem. Instead of adding new code and environment variables and making the system even more complex, we should just leave everything as is,

During working on this patch I've replaced the crc32() method with the call to hash_method(), which IMHO is welcome.

I also don't personally like the crc32, hence I like the choice which this patch gives me to use other algorithm (for which I've got HW support on my platform - e.g. MD5).

Quoted 3 lines

and you should try to find out why the CRC
calculation is so low for you. Checking if caches are enabled is probably among the things that should be done first.

L1 is enabled. L2 has been disabled on purpose (power consumption reduction).

Please note that the last revision of DFU is from 2004. I've contacted Greg KH (one of the original authors) and he replied that no new attempt to revise the standard was made.

The best however, would be to revise the standard to include such functionality to it. In the same time I'm fully aware that this is very unlikely to happen.

Dear Lukasz,

Quoted 8 lines · In message <20140515154334.626923b4@amdc2363> you wrote:

Quoted 5 lines

This reinforces my speculation that you are actually addressing the wrong problem. Instead of adding new code and environment variables and making the system even more complex, we should just leave everything as is,

During working on this patch I've replaced the crc32() method with the call to hash_method(), which IMHO is welcome.

Yes, indeed this is highly welcome. Thanks a lot for that!

Quoted 3 lines

I also don't personally like the crc32, hence I like the choice which this patch gives me to use other algorithm (for which I've got HW support on my platform - e.g. MD5).

Well, is this really useful? dfu-utils provides only CRC caculation, so where would you get the reference value for any other checksum metod from?

Quoted 7 lines

Quoted 4 lines

and you should try to find out why the CRC
calculation is so low for you. Checking if caches are enabled is probably among the things that should be done first.

L1 is enabled. L2 has been disabled on purpose (power consumption reduction).

This certainly contributes to slow code execution.

Quoted 3 lines

Please note that the last revision of DFU is from 2004. I've contacted Greg KH (one of the original authors) and he replied that no new attempt to revise the standard was made.

This may just mean that users were just happy with the current situation.

It's definitely better than if changed had been proposed but rejected.

Quoted 4 lines

The best however, would be to revise the standard to include such functionality to it. In the same time I'm fully aware that this is very unlikely to happen.

Why do you think it is unlikely?

Of course, it would require that
someone comes up with such a proposal in the first place. But you sound as if you were certain a proposal had no chance for being considered.

I may be naive, but should we not at least try before giving up?

Best regards,

Wolfgang Denk

Hi Wolfgang,

Quoted 22 lines

Dear Lukasz,

Quoted 19 lines · In message <20140515090904.32f1d13d@amdc2363> you wrote:

Quoted 11 lines

Quoted 6 lines

What I complained about is the change in behaviour. I asked to make the existing behaviour the default, so unaware users will not be affected. Only if you intentionally want some other behaviour you can then enable this by setting the env variable.

Well, looking at mainline usage of DFU, Lukasz is speaking for about half of the users / implementors. Since Denx is working with the other half, can you shed some light on actual use vs theoretical possibilities?

I don't want to urge anybody on making any conclusion here :-), but I would be very grateful if we could come up with an agreement.

As I've stated previously, my opinion is similar to the one presented by Tom in this message.

Quoted 7 lines

Quoted 2 lines

For me it would be best to not calculate any checksum on default and only enable it when needed.

I asked Heiko to run some actual tests on the boards where he has to maintain DFU for. For a 288 MiB image he did not measure any difference - with your patch applied, both with and without CRC enabled, we would get the same (slow) 1:54 minutes download time.

As I've spoken with Heiko, am33xx uses NAND memory. I deal with eMMC. Moreover, the size of "chunks" are different - 1 MiB and 32 MiB.

I must double check for the rationale for chunk size of 32 MiB at Trats/Trats2 boards. I suspect, that eMMC write performance depend on that.

I will perform some performance tests with 1 MiB chunk size and share the result.

Quoted 6 lines

This reinforces my speculation that you are actually addressing the wrong problem. Instead of adding new code and environment variables and making the system even more complex, we should just leave everything as is,

During working on this patch I've replaced the crc32() method with the call to hash_method(), which IMHO is welcome.

I also don't personally like the crc32, hence I like the choice which this patch gives me to use other algorithm (for which I've got HW support on my platform - e.g. MD5).

Quoted 3 lines

and you should try to find out why the CRC
calculation is so low for you. Checking if caches are enabled is probably among the things that should be done first.

L1 is enabled. L2 has been disabled on purpose (power consumption reduction).

Hi Wolfgang, Tom,

Quoted 35 lines

Hi Wolfgang,

Quoted 22 lines

Dear Lukasz,

Quoted 19 lines · In message<20140515090904.32f1d13d@amdc2363> you wrote:

Quoted 11 lines

Quoted 6 lines

What I complained about is the change in behaviour. I asked to make the existing behaviour the default, so unaware users will not be affected. Only if you intentionally want some other behaviour you can then enable this by setting the env variable.

Well, looking at mainline usage of DFU, Lukasz is speaking for about half of the users / implementors. Since Denx is working with the other half, can you shed some light on actual use vs theoretical possibilities?

I don't want to urge anybody on making any conclusion here :-), but I would be very grateful if we could come up with an agreement.

As I've stated previously, my opinion is similar to the one presented by Tom in this message.

Quoted 7 lines

Quoted 2 lines

For me it would be best to not calculate any checksum on default and only enable it when needed.

I asked Heiko to run some actual tests on the boards where he has to maintain DFU for. For a 288 MiB image he did not measure any difference - with your patch applied, both with and without CRC enabled, we would get the same (slow) 1:54 minutes download time.

As I've spoken with Heiko, am33xx uses NAND memory. I deal with eMMC. Moreover, the size of "chunks" are different - 1 MiB and 32 MiB.

Quoted 6 lines

I must double check for the rationale for chunk size of 32 MiB at Trats/Trats2 boards. I suspect, that eMMC write performance depend on that.

I will perform some performance tests with 1 MiB chunk size and share the result.

Unfortunately the 32 MiB is fixed for our platform. since lthor uses it by default.

Quoted 14 lines

Quoted 6 lines

This reinforces my speculation that you are actually addressing the wrong problem. Instead of adding new code and environment variables and making the system even more complex, we should just leave everything as is,

During working on this patch I've replaced the crc32() method with the call to hash_method(), which IMHO is welcome.

I also don't personally like the crc32, hence I like the choice which this patch gives me to use other algorithm (for which I've got HW support on my platform - e.g. MD5).

Quoted 6 lines

Quoted 3 lines

and you should try to find out why the CRC
calculation is so low for you. Checking if caches are enabled is probably among the things that should be done first.

L1 is enabled. L2 has been disabled on purpose (power consumption reduction).

Regarding L2 - our platform requires SMC calls to enable and manage L2 cache. In my opinion support for this in u-boot is an overkill.

Can we conclude this whole discussion? The main point was if we should keep calculating and displaying crc32 as default for DFU transfers.

I'm for the option to NOT display and calculate it by default (PATCH v3).

Hello Lukasz,

Am 16.05.2014 10:58, schrieb Lukasz Majewski:

Quoted 77 lines

Hi Wolfgang, Tom,

Quoted 36 lines

Hi Wolfgang,

Quoted 22 lines

Dear Lukasz,

Quoted 19 lines · In message<20140515090904.32f1d13d@amdc2363> you wrote:

Quoted 11 lines

Quoted 6 lines

What I complained about is the change in behaviour. I asked to make the existing behaviour the default, so unaware users will not be affected. Only if you intentionally want some other behaviour you can then enable this by setting the env variable.

Well, looking at mainline usage of DFU, Lukasz is speaking for about half of the users / implementors. Since Denx is working with the other half, can you shed some light on actual use vs theoretical possibilities?

I don't want to urge anybody on making any conclusion here :-), but I would be very grateful if we could come up with an agreement.

As I've stated previously, my opinion is similar to the one presented by Tom in this message.

Quoted 7 lines

Quoted 2 lines

For me it would be best to not calculate any checksum on default and only enable it when needed.

I asked Heiko to run some actual tests on the boards where he has to maintain DFU for. For a 288 MiB image he did not measure any difference - with your patch applied, both with and without CRC enabled, we would get the same (slow) 1:54 minutes download time.

As I've spoken with Heiko, am33xx uses NAND memory. I deal with eMMC. Moreover, the size of "chunks" are different - 1 MiB and 32 MiB.

Quoted 6 lines

I must double check for the rationale for chunk size of 32 MiB at Trats/Trats2 boards. I suspect, that eMMC write performance depend on that.

I will perform some performance tests with 1 MiB chunk size and share the result.

Unfortunately the 32 MiB is fixed for our platform. since lthor uses it by default.

Quoted 14 lines

Quoted 6 lines

This reinforces my speculation that you are actually addressing the wrong problem. Instead of adding new code and environment variables and making the system even more complex, we should just leave everything as is,

During working on this patch I've replaced the crc32() method with the call to hash_method(), which IMHO is welcome.

I also don't personally like the crc32, hence I like the choice which this patch gives me to use other algorithm (for which I've got HW support on my platform - e.g. MD5).

Quoted 6 lines

Quoted 3 lines

and you should try to find out why the CRC
calculation is so low for you. Checking if caches are enabled is probably among the things that should be done first.

L1 is enabled. L2 has been disabled on purpose (power consumption reduction).

Regarding L2 - our platform requires SMC calls to enable and manage L2 cache. In my opinion support for this in u-boot is an overkill.

Can we conclude this whole discussion? The main point was if we should keep calculating and displaying crc32 as default for DFU transfers.

Quoted 2 lines

I'm for the option to NOT display and calculate it by default (PATCH v3).

I talked with the siemens board customer, they also do not check/use the displayed value from U-Boot ...

So, for me it is OK to not display this value ...

but we should add
to DFU such a check ... or?

bye,
Heiko

Hello Lukasz,

Am 16.05.2014 10:58, schrieb Lukasz Majewski:

Quoted 77 lines

Hi Wolfgang, Tom,

Quoted 36 lines

Hi Wolfgang,

Quoted 22 lines

Dear Lukasz,

Quoted 19 lines · In message<20140515090904.32f1d13d@amdc2363> you wrote:

Quoted 11 lines

Quoted 6 lines

What I complained about is the change in behaviour. I asked to make the existing behaviour the default, so unaware users will not be affected. Only if you intentionally want some other behaviour you can then enable this by setting the env variable.

Well, looking at mainline usage of DFU, Lukasz is speaking for about half of the users / implementors. Since Denx is working with the other half, can you shed some light on actual use vs theoretical possibilities?

I don't want to urge anybody on making any conclusion here :-), but I would be very grateful if we could come up with an agreement.

As I've stated previously, my opinion is similar to the one presented by Tom in this message.

Quoted 7 lines

Quoted 2 lines

For me it would be best to not calculate any checksum on default and only enable it when needed.

I asked Heiko to run some actual tests on the boards where he has to maintain DFU for. For a 288 MiB image he did not measure any difference - with your patch applied, both with and without CRC enabled, we would get the same (slow) 1:54 minutes download time.

As I've spoken with Heiko, am33xx uses NAND memory. I deal with eMMC. Moreover, the size of "chunks" are different - 1 MiB and 32 MiB.

Quoted 6 lines

I must double check for the rationale for chunk size of 32 MiB at Trats/Trats2 boards. I suspect, that eMMC write performance depend on that.

I will perform some performance tests with 1 MiB chunk size and share the result.

Unfortunately the 32 MiB is fixed for our platform. since lthor uses it by default.

Quoted 14 lines

Quoted 6 lines

This reinforces my speculation that you are actually addressing the wrong problem. Instead of adding new code and environment variables and making the system even more complex, we should just leave everything as is,

During working on this patch I've replaced the crc32() method with the call to hash_method(), which IMHO is welcome.

I also don't personally like the crc32, hence I like the choice which this patch gives me to use other algorithm (for which I've got HW support on my platform - e.g. MD5).

Quoted 6 lines

Quoted 3 lines

and you should try to find out why the CRC
calculation is so low for you. Checking if caches are enabled is probably among the things that should be done first.

L1 is enabled. L2 has been disabled on purpose (power consumption reduction).

Regarding L2 - our platform requires SMC calls to enable and manage L2 cache. In my opinion support for this in u-boot is an overkill.

Can we conclude this whole discussion? The main point was if we should keep calculating and displaying crc32 as default for DFU transfers.

Quoted 2 lines

I'm for the option to NOT display and calculate it by default (PATCH v3).

I talked with the siemens board customer, they also do not check/use the displayed value from U-Boot ...

So, for me it is OK to not display this value ...

but we should add
to DFU such a check ... or?

bye,
Heiko