[PATCH 02/14] lmb: Use CONFIG_LMB_*_REGIONS only if they are defined
Tom Rini
trini at konsulko.com
Mon Aug 30 14:01:02 CEST 2021
On Mon, Aug 30, 2021 at 11:45:02AM +0200, Marek Vasut wrote:
> On 8/30/21 1:11 AM, Tom Rini wrote:
> > On Mon, Aug 30, 2021 at 01:00:02AM +0200, Marek Vasut wrote:
> > > On 8/30/21 12:51 AM, Tom Rini wrote:
> > > > On Mon, Aug 30, 2021 at 12:40:07AM +0200, Marek Vasut wrote:
> > > > > On 8/30/21 12:23 AM, Tom Rini wrote:
> > > > > > On Mon, Aug 30, 2021 at 12:19:59AM +0200, Marek Vasut wrote:
> > > > > > > On 8/30/21 12:10 AM, Tom Rini wrote:
> > > > > > > > On Sun, Aug 29, 2021 at 11:47:58PM +0200, Marek Vasut wrote:
> > > > > > > > > On 8/29/21 9:32 PM, Tom Rini wrote:
> > > > > > > > > > On Sun, Aug 29, 2021 at 09:24:46PM +0200, Marek Vasut wrote:
> > > > > > > > > > > On 8/29/21 8:02 PM, Tom Rini wrote:
> > > > > > > > > > > > On Sun, Aug 29, 2021 at 06:26:23PM +0200, Marek Vasut wrote:
> > > > > > > > > > > > > On 8/15/21 9:47 PM, Tom Rini wrote:
> > > > > > > > > > > > > > On Sun, Aug 15, 2021 at 08:13:02PM +0200, Marek Vasut wrote:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > > The CONFIG_LMB_*_REGIONS are defined only if CONFIG_LMB is enabled,
> > > > > > > > > > > > > > > protect access to those two config options to avoid undefined macro
> > > > > > > > > > > > > > > errors.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Signed-off-by: Marek Vasut <marek.vasut+renesas at gmail.com>
> > > > > > > > > > > > > > > Cc: Simon Glass <sjg at chromium.org>
> > > > > > > > > > > > > > > Cc: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com>
> > > > > > > > > > > > > > > Cc: Tom Rini <trini at konsulko.com>
> > > > > > > > > > > > > > > ---
> > > > > > > > > > > > > > > include/lmb.h | 4 ++--
> > > > > > > > > > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > diff --git a/include/lmb.h b/include/lmb.h
> > > > > > > > > > > > > > > index 3c4afdf9f0..fa1474a360 100644
> > > > > > > > > > > > > > > --- a/include/lmb.h
> > > > > > > > > > > > > > > +++ b/include/lmb.h
> > > > > > > > > > > > > > > @@ -44,7 +44,7 @@ struct lmb_property {
> > > > > > > > > > > > > > > struct lmb_region {
> > > > > > > > > > > > > > > unsigned long cnt;
> > > > > > > > > > > > > > > unsigned long max;
> > > > > > > > > > > > > > > -#if IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS)
> > > > > > > > > > > > > > > +#if IS_ENABLED(CONFIG_LMB) && IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS)
> > > > > > > > > > > >
> > > > > > > > > > > > This doesn't make sense to me, still. You cannot enable
> > > > > > > > > > > > CONFIG_LMB_USE_MAX_REGIONS without CONFIG_LMB as the former depends on
> > > > > > > > > > > > the latter in Kconfig.
> > > > > > > > > > > >
> > > > > > > > > > > > > > > struct lmb_property region[CONFIG_LMB_MAX_REGIONS];
> > > > > > > > > > > > > > > #else
> > > > > > > > > > > > > > > struct lmb_property *region;
> > > > > > > > > > > > > > > @@ -67,7 +67,7 @@ struct lmb_region {
> > > > > > > > > > > > > > > struct lmb {
> > > > > > > > > > > > > > > struct lmb_region memory;
> > > > > > > > > > > > > > > struct lmb_region reserved;
> > > > > > > > > > > > > > > -#if !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS)
> > > > > > > > > > > > > > > +#if IS_ENABLED(CONFIG_LMB) && !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS)
> > > > > > > > > > > > > > > struct lmb_property memory_regions[CONFIG_LMB_MEMORY_REGIONS];
> > > > > > > > > > > > > > > struct lmb_property reserved_regions[CONFIG_LMB_RESERVED_REGIONS];
> > > > > > > > > > > > > > > #endif
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > We shouldn't need this at all. LMB and LMB_USE_MAX_REGIONS are both in
> > > > > > > > > > > > > > Kconfig and have the dependencies expressed that way.
> > > > > > > > > > > > >
> > > > > > > > > > > > > However, CONFIG_LMB_MEMORY_REGIONS and CONFIG_LMB_RESERVED_REGIONS may be
> > > > > > > > > > > > > undefined if CONFIG_LMB and !CONFIG_LMB_USE_MAX_REGIONS . They are four
> > > > > > > > > > > > > different symbols.
> > > > > > > > > > > >
> > > > > > > > > > > > I'm still not seeing it, sorry. Is there some case where we're trying
> > > > > > > > > > > > to access a struct lmb without CONFIG_LMB enabled?
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > See build failure
> > > > > > > > > > > https://source.denx.de/u-boot/custodians/u-boot-sh/-/jobs/315331
> > > > > > > > > >
> > > > > > > > > > Ah, progress. Drop <lmb.h> from <image.h> since we already have a
> > > > > > > > > > forward declaration of struct lmb? But it's not failing without this
> > > > > > > > > > series too, so what's changing?
> > > > > > > > >
> > > > > > > > > See 01/14 in this series.
> > > > > > > >
> > > > > > > > Ah, so drop 1/14 then.
> > > > > > >
> > > > > > > Why ? That patch is correct.
> > > > > >
> > > > > > It's not quite right, 1/14 and then 2/14 are papering over the fact that
> > > > > > lmb.h, and it's including headers / files, need to be cleaned up so that
> > > > > > we don't need to have redundant tests in the header.
> > > > >
> > > > > 1/14 disables LMB and CMD_BDI for tools build, we do not need those, so 1/14
> > > > > is correct.
> > > >
> > > > We don't need to build u-boot at all for tools-only, only the tools-only
> > > > build target. It's just annoying to exclude the tools-only_defconfig from
> > > > "sandbox" in CI.
> > >
> > > So, what exactly is the problem with that 01/14 ? Please elaborate, I
> > > believe the patch is correct.
> >
> > You disable LMB in a target that's only building "all" in CI because
> > wasn't ever worth adding ",sandbox" to the all other arches job until
> > perhaps now.
> >
> > Disabling LMB in tools-only_defconfig then exposes that <lmb.h> can only
> > be included safely when CONFIG_LMB is set.
> >
> > Adding / extending an #if test in code for something that's already
> > checked for in Kconfig is bad. We spent so much time already removing
> > and shrinking #if tests in the code.
>
> So, the patch is correct, the headers need further clean up.
No, it's not. The first patch is wrong because disabling CONFIG_LMB is
invalid. The second patch is conceptually wrong because if we're
enforcing a check in C for a dependency that's enforced in Kconfig, we
have another problem to investigate. Which I did, LMB is non-optional.
> > > > > What kind of cleanup of lmb.h do you have in mind ?
> > > >
> > > > Remove it from include/image.h and fix any fall-out from that of files
> > > > that got <lmb.h> indirectly when they needed it directly instead.
> > >
> > > Uh ... that is likely for a separate series, and a big one.
> >
> > Honestly, checking again, I'm not sure LMB=n is valid, ever.
>
> Why wouldn't it be ? For tools, LMB=n is perfectly valid.
Because it's never valid to disable LMB, LMB is what protects the
running U-Boot.
It's nonsense to build u-boot on tools-only_defconfig but we don't have
a way currently to remove "u-boot" from the all target. Maybe once a
few more of the hard/tricky CONFIG symbols get migrated to Kconfig we
can then set tools-only_defconfig to NOT build u-boot at all.
> > That's how
> > we keep our running U-Boot from being trivially overwritten and a huge
> > number of security issues from being re-opened.
>
> Tools are not running U-Boot.
>
> > At this point, I think you should rework things to stop making
> > CONFIG_LMB be optional, it should be a def_bool y.
>
> I disagree, see above.
The only reason "tools-only_defconfig" builds a useless u-boot binary
today is in CI where it would be more work than it's worth to make CI
exclude that from the build list. But if you want to just do that
instead, I'll also accept adding -x tools-only to the azure/gitlab jobs
that build all other architectures, as tools-only is tested in its own
build job, for it's only valid build target.
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210830/81fcbba8/attachment.sig>
More information about the U-Boot
mailing list