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

[Heiko Schocher]

Hello Lukasz,

Sorry for answering so late to this thread ...

Am 15.05.2014 09:09, schrieb Lukasz Majewski:
> Hi Tom, Wolfgang,
>
>> On Fri, May 09, 2014 at 10:31:54AM +0200, Wolfgang Denk wrote:
>>> 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.
>>
>> True.  But this patch doesn't really change what you would have to do,
>> and arguably make it easier.
>>
>>>> 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.
>>
>>>> 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.
>>
>>>> 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.
>>
>>>> 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?
>>
>
> 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.

Hmm.. as we use the calculated crc only for printing it on the console,
the question is really, why should we calculate it?

I try this patch on the siemens boards and report if the speed
impact is there also so big as in your tests. (which board was this?
Are there caches enabled?)

And I ask the customer of the siemens boards, if they check the
crc value on the u-boot console output, if not, I vote for droping
the crc calculation as default ...

BTW: Should such a crc check not be added to dfu-util and u-boot?

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany