[PATCH 0/2] video: A couple of fixes for the vga16fb driver

[PATCH 0/2] video: A couple of fixes for the vga16fb driver

[Javier Martinez Canillas]

This patch series contains two fixes for the vga16fb driver. I looked at
the driver due a regression reported [0], caused by commit d391c5827107
("drivers/firmware: move x86 Generic System Framebuffers support").

The mentioned commit didn't change any logic but just moved the platform
device registration that matches the vesafb and efifb drivers to happen
later. And this caused the vga16fb driver to be probed even in machines
that don't have an EGA or VGA video adapter.

Patch #1 is fixing the wrong check to determine if either EGA or VGA is
used and patch #2 adds a check to the driver to only be loaded for EGA
and VGA 16 color graphic cards.

[0]: https://bugzilla.kernel.org/show_bug.cgi?id=215001

Best regards,
Javier


Javier Martinez Canillas (2):
  video: vga16fb: Fix logic that checks for the display standard
  video: vga16fb: Only probe for EGA and VGA 16 color graphic cards

 drivers/video/fbdev/vga16fb.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

-- 
2.33.1

[PATCH 1/2] video: vga16fb: Fix logic that checks for the display standard

[Javier Martinez Canillas]

The vga16fb framebuffer driver supports both Enhanced Graphics Adapter
(EGA) and Video Graphics Array (VGA) 16 color graphic cards.

But the logic to check whether the EGA or VGA standard are used is not
correct. It just checks if screen_info.orig_video_isVGA is set, but it
should check if is set to VIDEO_TYPE_VGAC instead.

This means that it assumes to be VGA even if is set to VIDEO_TYPE_EGAC.

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

 drivers/video/fbdev/vga16fb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/vga16fb.c b/drivers/video/fbdev/vga16fb.c
index e2757ff1c23d..3347c9b6a332 100644
--- a/drivers/video/fbdev/vga16fb.c
+++ b/drivers/video/fbdev/vga16fb.c
@@ -1332,7 +1332,7 @@ static int vga16fb_probe(struct platform_device *dev)
 	printk(KERN_INFO "vga16fb: mapped to 0x%p\n", info->screen_base);
 	par = info->par;
 
-	par->isVGA = screen_info.orig_video_isVGA;
+	par->isVGA = screen_info.orig_video_isVGA == VIDEO_TYPE_VGAC;
 	par->palette_blanked = 0;
 	par->vesa_blanked = 0;
 
-- 
2.33.1

Re: [PATCH 1/2] video: vga16fb: Fix logic that checks for the display standard

[Geert Uytterhoeven]

Hi Javier,

On Fri, Jan 7, 2022 at 9:00 PM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> The vga16fb framebuffer driver supports both Enhanced Graphics Adapter
> (EGA) and Video Graphics Array (VGA) 16 color graphic cards.
>
> But the logic to check whether the EGA or VGA standard are used is not
> correct. It just checks if screen_info.orig_video_isVGA is set, but it
> should check if is set to VIDEO_TYPE_VGAC instead.
>
> This means that it assumes to be VGA even if is set to VIDEO_TYPE_EGAC.
>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

Thanks for your patch!

> --- a/drivers/video/fbdev/vga16fb.c
> +++ b/drivers/video/fbdev/vga16fb.c
> @@ -1332,7 +1332,7 @@ static int vga16fb_probe(struct platform_device *dev)
>         printk(KERN_INFO "vga16fb: mapped to 0x%p\n", info->screen_base);
>         par = info->par;
>
> -       par->isVGA = screen_info.orig_video_isVGA;
> +       par->isVGA = screen_info.orig_video_isVGA == VIDEO_TYPE_VGAC;

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

>         par->palette_blanked = 0;
>         par->vesa_blanked = 0;
>

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

Re: [PATCH 1/2] video: vga16fb: Fix logic that checks for the display standard

[Kris Karas (Bug reporting)]

Groetje, Geert,

Geert Uytterhoeven wrote:
>
>> -       par->isVGA = screen_info.orig_video_isVGA;
>> +       par->isVGA = screen_info.orig_video_isVGA == VIDEO_TYPE_VGAC;
> All non-x86 architectures (except for 2 MIPS platforms) treat
> orig_video_isVGA as a boolean flag, and just assign 1 to it.
> Hence this change would break them.

I see a bit of a conflict with using orig_video_isVGA as a boolean. All 
the modern architecture-agnostic driver code, such as sysfb, 
sysfb_simplefb, and efifb, all use and expect orig_video_isVGA to be an 
integer.  On the other hand, the VGA driver for XEN first sets 
orig_video_isVGA  = 1 (boolean), and then VIDEO_TYPE_VLFB or 
VIDEO_TYPE_EFI (integer).  Overloading the definition for 
orig_video_isVGA to be both boolean and integer - within the same file - 
seems like a recipe for bugs to me.

That said, I think that wrapping the par->isVGA code, above, within a 
check for CONFIG_X86 seems safe and expedient.  But I would be much 
happier if the non-x86 architectures would set it to a proper integer 
value (even if fake) that coincidentally satisfies boolean "true", say 
VIDEO_TYPE_VGAC; that way, there would be no confusion as to data type 
in all the more recent architecture-agnostic framebuffer code.

Kris

Re: [PATCH 1/2] video: vga16fb: Fix logic that checks for the display standard

[Javier Martinez Canillas]

Hello Geert and Kara,

On 1/9/22 21:20, Kris Karas (Bug reporting) wrote:
> Groetje, Geert,
> 
> Geert Uytterhoeven wrote:
>>
>>> -       par->isVGA = screen_info.orig_video_isVGA;
>>> +       par->isVGA = screen_info.orig_video_isVGA == VIDEO_TYPE_VGAC;
>> All non-x86 architectures (except for 2 MIPS platforms) treat
>> orig_video_isVGA as a boolean flag, and just assign 1 to it.
>> Hence this change would break them.
> 
> I see a bit of a conflict with using orig_video_isVGA as a boolean. All 
> the modern architecture-agnostic driver code, such as sysfb, 
> sysfb_simplefb, and efifb, all use and expect orig_video_isVGA to be an 
> integer.  On the other hand, the VGA driver for XEN first sets 
> orig_video_isVGA  = 1 (boolean), and then VIDEO_TYPE_VLFB or 
> VIDEO_TYPE_EFI (integer).  Overloading the definition for 
> orig_video_isVGA to be both boolean and integer - within the same file - 
> seems like a recipe for bugs to me.
>

Agreed with Kara on this. I believe the non-x86 arches should be fixed
to set it to the correct value.
 
> That said, I think that wrapping the par->isVGA code, above, within a 
> check for CONFIG_X86 seems safe and expedient.  But I would be much 
> happier if the non-x86 architectures would set it to a proper integer 
> value (even if fake) that coincidentally satisfies boolean "true", say 
> VIDEO_TYPE_VGAC; that way, there would be no confusion as to data type 
> in all the more recent architecture-agnostic framebuffer code.
> 

Yes, I'll post a v2 to do that but hopefully the CONFIG_X86 guard could
be dropped in the future once all the architectures are fixed.

> Kris
> 

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

[PATCH 2/2] video: vga16fb: Only probe for EGA and VGA 16 color graphic cards

[Javier Martinez Canillas]

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

 drivers/video/fbdev/vga16fb.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/video/fbdev/vga16fb.c b/drivers/video/fbdev/vga16fb.c
index 3347c9b6a332..72b6aeceeff8 100644
--- 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;
+
+	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;
+
 	ret = platform_driver_register(&vga16fb_driver);
 
 	if (!ret) {
-- 
2.33.1

Re: [PATCH 2/2] video: vga16fb: Only probe for EGA and VGA 16 color graphic cards

[Geert Uytterhoeven]

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