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

[Simon Glass]

Hi Tom,

On Sun, 15 Jan 2023 at 15:04, Tom Rini <trini at konsulko.com> wrote:
>
> 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.

Thanks for the info. So in that case, since DM_EVENT depended on EVENT
and DM_EVENT was only implied (but EVENT was not enabled), it didn't
work.

Yes I agree 'select' is better here, to force it. But in this case the
'select EVENT' in 'DM_EVENT' is enough (as would be an 'imply EVENT').
I am quite happy with how it has turned out, but I do want to use
'select' sparingly as it limits what boards can modify.

Regards,
Simon