| Message ID | 20220208210824.2238981-10-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fbcon patches, take two | expand |
Hi Am 08.02.22 um 22:08 schrieb Daniel Vetter: > There's two minor behaviour changes in here: > - in error paths we now consistently call fb_ops->fb_release > - fb_release really can't fail (fbmem.c ignores it too) and there's no > reasonable cleanup we can do anyway. > > Note that everything in fbcon.c is protected by the big console_lock() > lock (especially all the global variables), so the minor changes in > ordering of setup/cleanup do not matter. > > v2: Explain a bit better why this is all correct (Sam) > > Acked-by: Sam Ravnborg <sam@ravnborg.org> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: Claudio Suarez <cssk@net-c.es> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > Cc: Du Cheng <ducheng2@gmail.com> > --- > drivers/video/fbdev/core/fbcon.c | 107 +++++++++++++++---------------- > 1 file changed, 53 insertions(+), 54 deletions(-) > > diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c > index 058e885d24f6..3e1a3e7bf527 100644 > --- a/drivers/video/fbdev/core/fbcon.c > +++ b/drivers/video/fbdev/core/fbcon.c > @@ -682,19 +682,37 @@ static int fbcon_invalid_charcount(struct fb_info *info, unsigned charcount) > > #endif /* CONFIG_MISC_TILEBLITTING */ > > +static int fbcon_open(struct fb_info *info) > +{ > + if (!try_module_get(info->fbops->owner)) > + return -ENODEV; > + > + if (info->fbops->fb_open && > + info->fbops->fb_open(info, 0)) { > + module_put(info->fbops->owner); > + return -ENODEV; > + } > + > + return 0; > +} > + > +static void fbcon_release(struct fb_info *info) > +{ > + if (info->fbops->fb_release) > + info->fbops->fb_release(info, 0); > + > + module_put(info->fbops->owner); > +} > > static int con2fb_acquire_newinfo(struct vc_data *vc, struct fb_info *info, > int unit, int oldidx) > { > struct fbcon_ops *ops = NULL; > - int err = 0; > - > - if (!try_module_get(info->fbops->owner)) > - err = -ENODEV; > + int err; > > - if (!err && info->fbops->fb_open && > - info->fbops->fb_open(info, 0)) > - err = -ENODEV; > + err = fbcon_open(info); > + if (err) > + return err; > > if (!err) { > ops = kzalloc(sizeof(struct fbcon_ops), GFP_KERNEL); > @@ -715,7 +733,7 @@ static int con2fb_acquire_newinfo(struct vc_data *vc, struct fb_info *info, > > if (err) { > con2fb_map[unit] = oldidx; > - module_put(info->fbops->owner); > + fbcon_release(info); > } > > return err; > @@ -726,45 +744,34 @@ static int con2fb_release_oldinfo(struct vc_data *vc, struct fb_info *oldinfo, > int oldidx, int found) > { > struct fbcon_ops *ops = oldinfo->fbcon_par; > - int err = 0, ret; > + int ret; > > - if (oldinfo->fbops->fb_release && > - oldinfo->fbops->fb_release(oldinfo, 0)) { > - con2fb_map[unit] = oldidx; We don't need oldidx any longer? There's some logic wrt to the parameter 'found' here and in set_con2fb_map() that appears to be relevant. Best regards Thomas > - if (!found && newinfo->fbops->fb_release) > - newinfo->fbops->fb_release(newinfo, 0); > - if (!found) > - module_put(newinfo->fbops->owner); > - err = -ENODEV; > - } > + fbcon_release(oldinfo); > > - if (!err) { > - fbcon_del_cursor_work(oldinfo); > - kfree(ops->cursor_state.mask); > - kfree(ops->cursor_data); > - kfree(ops->cursor_src); > - kfree(ops->fontbuffer); > - kfree(oldinfo->fbcon_par); > - oldinfo->fbcon_par = NULL; > - module_put(oldinfo->fbops->owner); > - /* > - If oldinfo and newinfo are driving the same hardware, > - the fb_release() method of oldinfo may attempt to > - restore the hardware state. This will leave the > - newinfo in an undefined state. Thus, a call to > - fb_set_par() may be needed for the newinfo. > - */ > - if (newinfo && newinfo->fbops->fb_set_par) { > - ret = newinfo->fbops->fb_set_par(newinfo); > + fbcon_del_cursor_work(oldinfo); > + kfree(ops->cursor_state.mask); > + kfree(ops->cursor_data); > + kfree(ops->cursor_src); > + kfree(ops->fontbuffer); > + kfree(oldinfo->fbcon_par); > + oldinfo->fbcon_par = NULL; > + /* > + If oldinfo and newinfo are driving the same hardware, > + the fb_release() method of oldinfo may attempt to > + restore the hardware state. This will leave the > + newinfo in an undefined state. Thus, a call to > + fb_set_par() may be needed for the newinfo. > + */ > + if (newinfo && newinfo->fbops->fb_set_par) { > + ret = newinfo->fbops->fb_set_par(newinfo); > > - if (ret) > - printk(KERN_ERR "con2fb_release_oldinfo: " > - "detected unhandled fb_set_par error, " > - "error code %d\n", ret); > - } > + if (ret) > + printk(KERN_ERR "con2fb_release_oldinfo: " > + "detected unhandled fb_set_par error, " > + "error code %d\n", ret); > } > > - return err; > + return 0; > } > > static void con2fb_init_display(struct vc_data *vc, struct fb_info *info, > @@ -919,7 +926,6 @@ static const char *fbcon_startup(void) > struct fbcon_display *p = &fb_display[fg_console]; > struct vc_data *vc = vc_cons[fg_console].d; > const struct font_desc *font = NULL; > - struct module *owner; > struct fb_info *info = NULL; > struct fbcon_ops *ops; > int rows, cols; > @@ -938,17 +944,12 @@ static const char *fbcon_startup(void) > if (!info) > return NULL; > > - owner = info->fbops->owner; > - if (!try_module_get(owner)) > + if (fbcon_open(info)) > return NULL; > - if (info->fbops->fb_open && info->fbops->fb_open(info, 0)) { > - module_put(owner); > - return NULL; > - } > > ops = kzalloc(sizeof(struct fbcon_ops), GFP_KERNEL); > if (!ops) { > - module_put(owner); > + fbcon_release(info); > return NULL; > } > > @@ -3314,10 +3315,6 @@ static void fbcon_exit(void) > } > > if (mapped) { > - if (info->fbops->fb_release) > - info->fbops->fb_release(info, 0); > - module_put(info->fbops->owner); > - > if (info->fbcon_par) { > struct fbcon_ops *ops = info->fbcon_par; > > @@ -3327,6 +3324,8 @@ static void fbcon_exit(void) > kfree(info->fbcon_par); > info->fbcon_par = NULL; > } > + > + fbcon_release(info); > } > } > }
On Thu, Feb 10, 2022 at 12:46:32PM +0100, Thomas Zimmermann wrote: > Hi > > Am 08.02.22 um 22:08 schrieb Daniel Vetter: > > There's two minor behaviour changes in here: > > - in error paths we now consistently call fb_ops->fb_release > > - fb_release really can't fail (fbmem.c ignores it too) and there's no > > reasonable cleanup we can do anyway. > > > > Note that everything in fbcon.c is protected by the big console_lock() > > lock (especially all the global variables), so the minor changes in > > ordering of setup/cleanup do not matter. > > > > v2: Explain a bit better why this is all correct (Sam) > > > > Acked-by: Sam Ravnborg <sam@ravnborg.org> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > Cc: Daniel Vetter <daniel@ffwll.ch> > > Cc: Claudio Suarez <cssk@net-c.es> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > > Cc: Du Cheng <ducheng2@gmail.com> > > --- > > drivers/video/fbdev/core/fbcon.c | 107 +++++++++++++++---------------- > > 1 file changed, 53 insertions(+), 54 deletions(-) > > > > diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c > > index 058e885d24f6..3e1a3e7bf527 100644 > > --- a/drivers/video/fbdev/core/fbcon.c > > +++ b/drivers/video/fbdev/core/fbcon.c > > @@ -682,19 +682,37 @@ static int fbcon_invalid_charcount(struct fb_info *info, unsigned charcount) > > #endif /* CONFIG_MISC_TILEBLITTING */ > > +static int fbcon_open(struct fb_info *info) > > +{ > > + if (!try_module_get(info->fbops->owner)) > > + return -ENODEV; > > + > > + if (info->fbops->fb_open && > > + info->fbops->fb_open(info, 0)) { > > + module_put(info->fbops->owner); > > + return -ENODEV; > > + } > > + > > + return 0; > > +} > > + > > +static void fbcon_release(struct fb_info *info) > > +{ > > + if (info->fbops->fb_release) > > + info->fbops->fb_release(info, 0); > > + > > + module_put(info->fbops->owner); > > +} > > static int con2fb_acquire_newinfo(struct vc_data *vc, struct fb_info *info, > > int unit, int oldidx) > > { > > struct fbcon_ops *ops = NULL; > > - int err = 0; > > - > > - if (!try_module_get(info->fbops->owner)) > > - err = -ENODEV; > > + int err; > > - if (!err && info->fbops->fb_open && > > - info->fbops->fb_open(info, 0)) > > - err = -ENODEV; > > + err = fbcon_open(info); > > + if (err) > > + return err; > > if (!err) { > > ops = kzalloc(sizeof(struct fbcon_ops), GFP_KERNEL); > > @@ -715,7 +733,7 @@ static int con2fb_acquire_newinfo(struct vc_data *vc, struct fb_info *info, > > if (err) { > > con2fb_map[unit] = oldidx; > > - module_put(info->fbops->owner); > > + fbcon_release(info); > > } > > return err; > > @@ -726,45 +744,34 @@ static int con2fb_release_oldinfo(struct vc_data *vc, struct fb_info *oldinfo, > > int oldidx, int found) > > { > > struct fbcon_ops *ops = oldinfo->fbcon_par; > > - int err = 0, ret; > > + int ret; > > - if (oldinfo->fbops->fb_release && > > - oldinfo->fbops->fb_release(oldinfo, 0)) { > > - con2fb_map[unit] = oldidx; > > We don't need oldidx any longer? > > There's some logic wrt to the parameter 'found' here and in set_con2fb_map() > that appears to be relevant. Yeah further patches clean this up more. Or did you see a potential bug here? I did ditch the fb_release error handling, simply because there's not really much we can do anyway, it shouldn't ever fail (that's a driver bug) and it was convoluting the code for no gain. But I might have missed something in this cargo-cult. -Daniel > > Best regards > Thomas > > > > - if (!found && newinfo->fbops->fb_release) > > - newinfo->fbops->fb_release(newinfo, 0); > > - if (!found) > > - module_put(newinfo->fbops->owner); > > - err = -ENODEV; > > - } > > + fbcon_release(oldinfo); > > - if (!err) { > > - fbcon_del_cursor_work(oldinfo); > > - kfree(ops->cursor_state.mask); > > - kfree(ops->cursor_data); > > - kfree(ops->cursor_src); > > - kfree(ops->fontbuffer); > > - kfree(oldinfo->fbcon_par); > > - oldinfo->fbcon_par = NULL; > > - module_put(oldinfo->fbops->owner); > > - /* > > - If oldinfo and newinfo are driving the same hardware, > > - the fb_release() method of oldinfo may attempt to > > - restore the hardware state. This will leave the > > - newinfo in an undefined state. Thus, a call to > > - fb_set_par() may be needed for the newinfo. > > - */ > > - if (newinfo && newinfo->fbops->fb_set_par) { > > - ret = newinfo->fbops->fb_set_par(newinfo); > > + fbcon_del_cursor_work(oldinfo); > > + kfree(ops->cursor_state.mask); > > + kfree(ops->cursor_data); > > + kfree(ops->cursor_src); > > + kfree(ops->fontbuffer); > > + kfree(oldinfo->fbcon_par); > > + oldinfo->fbcon_par = NULL; > > + /* > > + If oldinfo and newinfo are driving the same hardware, > > + the fb_release() method of oldinfo may attempt to > > + restore the hardware state. This will leave the > > + newinfo in an undefined state. Thus, a call to > > + fb_set_par() may be needed for the newinfo. > > + */ > > + if (newinfo && newinfo->fbops->fb_set_par) { > > + ret = newinfo->fbops->fb_set_par(newinfo); > > - if (ret) > > - printk(KERN_ERR "con2fb_release_oldinfo: " > > - "detected unhandled fb_set_par error, " > > - "error code %d\n", ret); > > - } > > + if (ret) > > + printk(KERN_ERR "con2fb_release_oldinfo: " > > + "detected unhandled fb_set_par error, " > > + "error code %d\n", ret); > > } > > - return err; > > + return 0; > > } > > static void con2fb_init_display(struct vc_data *vc, struct fb_info *info, > > @@ -919,7 +926,6 @@ static const char *fbcon_startup(void) > > struct fbcon_display *p = &fb_display[fg_console]; > > struct vc_data *vc = vc_cons[fg_console].d; > > const struct font_desc *font = NULL; > > - struct module *owner; > > struct fb_info *info = NULL; > > struct fbcon_ops *ops; > > int rows, cols; > > @@ -938,17 +944,12 @@ static const char *fbcon_startup(void) > > if (!info) > > return NULL; > > > > - owner = info->fbops->owner; > > - if (!try_module_get(owner)) > > + if (fbcon_open(info)) > > return NULL; > > - if (info->fbops->fb_open && info->fbops->fb_open(info, 0)) { > > - module_put(owner); > > - return NULL; > > - } > > ops = kzalloc(sizeof(struct fbcon_ops), GFP_KERNEL); > > if (!ops) { > > - module_put(owner); > > + fbcon_release(info); > > return NULL; > > } > > @@ -3314,10 +3315,6 @@ static void fbcon_exit(void) > > } > > if (mapped) { > > - if (info->fbops->fb_release) > > - info->fbops->fb_release(info, 0); > > - module_put(info->fbops->owner); > > - > > if (info->fbcon_par) { > > struct fbcon_ops *ops = info->fbcon_par; > > @@ -3327,6 +3324,8 @@ static void fbcon_exit(void) > > kfree(info->fbcon_par); > > info->fbcon_par = NULL; > > } > > + > > + fbcon_release(info); > > } > > } > > } > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > (HRB 36809, AG Nürnberg) > Geschäftsführer: Ivo Totev
diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c index 058e885d24f6..3e1a3e7bf527 100644 --- a/drivers/video/fbdev/core/fbcon.c +++ b/drivers/video/fbdev/core/fbcon.c @@ -682,19 +682,37 @@ static int fbcon_invalid_charcount(struct fb_info *info, unsigned charcount) #endif /* CONFIG_MISC_TILEBLITTING */ +static int fbcon_open(struct fb_info *info) +{ + if (!try_module_get(info->fbops->owner)) + return -ENODEV; + + if (info->fbops->fb_open && + info->fbops->fb_open(info, 0)) { + module_put(info->fbops->owner); + return -ENODEV; + } + + return 0; +} + +static void fbcon_release(struct fb_info *info) +{ + if (info->fbops->fb_release) + info->fbops->fb_release(info, 0); + + module_put(info->fbops->owner); +} static int con2fb_acquire_newinfo(struct vc_data *vc, struct fb_info *info, int unit, int oldidx) { struct fbcon_ops *ops = NULL; - int err = 0; - - if (!try_module_get(info->fbops->owner)) - err = -ENODEV; + int err; - if (!err && info->fbops->fb_open && - info->fbops->fb_open(info, 0)) - err = -ENODEV; + err = fbcon_open(info); + if (err) + return err; if (!err) { ops = kzalloc(sizeof(struct fbcon_ops), GFP_KERNEL); @@ -715,7 +733,7 @@ static int con2fb_acquire_newinfo(struct vc_data *vc, struct fb_info *info, if (err) { con2fb_map[unit] = oldidx; - module_put(info->fbops->owner); + fbcon_release(info); } return err; @@ -726,45 +744,34 @@ static int con2fb_release_oldinfo(struct vc_data *vc, struct fb_info *oldinfo, int oldidx, int found) { struct fbcon_ops *ops = oldinfo->fbcon_par; - int err = 0, ret; + int ret; - if (oldinfo->fbops->fb_release && - oldinfo->fbops->fb_release(oldinfo, 0)) { - con2fb_map[unit] = oldidx; - if (!found && newinfo->fbops->fb_release) - newinfo->fbops->fb_release(newinfo, 0); - if (!found) - module_put(newinfo->fbops->owner); - err = -ENODEV; - } + fbcon_release(oldinfo); - if (!err) { - fbcon_del_cursor_work(oldinfo); - kfree(ops->cursor_state.mask); - kfree(ops->cursor_data); - kfree(ops->cursor_src); - kfree(ops->fontbuffer); - kfree(oldinfo->fbcon_par); - oldinfo->fbcon_par = NULL; - module_put(oldinfo->fbops->owner); - /* - If oldinfo and newinfo are driving the same hardware, - the fb_release() method of oldinfo may attempt to - restore the hardware state. This will leave the - newinfo in an undefined state. Thus, a call to - fb_set_par() may be needed for the newinfo. - */ - if (newinfo && newinfo->fbops->fb_set_par) { - ret = newinfo->fbops->fb_set_par(newinfo); + fbcon_del_cursor_work(oldinfo); + kfree(ops->cursor_state.mask); + kfree(ops->cursor_data); + kfree(ops->cursor_src); + kfree(ops->fontbuffer); + kfree(oldinfo->fbcon_par); + oldinfo->fbcon_par = NULL; + /* + If oldinfo and newinfo are driving the same hardware, + the fb_release() method of oldinfo may attempt to + restore the hardware state. This will leave the + newinfo in an undefined state. Thus, a call to + fb_set_par() may be needed for the newinfo. + */ + if (newinfo && newinfo->fbops->fb_set_par) { + ret = newinfo->fbops->fb_set_par(newinfo); - if (ret) - printk(KERN_ERR "con2fb_release_oldinfo: " - "detected unhandled fb_set_par error, " - "error code %d\n", ret); - } + if (ret) + printk(KERN_ERR "con2fb_release_oldinfo: " + "detected unhandled fb_set_par error, " + "error code %d\n", ret); } - return err; + return 0; } static void con2fb_init_display(struct vc_data *vc, struct fb_info *info, @@ -919,7 +926,6 @@ static const char *fbcon_startup(void) struct fbcon_display *p = &fb_display[fg_console]; struct vc_data *vc = vc_cons[fg_console].d; const struct font_desc *font = NULL; - struct module *owner; struct fb_info *info = NULL; struct fbcon_ops *ops; int rows, cols; @@ -938,17 +944,12 @@ static const char *fbcon_startup(void) if (!info) return NULL; - owner = info->fbops->owner; - if (!try_module_get(owner)) + if (fbcon_open(info)) return NULL; - if (info->fbops->fb_open && info->fbops->fb_open(info, 0)) { - module_put(owner); - return NULL; - } ops = kzalloc(sizeof(struct fbcon_ops), GFP_KERNEL); if (!ops) { - module_put(owner); + fbcon_release(info); return NULL; } @@ -3314,10 +3315,6 @@ static void fbcon_exit(void) } if (mapped) { - if (info->fbops->fb_release) - info->fbops->fb_release(info, 0); - module_put(info->fbops->owner); - if (info->fbcon_par) { struct fbcon_ops *ops = info->fbcon_par; @@ -3327,6 +3324,8 @@ static void fbcon_exit(void) kfree(info->fbcon_par); info->fbcon_par = NULL; } + + fbcon_release(info); } } }