MIME-Version: 1.0
References: <20220107110723.323276-1-javierm@redhat.com> <20220107110723.323276-3-javierm@redhat.com>
In-Reply-To: <20220107110723.323276-3-javierm@redhat.com>
From:   Geert Uytterhoeven <geert@linux-m68k.org>
Date:   Sun, 9 Jan 2022 11:01:32 +0100
Message-ID: <CAMuHMdWvFL9GYM-dMdZeyZrQjr13Sgt-XXN19bc6jA3emn2Dcw@mail.gmail.com>
Subject: Re: [PATCH 2/2] video: vga16fb: Only probe for EGA and VGA 16 color
 graphic cards
To:     Javier Martinez Canillas <javierm@redhat.com>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Kris Karas <bugs-a21@moonlit-rail.com>,
        stable <stable@vger.kernel.org>, Borislav Petkov <bp@suse.de>,
        Daniel Vetter <daniel.vetter@ffwll.ch>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        DRI Development <dri-devel@lists.freedesktop.org>,
        Linux Fbdev development list <linux-fbdev@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Javier,

On Fri, Jan 7, 2022 at 9:00 PM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> The vga16fb framebuffer driver only supports Enhanced Graphics Adapter
> (EGA) and Video Graphics Array (VGA) 16 color graphic cards.
>
> But it doesn't check if the adapter is one of those or if a VGA16 mode
> is used. This means that the driver will be probed even if a VESA BIOS
> Extensions (VBE) or Graphics Output Protocol (GOP) interface is used.
>
> This issue has been present for a long time but it was only exposed by
> commit d391c5827107 ("drivers/firmware: move x86 Generic System
> Framebuffers support") since the platform device registration to match
> the {vesa,efi}fb drivers is done later as a consequence of that change.
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=215001
> Fixes: d391c5827107 ("drivers/firmware: move x86 Generic System Framebuffers support")
> Reported-by: Kris Karas <bugs-a21@moonlit-rail.com>
> Cc: <stable@vger.kernel.org> # 5.15.x
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> Tested-by: Kris Karas <bugs-a21@moonlit-rail.com>

Thanks for your patch!

> --- a/drivers/video/fbdev/vga16fb.c
> +++ b/drivers/video/fbdev/vga16fb.c
> @@ -1422,6 +1422,18 @@ static int __init vga16fb_init(void)
>
>         vga16fb_setup(option);
>  #endif
> +
> +       /* only EGA and VGA in 16 color graphic mode are supported */
> +       if (screen_info.orig_video_isVGA != VIDEO_TYPE_EGAC &&
> +           screen_info.orig_video_isVGA != VIDEO_TYPE_VGAC)
> +               return -ENODEV;

Probably these checks should be wrapped inside a check for CONFIG_X86?

All non-x86 architectures (except for 2 MIPS platforms) treat
orig_video_isVGA as a boolean flag, and just assign 1 to it.

> +
> +       if (screen_info.orig_video_mode != 0x0D &&      /* 320x200/4 (EGA) */
> +           screen_info.orig_video_mode != 0x0E &&      /* 640x200/4 (EGA) */
> +           screen_info.orig_video_mode != 0x10 &&      /* 640x350/4 (EGA) */
> +           screen_info.orig_video_mode != 0x12)        /* 640x480/4 (VGA) */
> +               return -ENODEV;
> +

Likewise.

A long time ago, I used vga16fb on a PPC box to use a standard PC
graphics card (initialized using an emulator for the card's BIOS ROM),
as a second display. The above changes would break such a use case.

>         ret = platform_driver_register(&vga16fb_driver);
>
>         if (!ret) {

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