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

[Lukasz Majewski]

Hi Wolfgang,

> Dear Lukasz,
> 
> In message <20140509085203.31133238 at 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.

I agree.

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

In this particular case I would need to chop the large file either at
dfu-util or on some script, where each chunk need to have the header
with its crc32. Then before storing each chunk I can assess if data
wasn't corrupted.

This would provide reliability.

Now, even that I have the crc32 calculated for a chunk, I don't have
the original one to compare.

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

+1

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

Ok. I will preserve the default behavior. However, personally I think
that for a long term this proposed solution is better.

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

It depends what do you want to do with it. If you have target
connected via serial to some test setup and log this output and process
it on HOST afterwards, then it is useful. 

Otherwise, you only see on console the CRC, which you can by hand
compare with crc calculated on your host. And this information displays
just after you stored the data to the medium (and corrupted the
previous one).

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

The standard only allow the file which is the input to dfu-util to be
protected by CRC. Then dfu-util check this value and strips off the
header.

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

To do that one would need to:

- chop the large file to several smaller ones (and the chunk size can
  be different for each platform and must be know for HOST utils)
- calculate crc32 for each chunk
- wrap it to some header not conforming to the DFU standard -it would
  be the u-boot extension
- send each chunk separately to target - by calling dfu-util several
  times.

Handling of this would be difficult because of the need of DFU state
machine extension.


> 
> Best regards,
> 
> Wolfgang Denk
> 



-- 
Best regards,

Lukasz Majewski

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