On 8/30/21 2:01 PM, Tom Rini wrote:
[...]
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.
Please explain why the patch disabling LMB support for tools-only build
is invalid. I disagree with this statement, LMB support in tools-only
build is useless, because LMB protects parts of running U-Boot from
being overwritten.
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.
Please explain why is LMB non-optional ? I disagree. LMB for tools-only
build is useless, hence it should not be enabled.
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.
We are talking about tools-only build here, not 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.
The tools-only build is also used elsewhere, to build just that, tools.
[...]
