linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] fbdev: Prevent probing generic drivers if a FB is already registered
@ 2021-11-11 11:11 Javier Martinez Canillas
  2021-11-11 11:39 ` Greg Kroah-Hartman
  2021-11-15 16:20 ` Geert Uytterhoeven
  0 siblings, 2 replies; 8+ messages in thread
From: Javier Martinez Canillas @ 2021-11-11 11:11 UTC (permalink / raw)
  To: linux-kernel
  Cc: Thomas Zimmermann, Sam Ravnborg, Peter Jones, Daniel Vetter,
	dri-devel, linux-fbdev, Hans de Goede, Ilya Trukhanov,
	Thorsten Leemhuis, Javier Martinez Canillas, Borislav Petkov,
	Greg Kroah-Hartman

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;
 
-- 
2.33.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] fbdev: Prevent probing generic drivers if a FB is already registered
  2021-11-11 11:11 [PATCH v2] fbdev: Prevent probing generic drivers if a FB is already registered Javier Martinez Canillas
@ 2021-11-11 11:39 ` Greg Kroah-Hartman
  2021-11-12 16:12   ` Daniel Vetter
  2021-11-15 16:20 ` Geert Uytterhoeven
  1 sibling, 1 reply; 8+ messages in thread
From: Greg Kroah-Hartman @ 2021-11-11 11:39 UTC (permalink / raw)
  To: Javier Martinez Canillas
  Cc: linux-kernel, Thomas Zimmermann, Sam Ravnborg, Peter Jones,
	Daniel Vetter, dri-devel, linux-fbdev, Hans de Goede,
	Ilya Trukhanov, Thorsten Leemhuis, Borislav Petkov

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] fbdev: Prevent probing generic drivers if a FB is already registered
  2021-11-11 11:39 ` Greg Kroah-Hartman
@ 2021-11-12 16:12   ` Daniel Vetter
  0 siblings, 0 replies; 8+ messages in thread
From: Daniel Vetter @ 2021-11-12 16:12 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Javier Martinez Canillas, linux-kernel, Thomas Zimmermann,
	Sam Ravnborg, Peter Jones, Daniel Vetter, dri-devel, linux-fbdev,
	Hans de Goede, Ilya Trukhanov, Thorsten Leemhuis,
	Borislav Petkov

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] fbdev: Prevent probing generic drivers if a FB is already registered
  2021-11-11 11:11 [PATCH v2] fbdev: Prevent probing generic drivers if a FB is already registered Javier Martinez Canillas
  2021-11-11 11:39 ` Greg Kroah-Hartman
@ 2021-11-15 16:20 ` Geert Uytterhoeven
  2021-11-16  9:30   ` Javier Martinez Canillas
  1 sibling, 1 reply; 8+ messages in thread
From: Geert Uytterhoeven @ 2021-11-15 16:20 UTC (permalink / raw)
  To: Javier Martinez Canillas
  Cc: Linux Kernel Mailing List, Thomas Zimmermann, Sam Ravnborg,
	Peter Jones, Daniel Vetter, DRI Development,
	Linux Fbdev development list, Hans de Goede, Ilya Trukhanov,
	Thorsten Leemhuis, Borislav Petkov, Greg Kroah-Hartman

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] fbdev: Prevent probing generic drivers if a FB is already registered
  2021-11-15 16:20 ` Geert Uytterhoeven
@ 2021-11-16  9:30   ` Javier Martinez Canillas
  2021-11-16  9:43     ` Geert Uytterhoeven
  0 siblings, 1 reply; 8+ messages in thread
From: Javier Martinez Canillas @ 2021-11-16  9:30 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Linux Kernel Mailing List, Thomas Zimmermann, Sam Ravnborg,
	Peter Jones, Daniel Vetter, DRI Development,
	Linux Fbdev development list, Hans de Goede, Ilya Trukhanov,
	Thorsten Leemhuis, Borislav Petkov, Greg Kroah-Hartman

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,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] fbdev: Prevent probing generic drivers if a FB is already registered
  2021-11-16  9:30   ` Javier Martinez Canillas
@ 2021-11-16  9:43     ` Geert Uytterhoeven
  2021-11-16 10:01       ` Javier Martinez Canillas
  0 siblings, 1 reply; 8+ messages in thread
From: Geert Uytterhoeven @ 2021-11-16  9:43 UTC (permalink / raw)
  To: Javier Martinez Canillas
  Cc: Linux Kernel Mailing List, Thomas Zimmermann, Sam Ravnborg,
	Peter Jones, Daniel Vetter, DRI Development,
	Linux Fbdev development list, Hans de Goede, Ilya Trukhanov,
	Thorsten Leemhuis, Borislav Petkov, Greg Kroah-Hartman

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] fbdev: Prevent probing generic drivers if a FB is already registered
  2021-11-16  9:43     ` Geert Uytterhoeven
@ 2021-11-16 10:01       ` Javier Martinez Canillas
  2021-11-16 13:49         ` Javier Martinez Canillas
  0 siblings, 1 reply; 8+ messages in thread
From: Javier Martinez Canillas @ 2021-11-16 10:01 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Linux Kernel Mailing List, Thomas Zimmermann, Sam Ravnborg,
	Peter Jones, Daniel Vetter, DRI Development,
	Linux Fbdev development list, Hans de Goede, Ilya Trukhanov,
	Thorsten Leemhuis, Borislav Petkov, Greg Kroah-Hartman

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,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] fbdev: Prevent probing generic drivers if a FB is already registered
  2021-11-16 10:01       ` Javier Martinez Canillas
@ 2021-11-16 13:49         ` Javier Martinez Canillas
  0 siblings, 0 replies; 8+ messages in thread
From: Javier Martinez Canillas @ 2021-11-16 13:49 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Linux Kernel Mailing List, Thomas Zimmermann, Sam Ravnborg,
	Peter Jones, Daniel Vetter, DRI Development,
	Linux Fbdev development list, Hans de Goede, Ilya Trukhanov,
	Thorsten Leemhuis, Borislav Petkov, Greg Kroah-Hartman

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,
-- 
2.33.1

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat


^ permalink raw reply related	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-11-16 13:49 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-11 11:11 [PATCH v2] fbdev: Prevent probing generic drivers if a FB is already registered Javier Martinez Canillas
2021-11-11 11:39 ` Greg Kroah-Hartman
2021-11-12 16:12   ` Daniel Vetter
2021-11-15 16:20 ` Geert Uytterhoeven
2021-11-16  9:30   ` Javier Martinez Canillas
2021-11-16  9:43     ` Geert Uytterhoeven
2021-11-16 10:01       ` Javier Martinez Canillas
2021-11-16 13:49         ` Javier Martinez Canillas

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).