[v2] 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>
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(+)

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

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

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]: