CONFIG_IS_ENABLED vs IS_ENABLED

[Simon Glass]

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?

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]

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?

Regards,
Simon

[1] https://patchwork.ozlabs.org/project/uboot/list/?series=262853&state=*
[2] https://patchwork.ozlabs.org/project/uboot/list/?series=338142
[3] https://source.denx.de/u-boot/custodians/u-boot-dm/-/pipelines/14830