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 > Signed-off-by: Daniel Vetter > Cc: Daniel Vetter > Cc: Claudio Suarez > Cc: Greg Kroah-Hartman > Cc: Tetsuo Handa > Cc: Du Cheng > --- > 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); > } > } > } -- 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