[U-Boot] arch/arm/lib/board.c - uninitialized vars

[U-Boot] arch/arm/lib/board.c - uninitialized vars

[Wolfgang Denk]

Hi,

I'm trying to clean up some warnings/errors detected when running
"cppcheck" on the U-Boot source tree.  For arch/arm/lib/board.c
I get this:

[arch/arm/lib/board.c:445]: (error) Uninitialized variable: id
[arch/arm/lib/board.c:422]: (error) Uninitialized variable: addr_sp

The problem is not usually detected by GCC depending on which macros
are active, here especially CONFIG_SPL_BUILD

The relevant code was last touched / introduced by commit f1d2b313:
"ARM: add relocation support" some two years ago...

I have some questions regarding the CONFIG_SPL_BUILD "else" case
(i. e. when building with CONFIG_SPL_BUILD defined):

422         addr_sp += 128; /* leave 32 words for abort-stack   */

Is this correct?  The stack is growing downward, so should the '+' not
be replaced by a '-', like we do a few lines above:

412         /* leave 3 words for abort-stack    */
413         addr_sp -= 12;

Why do we need 128 words in the CONFIG_SPL_BUILD case, but only 3
otherwise?

Should we not move the "alignment for ABI compliance" part outside the
CONFIG_SPL_BUILD if/else case, i. e. should this not always be done?


And of course, how should we correctly initialize the "id" and
"addr_sp" variable in both cases?

Best regards,

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: wd at denx.de
Der Dativ ist dem Genitiv sein Tod.

[U-Boot] arch/arm/lib/board.c - uninitialized vars

[Heiko Schocher]

Hello Wolfgang,

Am 06.11.2014 12:28, schrieb Wolfgang Denk:
> Hi,
>
> I'm trying to clean up some warnings/errors detected when running
> "cppcheck" on the U-Boot source tree.  For arch/arm/lib/board.c
> I get this:
>
> [arch/arm/lib/board.c:445]: (error) Uninitialized variable: id
> [arch/arm/lib/board.c:422]: (error) Uninitialized variable: addr_sp
>
> The problem is not usually detected by GCC depending on which macros
> are active, here especially CONFIG_SPL_BUILD
>
> The relevant code was last touched / introduced by commit f1d2b313:
> "ARM: add relocation support" some two years ago...
>
> I have some questions regarding the CONFIG_SPL_BUILD "else" case
> (i. e. when building with CONFIG_SPL_BUILD defined):
>
> 422         addr_sp += 128; /* leave 32 words for abort-stack   */
>
> Is this correct?  The stack is growing downward, so should the '+' not
> be replaced by a '-', like we do a few lines above:

No, this is a typo ... it must be a "-"

>
> 412         /* leave 3 words for abort-stack    */
> 413         addr_sp -= 12;
>
> Why do we need 128 words in the CONFIG_SPL_BUILD case, but only 3
> otherwise?

Good question ...

> Should we not move the "alignment for ABI compliance" part outside the
> CONFIG_SPL_BUILD if/else case, i. e. should this not always be done?

Yes, I think thats correct.

But looking into common/board_f.c ... there it is also done only
for the not SPL case ...

going back in history ...

http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/cpu/arm926ejs/start.S;h=cf40ce12928359c5238d02a32ed361ccc153c101;hb=a59e27997637a2395ae2cc7f809127f24119a167
here I see:

leave 32 words for abort-stack
leave 3 words for abort-stack
8-byte alignment for ABI compliance

without SPL (former PRELOADER) define mess ...

http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/cpu/arm1136/start.S;h=41eb82dae246b9509545f051daac605a02b7db05;hb=a59e27997637a2395ae2cc7f809127f24119a167#l179
here:

#ifdef CONFIG_PRELOADER
leave 32 words for abort-stack
#else
leave 3 words for abort-stack
#endif
8-byte alignment for ABI compliance

http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/cpu/pxa/start.S;h=e07c8c2e0e70744c45152c1649cef65f87825708;hb=a59e27997637a2395ae2cc7f809127f24119a167
here
leave 3 words for abort-stack
8-byte alignment for ABI compliance

no "leave 32 words for abort-stack"

... :-(

> And of course, how should we correctly initialize the "id" and
> "addr_sp" variable in both cases?

Hmm.. they are initialized for the !SPL case ... and in the
SPL case, board__init_f is used from ./arch/arm/lib/spl.c
or overwritten from SoC dependend code, like done in:

./arch/arm/cpu/arm926ejs/davinci/spl.c

-> so I think, we can drop the CONFIG_SPL_BUILD in
u-boot:/arch/arm/lib/board.c board_init_f(), as it is not used, thats
maybe the reason, why this issue never poped up!

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

[U-Boot] arch/arm/lib/board.c - uninitialized vars

[Wolfgang Denk]

Dear Heiko,

In message <545B6539.9090309@denx.de> you wrote:
> 
> -> so I think, we can drop the CONFIG_SPL_BUILD in
> u-boot:/arch/arm/lib/board.c board_init_f(), as it is not used, thats
> maybe the reason, why this issue never poped up!

I agree - this appears to be dead code.

Albert, what do you think?


And, most important: Who is going to provide a patch?

Best regards,

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: wd at denx.de
He'd been wrong, there _was_ a light at the end of the tunnel, and it
was a flamethrower.                         - Terry Pratchett, _Mort_