| Message ID | 20220131210552.482606-12-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | some fbcon patches, mostly locking | expand |
Hi Daniel, On Mon, Jan 31, 2022 at 10:05:42PM +0100, Daniel Vetter wrote: > 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. > > 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 fa30e1909164..eea2ee14b64c 100644 > --- a/drivers/video/fbdev/core/fbcon.c > +++ b/drivers/video/fbdev/core/fbcon.c > @@ -680,19 +680,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); > @@ -713,7 +731,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; > @@ -724,45 +742,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; The old code assigns con2fb_map[unit] before is calls newinfo->fbops->fb_release). I wonder if there can be any callback to fbcon where the value of con2fb_map[unit] matters? > - 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; These all look like candidates to stuff into fbcon_release() That would drop the nice symmetry but make it more consistent. I think we miss freeing ops->cursor_data in fbcon_exit(), but I did not follow all the code. With my ramblings considered the patch is Acked-by: Sam Ravnborg <sam@ravnborg.org> Sam > + /* > + 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, > @@ -917,7 +924,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; > @@ -936,17 +942,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; > } > > @@ -3331,10 +3332,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; > > @@ -3344,6 +3341,8 @@ static void fbcon_exit(void) > kfree(info->fbcon_par); > info->fbcon_par = NULL; > } > + > + fbcon_release(info); > } > } > } > -- > 2.33.0
Hi Daniel, > > > + kfree(ops->cursor_state.mask); > > + kfree(ops->cursor_data); > > + kfree(ops->cursor_src); > > + kfree(ops->fontbuffer); > > + kfree(oldinfo->fbcon_par); > > + oldinfo->fbcon_par = NULL; > These all look like candidates to stuff into fbcon_release() > That would drop the nice symmetry but make it more consistent. > > I think we miss freeing ops->cursor_data in fbcon_exit(), > but I did not follow all the code. We agree as I can see this was done in a later patch. Sam
On Thu, Feb 03, 2022 at 10:15:14PM +0100, Sam Ravnborg wrote: > Hi Daniel, > > On Mon, Jan 31, 2022 at 10:05:42PM +0100, Daniel Vetter wrote: > > 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. > > > > 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 fa30e1909164..eea2ee14b64c 100644 > > --- a/drivers/video/fbdev/core/fbcon.c > > +++ b/drivers/video/fbdev/core/fbcon.c > > @@ -680,19 +680,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); > > @@ -713,7 +731,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; > > @@ -724,45 +742,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; > The old code assigns con2fb_map[unit] before is calls > newinfo->fbops->fb_release). > I wonder if there can be any callback to fbcon where the value > of con2fb_map[unit] matters? It's all protected by console_lock, so other threads cannot see the inconsistent state. Essentially everything in fbcon.c is protected by console_lock(). Do you want me to hammer this in somewhere (maybe in the commit message), or good enough for your ack? -Daniel > > > > - 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; > These all look like candidates to stuff into fbcon_release() > That would drop the nice symmetry but make it more consistent. > > I think we miss freeing ops->cursor_data in fbcon_exit(), > but I did not follow all the code. > > With my ramblings considered the patch is > Acked-by: Sam Ravnborg <sam@ravnborg.org> > > Sam > > > + /* > > + 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, > > @@ -917,7 +924,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; > > @@ -936,17 +942,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; > > } > > > > @@ -3331,10 +3332,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; > > > > @@ -3344,6 +3341,8 @@ static void fbcon_exit(void) > > kfree(info->fbcon_par); > > info->fbcon_par = NULL; > > } > > + > > + fbcon_release(info); > > } > > } > > } > > -- > > 2.33.0
Hi Daniel, On Tue, Feb 08, 2022 at 02:48:29PM +0100, Daniel Vetter wrote: > On Thu, Feb 03, 2022 at 10:15:14PM +0100, Sam Ravnborg wrote: > > Hi Daniel, > > > > On Mon, Jan 31, 2022 at 10:05:42PM +0100, Daniel Vetter wrote: > > > 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. > > > > > > 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 fa30e1909164..eea2ee14b64c 100644 > > > --- a/drivers/video/fbdev/core/fbcon.c > > > +++ b/drivers/video/fbdev/core/fbcon.c > > > @@ -680,19 +680,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); > > > @@ -713,7 +731,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; > > > @@ -724,45 +742,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; > > The old code assigns con2fb_map[unit] before is calls > > newinfo->fbops->fb_release). > > I wonder if there can be any callback to fbcon where the value > > of con2fb_map[unit] matters? > > It's all protected by console_lock, so other threads cannot see the > inconsistent state. > > Essentially everything in fbcon.c is protected by console_lock(). > > Do you want me to hammer this in somewhere (maybe in the commit message), > or good enough for your ack? No need to spell it out more. Add my a-b and apply it. Sam
On Tue, Feb 08, 2022 at 07:24:03PM +0100, Sam Ravnborg wrote: > Hi Daniel, > > On Tue, Feb 08, 2022 at 02:48:29PM +0100, Daniel Vetter wrote: > > On Thu, Feb 03, 2022 at 10:15:14PM +0100, Sam Ravnborg wrote: > > > Hi Daniel, > > > > > > On Mon, Jan 31, 2022 at 10:05:42PM +0100, Daniel Vetter wrote: > > > > 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. > > > > > > > > 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 fa30e1909164..eea2ee14b64c 100644 > > > > --- a/drivers/video/fbdev/core/fbcon.c > > > > +++ b/drivers/video/fbdev/core/fbcon.c > > > > @@ -680,19 +680,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); > > > > @@ -713,7 +731,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; > > > > @@ -724,45 +742,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; > > > The old code assigns con2fb_map[unit] before is calls > > > newinfo->fbops->fb_release). > > > I wonder if there can be any callback to fbcon where the value > > > of con2fb_map[unit] matters? > > > > It's all protected by console_lock, so other threads cannot see the > > inconsistent state. > > > > Essentially everything in fbcon.c is protected by console_lock(). > > > > Do you want me to hammer this in somewhere (maybe in the commit message), > > or good enough for your ack? > > No need to spell it out more. I think this was a very good question, since I had to spend a few minutes figuring out what exactly and why I've done it too. So I'll add this explainer, it really should have been in the commit message! > Add my a-b and apply it. Well I need to resend, since there was a minor change due to rebasing on top of Helge's fbcon scrolling patches instead of mine. Thanks for your review -Daniel > > Sam
diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c index fa30e1909164..eea2ee14b64c 100644 --- a/drivers/video/fbdev/core/fbcon.c +++ b/drivers/video/fbdev/core/fbcon.c @@ -680,19 +680,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); @@ -713,7 +731,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; @@ -724,45 +742,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, @@ -917,7 +924,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; @@ -936,17 +942,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; } @@ -3331,10 +3332,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; @@ -3344,6 +3341,8 @@ static void fbcon_exit(void) kfree(info->fbcon_par); info->fbcon_par = NULL; } + + fbcon_release(info); } } }
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. 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(-)