mkimage: Add compatibility option for legacy Multi-File images
RFC PATCH 1/1 mkimage: Add compatibility option for legacy Multi-File images
During a few months, offsets of files in multi-file images were miscalculated, this has been fixed by 02b9b22446e3d7ad6a6382be17a1ce79a7de589b, but unfortunatly, some devices (I'm thinking of the Neo FreeRunner) are using a broken version of U-Boot.
This problem can easily be worked around at image creation time by adding one byte of junk at the end of the first (and optionnally second) file, if its size is a multiple of 4.
It's not really clean, but it shouldn't cause any problem. At least, I haven't encountered any using this patch.
--- tools/mkimage.c | 9 ++++++++- tools/mkimage.h | 1 + 2 files changed, 9 insertions(+), 1 deletions(-)
tools/mkimage.c +8 -1
@@ -243,6 +243,9 @@ main (int argc, char **argv)usage (); params.imagename = *++argv; goto NXTARG; +case 'p' : + params.pflag++; +break ;case 'v' : params.vflag++;break ;
@@ -383,6 +386,8 @@ NXTARG: ; params.cmdname, file, strerror(errno));exit (EXIT_FAILURE ); } +if (params.pflag && sep && (sbuf.st_size %4 ==0 )) + sbuf.st_size +=1 ; size =cpu_to_uimage (sbuf.st_size); }else { size =0 ;
@@ -556,7 +561,8 @@ copy_file (int ifd, const char *datafile, int pad)exit (EXIT_FAILURE ); } -if (pad && ((tail = size %4 ) !=0 )) { + tail = size %4 ; +if (pad && (params.pflag || tail !=0 )) {if (write (ifd, (char *)&zero,4 -tail) !=4 -tail) { fprintf (stderr, "%s: Write error on %s: %s\n",
@@ -586,6 +592,7 @@ usage ()" -e ==> set entry point to 'ep' (hex)\n" " -n ==> set image name to 'name'\n" " -d ==> use image data from 'datafile'\n" +" -p ==> force padding in multi-file images\n" " -x ==> set XIP (execute in place)\n" , params.cmdname); fprintf (stderr, " %s [-D dtc_options] -f fit-image.its fit-image\n",
tools/mkimage.h +1 -0
@@ -58,6 +58,7 @@ struct mkimage_params {int eflag;int fflag;int lflag; +int pflag;int vflag;int xflag;int os;
Signature · 1 line
1.7.1
RFC PATCH 1/1 mkimage: Add compatibility option for legacy Multi-File images
Hi Thibaut,
generally I'm not a fan to include workarounds for bugs which we do not have anymore in mainline U-Boot. Isn't there any other alternative for this? What do other people think?
If nobody objects to the genereal principle, then I have some requests below.
Quoted 8 lines
During a few months, offsets of files in multi-file images were miscalculated, this has been fixed by
02b9b22446e3d7ad6a6382be17a1ce79a7de589b, but unfortunatly, some devices (I'm thinking of the Neo FreeRunner) are using a broken version of U-Boot.This problem can easily be worked around at image creation time by adding one byte of junk at the end of the first (and optionnally second) file, if its size is a multiple of 4.
Hm, as I read it, you add 4 bytes (not one) in case the image is already padded correctly to 32-bit, correct? If so, then please correct the comment.
Quoted 43 lines
It's not really clean, but it shouldn't cause any problem. At least, I haven't encountered any using this patch.
Signed-off-by Thibaut Girka <[email protected]>--- tools/mkimage.c | 9 ++++++++- tools/mkimage.h | 1 + 2 files changed, 9 insertions(+), 1 deletions(-)tools/mkimage.c +7
-1@@ -243,6 +243,9 @@ main (int argc, char **argv)usage (); params.imagename = *++argv; goto NXTARG; +case 'p' : + params.pflag++; +break ;case 'v' : params.vflag++;break ;@@ -383,6 +386,8 @@ NXTARG: ; params.cmdname, file, strerror(errno));exit (EXIT_FAILURE ); } +if (params.pflag && sep && (sbuf.st_size %4 ==0 )) + sbuf.st_size +=1 ; size =cpu_to_uimage (sbuf.st_size); }else { size =0 ;@@ -556,7 +561,8 @@ copy_file (int ifd, const char *datafile, int pad)exit (EXIT_FAILURE ); } -if (pad && ((tail = size %4 ) !=0 )) { + tail = size %4 ; +if (pad && (params.pflag || tail !=0 )) {if (write (ifd, (char *)&zero,4 -tail) !=4 -tail) { fprintf (stderr, "%s: Write error on %s: %s\n",
tools/mkimage.c usage () · line 595
Quoted 4 lines
tools/mkimage.c +1
-0@@ -586,6 +592,7 @@ usage ()" -e ==> set entry point to 'ep' (hex)\n" " -n ==> set image name to 'name'\n" " -d ==> use image data from 'datafile'\n" +" -p ==> force padding in multi-file images\n"
This is no real padding, so please don't make it look like it is. Maybe use "q"(uirk) as an option character and change the description to indicate that this is not a "forced padding" but a "incorrect additional 32-bit padding to work around an old bug (see man-page)". A fix to "doc/mkimage.1" which now exists is also mandatory.
Quoted 8 lines
tools/mkimage.c +0
-0" -x ==> set XIP (execute in place)\n" , params.cmdname); fprintf (stderr, " %s [-D dtc_options] -f fit-image.its fit-image\n",tools/mkimage.h +0
-0
tools/mkimage.h struct mkimage_params { · line 64
Quoted 7 lines
tools/mkimage.h +1
-0@@ -58,6 +58,7 @@ struct mkimage_params {int eflag;int fflag;int lflag; +int pflag;int vflag;int xflag;int os;
Cheers
Detlev
Signature · 9 lines
Helena ist verh?ltnism??ig leicht zu besetzen. Eine Frau, zarteste Jugend
mit sinnlicher Reife verbindend; ?u?erst intelligent, indes von durchaus
weiblicher Denkart; phlegmatisch, aber sensibel; un?bertrefflich sch?n und
dabei von sehr pers?nlichem Charme - mehr wird da nicht verlangt.
-- Peter Hacks
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.deRFC PATCH 1/1 mkimage: Add compatibility option for legacy Multi-File images
Le lundi 30 ao?t 2010 ? 11:29 +0200, Detlev Zundel a ?crit :
Quoted 1 line
Hi Thibaut,
Hi,
Quoted 2 lines
generally I'm not a fan to include workarounds for bugs which we do not have anymore in mainline U-Boot.
Hm, yeah, I can understand that...
Quoted 1 line
Isn't there any other alternative for this?
Well, for my use case, we have to workaround this bug. We can do that (adding a byte at the end of some files) outside of mkimage, but it's really a u-boot thing, so, it could really go in there.
Quoted 2 lines
If nobody objects to the genereal principle, then I have some requests below.
[...]
Quoted 3 lines
Hm, as I read it, you add 4 bytes (not one) in case the image is already padded correctly to 32-bit, correct? If so, then please correct the comment.
Yes, you add one byte to the end of the file itself, and it'll add 4 bytes to the resulting image file.
Quoted 2 lines
Quoted 2 lines
It's not really clean, but it shouldn't cause any problem. At least, I haven't encountered any using this patch.
[...]
Quoted 11 lines
tools/mkimage.c usage () · line 595
Quoted 5 lines
tools/mkimage.c +1
-0@@ -586,6 +592,7 @@ usage ()" -e ==> set entry point to 'ep' (hex)\n" " -n ==> set image name to 'name'\n" " -d ==> use image data from 'datafile'\n" +" -p ==> force padding in multi-file images\n" This is no real padding, so please don't make it look like it is. Maybe use "q"(uirk) as an option character and change the description to indicate that this is not a "forced padding" but a "incorrect additional 32-bit padding to work around an old bug (see man-page)". A fix to "doc/mkimage.1" which now exists is also mandatory.
Yeah, indeed, it's not really a padding, I'll rephrase the command option description and document it in mkimage.1.
Regards,
Thibaut Girka.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100831/a33e0a33/attachment.pgp
RFC PATCH 1/1 mkimage: Add compatibility option for legacy Multi-File images
Hi Thibaut,
Quoted 7 lines
Le lundi 30 ao?t 2010 ? 11:29 +0200, Detlev Zundel a ?crit :
Quoted 1 line
Hi Thibaut,
Hi,
Quoted 2 lines
generally I'm not a fan to include workarounds for bugs which we do not have anymore in mainline U-Boot.
Quoted 8 lines
Hm, yeah, I can understand that...
Quoted 1 line
Isn't there any other alternative for this?
Well, for my use case, we have to workaround this bug. We can do that (adding a byte at the end of some files) outside of mkimage, but it's really a u-boot thing, so, it could really go in there.
If it goes in, can we set a date to remove it again? Or do we have to carry it around forever? If the latter is the case then I'm really not a big fan of it. If you can fix the problem up in the "project where it is a problem", then the fix would better belong there.
Cheers
Detlev
Signature · 8 lines
The mathematician's patterns, like the painter's or the poet's, must be beautiful; the ideas, like the colours or the words, must fit together in a harmonious way. Beauty is the first test: there is no permanent place in the world for ugly mathematics. -- G. H. Hardy -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de
RFC PATCH 1/1 mkimage: Add compatibility option for legacy Multi-File images
In reply to Thibaut GirkaDear Thibaut Girka,
Quoted 10 lines · In message <[email protected]> you wrote:
During a few months, offsets of files in multi-file images were miscalculated, this has been fixed by
02b9b22446e3d7ad6a6382be17a1ce79a7de589b, but unfortunatly, some devices (I'm thinking of the Neo FreeRunner) are using a broken version of U-Boot.This problem can easily be worked around at image creation time by adding one byte of junk at the end of the first (and optionnally second) file, if its size is a multiple of 4.
It's not really clean, but it shouldn't cause any problem. At least, I haven't encountered any using this patch.
In addition to Detlev's comments a few questions:
- Is my understanding correct that there is no problem with the current mainline code?
- You mentioned that commit 02b9b22 (i. e. v1.3.3-rc3-46-g02b9b22) fixed the problem - can you please also state which commit introduced the problem?
- If so, would it then not be better to update U-Boot on the devices that are running a broken version?
As this patch is really adding a quirk for a specific version (or set of versions) of U-Boot only, but without general use for the current mainline code (on contrary, it's making the code more complex without benefit to most of us), I tend to reject this patch for mainline.
If there has been one or more released versions with this problem present, we might consider a maintenance release for these, where we add the commit as a workaround.