[PATCH v2 32/32] lmb: add logic to print lmb flag strings

[Sughosh Ganu]

On Fri, 16 Aug 2024 at 02:03, Simon Glass <sjg at chromium.org> wrote:
>
> Hi Sughosh,
>
> On Wed, 14 Aug 2024 at 05:03, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> >
> > Instead of printing the LMB flags as numerical values, print them as
> > strings. This makes it easier to understand what flags are associated
> > with the lmb region. Also make corresponding changes to the bdinfo
> > command's test code.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> > ---
> > Changes since V1: New patch
> >
> >  lib/lmb.c         | 18 ++++++++++++++++--
> >  test/cmd/bdinfo.c |  4 ++--
> >  2 files changed, 18 insertions(+), 4 deletions(-)
>
> Reviewed-by: Simon Glass <sjg at chromium.org>
>
> But see below
>
> >
> > diff --git a/lib/lmb.c b/lib/lmb.c
> > index 37d2a72951..5c5b3e9bb5 100644
> > --- a/lib/lmb.c
> > +++ b/lib/lmb.c
> > @@ -26,6 +26,19 @@ DECLARE_GLOBAL_DATA_PTR;
> >
> >  static struct lmb lmb;
> >
> > +static void print_region_flags(enum lmb_flags flags)
> > +{
> > +       uint64_t bitpos;
> > +       const char *flag_str[] = { "LMB_NONE", "LMB_NOMAP", "LMB_NOOVERWRITE" };
>
> As mentioned, LMB_NONE shouldn't be a flag. For the other two, how
> about "no-map" and "no-overwrite"?

So, you don't want any value to be shown with LMB_NONE? I guess
LMB_NONE is indicative of the fact that the region does not have any
attributes, no?

-sughosh

>
> > +
> > +       do {
> > +               bitpos = fls(flags) - 1;
> > +               printf("%s", flag_str[bitpos]);
> > +               flags &= ~(1ull << bitpos);
> > +               flags ? puts(", ") : puts("\n");
> > +       } while (flags);
> > +}
> > +
> >  static void lmb_dump_region(struct alist *lmb_rgn_lst, char *name)
> >  {
> >         struct lmb_region *rgn = lmb_rgn_lst->data;
> > @@ -41,8 +54,9 @@ static void lmb_dump_region(struct alist *lmb_rgn_lst, char *name)
> >                 end = base + size - 1;
> >                 flags = rgn[i].flags;
> >
> > -               printf(" %s[%d]\t[0x%llx-0x%llx], 0x%08llx bytes flags: %x\n",
> > -                      name, i, base, end, size, flags);
> > +               printf(" %s[%d]\t[0x%llx-0x%llx], 0x%08llx bytes flags: ",
> > +                      name, i, base, end, size);
> > +               print_region_flags(flags);
> >         }
> >  }
> >
> > diff --git a/test/cmd/bdinfo.c b/test/cmd/bdinfo.c
> > index 7dd3f7ca5b..887defc28a 100644
> > --- a/test/cmd/bdinfo.c
> > +++ b/test/cmd/bdinfo.c
> > @@ -120,8 +120,8 @@ static int lmb_test_dump_region(struct unit_test_state *uts,
> >                         ut_assert_nextlinen(" %s[%d]\t[", name, i);
> >                         continue;
> >                 }
> > -               ut_assert_nextline(" %s[%d]\t[0x%llx-0x%llx], 0x%08llx bytes flags: %x",
> > -                                  name, i, base, end, size, flags);
> > +               ut_assert_nextlinen(" %s[%d]\t[0x%llx-0x%llx], 0x%08llx bytes flags: ",
> > +                                  name, i, base, end, size);
> >         }
> >
> >         return 0;
> > --
> > 2.34.1
> >
>
> Regards,
> Simon