Re: [PATCH] [RESEND] drm: fb_helper: fix CONFIG_FB dependency

[Jani Nikula]

On Wed, 27 Oct 2021, Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote:
> On Thu, 30 Sep 2021, Daniel Vetter <daniel@xxxxxxxx> wrote:
>> On Mon, Sep 27, 2021 at 09:23:45AM -0700, Kees Cook wrote:
>>> On Mon, Sep 27, 2021 at 04:28:02PM +0200, Arnd Bergmann wrote:
>>> > From: Arnd Bergmann <arnd@xxxxxxxx>
>>> > 
>>> > With CONFIG_FB=m and CONFIG_DRM=y, we get a link error in the fb helper:
>>> > 
>>> > aarch64-linux-ld: drivers/gpu/drm/drm_fb_helper.o: in function `drm_fb_helper_alloc_fbi':
>>> > (.text+0x10cc): undefined reference to `framebuffer_alloc'
>>> > 
>>> > Tighten the dependency so it is only allowed in the case that DRM can
>>> > link against FB.
>>> > 
>>> > Fixes: f611b1e7624c ("drm: Avoid circular dependencies for CONFIG_FB")
>>> > Link: https://lore.kernel.org/all/20210721152211.2706171-1-arnd@xxxxxxxxxx/
>>> > Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx>
>>> 
>>> Thanks for fixing this!
>>> 
>>> Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx>
>>
>> Stuffed into drm-misc-next.
>
> The problem is, I don't think the patch is semantically correct.
>
> drm_fb_helper.o is not part of drm.ko, it's part of
> drm_kms_helper.ko. This adds some sort of indirect dependency via DRM
> which might work, maybe by coincidence, maybe not - but it's certainly
> not obvious.
>
> The likely culprit is, again, the overuse of select, and in this case
> select DRM_KMS_HELPER. And DRM_KMS_HELPER should depend on FB if
> DRM_FBDEV_EMULATION=y. That's the problem.

Almost all of the recurring Kconfig related dependency issues would go
away by following Documentation/kbuild/kconfig-language.rst:

  Note:
	select should be used with care. select will force
	a symbol to a value without visiting the dependencies.
	By abusing select you are able to select a symbol FOO even
	if FOO depends on BAR that is not set.
	In general use select only for non-visible symbols
	(no prompts anywhere) and for symbols with no dependencies.
	That will limit the usefulness but on the other hand avoid
	the illegal configurations all over.

If the kconfig parser had a lint mode to issue a warning for all of
those uses, and someone persistent enough followed through with fixing
them, we'd all be better off.

Oh, and maybe the menuconfig tools also need better ways to recursively
enable config options with dependencies, because one of the reasons
people like select is the convenience of just enabling a config option,
and it selects everything that's needed (albeit with the occasional
dependency issues). With dependencies, you need to start with the leaf
dependencies and work your way up to what you need, and it's not easy.


BR,
Jani.


>
> All of the drm Kconfigs could use an overhaul to be semantically
> correct, but that's a hill nobody wants to die on. Instead we keep
> piling up tweaks to paper over the issues, ad infinitum.
>
> (And this ties to a previous comment I had about the organization of
> files under drm/, a hundred files in one big lump that belong to
> different modules, and it's not helping people figure out the
> dependencies.)
>
>
> BR,
> Jani.
>
>
> PS. I was brought here via [1] which is another complicated "fix" to the
> same problem.
>
>
> [1] https://lore.kernel.org/r/20211027072044.4105113-1-javierm@xxxxxxxxxx

-- 
Jani Nikula, Intel Open Source Graphics Center