Re: CONFIG_IS_ENABLED vs IS_ENABLED

[Tom Rini <trini@konsulko.com>]

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

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

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

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

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

-- 
Tom

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