[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 <20140515090904.32f1d13d at amdc2363> you wrote:
> > 
> > > > 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.
> 
> I asked Heiko to run some actual tests on the boards where he has to
> maintain DFU for.  For a 288 MiB image he did not measure any
> difference - with your patch applied, both with and without CRC
> enabled, we would get the same (slow) 1:54 minutes download time.

As I've spoken with Heiko, am33xx uses NAND memory. I deal with eMMC.
Moreover, the size of "chunks" are different - 1 MiB and 32 MiB.

I must double check for the rationale for chunk size of 32 MiB at
Trats/Trats2 boards. I suspect, that eMMC write performance depend
on that.

I will perform some performance tests with 1 MiB chunk size and share
the result.

> 
> This reinforces my speculation that you are actually addressing the
> wrong problem.  Instead of adding new code and environment variables
> and making the system even more complex, we should just leave
> everything as is, 

During working on this patch I've replaced the crc32() method with the
call to hash_method(), which IMHO is welcome.

I also don't personally like the crc32, hence I like the choice which
this patch gives me to use other algorithm (for which I've got HW
support on my platform - e.g. MD5).

> and you should try to find out why the CRC
> calculation is so low for you.  Checking if caches are enabled is
> probably among the things that should be done first.

L1 is enabled. L2 has been disabled on purpose (power consumption
reduction). 

> 
> 
> Regarding the checksumming topic in general:  the fact that the DFU
> standard defines a method to verify the checksum of the image (dwCRC
> field in the DFU File Suffix), but does not transmit this vital data
> to the target so the actual file download and storage procedure on the
> target is completely unprotected is IMO a serious design flaw of the
> DFU protocl.  Do you think it would be possible to have this augmented
> / fixed?

Please note that the last revision of DFU is from 2004. I've contacted
Greg KH (one of the original authors) and he replied that no new attempt
to revise the standard was made. 

It is possible to fix this problem, by augmenting the current
implementation.

I saw the idea [*] of defining only one (or special) alt setting and in
this one file embed the header with integrity data, list of
files/partitions images included in this file, and even the information
to where we want to write. In this way we would still comply with DFU
1.1 standard, which would be "wrapped" to some host scripts and special
u-boot code. It even would be possible to leave the current code
untouched. 

The original link with the idea [*]:
http://codelectron.wordpress.com/2014/02/28/flexible-firmware-upgrade/

And the ML discussion:
http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/181715/match=proposal+hack+efficient+usb+dfu+linux+based+boards

The best however, would be to revise the standard to include such
functionality to it. In the same time I'm fully aware that this is
very unlikely to happen.

> 
> 
> 
> Best regards,
> 
> Wolfgang Denk
> 


-- 
Best regards,

Lukasz Majewski

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