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

[Joakim Tjernlund]

Mike Frysinger <vapier at gentoo.org> wrote on 31/12/2009 19:44:40:

Happy new year :)

>
> 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.

>
> > --- 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.

>
> > --- a/include/linux/ctype.h
> > +++ b/include/linux/ctype.h
> >
> > -extern unsigned char _ctype[];
> > +extern const unsigned char _ctype[];
> >
> > --- a/lib_generic/ctype.c
> > +++ b/lib_generic/ctype.c
> >
> > -unsigned char _ctype[] = {
> > +const unsigned char _ctype[] = {
> >
> > --- a/tools/updater/ctype.c
> > +++ b/tools/updater/ctype.c
> >
> > -unsigned char _ctype[] = {
> > +const unsigned char _ctype[] = {
>
> seems like these const changes should be split and pushed separately

Yeah, can't remember if this was required or if this was just something
I noticed while adding LINK_OFF

>
> > --- 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.

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.

      Jocke