Dear Michael, Ping!
In message <20101026210959.51803136...@gemini.denx.de> I wrote: > Dear Michael Zaidman, > > In message > <d520c6ef298416a03789ebfa4e05e257b5331693.1284965175.git.michael.zaid...@gmail.com> > you wrote: > > - Revives POST for blackfin arch; > > - Removes redundant code: > > arch/blackfin/lib/post.c > > arch/powerpc/cpu/ppc4xx/commproc.c > > arch/powerpc/cpu/mpc512x/common.c > > - fixes up the post_word_{load|store} usage. > > Unfortunately it turns out that the code now contains a few nasty > bugs... > > ... > > #define CONFIG_SYS_GBL_DATA_OFFSET (CONFIG_SYS_INIT_RAM_END - > > CONFIG_SYS_GBL_DATA_SIZE) > > -#define CONFIG_SYS_POST_WORD_ADDR (CONFIG_SYS_GBL_DATA_OFFSET - 0x4) > > -#define CONFIG_SYS_INIT_SP_OFFSET CONFIG_SYS_POST_WORD_ADDR > > +#define CONFIG_SYS_INIT_SP_OFFSET (CONFIG_SYS_GBL_DATA_OFFSET - 0x4) > > This is a seriously broken design, as it sneaks in storage for a > variable in a storage location where it is not expected. > > The "official" layout is that we have CONFIG_SYS_INIT_RAM_BYTES > available; the top CONFIG_SYS_GBL_DATA_SIZE bytes (now > GENERATED_GBL_DATA_SIZE) are used for global data, and the part below > is used for the stack. No other room is reserved there. > > Shifting down the stack by 4 bytes as it's done here causes that the > stack is not correctly aligned any more, which may cause really nasty > subsequent errors. > > But it's even worse. > > > diff --git a/include/configs/mpc5121-common.h > > b/include/configs/mpc5121-common.h > > index 96fab20..afae1ab 100644 > > --- a/include/configs/mpc5121-common.h > > +++ b/include/configs/mpc5121-common.h > ... > > -#define CONFIG_SYS_POST_WORD_ADDR (CONFIG_SYS_GBL_DATA_OFFSET - 0x4) > > -#define CONFIG_SYS_INIT_SP_OFFSET CONFIG_SYS_POST_WORD_ADDR > > +#define CONFIG_SYS_INIT_SP_OFFSET (CONFIG_SYS_GBL_DATA_OFFSET - 0x4) > > There the same is done, but what happens actually? > > Have a look how the stack setup gets implemented in > "arch/powerpc/cpu/mpc512x/start.S": > > ... > 244 in_flash: > 245 lis r1, (CONFIG_SYS_INIT_RAM_ADDR + > CONFIG_SYS_GBL_DATA_OFFSET)@h > 246 ori r1, r1, (CONFIG_SYS_INIT_RAM_ADDR + > CONFIG_SYS_GBL_DATA_OFFSET)@l > 247 > 248 li r0, 0 /* Make room for stack frame header and */ > 249 stwu r0, -4(r1) /* clear final stack frame so that */ > 250 stwu r0, -4(r1) /* stack backtraces terminate cleanly */ > ... > > As you can see, the code does not use CONFIG_SYS_INIT_SP_OFFSET at > all; instead it performs a calculation which should be redundant, but > in the current code it means that the location of the POST_WORD is > right in the initial stack. > > I did not check if the code for other processors has similar issues. > > > "Reserving" private storage like that is bad, as other involved > parties probably have no knowledge of such a private reservation. > > > Why do we not simply reserve a word in the global data structure instead? > > > This bug needs pretty urgent fixing. > > > Best regards, > > Wolfgang Denk Viele Grüße, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de If a train station is a place where a train stops, then what's a workstation? _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot