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

login
register
mail settings
Submitter Chase Douglas
Date March 25, 2010, 4:53 p.m.
Message ID <1269536010-2488-1-git-send-email-chase.douglas@canonical.com>
Download mbox | patch
Permalink /patch/48545/
State Superseded
Headers show

Comments

Chase Douglas - March 25, 2010, 4:53 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 |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Bryce Harrington - March 25, 2010, 6:27 p.m.
This is great to see.  My one suggestion would be perhaps making the log
message be a warning (or just info) rather than an error, to prevent
users mistakenly thinking lack of vga16fb is causing some other random
bug they're having?

On Thu, Mar 25, 2010 at 12:53:30PM -0400, 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).
> 
> 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 |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/video/vga16fb.c b/drivers/video/vga16fb.c
> index efde41d..b1d62ef 100644
> --- a/drivers/video/vga16fb.c
> +++ b/drivers/video/vga16fb.c
> @@ -1355,7 +1355,7 @@ static int __init vga16fb_probe(struct platform_device *dev)
>  
>  	vga16fb_update_fix(info);
>  
> -	if (register_framebuffer(info) < 0) {
> +	if (num_registered_fb > 0 || register_framebuffer(info) < 0) {
>  		printk(KERN_ERR "vga16fb: unable to register framebuffer\n");
>  		ret = -EINVAL;
>  		goto err_check_var;
> -- 
> 1.7.0
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
Stefan Bader - March 25, 2010, 6:34 p.m.
Bryce Harrington wrote:
> This is great to see.  My one suggestion would be perhaps making the log
> message be a warning (or just info) rather than an error, to prevent
> users mistakenly thinking lack of vga16fb is causing some other random
> bug they're having?
> 
> On Thu, Mar 25, 2010 at 12:53:30PM -0400, 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).
>>
>> 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 |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/video/vga16fb.c b/drivers/video/vga16fb.c
>> index efde41d..b1d62ef 100644
>> --- a/drivers/video/vga16fb.c
>> +++ b/drivers/video/vga16fb.c
>> @@ -1355,7 +1355,7 @@ static int __init vga16fb_probe(struct platform_device *dev)
>>  
>>  	vga16fb_update_fix(info);
>>  
>> -	if (register_framebuffer(info) < 0) {
>> +	if (num_registered_fb > 0 || register_framebuffer(info) < 0) {
>>  		printk(KERN_ERR "vga16fb: unable to register framebuffer\n");
>>  		ret = -EINVAL;
>>  		goto err_check_var;
>> -- 
>> 1.7.0
>>
>>
>> -- 
>> kernel-team mailing list
>> kernel-team@lists.ubuntu.com
>> https://lists.ubuntu.com/mailman/listinfo/kernel-team
> 
This might be an idea. Ok the patch would get a little bigger but probably we
want at least to differentiate between the case where it cannot register
compared to the one where it does not want to.
And I want to sleep a night over it. It feels a bit like a case where dragons
might be hidden. Though we probably want to have it in quick to catch any
outfall rather sooner that later.

Stefan
Chase Douglas - March 25, 2010, 6:35 p.m.
On Thu, Mar 25, 2010 at 2:27 PM, Bryce Harrington <bryce@canonical.com> wrote:
> This is great to see.  My one suggestion would be perhaps making the log
> message be a warning (or just info) rather than an error, to prevent
> users mistakenly thinking lack of vga16fb is causing some other random
> bug they're having?

Heh, the "warning" that is output to dmesg by this is:

[   20.586867] vga16fb: initializing
[   20.586870] vga16fb: mapped to 0xffff8800000a0000
[   20.586874] vga16fb: unable to register framebuffer
[   20.587074] vga16fb: probe of vga16fb.0 failed with error -22

I can make a new patch to dampen the messages, but I went for as small
a change as possible. If it's decided that we should go this route,
then I can submit a new patch.

-- Chase
Amit Kucheria - March 26, 2010, 8:09 a.m.
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).
> 
> 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 |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/video/vga16fb.c b/drivers/video/vga16fb.c
> index efde41d..b1d62ef 100644
> --- a/drivers/video/vga16fb.c
> +++ b/drivers/video/vga16fb.c
> @@ -1355,7 +1355,7 @@ static int __init vga16fb_probe(struct platform_device *dev)
>  
>  	vga16fb_update_fix(info);
>  
> -	if (register_framebuffer(info) < 0) {
> +	if (num_registered_fb > 0 || register_framebuffer(info) < 0) {
>  		printk(KERN_ERR "vga16fb: unable to register framebuffer\n");
>  		ret = -EINVAL;
>  		goto err_check_var;
> -- 
> 1.7.0

We should differentiate the case when there was a real error from the one
where we bailed out on purpose. I think the extra cost to patch size would be
justified.

Regards,
Amit

Patch

diff --git a/drivers/video/vga16fb.c b/drivers/video/vga16fb.c
index efde41d..b1d62ef 100644
--- a/drivers/video/vga16fb.c
+++ b/drivers/video/vga16fb.c
@@ -1355,7 +1355,7 @@  static int __init vga16fb_probe(struct platform_device *dev)
 
 	vga16fb_update_fix(info);
 
-	if (register_framebuffer(info) < 0) {
+	if (num_registered_fb > 0 || register_framebuffer(info) < 0) {
 		printk(KERN_ERR "vga16fb: unable to register framebuffer\n");
 		ret = -EINVAL;
 		goto err_check_var;