Re: CONFIG_IS_ENABLED vs IS_ENABLED

[Tom Rini <trini@konsulko.com>]

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

On Thu, Jan 26, 2023 at 02:29:53PM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Thu, 26 Jan 2023 at 11:16, Tom Rini <trini@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@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@lairdconnect.com> wrote:
> > > > > >
> > > > > >
> > > > > >
> > > > > > ________________________________
> > > > > > From: Troy Kisky
> > > > > > Sent: Tuesday, January 24, 2023 2:52 PM
> > > > > > To: u-boot@lists.denx.de <u-boot@lists.denx.de>; sbabic@denx.de <sbabic@denx.de>; trini@konsulko.com <trini@konsulko.com>; festevam@gmail.com <festevam@gmail.com>
> > > > > > Cc: sjg@chromium.org <sjg@chromium.org>; marex@denx.de <marex@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.

I'm not convinced #if CONFIG(FOO) is better, but OK.

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

And could come up wit IF_ENABLED_STRING() I suppose. But I'm still not
sure it'll read better / more clearly.

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

OK.

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

There's certainly some bugs, but there's also some intentionally done
ones, such as around SKIP_LOWLEVEL_INIT

-- 
Tom

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