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 Signed-off-by: Javier Martinez Canillas javierm@redhat.com Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch ---
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/efifb.c | 11 +++++++++++ drivers/video/fbdev/simplefb.c | 11 +++++++++++ 2 files changed, 22 insertions(+)
diff --git drivers/video/fbdev/efifb.c drivers/video/fbdev/efifb.c index edca3703b964..ea42ba6445b2 100644 --- drivers/video/fbdev/efifb.c +++ drivers/video/fbdev/efifb.c @@ -351,6 +351,17 @@ static int efifb_probe(struct platform_device *dev) char *option = NULL; efi_memory_desc_t md;
+ /* + * 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 (num_registered_fb > 0) { + dev_err(&dev->dev, + "efifb: a framebuffer is already registered\n"); + return -EINVAL; + } + if (screen_info.orig_video_isVGA != VIDEO_TYPE_EFI || pci_dev_disabled) return -ENODEV;
diff --git drivers/video/fbdev/simplefb.c drivers/video/fbdev/simplefb.c index 62f0ded70681..b63074fd892e 100644 --- drivers/video/fbdev/simplefb.c +++ drivers/video/fbdev/simplefb.c @@ -407,6 +407,17 @@ static int simplefb_probe(struct platform_device *pdev) struct simplefb_par *par; struct resource *mem;
+ /* + * 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 (num_registered_fb > 0) { + dev_err(&pdev->dev, + "simplefb: a framebuffer is already registered\n"); + return -EINVAL; + } + if (fb_get_options("simplefb", NULL)) return -ENODEV;
On Thu, Nov 11, 2021 at 12:11:20PM +0100, Javier Martinez Canillas wrote:
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 Signed-off-by: Javier Martinez Canillas javierm@redhat.com Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
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).
That does not mean that it will make it into the stable tree. Please read: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html for how to do this properly.
thanks,
greg k-h
On Thu, Nov 11, 2021 at 12:39:28PM +0100, Greg Kroah-Hartman wrote:
On Thu, Nov 11, 2021 at 12:11:20PM +0100, Javier Martinez Canillas wrote:
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 Signed-off-by: Javier Martinez Canillas javierm@redhat.com Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
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).
That does not mean that it will make it into the stable tree. Please read: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html for how to do this properly.
Defacto your auto-picker is aggressive enough that just Fixes: is actually good enough to get it into stable :-)
But yeah explicit cc: stable can't hurt. -Daniel
Hi Javier,
On Thu, Nov 11, 2021 at 12:13 PM Javier Martinez Canillas javierm@redhat.com wrote:
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 Signed-off-by: Javier Martinez Canillas javierm@redhat.com Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
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/efifb.c | 11 +++++++++++ drivers/video/fbdev/simplefb.c | 11 +++++++++++ 2 files changed, 22 insertions(+)
diff --git drivers/video/fbdev/efifb.c drivers/video/fbdev/efifb.c index edca3703b964..ea42ba6445b2 100644 --- drivers/video/fbdev/efifb.c +++ drivers/video/fbdev/efifb.c @@ -351,6 +351,17 @@ static int efifb_probe(struct platform_device *dev) char *option = NULL; efi_memory_desc_t md;
/** 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 (num_registered_fb > 0) {
Who says this registered fbdev is driving the same hardware as efifb? This might be e.g. a small external display connected to i2c or spi.
dev_err(&dev->dev,"efifb: a framebuffer is already registered\n");return -EINVAL;}if (screen_info.orig_video_isVGA != VIDEO_TYPE_EFI || pci_dev_disabled) return -ENODEV;diff --git drivers/video/fbdev/simplefb.c drivers/video/fbdev/simplefb.c index 62f0ded70681..b63074fd892e 100644 --- drivers/video/fbdev/simplefb.c +++ drivers/video/fbdev/simplefb.c @@ -407,6 +407,17 @@ static int simplefb_probe(struct platform_device *pdev) struct simplefb_par *par; struct resource *mem;
/** 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 (num_registered_fb > 0) {
Likewise.
dev_err(&pdev->dev,"simplefb: a framebuffer is already registered\n");return -EINVAL;}if (fb_get_options("simplefb", NULL)) return -ENODEV;
Gr{oetje,eeting}s,
Geert
-- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
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
Hello Geert,
On 11/15/21 17:20, Geert Uytterhoeven wrote:
[snip]
@@ -351,6 +351,17 @@ static int efifb_probe(struct platform_device *dev) char *option = NULL; efi_memory_desc_t md;
/** 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 (num_registered_fb > 0) {Who says this registered fbdev is driving the same hardware as efifb? This might be e.g. a small external display connected to i2c or spi.
dev_err(&dev->dev,"efifb: a framebuffer is already registered\n");return -EINVAL;}
That's true, although I wonder if the {efi,simple}fb drivers should even be probed in that case. As I see it, these are always a best effort that are only useful for earlycon or if there isn't another display driver supported.
Since there may be other conditions needed in order for these to work. For example, when using the u-boot EFI stub in most cases the unused clocks and power domains can't be gated or otherwise the firmware frame buffer could go away (e.g: will need to boot with "clk_ignore_unused" and "pd_ignore_unused").
Same for the simplefb driver, if the DT node is missing resources that are needed by the display controller to continue working (clocks, regulators, power domains), the firmware setup framebuffer will go away at some point.
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.
Best regards,
Hi Javier,
On Tue, Nov 16, 2021 at 10:30 AM Javier Martinez Canillas javierm@redhat.com wrote:
On 11/15/21 17:20, Geert Uytterhoeven wrote:
@@ -351,6 +351,17 @@ static int efifb_probe(struct platform_device *dev) char *option = NULL; efi_memory_desc_t md;
/** 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 (num_registered_fb > 0) {Who says this registered fbdev is driving the same hardware as efifb? This might be e.g. a small external display connected to i2c or spi.
dev_err(&dev->dev,"efifb: a framebuffer is already registered\n");return -EINVAL;}That's true, although I wonder if the {efi,simple}fb drivers should even be probed in that case. As I see it, these are always a best effort that are only useful for earlycon or if there isn't another display driver supported.
Since there may be other conditions needed in order for these to work. For example, when using the u-boot EFI stub in most cases the unused clocks and power domains can't be gated or otherwise the firmware frame buffer could go away (e.g: will need to boot with "clk_ignore_unused" and "pd_ignore_unused").
Same for the simplefb driver, if the DT node is missing resources that are needed by the display controller to continue working (clocks, regulators, power domains), the firmware setup framebuffer will go away at some point.
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...
But as you're only impacting efifb and simplefb, thus not crippling generic fbdev support, I don't care that much.
Gr{oetje,eeting}s,
Geert
-- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
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
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.
Another option is to just say that DRM drivers should be built as a module if the {efi,simple}fb driver are built-in.
Best regards,
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,
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Geert Uytterhoeven -
Greg Kroah-Hartman -
Javier Martinez Canillas