CONFIG_IS_ENABLED vs IS_ENABLED

[Simon Glass]

Hi Tom,

On Thu, 26 Jan 2023 at 11:16, Tom Rini <trini at konsulko.com> wrote:
>
> On Thu, Jan 26, 2023 at 11:04:21AM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Thu, 26 Jan 2023 at 10:21, Tom Rini <trini at konsulko.com> wrote:
> > >
> > > On Tue, Jan 24, 2023 at 06:36:00PM -0700, Simon Glass wrote:
> > > > Hi Troy,
> > > >
> > > > On Tue, 24 Jan 2023 at 16:31, Troy Kisky <Troy.Kisky at lairdconnect.com> wrote:
> > > > >
> > > > >
> > > > >
> > > > > ________________________________
> > > > > From: Troy Kisky
> > > > > Sent: Tuesday, January 24, 2023 2:52 PM
> > > > > To: u-boot at lists.denx.de <u-boot at lists.denx.de>; sbabic at denx.de <sbabic at denx.de>; trini at konsulko.com <trini at konsulko.com>; festevam at gmail.com <festevam at gmail.com>
> > > > > Cc: sjg at chromium.org <sjg at chromium.org>; marex at denx.de <marex at denx.de>
> > > > > Subject: CONFIG_IS_ENABLED vs IS_ENABLED
> > > > >
> > > > > Hi Guys
> > > > >
> > > > > In a recent debugging session, I stumbled across this line
> > > > > drivers/mmc/mmc.c:      if (CONFIG_IS_ENABLED(MMC_QUIRKS) && mmc->quirks & quirk)
> > > > >
> > > > > which prevents retries in SPL code, and was causing booting from an SD card to fail.
> > > > > So I wrote a little script to print uses of
> > > > > CONFIG_IS_ENABLED(x) which might need to be
> > > > > IS_ENABLED(CONFIG_x) like the above one.
> > > > >
> > > > > Here it is if you want to try it out.
> > > > >
> > > > > git grep CONFIG_IS_ENABLED|sed -n -e "s/\(CONFIG_IS_ENABLED([0-9a-zA-Z_]*)\)/\n\1\n/gp"| \
> > > > > sed -n -r "s/CONFIG_IS_ENABLED\(([0-9a-zA-Z_]+)\)/\1/p" |sort -u|xargs -I {} \
> > > > > sh -c "git grep -E 'config [ST]PL_{}' | grep -q -E -w '[ST]PL_{}' || git grep 'CONFIG_IS_ENABLED({})'"
> > > > >
> > > > > It prints CONFIG_IS_ENABLED(x) uses where there is no SPL_x or TPL_x.
> > > > >
> > > > > BR
> > > > > Troy
> > > > >
> > > > > _______
> > > > > And here is the opposite check
> > > > >
> > > > > git grep -w IS_ENABLED|sed -n -e "s/\(IS_ENABLED(CONFIG_[0-9a-zA-Z_]*)\)/\n\1\n/gp"| \
> > > > > sed -n -r "s/IS_ENABLED\(CONFIG_([0-9a-zA-Z_]+)\)/\1/p" |sort -u|xargs -I {} \
> > > > > sh -c "git grep -E 'config [ST]PL_{}' | grep -q -E -w '[ST]PL_{}' && git grep 'IS_ENABLED(CONFIG_{})'"
> > > > >
> > > > >
> > > > > It prints uses of IS_ENABLED(CONFIG_x) where CONFIG_SPL_x exists.
> > > >
> > > > Thank you for that. We definitely have quite a few of these.
> > > >
> > > > By a great coincidence I updated moveconfig.py to do something a
> > > > little like that:.
> > > >
> > > > https://patchwork.ozlabs.org/project/uboot/patch/20230123220031.3540724-2-sjg@chromium.org/
> > >
> > > I think this also shows that we might really want to just drop the
> > > checkpatch.pl note about IS_ENABLED / CONFIG_IS_ENABLED, it's getting
> > > used in a lot of wrong places where it's not helpful. It's not the root
> > > cause here (where a compile time check that allows for the rest of the
> > > code to be statically checked still is OK), but it's part of the
> > > problem.
> >
> > Firstly, we want to drop the use of #ifdef so what should we say instead?
>
> I'm not sure that dropping #ifdef in and of itself is a good goal.
> #if IS_ENABLED(CONFIG_FOO)
> does not read better
> #ifdef CONFIG_FOO

So far in my prototype I have implemented

#if CONFIG(FOO)

and

if (CONFIG(FOO))

which replaces direct use of CONFIG_FOO and also
CONFIG_IS_ENABLED(FOO). We could ban use of CONFIG_FOO easily enough.

>
> And we have a lot of cases of the former that I'm not sure can
> logically or helpfully be replaced with
>         if (IS_ENABLED(CONFIG_FOO))
> either for functional / legibility reasons (ie it doesn't read better
> and just getting static analysis isn't a great reasons, more indent
> makes the code harder to follow) or isn't possible because things like:
> #ifdef CONFIG_FOO
>         int i = CONFIG_BAZ;
>         ...
> #endif
>
> Can't be replaced with if (IS_ENABLED(CONFIG_FOO)) { ... } when
> CONFIG_BAZ is only ever asked when CONFIG_FOO is enabled. And we can't
> define CONFIG_BAZ to 0 if not set (that leads back to introducing
> CONFIG things being defined).

We have IF_ENABLED_INT() so can do things like that

>
> > Secondly, I think we should fix all this by splitting the config,
> > along the lines of my old series [1]. I hit similar problems to Troy
> > and have modified moveconfig.py to detect these (hence my recent
> > series [2]). I will see if I can get some sort of series out by
> > Monday. I had something pretty close(TM) but it failed on a few qemu
> > tests [3]
>
> I don't disagree with seeing what things look like with a "split" config
> again, but I think that fundamentally Yamada-san was right way back, and
> we really need to move towards separate config files for each stage and
> then combine them when applicable.

Well let's see what you think once I get this next version of the series out.

> That's also not easy, but I'm also
> not sure how to deal with the cases where we intentionally have
> CONFIG_FOO=n CONFIG_SPL_FOO=y without doing what we're doing today, at
> some level.

Well I believe these are bugs, but don't know how many. Will probably
have some idea by early next week.

>
> > Troy, are you able to create patches for the problems you find, and
> > perhaps adding your checks to a Makefile rule or script so we can run
> > it in CI?
>
> What we're doing today does need to be analyzed and some good number of
> cases corrected I am sure.

Yes.

Regards,
Simon