| Message ID | 20220131210552.482606-19-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | some fbcon patches, mostly locking | expand |
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
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
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 --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(); }
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(-)