Re: [PATCH 09/32] spl: Avoid an #ifdef when printing gd->malloc_ptr

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 4370 bytes --]

On Fri, Sep 22, 2023 at 12:27:36PM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Thu, 21 Sept 2023 at 09:36, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Wed, Sep 20, 2023 at 07:03:34PM -0600, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Wed, 30 Aug 2023 at 15:39, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Wed, Aug 30, 2023 at 12:04:40PM -0600, Simon Glass wrote:
> > > > > Use an accessor in the header file to avoid this.
> > > > >
> > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > ---
> > > > >
> > > > >  common/spl/spl.c                  | 9 +++++----
> > > > >  include/asm-generic/global_data.h | 7 +++++++
> > > > >  2 files changed, 12 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/common/spl/spl.c b/common/spl/spl.c
> > > > > index f0a90c280da..f5cef81000c 100644
> > > > > --- a/common/spl/spl.c
> > > > > +++ b/common/spl/spl.c
> > > > > @@ -876,10 +876,11 @@ void board_init_r(gd_t *dummy1, ulong dummy2)
> > > > >       } else {
> > > > >               debug("Unsupported OS image.. Jumping nevertheless..\n");
> > > > >       }
> > > > > -#if CONFIG_VAL(SYS_MALLOC_F_LEN) && !defined(CONFIG_SPL_SYS_MALLOC_SIZE)
> > > > > -     debug("SPL malloc() used 0x%lx bytes (%ld KB)\n", gd->malloc_ptr,
> > > > > -           gd->malloc_ptr / 1024);
> > > > > -#endif
> > > > > +     if (IS_ENABLED(CONFIG_SYS_MALLOC_F) &&
> > > > > +         !IS_ENABLED(CONFIG_SPL_SYS_MALLOC_SIZE))
> > > > > +             debug("SPL malloc() used 0x%lx bytes (%ld KB)\n",
> > > > > +                   gd_malloc_ptr(), gd_malloc_ptr() / 1024);
> >
> > This is not more readable.  But I guess my comment was unclear as you're
> > mixing changes here.  gd_malloc_ptr() rather than gd->malloc_ptr is a
> > wash, to me.  I think one could argue it's not a helpful abstraction.
> > but it's so that you can avoid CONFIG_VAL here, and say "if (...)" not
> > "#if ..." here.
> 
> Yes,
> 
> >
> > > > > +
> > > > >       bootstage_mark_name(get_bootstage_id(false), "end phase");
> > > > >  #ifdef CONFIG_BOOTSTAGE_STASH
> > > > >       ret = bootstage_stash((void *)CONFIG_BOOTSTAGE_STASH_ADDR,
> > > > > diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
> > > > > index 8fc205ded1a..edf9ff6823f 100644
> > > > > --- a/include/asm-generic/global_data.h
> > > > > +++ b/include/asm-generic/global_data.h
> > > > > @@ -573,6 +573,13 @@ static_assert(sizeof(struct global_data) == GD_SIZE);
> > > > >  #define gd_malloc_start()    0
> > > > >  #define gd_set_malloc_start(val)
> > > > >  #endif
> > > > > +
> > > > > +#if CONFIG_VAL(SYS_MALLOC_F_LEN)
> > > > > +#define gd_malloc_ptr()              gd->malloc_ptr
> > > > > +#else
> > > > > +#define gd_malloc_ptr()              0L
> > > > > +#endif
> > > > > +
> > > > >  /**
> > > > >   * enum gd_flags - global data flags
> > > > >   *
> > > >
> > > > This is another case where readability is not improved. I also have a
> > > > bad feeling that changing that exact area had some unintended
> > > > consequences from the compiler, that totally should not have happened.
> > > > But maybe that was something in a similar code section instead.
> > >
> > > The improvement is in the C file...here we have an accessor in the
> > > header file as has been done elsewhere.
> > >
> > > Do you have any more details on the problem you mention? I will align
> > > the accessor to the struct member which should resolve it.
> >
> > It's unfortunately one of those cases to do a global before/after build
> > and see if anything does or does not get tripped up.  As I believe I did
> > use CONFIG_VAL there and not a check on CONFIG_SYS_MALLOC_F itself for a
> > reason, at the time.
> 
> But the CONFIG_VAL symbols actually depends on CONFIG_SYS_MALLOC_F, so
> I can't imagine what it might be. Of course if the value is 0, then
> the 'if CONFIG_VAL()' test would fail, but to what purpose?

Yes, there's been a time or two where I banged my head against the
figurative wall trying to understand what exactly the compiler could be
seeing as why to change something.  I don't have a "reasonable"
explanation.  Readability aside, "tidy things up" changes need to be on
their own so that their impact can be seen aside from the new
functionality changes.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]