Re: [PATCH v2] fbdev: Prevent probing generic drivers if a FB is already registered

[Geert Uytterhoeven]

Hi Javier,

On Tue, Nov 16, 2021 at 10:30 AM Javier Martinez Canillas
<javierm@xxxxxxxxxx> 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@xxxxxxxxxxxxxx

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