| Message ID | 1269536010-2488-1-git-send-email-chase.douglas@canonical.com |
|---|---|
| State | Superseded |
| Headers | show |
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
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
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
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
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;
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(-)