Hi Tom,
On Thu, 26 Jan 2023 at 14:33, Tom Rini <tr...@konsulko.com> wrote:
>
> 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 <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.
>
> I'm not convinced #if CONFIG(FOO) is better, but OK.
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.
No it isn't that great, but we might get used to it. It is rare but
does resolve one of the two main remaining needs for #ifdef
>
> > > > 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
Ah OK. Well if there is SPL_ symbol as well, as in this case, then
perhaps it is clear what the intention is?
Regards,
Simon
