MIME-Version: 1.0
References: <20211111111120.1344613-1-javierm@redhat.com> <CAMuHMdWA2V_KDpcpMw3yRKmN+6YDjmysJoz6D-6JjJs-3+XYTQ@mail.gmail.com>
 <579a584a-68af-d5c9-0547-30cb1592d46f@redhat.com>
In-Reply-To: <579a584a-68af-d5c9-0547-30cb1592d46f@redhat.com>
From:   Geert Uytterhoeven <geert@linux-m68k.org>
Date:   Tue, 16 Nov 2021 10:43:59 +0100
Message-ID: <CAMuHMdWotEN1TtTr7douLkQPkpXE-rQgamM3GOYO1XNHbUiguw@mail.gmail.com>
Subject: Re: [PATCH v2] fbdev: Prevent probing generic drivers if a FB is
 already registered
To:     Javier Martinez Canillas <javierm@redhat.com>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        Sam Ravnborg <sam@ravnborg.org>,
        Peter Jones <pjones@redhat.com>,
        Daniel Vetter <daniel.vetter@ffwll.ch>,
        DRI Development <dri-devel@lists.freedesktop.org>,
        Linux Fbdev development list <linux-fbdev@vger.kernel.org>,
        Hans de Goede <hdegoede@redhat.com>,
        Ilya Trukhanov <lahvuun@gmail.com>,
        Thorsten Leemhuis <regressions@leemhuis.info>,
        Borislav Petkov <bp@suse.de>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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