[U-Boot] [PATCH v3] dfu: Introduction of the "dfu_hash_algo" env variable for checksum method setting

[Wolfgang Denk <wd@denx.de>]

Dear Lukasz,

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.

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

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

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

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

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

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

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

It is pretty useful information, isn't it?

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

> 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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
I don't mind criticism. You know me. I've  never  been  one  to  take
offence  at  criticism. No one could say I'm the sort to take offence
at criticism -- Not twice, anyway. Not without blowing bubbles.
                                  - Terry Pratchett, _Witches Abroad_