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

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

-- 
Best regards,

Lukasz Majewski

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