[U-Boot] [PATCH] test: dfu: script: wrong md5sum on nand partitions

[U-Boot] [PATCH] test: dfu: script: wrong md5sum on nand partitions

[Heiko Schocher]

when uplaoding a file, at least from a nand partition, the complete
mtd nand partition size is transferred. This leads in a wrong md5sum
as the filesize is different between the downloaded file and the
uploaded file. Limit the uploaded filesize to the downloaded
fixes this.

Signed-off-by: Heiko Schocher <hs@denx.de>
Cc: Lukasz Majewski <l.majewski@samsung.com>
Cc: Stephen Warren <swarren@nvidia.com>
Cc: Roger Meier <r.meier@siemens.com>

---

Tested this on the siemens boards, with current ml I get:

Init script for generating data necessary for DFU test script OK
=========================================================================================
DFU EP0 transmission test program
Trouble shoot -> disable DBG (even the KERN_DEBUG) in the UDC driver
@ -> TRATS2 # dfu 0 mmc 0
 =========================================================================================
File: dat_960.img
TX: md5sum:2947e8f573bc34ba3791f991bd2da869
RX: md5sum:cde0b2024d8ddb55eee9d4b51264ad67
    -------> FAILED

with this patch:

Init script for generating data necessary for DFU test script OK
=========================================================================================
DFU EP0 transmission test program
Trouble shoot -> disable DBG (even the KERN_DEBUG) in the UDC driver
@ -> TRATS2 # dfu 0 mmc 0
 =========================================================================================
File: dat_960.img
TX: md5sum:2947e8f573bc34ba3791f991bd2da869
RX: md5sum:2947e8f573bc34ba3791f991bd2da869
    -------> OK
---
 test/dfu/dfu_gadget_test.sh | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/test/dfu/dfu_gadget_test.sh b/test/dfu/dfu_gadget_test.sh
index 2f5b7db..5084a29 100755
--- a/test/dfu/dfu_gadget_test.sh
+++ b/test/dfu/dfu_gadget_test.sh
@@ -45,6 +45,7 @@ dfu_test_file () {
     printf "$COLOUR_GREEN ========================================================================================= $COLOUR_DEFAULT\n"
     printf "File:$COLOUR_GREEN %s $COLOUR_DEFAULT\n" $1
 
+    filesize=$(stat -c '%s' $1)
     dfu-util -D $1 -a $TARGET_ALT_SETTING >> $LOG_FILE 2>&1 || die $?
 
     echo -n "TX: "
@@ -54,9 +55,11 @@ dfu_test_file () {
 
     dfu-util -D ${DIR}/dfudummy.bin -a $TARGET_ALT_SETTING_B >> $LOG_FILE 2>&1 || die $?
 
+    N_FILE_FULL=$DIR$RCV_DIR${1:2}"_rcv_full"
     N_FILE=$DIR$RCV_DIR${1:2}"_rcv"
 
-    dfu-util -U $N_FILE -a $TARGET_ALT_SETTING >> $LOG_FILE 2>&1 || die $?
+    dfu-util -U $N_FILE_FULL -a $TARGET_ALT_SETTING >> $LOG_FILE 2>&1 || die $?
+    dd if=$N_FILE_FULL of=$N_FILE bs=$filesize count=1 >> $LOG_FILE 2>&1 || die $?
 
     echo -n "RX: "
     calculate_md5sum $N_FILE
-- 
1.8.3.1

[U-Boot] [PATCH] test: dfu: script: wrong md5sum on nand partitions

[Stephen Warren]

On 09/12/2014 12:27 AM, Heiko Schocher wrote:
> when uplaoding a file, at least from a nand partition, the complete
> mtd nand partition size is transferred. This leads in a wrong md5sum
> as the filesize is different between the downloaded file and the
> uploaded file. Limit the uploaded filesize to the downloaded
> fixes this.

I was going to say that it'd be better to fix U-Boot's NAND code to 
transfer the correct amount of data. However, I suppose the correct 
amount *is* the whole partition for anything other than a filesystem 
file. As such, I'd suggest replacing "nand" in the patch description 
with something else, since presumably the exact same issue applies to 
partitions on eMMC. The issue applies to any partition.

I wonder if there's a way to distinguish between file tests and 
partition tests in dfu_gadget_test.sh, so that the $N_FILE_FULL -> 
$N_FILE conversion can be applied only for partitions. Otherwise, a file 
upload/download test could end up changing (increasing) the file length 
and this bug wouldn't be detected.

[U-Boot] [PATCH] test: dfu: script: wrong md5sum on nand partitions

[Heiko Schocher]

Hello Stephen,

add Lukasz to Cc ...

Am 12.09.2014 16:53, schrieb Stephen Warren:
> On 09/12/2014 12:27 AM, Heiko Schocher wrote:
>> when uplaoding a file, at least from a nand partition, the complete
>> mtd nand partition size is transferred. This leads in a wrong md5sum
>> as the filesize is different between the downloaded file and the
>> uploaded file. Limit the uploaded filesize to the downloaded
>> fixes this.
>
> I was going to say that it'd be better to fix U-Boot's NAND code to transfer the correct amount of data. However, I suppose the correct amount *is* the whole partition for anything other than a filesystem file. As such, I'd suggest replacing "nand" in the
> patch description with something else, since presumably the exact same issue applies to partitions on eMMC. The issue applies to any partition.

Yes, you are right.

> I wonder if there's a way to distinguish between file tests and partition tests in dfu_gadget_test.sh, so that the $N_FILE_FULL -> $N_FILE conversion can be applied only for partitions. Otherwise, a file upload/download test could end up changing
> (increasing) the file length and this bug wouldn't be detected.

Lukasz could better comment on this ... currently filesize is changed
on a raw partition, as the test download for example 960 bytes, and
reads back the hole partition size ... which leads in a wrong md5sum.

It would be good to have in the DFU protcol a length parameter ...
so a device could at transfer start decide, if the filesize fits into
the partition, if not, no need to tranfser the hole file, and detect
this error at the end (with a broken partition now) ...

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

[U-Boot] [PATCH] test: dfu: script: wrong md5sum on nand partitions

[Lukasz Majewski]

Hi Heiko,

> Hello Stephen,
> 
> add Lukasz to Cc ...
> 
> Am 12.09.2014 16:53, schrieb Stephen Warren:
> > On 09/12/2014 12:27 AM, Heiko Schocher wrote:
> >> when uplaoding a file, at least from a nand partition, the complete
> >> mtd nand partition size is transferred. This leads in a wrong
> >> md5sum as the filesize is different between the downloaded file
> >> and the uploaded file. Limit the uploaded filesize to the
> >> downloaded fixes this.
> >
> > I was going to say that it'd be better to fix U-Boot's NAND code to
> > transfer the correct amount of data. However, I suppose the correct
> > amount *is* the whole partition for anything other than a
> > filesystem file. As such, I'd suggest replacing "nand" in the patch
> > description with something else, since presumably the exact same
> > issue applies to partitions on eMMC. The issue applies to any
> > partition.
> 
> Yes, you are right.

It is the case with testing "raw"/"partition" write. 

With NAND the procedure is as follows:
1. Erase NAND (0xFF on the whole partition)
2. Store the new partition (very rarely the partition.img == NAND
partition size). Very often partition.img < NAND partition.
3. The NAND code reads the whole partition (including the 0xFF padding).

This padding causes md5sum to be wrong.

> 
> > I wonder if there's a way to distinguish between file tests and
> > partition tests in dfu_gadget_test.sh, so that the $N_FILE_FULL ->
> > $N_FILE conversion can be applied only for partitions. Otherwise, a
> > file upload/download test could end up changing (increasing) the
> > file length and this bug wouldn't be detected.
> 
> Lukasz could better comment on this ... currently filesize is changed
> on a raw partition, as the test download for example 960 bytes, and
> reads back the hole partition size ... which leads in a wrong md5sum.
> 
> It would be good to have in the DFU protcol a length parameter ...
> so a device could at transfer start decide, if the filesize fits into
> the partition, if not, no need to tranfser the hole file, and detect
> this error at the end (with a broken partition now) ...

Yes, the lack of "size" parameter is very inconvenient (for example
we must then allocate bigger buffers than needed). 

However, for testing purposes we can introduce new test file name
"part_<file_size>.img" - e.g. part_32M.img.
Then this would indicate the test for the whole partition.

The legacy dat_<file_size>.img (e.g. dat_960.img) would be truncated if
needed in the script. 

This is a quick solution.

Other thoughts:

1. In the dfu-util repo there is the dfu-prefix tool. It allows
generating some special prefix for LPC and TI Stellaris SoCs. 
Maybe we could think about "u-boot" prefix and put there for example
binary size and signature data.

2. The dfu-util 0.7 on the HOST when -l is typed shows the name="" and
serial="". If it was possible to add serial, then we could extend
dfu-util to show size of the alt setting.

In this way we could always perform truncation on the uploaded file in
the host.

> 
> bye,
> Heiko



-- 
Best regards,

Lukasz Majewski

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

[U-Boot] [PATCH] test: dfu: script: wrong md5sum on nand partitions

[Heiko Schocher]

Hello Lukasz,

Am 15.09.2014 11:45, schrieb Lukasz Majewski:
> Hi Heiko,
>
>> Hello Stephen,
>>
>> add Lukasz to Cc ...
>>
>> Am 12.09.2014 16:53, schrieb Stephen Warren:
>>> On 09/12/2014 12:27 AM, Heiko Schocher wrote:
>>>> when uplaoding a file, at least from a nand partition, the complete
>>>> mtd nand partition size is transferred. This leads in a wrong
>>>> md5sum as the filesize is different between the downloaded file
>>>> and the uploaded file. Limit the uploaded filesize to the
>>>> downloaded fixes this.
>>>
>>> I was going to say that it'd be better to fix U-Boot's NAND code to
>>> transfer the correct amount of data. However, I suppose the correct
>>> amount *is* the whole partition for anything other than a
>>> filesystem file. As such, I'd suggest replacing "nand" in the patch
>>> description with something else, since presumably the exact same
>>> issue applies to partitions on eMMC. The issue applies to any
>>> partition.
>>
>> Yes, you are right.
>
> It is the case with testing "raw"/"partition" write.
>
> With NAND the procedure is as follows:
> 1. Erase NAND (0xFF on the whole partition)
> 2. Store the new partition (very rarely the partition.img == NAND
> partition size). Very often partition.img<  NAND partition.
> 3. The NAND code reads the whole partition (including the 0xFF padding).
>
> This padding causes md5sum to be wrong.

Yes.

>>> I wonder if there's a way to distinguish between file tests and
>>> partition tests in dfu_gadget_test.sh, so that the $N_FILE_FULL ->
>>> $N_FILE conversion can be applied only for partitions. Otherwise, a
>>> file upload/download test could end up changing (increasing) the
>>> file length and this bug wouldn't be detected.
>>
>> Lukasz could better comment on this ... currently filesize is changed
>> on a raw partition, as the test download for example 960 bytes, and
>> reads back the hole partition size ... which leads in a wrong md5sum.
>>
>> It would be good to have in the DFU protcol a length parameter ...
>> so a device could at transfer start decide, if the filesize fits into
>> the partition, if not, no need to tranfser the hole file, and detect
>> this error at the end (with a broken partition now) ...
>
> Yes, the lack of "size" parameter is very inconvenient (for example
> we must then allocate bigger buffers than needed).
>
> However, for testing purposes we can introduce new test file name
> "part_<file_size>.img" - e.g. part_32M.img.
> Then this would indicate the test for the whole partition.

Hmm... yes, but I fear, that every board has different partition size,
so we need a lot of files ...

> The legacy dat_<file_size>.img (e.g. dat_960.img) would be truncated if
> needed in the script.
>
> This is a quick solution.

Yes ... I do not prefer this solution. Why not cutting the readden
file to the length we transferred? Or fill the original file with
0xff until we reached the filesize we read from the device?

> Other thoughts:
>
> 1. In the dfu-util repo there is the dfu-prefix tool. It allows
> generating some special prefix for LPC and TI Stellaris SoCs.
> Maybe we could think about "u-boot" prefix and put there for example
> binary size and signature data.

Hmm.. I do not like such special headers ... but this is just a
personal opinion ...

> 2. The dfu-util 0.7 on the HOST when -l is typed shows the name="" and
> serial="". If it was possible to add serial, then we could extend
> dfu-util to show size of the alt setting.

[root at ts8 ~]# dfu-util -l
dfu-util 0.7

Copyright 2005-2008 Weston Schmidt, Harald Welte and OpenMoko Inc.
Copyright 2010-2012 Tormod Volden and Stefan Schmidt
This program is Free Software and has ABSOLUTELY NO WARRANTY
Please report bugs to dfu-util at lists.gnumonks.org

Found DFU: [0908:02c5] devnum=0, cfg=1, intf=0, alt=0, name="spl"
Found DFU: [0908:02c5] devnum=0, cfg=1, intf=0, alt=1, name="spl.backup1"
Found DFU: [0908:02c5] devnum=0, cfg=1, intf=0, alt=2, name="spl.backup2"
Found DFU: [0908:02c5] devnum=0, cfg=1, intf=0, alt=3, name="spl.backup3"
Found DFU: [0908:02c5] devnum=0, cfg=1, intf=0, alt=4, name="u-boot"
Found DFU: [0908:02c5] devnum=0, cfg=1, intf=0, alt=5, name="kernel_a"
Found DFU: [0908:02c5] devnum=0, cfg=1, intf=0, alt=6, name="kernel_b"
Found DFU: [0908:02c5] devnum=0, cfg=1, intf=0, alt=7, name="rootfs"
[root at ts8 ~]#

I do not see "serial=" for the siemens boards ...

But if we could add here a "size", that would be a good thing to
have, so we coud at least read the size, and if our image fits not
in it, break the transfer, before starting it ...

and for the test script, we could read the size, and create a
file, which fits into the partition ...

> In this way we could always perform truncation on the uploaded file in
> the host.

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

[U-Boot] [PATCH] test: dfu: script: wrong md5sum on nand partitions

[Lukasz Majewski]

Hi Heiko,

> Hello Lukasz,
> 
> Am 15.09.2014 11:45, schrieb Lukasz Majewski:
> > Hi Heiko,
> >
> >> Hello Stephen,
> >>
> >> add Lukasz to Cc ...
> >>
> >> Am 12.09.2014 16:53, schrieb Stephen Warren:
> >>> On 09/12/2014 12:27 AM, Heiko Schocher wrote:
> >>>> when uplaoding a file, at least from a nand partition, the
> >>>> complete mtd nand partition size is transferred. This leads in a
> >>>> wrong md5sum as the filesize is different between the downloaded
> >>>> file and the uploaded file. Limit the uploaded filesize to the
> >>>> downloaded fixes this.
> >>>
> >>> I was going to say that it'd be better to fix U-Boot's NAND code
> >>> to transfer the correct amount of data. However, I suppose the
> >>> correct amount *is* the whole partition for anything other than a
> >>> filesystem file. As such, I'd suggest replacing "nand" in the
> >>> patch description with something else, since presumably the exact
> >>> same issue applies to partitions on eMMC. The issue applies to any
> >>> partition.
> >>
> >> Yes, you are right.
> >
> > It is the case with testing "raw"/"partition" write.
> >
> > With NAND the procedure is as follows:
> > 1. Erase NAND (0xFF on the whole partition)
> > 2. Store the new partition (very rarely the partition.img == NAND
> > partition size). Very often partition.img<  NAND partition.
> > 3. The NAND code reads the whole partition (including the 0xFF
> > padding).
> >
> > This padding causes md5sum to be wrong.
> 
> Yes.
> 
> >>> I wonder if there's a way to distinguish between file tests and
> >>> partition tests in dfu_gadget_test.sh, so that the $N_FILE_FULL ->
> >>> $N_FILE conversion can be applied only for partitions. Otherwise,
> >>> a file upload/download test could end up changing (increasing) the
> >>> file length and this bug wouldn't be detected.
> >>
> >> Lukasz could better comment on this ... currently filesize is
> >> changed on a raw partition, as the test download for example 960
> >> bytes, and reads back the hole partition size ... which leads in a
> >> wrong md5sum.
> >>
> >> It would be good to have in the DFU protcol a length parameter ...
> >> so a device could at transfer start decide, if the filesize fits
> >> into the partition, if not, no need to tranfser the hole file, and
> >> detect this error at the end (with a broken partition now) ...
> >
> > Yes, the lack of "size" parameter is very inconvenient (for example
> > we must then allocate bigger buffers than needed).
> >
> > However, for testing purposes we can introduce new test file name
> > "part_<file_size>.img" - e.g. part_32M.img.
> > Then this would indicate the test for the whole partition.
> 
> Hmm... yes, but I fear, that every board has different partition size,
> so we need a lot of files ...
> 
> > The legacy dat_<file_size>.img (e.g. dat_960.img) would be
> > truncated if needed in the script.
> >
> > This is a quick solution.
> 
> Yes ... I do not prefer this solution. Why not cutting the readden
> file to the length we transferred? 

I think that the truncation of the received file is acceptable.
If we were about to test partition write, then we could prepare images
with the size equal to the partition size.

> Or fill the original file with
> 0xff until we reached the filesize we read from the device?
> 
> > Other thoughts:
> >
> > 1. In the dfu-util repo there is the dfu-prefix tool. It allows
> > generating some special prefix for LPC and TI Stellaris SoCs.
> > Maybe we could think about "u-boot" prefix and put there for example
> > binary size and signature data.
> 
> Hmm.. I do not like such special headers ... but this is just a
> personal opinion ...
> 
> > 2. The dfu-util 0.7 on the HOST when -l is typed shows the name=""
> > and serial="". If it was possible to add serial, then we could
> > extend dfu-util to show size of the alt setting.
> 
> [root at ts8 ~]# dfu-util -l
> dfu-util 0.7
> 
> Copyright 2005-2008 Weston Schmidt, Harald Welte and OpenMoko Inc.
> Copyright 2010-2012 Tormod Volden and Stefan Schmidt
> This program is Free Software and has ABSOLUTELY NO WARRANTY
> Please report bugs to dfu-util at lists.gnumonks.org
> 
> Found DFU: [0908:02c5] devnum=0, cfg=1, intf=0, alt=0, name="spl"
> Found DFU: [0908:02c5] devnum=0, cfg=1, intf=0, alt=1,
> name="spl.backup1" Found DFU: [0908:02c5] devnum=0, cfg=1, intf=0,
> alt=2, name="spl.backup2" Found DFU: [0908:02c5] devnum=0, cfg=1,
> intf=0, alt=3, name="spl.backup3" Found DFU: [0908:02c5] devnum=0,
> cfg=1, intf=0, alt=4, name="u-boot" Found DFU: [0908:02c5] devnum=0,
> cfg=1, intf=0, alt=5, name="kernel_a" Found DFU: [0908:02c5]
> devnum=0, cfg=1, intf=0, alt=6, name="kernel_b" Found DFU:
> [0908:02c5] devnum=0, cfg=1, intf=0, alt=7, name="rootfs" [root at ts8
> ~]#
> 
> I do not see "serial=" for the siemens boards ...

This is strange. Could you checkout and build following dfu-util
version:

Repository:
dfu-util-repo   git://gitorious.org/dfu-util/dfu-util.git

SHA1: dc15eefbb8f928aed932fa1e8daa341e34d0b096

With exynos I see the serial="" output.

> 
> But if we could add here a "size", that would be a good thing to
> have, so we coud at least read the size, and if our image fits not
> in it, break the transfer, before starting it ...
> 
> and for the test script, we could read the size, and create a
> file, which fits into the partition ...
> 
> > In this way we could always perform truncation on the uploaded file
> > in the host.
> 
> bye,
> Heiko



-- 
Best regards,

Lukasz Majewski

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

[U-Boot] [PATCH] test: dfu: script: wrong md5sum on nand partitions

[Scott Wood]

On Mon, 2014-09-15 at 13:55 +0200, Lukasz Majewski wrote:
> Hi Heiko,
> 
> > Hello Lukasz,
> > 
> > Am 15.09.2014 11:45, schrieb Lukasz Majewski:
> > > The legacy dat_<file_size>.img (e.g. dat_960.img) would be
> > > truncated if needed in the script.
> > >
> > > This is a quick solution.
> > 
> > Yes ... I do not prefer this solution. Why not cutting the readden
> > file to the length we transferred? 
> 
> I think that the truncation of the received file is acceptable.
> If we were about to test partition write, then we could prepare images
> with the size equal to the partition size.

Keep in mind that the exact partition size can vary, even for the same
board, due to bad blocks.

-Scott