Patchwork [v2] UBUNTU: SAUCE: Don't register vga16fb framebuffer if other framebuffers are present

login
register
mail settings
Submitter Chase Douglas
Date March 25, 2010, 6:59 p.m.
Message ID <1269543582-2242-1-git-send-email-chase.douglas@canonical.com>
Download mbox | patch
Permalink /patch/48585/
State Accepted
Delegated to: Andy Whitcroft
Headers show

Comments

Chase Douglas - March 25, 2010, 6:59 p.m.
Using the vga16fb framebuffer is not safe when other framebuffers are
present. This fixes the case where the vga16fb module is loaded after
a better framebuffer is loaded (since vga16fb is by definition the
worst-case framebuffer).

There does not appear to be any locking around the num_registered_fb, so
this is a hack at best. However, in Lucid we build vga16fb as a module.
Modules are loaded serially, so this should be ok for Lucid. In M, we
will transition to efifb and drop vga16fb, so this is a one time hack.

This prevents sudo lshw from corrupting /dev/fb0 by writing to vga16fb
through /dev/fb1.

BugLink: http://bugs.launchpad.net/bugs/527369

Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
---
 drivers/video/vga16fb.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)
Chase Douglas - March 25, 2010, 7:03 p.m.
Version 2 spits out the following at boot:

[   19.806985] vga16fb: initializing
[   19.806989] vga16fb: mapped to 0xffff8800000a0000
[   19.806992] vga16fb: not registering due to another framebuffer present

-- Chase

On Thu, Mar 25, 2010 at 2:59 PM, Chase Douglas
<chase.douglas@canonical.com> wrote:
> Using the vga16fb framebuffer is not safe when other framebuffers are
> present. This fixes the case where the vga16fb module is loaded after
> a better framebuffer is loaded (since vga16fb is by definition the
> worst-case framebuffer).
>
> There does not appear to be any locking around the num_registered_fb, so
> this is a hack at best. However, in Lucid we build vga16fb as a module.
> Modules are loaded serially, so this should be ok for Lucid. In M, we
> will transition to efifb and drop vga16fb, so this is a one time hack.
>
> This prevents sudo lshw from corrupting /dev/fb0 by writing to vga16fb
> through /dev/fb1.
>
> BugLink: http://bugs.launchpad.net/bugs/527369
>
> Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> ---
>  drivers/video/vga16fb.c |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/video/vga16fb.c b/drivers/video/vga16fb.c
> index efde41d..d8f8313 100644
> --- a/drivers/video/vga16fb.c
> +++ b/drivers/video/vga16fb.c
> @@ -1355,6 +1355,16 @@ static int __init vga16fb_probe(struct platform_device *dev)
>
>        vga16fb_update_fix(info);
>
> +       /*
> +        * Ubuntu: Don't register vga16fb if another fb exists. Bad interactions
> +        * can occur.
> +        */
> +       if (num_registered_fb > 0) {
> +               printk(KERN_NOTICE "vga16fb: not registering due to another "
> +                      "framebuffer present\n");
> +               goto err_check_var;
> +       }
> +
>        if (register_framebuffer(info) < 0) {
>                printk(KERN_ERR "vga16fb: unable to register framebuffer\n");
>                ret = -EINVAL;
> --
> 1.7.0
>
>
> --
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
>
Amit Kucheria - March 26, 2010, 8:12 a.m.
I should learn to read my email in the reverse time-stamp order. I'd save on
many replies :)


On 10 Mar 25, Chase Douglas wrote:
> Using the vga16fb framebuffer is not safe when other framebuffers are
> present. This fixes the case where the vga16fb module is loaded after
> a better framebuffer is loaded (since vga16fb is by definition the
> worst-case framebuffer).

So what happens in the case where vga16fb is the first one loaded and some
other (better) fb is loaded later?

> There does not appear to be any locking around the num_registered_fb, so
> this is a hack at best. However, in Lucid we build vga16fb as a module.
> Modules are loaded serially, so this should be ok for Lucid. In M, we
> will transition to efifb and drop vga16fb, so this is a one time hack.
> 
> This prevents sudo lshw from corrupting /dev/fb0 by writing to vga16fb
> through /dev/fb1.
> 
> BugLink: http://bugs.launchpad.net/bugs/527369
> 
> Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> ---
>  drivers/video/vga16fb.c |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/video/vga16fb.c b/drivers/video/vga16fb.c
> index efde41d..d8f8313 100644
> --- a/drivers/video/vga16fb.c
> +++ b/drivers/video/vga16fb.c
> @@ -1355,6 +1355,16 @@ static int __init vga16fb_probe(struct platform_device *dev)
>  
>  	vga16fb_update_fix(info);
>  
> +	/*
> +	 * Ubuntu: Don't register vga16fb if another fb exists. Bad interactions
> +	 * can occur.
> +	 */
> +	if (num_registered_fb > 0) {
> +		printk(KERN_NOTICE "vga16fb: not registering due to another "
> +		       "framebuffer present\n");
> +		goto err_check_var;
> +	}
> +
>  	if (register_framebuffer(info) < 0) {
>  		printk(KERN_ERR "vga16fb: unable to register framebuffer\n");
>  		ret = -EINVAL;
> -- 
> 1.7.0
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
Chase Douglas - March 26, 2010, 11:51 a.m.
On Fri, Mar 26, 2010 at 4:12 AM, Amit Kucheria
<amit.kucheria@canonical.com> wrote:
> On 10 Mar 25, Chase Douglas wrote:
>> Using the vga16fb framebuffer is not safe when other framebuffers are
>> present. This fixes the case where the vga16fb module is loaded after
>> a better framebuffer is loaded (since vga16fb is by definition the
>> worst-case framebuffer).
>
> So what happens in the case where vga16fb is the first one loaded and some
> other (better) fb is loaded later?

There's two answers:

1. This patch is in vga16fb itself, so it only prevents itself from
registering as a framebuffer. If vga16fb gets /dev/fb0, any other
framebuffer can still register further framebuffers.

2. That doesn't really matter cause we're screwed if vga16fb is first
when we try to use KMS. KMS drivers have their own fb, but only
/dev/fb0 is ever used, so you get bad interactions in that case.
Thankfully, no one seems to be having this issue anymore because the
KMS drivers are loaded before vga16fb and module loading is serialized
by the kernel. If we do find it happens, we may have to tell people to
blacklist vga16fb manually. All this is secondary to the issue fixed
by this patch, however.

-- Chase
Stefan Bader - March 26, 2010, 12:54 p.m.
Chase Douglas wrote:
> On Fri, Mar 26, 2010 at 4:12 AM, Amit Kucheria
> <amit.kucheria@canonical.com> wrote:
>> On 10 Mar 25, Chase Douglas wrote:
>>> Using the vga16fb framebuffer is not safe when other framebuffers are
>>> present. This fixes the case where the vga16fb module is loaded after
>>> a better framebuffer is loaded (since vga16fb is by definition the
>>> worst-case framebuffer).
>> So what happens in the case where vga16fb is the first one loaded and some
>> other (better) fb is loaded later?
> 
> There's two answers:
> 
> 1. This patch is in vga16fb itself, so it only prevents itself from
> registering as a framebuffer. If vga16fb gets /dev/fb0, any other
> framebuffer can still register further framebuffers.
> 
> 2. That doesn't really matter cause we're screwed if vga16fb is first
> when we try to use KMS. KMS drivers have their own fb, but only
> /dev/fb0 is ever used, so you get bad interactions in that case.
> Thankfully, no one seems to be having this issue anymore because the
> KMS drivers are loaded before vga16fb and module loading is serialized
> by the kernel. If we do find it happens, we may have to tell people to
> blacklist vga16fb manually. All this is secondary to the issue fixed
> by this patch, however.
> 
> -- Chase
> 
I think I will proceed with this by
a) currently test building with it applied
b) test that build on a few local systems
c) if it seems to work include it in this weeks upload but we will need
   to pay close attention on it

-Stefan
Stefan Bader - March 26, 2010, 5:37 p.m.
I tested the patch on 4 systems, 2 radeonfb, 1 nouveau and one i915.
All behaved in the same (more or less broken) way as before. Three would have
loaded vga16fb while they already had another fb running. As I see no regression
here, I went ahead, acked and applied it.
Lets have a keen eye on gfx bugs next week.

Stefan

Applied to Lucid master

Patch

diff --git a/drivers/video/vga16fb.c b/drivers/video/vga16fb.c
index efde41d..d8f8313 100644
--- a/drivers/video/vga16fb.c
+++ b/drivers/video/vga16fb.c
@@ -1355,6 +1355,16 @@  static int __init vga16fb_probe(struct platform_device *dev)
 
 	vga16fb_update_fix(info);
 
+	/*
+	 * Ubuntu: Don't register vga16fb if another fb exists. Bad interactions
+	 * can occur.
+	 */
+	if (num_registered_fb > 0) {
+		printk(KERN_NOTICE "vga16fb: not registering due to another "
+		       "framebuffer present\n");
+		goto err_check_var;
+	}
+
 	if (register_framebuffer(info) < 0) {
 		printk(KERN_ERR "vga16fb: unable to register framebuffer\n");
 		ret = -EINVAL;