diff mbox series

[18/21] fbcon: untangle fbcon_exit

Message ID 20220131210552.482606-19-daniel.vetter@ffwll.ch (mailing list archive)
State Superseded
Headers show
Series some fbcon patches, mostly locking | expand

Commit Message

Daniel Vetter Jan. 31, 2022, 9:05 p.m. UTC
There's a bunch of confusions going on here:
- The deferred fbcon setup notifier should only be cleaned up from
  fb_console_exit(), to be symmetric with fb_console_init()
- We also need to make sure we don't race with the work, which means
  temporarily dropping the console lock (or we can deadlock)
- That also means no point in clearing deferred_takeover, we are
  unloading everything anyway.
- Finally rename fbcon_exit to fbcon_release_all and move it, since
  that's what's it doing when being called from consw->con_deinit
  through fbcon_deinit.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Claudio Suarez <cssk@net-c.es>
Cc: Du Cheng <ducheng2@gmail.com>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 drivers/video/fbdev/core/fbcon.c | 63 ++++++++++++++++----------------
 1 file changed, 32 insertions(+), 31 deletions(-)

Comments

Sam Ravnborg Feb. 4, 2022, 8:06 p.m. UTC | #1
Hi Daniel,

On Mon, Jan 31, 2022 at 10:05:49PM +0100, Daniel Vetter wrote:
> There's a bunch of confusions going on here:
> - The deferred fbcon setup notifier should only be cleaned up from
>   fb_console_exit(), to be symmetric with fb_console_init()
> - We also need to make sure we don't race with the work, which means
>   temporarily dropping the console lock (or we can deadlock)
> - That also means no point in clearing deferred_takeover, we are
>   unloading everything anyway.
> - Finally rename fbcon_exit to fbcon_release_all and move it, since
>   that's what's it doing when being called from consw->con_deinit
>   through fbcon_deinit.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Claudio Suarez <cssk@net-c.es>
> Cc: Du Cheng <ducheng2@gmail.com>
> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
>  drivers/video/fbdev/core/fbcon.c | 63 ++++++++++++++++----------------
>  1 file changed, 32 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> index 5c14e24d14a1..22581952b4fd 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -185,7 +185,6 @@ static void fbcon_set_disp(struct fb_info *info, struct fb_var_screeninfo *var,
>  			   int unit);
>  static void fbcon_modechanged(struct fb_info *info);
>  static void fbcon_set_all_vcs(struct fb_info *info);
> -static void fbcon_exit(void);
>  
>  static struct device *fbcon_device;
>  
> @@ -1149,6 +1148,27 @@ static void fbcon_free_font(struct fbcon_display *p, bool freefont)
>  
>  static void set_vc_hi_font(struct vc_data *vc, bool set);
>  
> +static void fbcon_release_all(void)
> +{
> +	struct fb_info *info;
> +	int i, j, mapped;
> +
> +	for_each_registered_fb(i) {
> +		mapped = 0;
> +		info = registered_fb[i];
> +
> +		for (j = first_fb_vc; j <= last_fb_vc; j++) {
> +			if (con2fb_map[j] == i) {
> +				mapped = 1;
> +				con2fb_map[j] = -1;
> +			}
> +		}
> +
> +		if (mapped)
> +			fbcon_release(info);
> +	}
> +}
> +
>  static void fbcon_deinit(struct vc_data *vc)
>  {
>  	struct fbcon_display *p = &fb_display[vc->vc_num];
> @@ -1188,7 +1208,7 @@ static void fbcon_deinit(struct vc_data *vc)
>  		set_vc_hi_font(vc, false);
>  
>  	if (!con_is_bound(&fb_con))
> -		fbcon_exit();
> +		fbcon_release_all();
>  
>  	if (vc->vc_num == logo_shown)
>  		logo_shown = FBCON_LOGO_CANSHOW;
> @@ -3316,34 +3336,6 @@ static void fbcon_start(void)
>  #endif
>  }
>  
> -static void fbcon_exit(void)
> -{
> -	struct fb_info *info;
> -	int i, j, mapped;
> -
> -#ifdef CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER
> -	if (deferred_takeover) {
> -		dummycon_unregister_output_notifier(&fbcon_output_nb);
> -		deferred_takeover = false;
> -	}
> -#endif
> -
> -	for_each_registered_fb(i) {
> -		mapped = 0;
> -		info = registered_fb[i];
> -
> -		for (j = first_fb_vc; j <= last_fb_vc; j++) {
> -			if (con2fb_map[j] == i) {
> -				mapped = 1;
> -				con2fb_map[j] = -1;
> -			}
> -		}
> -
> -		if (mapped)
> -			fbcon_release(info);
> -	}
> -}
> -
>  void __init fb_console_init(void)
>  {
>  	int i;
> @@ -3383,10 +3375,19 @@ static void __exit fbcon_deinit_device(void)
>  
>  void __exit fb_console_exit(void)
>  {
> +#ifdef CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER
> +	console_lock();
> +	if (deferred_takeover)
> +		dummycon_unregister_output_notifier(&fbcon_output_nb);
> +	console_unlock();
> +
> +	cancel_work_sync(&fbcon_deferred_takeover_work);
> +#endif
> +
>  	console_lock();
>  	fbcon_deinit_device();
>  	device_destroy(fb_class, MKDEV(0, 0));
> -	fbcon_exit();
> +
We loose the call to fbcon_release_all() here.
We have part of the old fbcon_exit() above, but miss the release parts.

Maybe I missed something obvious?

The rest looks fine.

	Sam

>  	do_unregister_con_driver(&fb_con);
>  	console_unlock();
>  }	
> -- 
> 2.33.0
Daniel Vetter Feb. 8, 2022, 1:58 p.m. UTC | #2
On Fri, Feb 04, 2022 at 09:06:38PM +0100, Sam Ravnborg wrote:
> Hi Daniel,
> 
> On Mon, Jan 31, 2022 at 10:05:49PM +0100, Daniel Vetter wrote:
> > There's a bunch of confusions going on here:
> > - The deferred fbcon setup notifier should only be cleaned up from
> >   fb_console_exit(), to be symmetric with fb_console_init()
> > - We also need to make sure we don't race with the work, which means
> >   temporarily dropping the console lock (or we can deadlock)
> > - That also means no point in clearing deferred_takeover, we are
> >   unloading everything anyway.
> > - Finally rename fbcon_exit to fbcon_release_all and move it, since
> >   that's what's it doing when being called from consw->con_deinit
> >   through fbcon_deinit.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Claudio Suarez <cssk@net-c.es>
> > Cc: Du Cheng <ducheng2@gmail.com>
> > Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> > ---
> >  drivers/video/fbdev/core/fbcon.c | 63 ++++++++++++++++----------------
> >  1 file changed, 32 insertions(+), 31 deletions(-)
> > 
> > diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> > index 5c14e24d14a1..22581952b4fd 100644
> > --- a/drivers/video/fbdev/core/fbcon.c
> > +++ b/drivers/video/fbdev/core/fbcon.c
> > @@ -185,7 +185,6 @@ static void fbcon_set_disp(struct fb_info *info, struct fb_var_screeninfo *var,
> >  			   int unit);
> >  static void fbcon_modechanged(struct fb_info *info);
> >  static void fbcon_set_all_vcs(struct fb_info *info);
> > -static void fbcon_exit(void);
> >  
> >  static struct device *fbcon_device;
> >  
> > @@ -1149,6 +1148,27 @@ static void fbcon_free_font(struct fbcon_display *p, bool freefont)
> >  
> >  static void set_vc_hi_font(struct vc_data *vc, bool set);
> >  
> > +static void fbcon_release_all(void)
> > +{
> > +	struct fb_info *info;
> > +	int i, j, mapped;
> > +
> > +	for_each_registered_fb(i) {
> > +		mapped = 0;
> > +		info = registered_fb[i];
> > +
> > +		for (j = first_fb_vc; j <= last_fb_vc; j++) {
> > +			if (con2fb_map[j] == i) {
> > +				mapped = 1;
> > +				con2fb_map[j] = -1;
> > +			}
> > +		}
> > +
> > +		if (mapped)
> > +			fbcon_release(info);
> > +	}
> > +}
> > +
> >  static void fbcon_deinit(struct vc_data *vc)
> >  {
> >  	struct fbcon_display *p = &fb_display[vc->vc_num];
> > @@ -1188,7 +1208,7 @@ static void fbcon_deinit(struct vc_data *vc)
> >  		set_vc_hi_font(vc, false);
> >  
> >  	if (!con_is_bound(&fb_con))
> > -		fbcon_exit();
> > +		fbcon_release_all();
> >  
> >  	if (vc->vc_num == logo_shown)
> >  		logo_shown = FBCON_LOGO_CANSHOW;
> > @@ -3316,34 +3336,6 @@ static void fbcon_start(void)
> >  #endif
> >  }
> >  
> > -static void fbcon_exit(void)
> > -{
> > -	struct fb_info *info;
> > -	int i, j, mapped;
> > -
> > -#ifdef CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER
> > -	if (deferred_takeover) {
> > -		dummycon_unregister_output_notifier(&fbcon_output_nb);
> > -		deferred_takeover = false;
> > -	}
> > -#endif
> > -
> > -	for_each_registered_fb(i) {
> > -		mapped = 0;
> > -		info = registered_fb[i];
> > -
> > -		for (j = first_fb_vc; j <= last_fb_vc; j++) {
> > -			if (con2fb_map[j] == i) {
> > -				mapped = 1;
> > -				con2fb_map[j] = -1;
> > -			}
> > -		}
> > -
> > -		if (mapped)
> > -			fbcon_release(info);
> > -	}
> > -}
> > -
> >  void __init fb_console_init(void)
> >  {
> >  	int i;
> > @@ -3383,10 +3375,19 @@ static void __exit fbcon_deinit_device(void)
> >  
> >  void __exit fb_console_exit(void)
> >  {
> > +#ifdef CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER
> > +	console_lock();
> > +	if (deferred_takeover)
> > +		dummycon_unregister_output_notifier(&fbcon_output_nb);
> > +	console_unlock();
> > +
> > +	cancel_work_sync(&fbcon_deferred_takeover_work);
> > +#endif
> > +
> >  	console_lock();
> >  	fbcon_deinit_device();
> >  	device_destroy(fb_class, MKDEV(0, 0));
> > -	fbcon_exit();
> > +
> We loose the call to fbcon_release_all() here.
> We have part of the old fbcon_exit() above, but miss the release parts.
> 
> Maybe I missed something obvious?

Ah yes that's the entire point of this change. The release_all in the
fbcon exit path was only needed when fbcon was a separate module
indepedent from core fb.ko. Which means it was possible to unload fbcon
while having fbdev drivers registered.

But since we've merged them that has become impossible, so by the time the
fb.ko module can be unloaded, there's guaranteed to be no fbdev drivers
left. And hence removing them is pointless.

Ack with that explainer added to the commit message?
-Daniel

> 
> The rest looks fine.
> 
> 	Sam
> 
> >  	do_unregister_con_driver(&fb_con);
> >  	console_unlock();
> >  }	
> > -- 
> > 2.33.0
Sam Ravnborg Feb. 8, 2022, 6:27 p.m. UTC | #3
On Tue, Feb 08, 2022 at 02:58:21PM +0100, Daniel Vetter wrote:
> On Fri, Feb 04, 2022 at 09:06:38PM +0100, Sam Ravnborg wrote:
> > Hi Daniel,
> > 
> > On Mon, Jan 31, 2022 at 10:05:49PM +0100, Daniel Vetter wrote:
> > > There's a bunch of confusions going on here:
> > > - The deferred fbcon setup notifier should only be cleaned up from
> > >   fb_console_exit(), to be symmetric with fb_console_init()
> > > - We also need to make sure we don't race with the work, which means
> > >   temporarily dropping the console lock (or we can deadlock)
> > > - That also means no point in clearing deferred_takeover, we are
> > >   unloading everything anyway.
> > > - Finally rename fbcon_exit to fbcon_release_all and move it, since
> > >   that's what's it doing when being called from consw->con_deinit
> > >   through fbcon_deinit.
> > > 
> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Cc: Claudio Suarez <cssk@net-c.es>
> > > Cc: Du Cheng <ducheng2@gmail.com>
> > > Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> > > ---
> > >  drivers/video/fbdev/core/fbcon.c | 63 ++++++++++++++++----------------
> > >  1 file changed, 32 insertions(+), 31 deletions(-)
> > > 
> > > diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> > > index 5c14e24d14a1..22581952b4fd 100644
> > > --- a/drivers/video/fbdev/core/fbcon.c
> > > +++ b/drivers/video/fbdev/core/fbcon.c
> > > @@ -185,7 +185,6 @@ static void fbcon_set_disp(struct fb_info *info, struct fb_var_screeninfo *var,
> > >  			   int unit);
> > >  static void fbcon_modechanged(struct fb_info *info);
> > >  static void fbcon_set_all_vcs(struct fb_info *info);
> > > -static void fbcon_exit(void);
> > >  
> > >  static struct device *fbcon_device;
> > >  
> > > @@ -1149,6 +1148,27 @@ static void fbcon_free_font(struct fbcon_display *p, bool freefont)
> > >  
> > >  static void set_vc_hi_font(struct vc_data *vc, bool set);
> > >  
> > > +static void fbcon_release_all(void)
> > > +{
> > > +	struct fb_info *info;
> > > +	int i, j, mapped;
> > > +
> > > +	for_each_registered_fb(i) {
> > > +		mapped = 0;
> > > +		info = registered_fb[i];
> > > +
> > > +		for (j = first_fb_vc; j <= last_fb_vc; j++) {
> > > +			if (con2fb_map[j] == i) {
> > > +				mapped = 1;
> > > +				con2fb_map[j] = -1;
> > > +			}
> > > +		}
> > > +
> > > +		if (mapped)
> > > +			fbcon_release(info);
> > > +	}
> > > +}
> > > +
> > >  static void fbcon_deinit(struct vc_data *vc)
> > >  {
> > >  	struct fbcon_display *p = &fb_display[vc->vc_num];
> > > @@ -1188,7 +1208,7 @@ static void fbcon_deinit(struct vc_data *vc)
> > >  		set_vc_hi_font(vc, false);
> > >  
> > >  	if (!con_is_bound(&fb_con))
> > > -		fbcon_exit();
> > > +		fbcon_release_all();
> > >  
> > >  	if (vc->vc_num == logo_shown)
> > >  		logo_shown = FBCON_LOGO_CANSHOW;
> > > @@ -3316,34 +3336,6 @@ static void fbcon_start(void)
> > >  #endif
> > >  }
> > >  
> > > -static void fbcon_exit(void)
> > > -{
> > > -	struct fb_info *info;
> > > -	int i, j, mapped;
> > > -
> > > -#ifdef CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER
> > > -	if (deferred_takeover) {
> > > -		dummycon_unregister_output_notifier(&fbcon_output_nb);
> > > -		deferred_takeover = false;
> > > -	}
> > > -#endif
> > > -
> > > -	for_each_registered_fb(i) {
> > > -		mapped = 0;
> > > -		info = registered_fb[i];
> > > -
> > > -		for (j = first_fb_vc; j <= last_fb_vc; j++) {
> > > -			if (con2fb_map[j] == i) {
> > > -				mapped = 1;
> > > -				con2fb_map[j] = -1;
> > > -			}
> > > -		}
> > > -
> > > -		if (mapped)
> > > -			fbcon_release(info);
> > > -	}
> > > -}
> > > -
> > >  void __init fb_console_init(void)
> > >  {
> > >  	int i;
> > > @@ -3383,10 +3375,19 @@ static void __exit fbcon_deinit_device(void)
> > >  
> > >  void __exit fb_console_exit(void)
> > >  {
> > > +#ifdef CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER
> > > +	console_lock();
> > > +	if (deferred_takeover)
> > > +		dummycon_unregister_output_notifier(&fbcon_output_nb);
> > > +	console_unlock();
> > > +
> > > +	cancel_work_sync(&fbcon_deferred_takeover_work);
> > > +#endif
> > > +
> > >  	console_lock();
> > >  	fbcon_deinit_device();
> > >  	device_destroy(fb_class, MKDEV(0, 0));
> > > -	fbcon_exit();
> > > +
> > We loose the call to fbcon_release_all() here.
> > We have part of the old fbcon_exit() above, but miss the release parts.
> > 
> > Maybe I missed something obvious?
> 
> Ah yes that's the entire point of this change. The release_all in the
> fbcon exit path was only needed when fbcon was a separate module
> indepedent from core fb.ko. Which means it was possible to unload fbcon
> while having fbdev drivers registered.
> 
> But since we've merged them that has become impossible, so by the time the
> fb.ko module can be unloaded, there's guaranteed to be no fbdev drivers
> left. And hence removing them is pointless.
Makes sense, thanks for the explanation.

> 
> Ack with that explainer added to the commit message?
Yes, then it is
Acked-by: Sam Ravnborg <sam@ravnborg.org>
diff mbox series

Patch

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index 5c14e24d14a1..22581952b4fd 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -185,7 +185,6 @@  static void fbcon_set_disp(struct fb_info *info, struct fb_var_screeninfo *var,
 			   int unit);
 static void fbcon_modechanged(struct fb_info *info);
 static void fbcon_set_all_vcs(struct fb_info *info);
-static void fbcon_exit(void);
 
 static struct device *fbcon_device;
 
@@ -1149,6 +1148,27 @@  static void fbcon_free_font(struct fbcon_display *p, bool freefont)
 
 static void set_vc_hi_font(struct vc_data *vc, bool set);
 
+static void fbcon_release_all(void)
+{
+	struct fb_info *info;
+	int i, j, mapped;
+
+	for_each_registered_fb(i) {
+		mapped = 0;
+		info = registered_fb[i];
+
+		for (j = first_fb_vc; j <= last_fb_vc; j++) {
+			if (con2fb_map[j] == i) {
+				mapped = 1;
+				con2fb_map[j] = -1;
+			}
+		}
+
+		if (mapped)
+			fbcon_release(info);
+	}
+}
+
 static void fbcon_deinit(struct vc_data *vc)
 {
 	struct fbcon_display *p = &fb_display[vc->vc_num];
@@ -1188,7 +1208,7 @@  static void fbcon_deinit(struct vc_data *vc)
 		set_vc_hi_font(vc, false);
 
 	if (!con_is_bound(&fb_con))
-		fbcon_exit();
+		fbcon_release_all();
 
 	if (vc->vc_num == logo_shown)
 		logo_shown = FBCON_LOGO_CANSHOW;
@@ -3316,34 +3336,6 @@  static void fbcon_start(void)
 #endif
 }
 
-static void fbcon_exit(void)
-{
-	struct fb_info *info;
-	int i, j, mapped;
-
-#ifdef CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER
-	if (deferred_takeover) {
-		dummycon_unregister_output_notifier(&fbcon_output_nb);
-		deferred_takeover = false;
-	}
-#endif
-
-	for_each_registered_fb(i) {
-		mapped = 0;
-		info = registered_fb[i];
-
-		for (j = first_fb_vc; j <= last_fb_vc; j++) {
-			if (con2fb_map[j] == i) {
-				mapped = 1;
-				con2fb_map[j] = -1;
-			}
-		}
-
-		if (mapped)
-			fbcon_release(info);
-	}
-}
-
 void __init fb_console_init(void)
 {
 	int i;
@@ -3383,10 +3375,19 @@  static void __exit fbcon_deinit_device(void)
 
 void __exit fb_console_exit(void)
 {
+#ifdef CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER
+	console_lock();
+	if (deferred_takeover)
+		dummycon_unregister_output_notifier(&fbcon_output_nb);
+	console_unlock();
+
+	cancel_work_sync(&fbcon_deferred_takeover_work);
+#endif
+
 	console_lock();
 	fbcon_deinit_device();
 	device_destroy(fb_class, MKDEV(0, 0));
-	fbcon_exit();
+
 	do_unregister_con_driver(&fb_con);
 	console_unlock();
 }