[U-Boot,3/3] dfu: Introduction of the "dfu_checksum_method" env variable for checksum method setting

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 <l.majewski@samsung.com>
---
 drivers/dfu/dfu.c |   34 ++++++++++++++++++++++++++++++----
 include/dfu.h     |    5 +++++
 2 files changed, 35 insertions(+), 4 deletions(-)

On Monday, March 31, 2014 at 10:48:49 AM, Lukasz Majewski wrote:
> 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,

> On Monday, March 31, 2014 at 10:48:49 AM, Lukasz Majewski wrote:
> > 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 ?

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.

> 
> Best regards,
> Marek Vasut

On Monday, March 31, 2014 at 11:24:31 AM, Lukasz Majewski wrote:
> Hi Marek,
> 
> > On Monday, March 31, 2014 at 10:48:49 AM, Lukasz Majewski wrote:
> > > 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 ?
> 
> 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.

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

Hi Marek,

> On Monday, March 31, 2014 at 11:24:31 AM, Lukasz Majewski wrote:
> > Hi Marek,
> > 
> > > On Monday, March 31, 2014 at 10:48:49 AM, Lukasz Majewski wrote:
> > > > 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 ?
> > 
> > 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.
> 
> 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.

I will look closer to the hash_command() - maybe it would be enough to
only call it with a proper parameter.

Thanks for tip.

> 
> Best regards,
> Marek Vasut

Hi Lukasz,

On Mar 31, 2014, at 11:48 AM, Lukasz Majewski wrote:

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

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


> Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> ---
> drivers/dfu/dfu.c |   34 ++++++++++++++++++++++++++++++----
> include/dfu.h     |    5 +++++
> 2 files changed, 35 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
> index e15f959..5d50b47 100644
> --- a/drivers/dfu/dfu.c
> +++ b/drivers/dfu/dfu.c
> @@ -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;
> diff --git a/include/dfu.h b/include/dfu.h
> index 751f0fd..855d6dc 100644
> --- a/include/dfu.h
> +++ b/include/dfu.h
> @@ -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 {
> -- 
> 1.7.10.4
>

Hi Pantelis,

> Hi Lukasz,
> 
> On Mar 31, 2014, at 11:48 AM, Lukasz Majewski wrote:
> 
> > 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]
> > 
> 
> 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.


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


> 
> Regards
> 
> -- Pantelis
> 
> 
> > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> > ---
> > drivers/dfu/dfu.c |   34 ++++++++++++++++++++++++++++++----
> > include/dfu.h     |    5 +++++
> > 2 files changed, 35 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
> > index e15f959..5d50b47 100644
> > --- a/drivers/dfu/dfu.c
> > +++ b/drivers/dfu/dfu.c
> > @@ -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;
> > diff --git a/include/dfu.h b/include/dfu.h
> > index 751f0fd..855d6dc 100644
> > --- a/include/dfu.h
> > +++ b/include/dfu.h
> > @@ -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 {
> > -- 
> > 1.7.10.4
> >

Hi Lukasz,

On Mar 31, 2014, at 3:04 PM, Lukasz Majewski wrote:

> Hi Pantelis,
> 
>> Hi Lukasz,
>> 
>> On Mar 31, 2014, at 11:48 AM, Lukasz Majewski wrote:
>> 
>>> 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]
>>> 
>> 
>> 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.
> 

Thanks for the report. Would it be too much to ask for the time it
takes to do a crc32 of the same image on user-space after boot?

I'm interested in the effect the disabling of L2 has.

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

You could do a basic tradeoff of speed vs memory by creating larger tables
but it might not work if we're cache trashing.

Or even try using CRC with smaller tables (i.e. 4 bits) which would not affect
the cache much.

Regards

-- Pantelis

>> 
>> Regards
>> 
>> -- Pantelis
>> 
>> 
>>> Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
>>> ---
>>> drivers/dfu/dfu.c |   34 ++++++++++++++++++++++++++++++----
>>> include/dfu.h     |    5 +++++
>>> 2 files changed, 35 insertions(+), 4 deletions(-)
>>> 
>>> diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
>>> index e15f959..5d50b47 100644
>>> --- a/drivers/dfu/dfu.c
>>> +++ b/drivers/dfu/dfu.c
>>> @@ -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;
>>> diff --git a/include/dfu.h b/include/dfu.h
>>> index 751f0fd..855d6dc 100644
>>> --- a/include/dfu.h
>>> +++ b/include/dfu.h
>>> @@ -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 {
>>> -- 
>>> 1.7.10.4
>>> 
> 
> 
> 
> -- 
> Best regards,
> 
> Lukasz Majewski
> 
> Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Hi Lukasz,

Hmm, looking at the code the crc is updated when draining the buffer.
That means that in essence you're working with cache-cold data.

Can you try performing the crc32 right in the dfu_write() function
just after the memcpy?

Regards

-- Pantelis
 

On Mar 31, 2014, at 3:04 PM, Lukasz Majewski wrote:

> Hi Pantelis,
> 
>> Hi Lukasz,
>> 
>> On Mar 31, 2014, at 11:48 AM, Lukasz Majewski wrote:
>> 
>>> 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]
>>> 
>> 
>> 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.
> 
> 
>> 
>> 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.
> 
> 
>> 
>> Regards
>> 
>> -- Pantelis
>> 
>> 
>>> Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
>>> ---
>>> drivers/dfu/dfu.c |   34 ++++++++++++++++++++++++++++++----
>>> include/dfu.h     |    5 +++++
>>> 2 files changed, 35 insertions(+), 4 deletions(-)
>>> 
>>> diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
>>> index e15f959..5d50b47 100644
>>> --- a/drivers/dfu/dfu.c
>>> +++ b/drivers/dfu/dfu.c
>>> @@ -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;
>>> diff --git a/include/dfu.h b/include/dfu.h
>>> index 751f0fd..855d6dc 100644
>>> --- a/include/dfu.h
>>> +++ b/include/dfu.h
>>> @@ -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 {
>>> -- 
>>> 1.7.10.4
>>> 
> 
> 
> 
> -- 
> Best regards,
> 
> Lukasz Majewski
> 
> Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

On Mon, Mar 31, 2014 at 10:48:49AM +0200, Lukasz Majewski wrote:

> 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 <l.majewski@samsung.com>

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?

On Monday, March 31, 2014 at 08:05:17 PM, Tom Rini wrote:
> On Mon, Mar 31, 2014 at 10:48:49AM +0200, Lukasz Majewski wrote:
> > 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 <l.majewski@samsung.com>
> 
> 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.

Best regards,
Marek Vasut

On Mon, Mar 31, 2014 at 08:15:41PM +0200, Marek Vasut wrote:
> On Monday, March 31, 2014 at 08:05:17 PM, Tom Rini wrote:
> > On Mon, Mar 31, 2014 at 10:48:49AM +0200, Lukasz Majewski wrote:
> > > 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 <l.majewski@samsung.com>
> > 
> > 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.

That's what I figured.  So we've got the default backwards.

On Mon, 31 Mar 2014 14:05:17 -0400
Tom Rini <trini@ti.com> wrote:

> On Mon, Mar 31, 2014 at 10:48:49AM +0200, Lukasz Majewski wrote:
> 
> > 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 <l.majewski@samsung.com>
> 
> 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).

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

For large files (400 MiB in this case) it is useless since we store
data as it goes (with 32 MiB chunks). Also, as we send the large files
we experience the biggest performance penalty from CRC32 calculation.
It takes considerable time and in my opinion now serves only for debug
purposes, to provide the final CRC for comparison with original one,
even though the file is already on flash.

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 think that we can improve the crc32 performance with calculating it
for smaller chunks, which already fits L1 cache. Now they are calculated
for 32 MiB.


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

Now we rely solely on testing. Downloading file, checking CRC and
compare with original.
I also have some automated tests, which utilize MD5.

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.


> 

Best regards,
Lukasz Majewski

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

On 03/31/2014 04:44 PM, Lukasz Majewski wrote:
> On Mon, 31 Mar 2014 14:05:17 -0400
> Tom Rini <trini@ti.com> wrote:
> 
>> On Mon, Mar 31, 2014 at 10:48:49AM +0200, Lukasz Majewski wrote:
>>
>>> 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 <l.majewski@samsung.com>
>>
>> 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).

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

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

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

On Mon, Mar 31, 2014 at 10:44 PM, Lukasz Majewski wrote:
> 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).

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,

> On Mon, Mar 31, 2014 at 10:44 PM, Lukasz Majewski wrote:
> > 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).
> 
> 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.

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

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

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

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

> 
> Regards,
> Tormod

Hi Tom,

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 03/31/2014 04:44 PM, Lukasz Majewski wrote:
> > On Mon, 31 Mar 2014 14:05:17 -0400
> > Tom Rini <trini@ti.com> wrote:
> > 
> >> On Mon, Mar 31, 2014 at 10:48:49AM +0200, Lukasz Majewski wrote:
> >>
> >>> 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 <l.majewski@samsung.com>
> >>
> >> 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).
> 
> 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.

Yes, indeed.

> 
> [snip]
> > 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]
> 
> > 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.
> 
> Well, is there work being done on the dfu-util side currently or
> planned in the near future to do #1 per the spec?

As Tormod has written, dfu-util supports it for initial setting, but is
sending data with this information stripped. Hence u-boot shall not
calculate CRC32 unless we plan to add a customized prefix for crucial
binaries.

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

Thanks for your opinion. We share similar view on the issue.

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

Hello.

On Tue, 2014-04-01 at 11:00, Lukasz Majewski wrote:
> Hi Tormod,
> 
> > On Mon, Mar 31, 2014 at 10:44 PM, Lukasz Majewski wrote:
> > > 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).
> > 
> > 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.

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

Correct. That is how the DFU spec defines it.

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

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

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

regards
Stefan Schmidt

Hi Stefan,

> Hello.
> 
> On Tue, 2014-04-01 at 11:00, Lukasz Majewski wrote:
> > Hi Tormod,
> > 
> > > On Mon, Mar 31, 2014 at 10:44 PM, Lukasz Majewski wrote:
> > > > 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).
> > > 
> > > 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? 

It seems, that some miscommunication has crept in. What I meant was that
majority of developers, who are using deb based distro (debian, ubuntu),
would be lazy and use the apt-get/aptitude utility to install dfu-util.

It doesn't mean, that I'm not using the newest dfu-util (I recall that
there was some issue with libusb).

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

Maybe this is the thing to do.

> 
> > > 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.
> 
> Correct. That is how the DFU spec defines it.

Now it is clear.

> 
> > > 
> > > 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.
> 
> > > 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.
> > 
> > > 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.
> 
> It all depends on how much you want to be compatible with the DFU
> spec.

I would like to be as close to the standard as possible. Otherwise I
could be blamed for breaking compatibility.

> 
> regards
> Stefan Schmidt