| Message ID | yw1xziki5w3t.fsf@unicorn.mansr.com |
|---|---|
| State | RFC |
| Delegated to: | Tom Rini |
| Headers | show |
On Tue, Nov 29, 2016 at 4:06 AM, Måns Rullgård <mans@mansr.com> wrote: > Tim Harvey <tharvey@gateworks.com> writes: > >> Greetings, >> >> In debugging an issue with a rather old branch of U-Boot (2015-04) I >> found that the static assignment of a data buffer was not 32-bit >> aligned which caused data aborts. However I find that current U-boot >> master does not suffer this issue and no matter what I declare static >> before the particular variable its always 32-bit aligned. I don't see >> any code changes that would cause this and in both cases I'm building >> with the same gcc5.2.0 toolchain. >> >> The code in question is this this from cmd/fdt.c: >> >> } else if (argv[1][0] == 's') { >> char *pathp; /* path */ >> char *prop; /* property */ >> int nodeoffset; /* node offset from libfdt */ >> static char data[SCRATCHPAD]; /* storage for the property */ >> int len; /* new length of the property */ >> int ret; >> >> What guarantee's that 'data' above is 32-bit aligned in master that is >> missing from the older U-Boot branch I'm using? Perhaps there is some >> arg in a Makefile that I'm missing? > > There are no such guarantees. Things often end up 4-byte aligned by > chance simply because most of them are a multiple of 4 bytes in size. > The code should be fixed, either by explicitly aligning the buffer or by > using the put_unaligned_b32() accessor in fdt_parse_prop(). Here's an > untested patch: Måns, My thoughts as well but what I found on a 2015.04 U-Boot adding a 'static char foo[1]' before the 'static char data[SCRATCHPAD]' and printing the addrs: foo:4ff9e0b2 data:4ff9e0b3 (what I would have expected... no alignment guarantees and clearly not quad-byte aligned) The same change on master: foo:4ff9fff4 data:4ff9fbf0 (not at all what I expected - quad byte aligned and more) Thus I figured there was something in a Makefile that was directing the compiler (same compiler in both cases) I find that merely including '<asm/unaligned.h>' (with no code changes) changes the alignment and aligns propery??? Tim
Tim Harvey <tharvey@gateworks.com> writes: > On Tue, Nov 29, 2016 at 4:06 AM, Måns Rullgård <mans@mansr.com> wrote: >> Tim Harvey <tharvey@gateworks.com> writes: >> >>> Greetings, >>> >>> In debugging an issue with a rather old branch of U-Boot (2015-04) I >>> found that the static assignment of a data buffer was not 32-bit >>> aligned which caused data aborts. However I find that current U-boot >>> master does not suffer this issue and no matter what I declare static >>> before the particular variable its always 32-bit aligned. I don't see >>> any code changes that would cause this and in both cases I'm building >>> with the same gcc5.2.0 toolchain. >>> >>> The code in question is this this from cmd/fdt.c: >>> >>> } else if (argv[1][0] == 's') { >>> char *pathp; /* path */ >>> char *prop; /* property */ >>> int nodeoffset; /* node offset from libfdt */ >>> static char data[SCRATCHPAD]; /* storage for the property */ >>> int len; /* new length of the property */ >>> int ret; >>> >>> What guarantee's that 'data' above is 32-bit aligned in master that is >>> missing from the older U-Boot branch I'm using? Perhaps there is some >>> arg in a Makefile that I'm missing? >> >> There are no such guarantees. Things often end up 4-byte aligned by >> chance simply because most of them are a multiple of 4 bytes in size. >> The code should be fixed, either by explicitly aligning the buffer or by >> using the put_unaligned_b32() accessor in fdt_parse_prop(). Here's an >> untested patch: > > Måns, > > My thoughts as well but what I found on a 2015.04 U-Boot adding a > 'static char foo[1]' before the 'static char data[SCRATCHPAD]' and > printing the addrs: > foo:4ff9e0b2 > data:4ff9e0b3 > (what I would have expected... no alignment guarantees and clearly not > quad-byte aligned) > > The same change on master: > foo:4ff9fff4 > data:4ff9fbf0 > (not at all what I expected - quad byte aligned and more) > > Thus I figured there was something in a Makefile that was directing > the compiler (same compiler in both cases) > > I find that merely including '<asm/unaligned.h>' (with no code > changes) changes the alignment and aligns propery??? Compilers are fickle creatures.
diff --git a/cmd/fdt.c b/cmd/fdt.c index b503357dc3a8..560b8cc9df61 100644 --- a/cmd/fdt.c +++ b/cmd/fdt.c @@ -17,6 +17,7 @@ #include <fdt_support.h> #include <mapmem.h> #include <asm/io.h> +#include <asm/unaligned.h> #define MAX_LEVEL 32 /* how deeply nested we will go */ #define SCRATCHPAD 1024 /* bytes of scratchpad memory */ @@ -766,7 +767,7 @@ static int fdt_parse_prop(char * const *newval, int count, char *data, int *len) cp = newp; tmp = simple_strtoul(cp, &newp, 0); - *(__be32 *)data = __cpu_to_be32(tmp); + put_unaligned_be32(tmp, data); data += 4; *len += 4;