Re: [Intel-gfx] [PATCH 11/21] fbcon: Extract fbcon_open/release helpers

2022-02-08 Thread Daniel Vetter
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 
> > > > 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 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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch


Re: [PATCH 11/21] fbcon: Extract fbcon_open/release helpers

2022-02-08 Thread Sam Ravnborg
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 
> > > 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 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


Re: [PATCH 11/21] fbcon: Extract fbcon_open/release helpers

2022-02-08 Thread Daniel Vetter
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 
> > 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 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 

Re: [PATCH 11/21] fbcon: Extract fbcon_open/release helpers

2022-02-03 Thread Sam Ravnborg
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


Re: [PATCH 11/21] fbcon: Extract fbcon_open/release helpers

2022-02-03 Thread Sam Ravnborg
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 
> 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 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

> + /*
> +   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 && 

[PATCH 11/21] fbcon: Extract fbcon_open/release helpers

2022-01-31 Thread 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.

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 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