On 11/16/21 11:01, Javier Martinez Canillas wrote:
Hello Geert,
On 11/16/21 10:43, Geert Uytterhoeven wrote:
[snip]
So this is already a fragile solution and $SUBJECT doesn't make things worse IMO. Since not having something like this can lead to issues as reported by:
https://lore.kernel.org/all/20211110200253.rfudkt3edbd3nsyj@lahvuun/
We could probably do some smarter here by providing a function that checks if the registered fbdev drivers matches the aperture base. But I'm unsure if that's worth it. After all, fbdev drivers are likely to be disabled by most distros soon now that we have the simpledrm driver.
Checking the aperture base is what was done in all other cases of preventing generic (fbdev) drivers from stepping on specific drivers' toes...
Ok, I can re-spin the patch checking if the aperture ranges overlap. There's an apertures_overlap() function in drivers/video/fbdev/core/fbmem.c that can be exported for fbdev drivers to use.
So I tried the following patch [0]. But when testing on a VM, the efifb driver is probed even after the virtio_gpu driver has already been probed. Being a DRM driver, it doesn't use the fbdev infra and AFAIU doesn't reserve any apertures.
When the {efi,simple}fb drivers check if there's an aperture already reserved using the fb_aperture_registered() helper, this just returns false even when a driver for the same hardware was already registered. The kernel log says:
[ 0.891512] checking generic (0 0) vs hw (c0000000 1d5000)
That is because when DRM_FBDEV_EMULATION=y, the virtio_gpu driver registers an fbdev but without any aperture set.
I discussed this with Thomas and even though $SUBJECT is just a workaround, it seems that is the best we can do as an heuristic to prevent the generic fbdev drivers to be probed after a native DRM driver.
[0]:
From d962c20bc9fd90c2525d79b69e632d99e8050fc5 Mon Sep 17 00:00:00 2001
From: Javier Martinez Canillas javierm@redhat.com Date: Thu, 11 Nov 2021 00:55:06 +0100 Subject: [PATCH v4] fbdev: Prevent probing generic drivers if a FB is already registered
The efifb and simplefb drivers just render to a pre-allocated frame buffer and rely on the display hardware being initialized before the kernel boots.
But if another driver already probed correctly and registered a fbdev, the generic drivers shouldn't be probed since an actual driver for the display hardware is already present.
This is more likely to occur after commit d391c5827107 ("drivers/firmware: move x86 Generic System Framebuffers support") since the "efi-framebuffer" and "simple-framebuffer" platform devices are registered at a later time.
Link: https://lore.kernel.org/r/20211110200253.rfudkt3edbd3nsyj@lahvuun/ Fixes: d391c5827107 ("drivers/firmware: move x86 Generic System Framebuffers support") Reported-by: Ilya Trukhanov lahvuun@gmail.com Cc: stable@vger.kernel.org # 5.15.x Signed-off-by: Javier Martinez Canillas javierm@redhat.com ---
Changes in v4: - Only fail to probe if a registered fbdev has overlapping aperture (Geert).
Changes in v3: - Cc stable@vger.kernel.org since a Fixes: tag is not enough (gregkh).
Changes in v2: - Add a Link: tag with a reference to the bug report (Thorsten Leemhuis). - Add a comment explaining why the probe fails earlier (Daniel Vetter). - Add a Fixes: tag for stable to pick the fix (Daniel Vetter). - Add Daniel Vetter's Reviewed-by: tag. - Improve the commit message and mention the culprit commit
drivers/video/fbdev/core/fbmem.c | 16 ++++++++++++++++ drivers/video/fbdev/efifb.c | 11 +++++++++++ drivers/video/fbdev/simplefb.c | 11 +++++++++++ include/linux/fb.h | 1 + 4 files changed, 39 insertions(+)
diff --git drivers/video/fbdev/core/fbmem.c drivers/video/fbdev/core/fbmem.c index 826175ad88a2..9906b83132cb 100644 --- drivers/video/fbdev/core/fbmem.c +++ drivers/video/fbdev/core/fbmem.c @@ -1546,6 +1546,22 @@ static bool fb_do_apertures_overlap(struct apertures_struct *gena, return false; }
+bool fb_aperture_registered(struct apertures_struct *a) +{ + int i; + + for_each_registered_fb(i) { + struct apertures_struct *gen_aper; + + gen_aper = registered_fb[i]->apertures; + if (fb_do_apertures_overlap(gen_aper, a)) + return true; + } + + return false; +} +EXPORT_SYMBOL(fb_aperture_registered); + static void do_unregister_framebuffer(struct fb_info *fb_info);
#define VGA_FB_PHYS 0xA0000 diff --git drivers/video/fbdev/efifb.c drivers/video/fbdev/efifb.c index edca3703b964..1ad6698b2e05 100644 --- drivers/video/fbdev/efifb.c +++ drivers/video/fbdev/efifb.c @@ -457,6 +457,17 @@ static int efifb_probe(struct platform_device *dev) info->apertures->ranges[0].base = efifb_fix.smem_start; info->apertures->ranges[0].size = size_remap;
+ /* + * Generic drivers must not be registered if a framebuffer exists. + * If a native driver was probed, the display hardware was already + * taken and attempting to use the system framebuffer is dangerous. + */ + if (fb_aperture_registered(info->apertures)) { + dev_err(&dev->dev, + "efifb: a framebuffer is already registered\n"); + return -EINVAL; + } + if (efi_enabled(EFI_MEMMAP) && !efi_mem_desc_lookup(efifb_fix.smem_start, &md)) { if ((efifb_fix.smem_start + efifb_fix.smem_len) > diff --git drivers/video/fbdev/simplefb.c drivers/video/fbdev/simplefb.c index 62f0ded70681..3ad0f538ca91 100644 --- drivers/video/fbdev/simplefb.c +++ drivers/video/fbdev/simplefb.c @@ -456,6 +456,17 @@ static int simplefb_probe(struct platform_device *pdev) info->apertures->ranges[0].base = info->fix.smem_start; info->apertures->ranges[0].size = info->fix.smem_len;
+ /* + * Generic drivers must not be registered if a framebuffer exists. + * If a native driver was probed, the display hardware was already + * taken and attempting to use the system framebuffer is dangerous. + */ + if (fb_aperture_registered(info->apertures)) { + dev_err(&pdev->dev, + "simplefb: a framebuffer is already registered\n"); + return -EINVAL; + } + info->fbops = &simplefb_ops; info->flags = FBINFO_DEFAULT | FBINFO_MISC_FIRMWARE; info->screen_base = ioremap_wc(info->fix.smem_start, diff --git include/linux/fb.h include/linux/fb.h index 6f3db99ab990..f1fbdb39932b 100644 --- include/linux/fb.h +++ include/linux/fb.h @@ -604,6 +604,7 @@ extern ssize_t fb_sys_write(struct fb_info *info, const char __user *buf, size_t count, loff_t *ppos);
/* drivers/video/fbmem.c */ +extern bool fb_aperture_registered(struct apertures_struct *a); extern int register_framebuffer(struct fb_info *fb_info); extern void unregister_framebuffer(struct fb_info *fb_info); extern int remove_conflicting_pci_framebuffers(struct pci_dev *pdev,