Hi Tom,
On Thu, 26 Jan 2023 at 11:16, Tom Rini <tr...@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 <tr...@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.ki...@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>; sba...@denx.de
> > > > > <sba...@denx.de>; tr...@konsulko.com <tr...@konsulko.com>;
> > > > > feste...@gmail.com <feste...@gmail.com>
> > > > > Cc: s...@chromium.org <s...@chromium.org>; ma...@denx.de
> > > > > <ma...@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-...@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
