On Wed, Oct 27, 2021 at 1:47 PM Jani Nikula jani.nikula@linux.intel.com wrote:
On Thu, 30 Sep 2021, Daniel Vetter daniel@ffwll.ch 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@arndb.de
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@kernel.org/ Signed-off-by: Arnd Bergmann arnd@arndb.de
Thanks for fixing this!
Reviewed-by: Kees Cook keescook@chromium.org
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.
Right, how about this change on top?
--- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -117,9 +117,8 @@ config DRM_DEBUG_MODESET_LOCK
config DRM_FBDEV_EMULATION bool "Enable legacy fbdev support for your modesetting driver" - depends on DRM - depends on FB=y || FB=DRM - select DRM_KMS_HELPER + depends on DRM_KMS_HELPER + depends on FB=y || FB=DRM_KMS_HELPER select FB_CFB_FILLRECT select FB_CFB_COPYAREA select FB_CFB_IMAGEBLIT
That would probably make it work for DRM=y, FB=m, DRM_KMS_HELPER=m, but it needs more randconfig testing, which I can help with.
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.
This is something we can't easily express in Kconfig, as we can't add the dependency to a symbol that only gets selected by other drivers, which is why the dependency has to be in the user-visible symbol, in this case DRM_FBDEV_EMULATION.
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.
Yes, that is a big issue, though we have similar problems with drivers/media and net/.
On a related note, I did manage to sort out the backlight dependency issue (intel_panel.c:(.text+0x2f58): undefined reference to `backlight_device_register'), but haven't sent that one again yet, but I can if you like. This one changes DRM_I915 and all of drivers/video/fbdev from 'select BACKLIGHT_CLASS_DEVICE' to 'depends on', which I think moves everything into broadly the right direction.
Let me know if you would like me to send those now, or have a look at the top 3 patches in [1] if you are interested. This has passed a few thousand randconfig builds and should not depend on additional patches.
Arnd
[1] https://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git/log/?h=r...