Re: [PATCH 00/11] drm: Restore helper usability

[Geert Uytterhoeven]

Hi Jani,

CC kbuild

On Mon, Apr 22, 2024 at 7:00 PM Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote:
> On Mon, 22 Apr 2024, "Arnd Bergmann" <arnd@xxxxxxxx> wrote:
> > I'm not sure where this misunderstanding comes from, as you
> > seem to be repeating the same incorrect assumption about
> > how select works that Maxime wrote in his changelog. To clarify,
> > this works exactly as one would expect:
> >
> > config HELPER_A
> >        tristate
> >
> > config HELPER_B
> >        tristate
> >        select HELPER_A
> >
> > config DRIVER
> >        tristate "Turn on the driver and the helpers it uses"
> >        select HELPER_B # this recursively selects HELPER_A
> >
> > Whereas this one is broken:
> >
> > config FEATURE_A
> >        tristate "user visible if I2C is enabled"
> >        depends on I2C
> >
> > config HELPER_B
> >        tristate # hidden
> >        select FEATURE_A
> >
> > config DRIVER
> >        tristate "This driver is broken if I2C is disabled"
> >        select HELPER_B
>
> This case is really what I was referring to, although I was sloppy with
> words there. I understand that select does work recursively for selects.
>
> >>   There is no end to this, it just goes on and on, as the
> >>   dependencies of the selected symbols change over time. Often the
> >>   selects require unintuitive if patterns that are about the
> >>   implementation details of the symbol being selected.
> >
> > Agreed, that is the problem I frequently face with drivers/gpu/drm,
> > and most of the time it can only be solved by rewriting the whole
> > system to not select user-visible symbol at all.
> >
> > Using 'depends on' by itself is unfortunately not enough to
> > avoid /all/ the problems. See e.g. today's failure
> >
> > config DRM_DISPLAY_HELPER
> >        tristate "DRM Display Helpers"
> >        default y
> >
> > config DRM_DISPLAY_DP_HELPER
> >        bool "DRM DisplayPort Helpers"
> >        depends on DRM_DISPLAY_HELPER
> >
> > config DRM_PANEL_LG_SW43408
> >        tristate "LG SW43408 panel"
> >        depends on DRM_DISPLAY_DP_HELPER
> >
> > This version is still broken for DRM_DISPLAY_HELPER=m,
> > DRM_DISPLAY_DP_HELPER=m, DRM_PANEL_LG_SW43408=y because
> > the dependency on the bool symbol is not enough to
> > ensure that DRM_DISPLAY_HELPER is also built-in, so you
> > still need explicit dependencies on both
> > DRM_DISPLAY_HELPER and DRM_DISPLAY_DP_HELPER in the users.
> >
> > This can be solved by making DRM_DISPLAY_DP_HELPER a
> > tristate symbol and adjusting the #ifdef checks and
> > Makefile logic accordingly, which is exactly what you'd
> > need to do to make it work with 'select' as well.
>
> So bool is kind of problematic for depends on and select even when it's
> not really used for describing builtin vs. no, but rather yes vs. no?

Yes, the underlying issue is that bool is used for two different things:
  A. To enable a driver module that can be only built-in,
  B. To enable an option or feature of a driver or subsystem.

Without this distinction, dependencies cannot be auto-propagated 100%
correctly.  Fixing that would require introducing a third type (and possibly
renaming the existing ones to end up with 3 good names).

Actually two types could work:
  1. driver,
  2. option,
as case A is just a driver that can only be built-in (i.e. "depends on y",
which is similar to the behavior with CONFIG_MODULES=n).

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds