[U-Boot] [PATCH 2/4] Use LINK_OFF to access global data

[Mike Frysinger <vapier@gentoo.org>]

On Thursday 31 December 2009 20:39:10 Joakim Tjernlund wrote:
> Mike Frysinger <vapier@gentoo.org> wrote on 31/12/2009 19:44:40:
> > On Wednesday 30 December 2009 10:08:30 Joakim Tjernlund wrote:
> > > --- a/common/cmd_nvedit.c
> > > +++ b/common/cmd_nvedit.c
> > > @@ -512,6 +512,7 @@ char *getenv (char *name)
> > >  {
> > >     int i, nxt;
> > >
> > > +   name = LINK_OFF(name);
> > >     WATCHDOG_RESET();
> > >
> > >     for (i=0; env_get_char(i) != '\0'; i=nxt+1) {
> > > @@ -534,6 +535,7 @@ int getenv_r (char *name, char *buf, unsigned len)
> > >  {
> > >     int i, nxt;
> > >
> > > +   name = LINK_OFF(name);
> > >     for (i=0; env_get_char(i) != '\0'; i=nxt+1) {
> > >        int val, n;
> >
> > you have no guarantee that getenv() is called with a const string which
> > is in the .rodata section.  there's code that generates the env name in a
> > buffer on the stack and gives that to getenv().  does LINK_OFF() still
> > work then ?
> 
> True. LINK_OFF will not work iff link addr != load addr and name isn't a
>  const string. Basically if you want to use the LINK_OFF feature you have
>  to use a const string.

some of the other functions in these patch sets fall into the same issue ... 
like the output functions

> > > --- a/common/console.c
> > > +++ b/common/console.c
> > > @@ -346,7 +346,7 @@ void putc(const char c)
> > >     }
> > >  }
> > >
> > > -void puts(const char *s)
> > > +static void printf_puts(const char *s)
> > >  {
> > >  #ifdef CONFIG_SILENT_CONSOLE
> > >     if (gd->flags & GD_FLG_SILENT)
> > > @@ -367,12 +367,18 @@ void puts(const char *s)
> > >     }
> > >  }
> > >
> > > +void puts(const char *s)
> > > +{
> > > +   printf_puts(LINK_OFF(s));
> > > +}
> >
> > and if CONFIG_LINK_OFF isnt defined, does gcc correctly inline this ?  if
> > not, i think there needs to be #ifdef CONFIG_LINK_OFF handling here.
> 
> Possibly, however LINK_OFF is is a NOP if CONFIG_LINK_OFF isn't defined.

yes, but that doesnt mean gcc takes care of inlining all of printf_puts() into 
the puts() and all the new call sites go to puts()

> > > --- a/lib_generic/crc32.c
> > > +++ b/lib_generic/crc32.c
> > > @@ -156,6 +156,11 @@
> > >  */ uint32_t ZEXPORT crc32 (uint32_t crc, const Bytef *buf, uInt len) {
> > > +#ifdef LINK_OFF
> > > +    const uint32_t *crc_tab = LINK_OFF(crc_table);
> > > +#else
> > > +    const uint32_t *crc_tab = crc_table;
> > > +#endif
> >
> > the patch 1/4 you posted always defines LINK_OFF.  it's CONFIG_LINK_OFF
> > which is dynamic.
> 
> Yes, but LINK_OFF will work too. I can change this though, it looks better.

my point is that it should either be checking CONFIG_LINK_OFF or always using 
LINK_OFF (since it nops when the config is off as you point out)

> The bigger question is if the LINN_OFF changes in general are acceptable to
>  u-boot. Any board/arch that doesn't want this functionality should not
>  notice I think.

i dont have any plans on wanting this, and it seems pretty invasive ... and 
easy to introduce new code that breaks PIC people but no one else really 
notices ...
-mike
-------------- 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/20100101/53c5c142/attachment.pgp