[PATCH 2/2] event: Correct dependencies on the EVENT framework

[Tom Rini]

On Sun, Jan 15, 2023 at 02:52:35PM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Sun, 15 Jan 2023 at 06:45, Tom Rini <trini at konsulko.com> wrote:
> >
> > On Sat, Jan 14, 2023 at 06:31:05PM -0700, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Sat, 14 Jan 2023 at 13:49, Tom Rini <trini at konsulko.com> wrote:
> > > >
> > > > The event framework is just that, a framework. Enabling it by itself
> > > > does nothing, so we shouldn't ask the user about it. Reword (and correct
> > > > typos) around this the option and help text. This also applies to
> > > > DM_EVENT, so reword as well.
> > > >
> > > > With this, it's time to address the larger problems. When functionality
> > > > uses events, typically via EVENT_SPY, the appropriate framework then
> > > > must be select'd and NOT imply'd. As the functionality will cease to
> > > > work (and so, platforms will fail to boot) this is non-optional and
> > > > where select is appropriate. Audit the current users of EVENT_SPY to
> > > > have a more fine-grained approach to select'ing the framework where
> > > > used.
> > > >
> > > > Cc: Simon Glass <sjg at chromium.org>
> > > > Reported-by: Oliver Graute <Oliver.Graute at kococonnector.com>
> > > > Reported-by: Francesco Dolcini <francesco.dolcini at toradex.com>
> > > > Fixes: 7fe32b3442f0 ("event: Convert arch_cpu_init_dm() to use events")
> > > > Fixes: 42fdcebf859f ("event: Convert misc_init_f() to use events")
> > > > Signed-off-by: Tom Rini <trini at konsulko.com>
> > > > ---
> > > >  arch/Kconfig                     |  6 +++---
> > > >  arch/arm/Kconfig                 |  9 ++++-----
> > > >  arch/arm/mach-omap2/Kconfig      |  3 +++
> > > >  arch/mips/Kconfig                |  2 +-
> > > >  arch/powerpc/cpu/mpc85xx/Kconfig |  1 +
> > > >  arch/x86/Kconfig                 |  1 +
> > > >  arch/x86/cpu/baytrail/Kconfig    |  1 +
> > > >  arch/x86/cpu/broadwell/Kconfig   |  1 +
> > > >  arch/x86/cpu/ivybridge/Kconfig   |  1 +
> > > >  arch/x86/cpu/quark/Kconfig       |  1 +
> > > >  board/google/Kconfig             |  1 +
> > > >  board/keymile/Kconfig            |  1 +
> > > >  boot/Kconfig                     |  1 +
> > > >  cmd/Kconfig                      |  1 +
> > > >  common/Kconfig                   | 17 ++++++++---------
> > > >  drivers/core/Kconfig             |  9 +++++----
> > > >  drivers/cpu/Kconfig              |  1 -
> > >
> > > Reviewed-by: Simon Glass <sjg at chromium.org>
> > >
> > > Tested on chromebook-coral:
> > > Tested-by: Simon Glass <sjg at chromium.org>
> > >
> > > I am not quite sure what the effective difference is between select
> > > and imply. Doesn't this suggest that there are some conflicting config
> > > options? The only way imply would be disabled ( I thought) is if a
> > > board does it explicitly?
> >
> > So there's two parts to it. As a framework, the option needs to be
> > select'd, not implied. It doesn't make sense to disable the event
> > framework and then wonder why VBE doesn't work. The next part however is
> >
> > Fixes: c5ef2025579e ("dm: fix DM_EVENT dependencies")
> 
> OK
> 
> >
> > should have been part of it too. That commit turned all of the platforms
> > which had imply DM_EVENT (and so from DM_EVENT, imply EVENT) in to
> > depends on EVENT being set, and if it wasn't set (or select'd, only
> > EFI_LOADER was select'ing it at the time) in to not enabling DM_EVENT,
> > and since imply options can be off, no warnings were thrown by the build
> > system. And since all of the non-dynamic EVENT stuff is linker-based,
> > no binary size changes happened either.
> 
> So does this mean that select is 'forcing' the option to be enabled,
> even if it conflicts with something else, or depends on something
> which is not set?
> 
> I'd quite like to understand this better for the future. Could you
> point to a board that was broken and now fixed, so I can have a fiddle
> with it?

While kernel centric at times (of course)
https://www.kernel.org/doc/html/latest/kbuild/kconfig-language.html#menu-attributes
covers things well, especially for the language itself. So yes, select
forces the option to be enabled. If there's an unmet (depends on ...)
dependency, then it will emit a warning to tell you as such. An example
of a previously broken board would be apalis-imx8 as it does not enable
EFI_LOADER so no DM_EVENT / EVENT and so:
arch/arm/mach-imx/imx8/cpu.c:EVENT_SPY(EVT_DM_POST_INIT, imx8_init_mu);
wasn't called and the platform would silently fail to boot.

-- 
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/20230115/8b195cd6/attachment.sig>