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 > > > > > > > > > > > > > > > Cc: Simon Glass > > > > > > > > > > > > > > > Cc: Simon Goldschmidt > > > > > > > > > > > > > > > Cc: Tom Rini > > > > > > > > > > > > > > > --- > > > > > > > > > > > > > > > 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 from 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 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 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