[09/33] fbcon: Remove fbcon_has_exited
diff mbox series

Message ID 20190524085354.27411-10-daniel.vetter@ffwll.ch
State New
Headers show
Series
  • fbcon notifier begone!
Related show

Commit Message

Daniel Vetter May 24, 2019, 8:53 a.m. UTC
This is unused code since

commit 6104c37094e729f3d4ce65797002112735d49cd1
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Tue Aug 1 17:32:07 2017 +0200

    fbcon: Make fbcon a built-time depency for fbdev

when fbcon was made a compile-time static dependency of fbdev. We
can't exit fbcon anymore without exiting fbdev first, which only works
if all fbdev drivers have unloaded already. Hence this is all dead
code.

v2: I missed that fbcon_exit is also called from con_deinit stuff, and
there fbcon_has_exited prevents double-cleanup. But we can fix that
by properly resetting con2fb_map[] to all -1, which is used everywhere
else to indicate "no fb_info allocate to this console". With that
change the double-cleanup (which resulted in a module refcount underflow,
among other things) is prevented.

Aside: con2fb_map is a signed char, so don't register more than 128 fb_info
or hilarity will ensue.

v3: CI showed me that I still didn't fully understand what's going on
here. The leaked references in con2fb_map have been used upon
rebinding the fb console in fbcon_init. It worked because fbdev
unregistering still cleaned out con2fb_map, and reset it to info_idx.
If the last fbdev driver unregistered, then it also reset info_idx,
and unregistered the fbcon driver.

Imo that's all a bit fragile, so let's keep the con2fb_map reset to
-1, and in fbcon_init pick info_idx if we're starting fresh. That
means unbinding and rebinding will cleanse the mapping, but why are
you doing that if you want to retain the mapping, so should be fine.

Also, I think info_idx == -1 is impossible in fbcon_init - we
unregister the fbcon in that case. So catch&warn about that.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: "Noralf Trønnes" <noralf@tronnes.org>
Cc: Yisheng Xie <ysxie@foxmail.com>
Cc: Konstantin Khorenko <khorenko@virtuozzo.com>
Cc: Prarit Bhargava <prarit@redhat.com>
Cc: Kees Cook <keescook@chromium.org>
---
 drivers/video/fbdev/core/fbcon.c | 39 +++++---------------------------
 1 file changed, 6 insertions(+), 33 deletions(-)

Comments

Sam Ravnborg May 25, 2019, 3:38 p.m. UTC | #1
Hi Daniel.

One detail I noticed while brosing the changes.

>  
> @@ -1064,9 +1062,13 @@ static void fbcon_init(struct vc_data *vc, int init)
>  	int logo = 1, new_rows, new_cols, rows, cols, charcnt = 256;
>  	int cap, ret;
>  
> -	if (info_idx == -1 || info == NULL)
> +	if (WARN_ON(info_idx == -1))
>  	    return;
>  
> +	if (con2fb_map[vc->vc_num] == -1)
> +		con2fb_map[vc->vc_num] = info_idx;
> +
> +	info = registered_fb[con2fb_map[vc->vc_num]];
>  	cap = info->flags;

When info is defined it is also assigned:
struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];

As the test for info is gone this assignment is no longer
requrired and can be deleted.

The code now assumes that there is always an fb_info if con2fb_map[]
is not set to -1. I could not determine if this is OK, but this
likely boils down to your locking concern of registered_fb.

	Sam

>  
>  	if (logo_shown < 0 && console_loglevel <= CONSOLE_LOGLEVEL_QUIET)
> @@ -3336,14 +3338,6 @@ static int fbcon_event_notify(struct notifier_block *self,
>  	struct fb_blit_caps *caps;
>  	int idx, ret = 0;
>  
> -	/*
> -	 * ignore all events except driver registration and deregistration
> -	 * if fbcon is not active
> -	 */
> -	if (fbcon_has_exited && !(action == FB_EVENT_FB_REGISTERED ||
> -				  action == FB_EVENT_FB_UNREGISTERED))
> -		goto done;
> -
>  	switch(action) {
>  	case FB_EVENT_SUSPEND:
>  		fbcon_suspended(info);
> @@ -3396,7 +3390,6 @@ static int fbcon_event_notify(struct notifier_block *self,
>  		fbcon_remap_all(idx);
>  		break;
>  	}
> -done:
>  	return ret;
>  }
>  
> @@ -3443,9 +3436,6 @@ static ssize_t store_rotate(struct device *device,
>  	int rotate, idx;
>  	char **last = NULL;
>  
> -	if (fbcon_has_exited)
> -		return count;
> -
>  	console_lock();
>  	idx = con2fb_map[fg_console];
>  
> @@ -3468,9 +3458,6 @@ static ssize_t store_rotate_all(struct device *device,
>  	int rotate, idx;
>  	char **last = NULL;
>  
> -	if (fbcon_has_exited)
> -		return count;
> -
>  	console_lock();
>  	idx = con2fb_map[fg_console];
>  
> @@ -3491,9 +3478,6 @@ static ssize_t show_rotate(struct device *device,
>  	struct fb_info *info;
>  	int rotate = 0, idx;
>  
> -	if (fbcon_has_exited)
> -		return 0;
> -
>  	console_lock();
>  	idx = con2fb_map[fg_console];
>  
> @@ -3514,9 +3498,6 @@ static ssize_t show_cursor_blink(struct device *device,
>  	struct fbcon_ops *ops;
>  	int idx, blink = -1;
>  
> -	if (fbcon_has_exited)
> -		return 0;
> -
>  	console_lock();
>  	idx = con2fb_map[fg_console];
>  
> @@ -3543,9 +3524,6 @@ static ssize_t store_cursor_blink(struct device *device,
>  	int blink, idx;
>  	char **last = NULL;
>  
> -	if (fbcon_has_exited)
> -		return count;
> -
>  	console_lock();
>  	idx = con2fb_map[fg_console];
>  
> @@ -3668,9 +3646,6 @@ static void fbcon_exit(void)
>  	struct fb_info *info;
>  	int i, j, mapped;
>  
> -	if (fbcon_has_exited)
> -		return;
> -
>  #ifdef CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER
>  	if (deferred_takeover) {
>  		dummycon_unregister_output_notifier(&fbcon_output_nb);
> @@ -3695,7 +3670,7 @@ static void fbcon_exit(void)
>  		for (j = first_fb_vc; j <= last_fb_vc; j++) {
>  			if (con2fb_map[j] == i) {
>  				mapped = 1;
> -				break;
> +				con2fb_map[j] = -1;
>  			}
>  		}
>  
> @@ -3718,8 +3693,6 @@ static void fbcon_exit(void)
>  				info->queue.func = NULL;
>  		}
>  	}
> -
> -	fbcon_has_exited = 1;
>  }
>  
>  void __init fb_console_init(void)
> -- 
> 2.20.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Daniel Vetter May 27, 2019, 6:10 a.m. UTC | #2
On Sat, May 25, 2019 at 05:38:26PM +0200, Sam Ravnborg wrote:
> Hi Daniel.
> 
> One detail I noticed while brosing the changes.
> 
> >  
> > @@ -1064,9 +1062,13 @@ static void fbcon_init(struct vc_data *vc, int init)
> >  	int logo = 1, new_rows, new_cols, rows, cols, charcnt = 256;
> >  	int cap, ret;
> >  
> > -	if (info_idx == -1 || info == NULL)
> > +	if (WARN_ON(info_idx == -1))
> >  	    return;
> >  
> > +	if (con2fb_map[vc->vc_num] == -1)
> > +		con2fb_map[vc->vc_num] = info_idx;
> > +
> > +	info = registered_fb[con2fb_map[vc->vc_num]];
> >  	cap = info->flags;
> 
> When info is defined it is also assigned:
> struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
> 
> As the test for info is gone this assignment is no longer
> requrired and can be deleted.

We use it later on, so we definitely still need info. But I indeed forgot
to delete the initial assignment of info at the function start. Is that
what you mean here?
>
> The code now assumes that there is always an fb_info if con2fb_map[]
> is not set to -1. I could not determine if this is OK, but this
> likely boils down to your locking concern of registered_fb.

Yup, see how fb_registered/unregistered manage this. I think that part
actually all works out correctly (as long as everyone is holding
console_lock). But it's a bit unpretty that fbcon digs around in the raw
registered_fb array instead of going through the refcounted helpers, and
having a full refcounted pointer. Much easier to convince yourself of that
than chasing indices assigments all over imo.
-Daniel

> 
> 	Sam
> 
> >  
> >  	if (logo_shown < 0 && console_loglevel <= CONSOLE_LOGLEVEL_QUIET)
> > @@ -3336,14 +3338,6 @@ static int fbcon_event_notify(struct notifier_block *self,
> >  	struct fb_blit_caps *caps;
> >  	int idx, ret = 0;
> >  
> > -	/*
> > -	 * ignore all events except driver registration and deregistration
> > -	 * if fbcon is not active
> > -	 */
> > -	if (fbcon_has_exited && !(action == FB_EVENT_FB_REGISTERED ||
> > -				  action == FB_EVENT_FB_UNREGISTERED))
> > -		goto done;
> > -
> >  	switch(action) {
> >  	case FB_EVENT_SUSPEND:
> >  		fbcon_suspended(info);
> > @@ -3396,7 +3390,6 @@ static int fbcon_event_notify(struct notifier_block *self,
> >  		fbcon_remap_all(idx);
> >  		break;
> >  	}
> > -done:
> >  	return ret;
> >  }
> >  
> > @@ -3443,9 +3436,6 @@ static ssize_t store_rotate(struct device *device,
> >  	int rotate, idx;
> >  	char **last = NULL;
> >  
> > -	if (fbcon_has_exited)
> > -		return count;
> > -
> >  	console_lock();
> >  	idx = con2fb_map[fg_console];
> >  
> > @@ -3468,9 +3458,6 @@ static ssize_t store_rotate_all(struct device *device,
> >  	int rotate, idx;
> >  	char **last = NULL;
> >  
> > -	if (fbcon_has_exited)
> > -		return count;
> > -
> >  	console_lock();
> >  	idx = con2fb_map[fg_console];
> >  
> > @@ -3491,9 +3478,6 @@ static ssize_t show_rotate(struct device *device,
> >  	struct fb_info *info;
> >  	int rotate = 0, idx;
> >  
> > -	if (fbcon_has_exited)
> > -		return 0;
> > -
> >  	console_lock();
> >  	idx = con2fb_map[fg_console];
> >  
> > @@ -3514,9 +3498,6 @@ static ssize_t show_cursor_blink(struct device *device,
> >  	struct fbcon_ops *ops;
> >  	int idx, blink = -1;
> >  
> > -	if (fbcon_has_exited)
> > -		return 0;
> > -
> >  	console_lock();
> >  	idx = con2fb_map[fg_console];
> >  
> > @@ -3543,9 +3524,6 @@ static ssize_t store_cursor_blink(struct device *device,
> >  	int blink, idx;
> >  	char **last = NULL;
> >  
> > -	if (fbcon_has_exited)
> > -		return count;
> > -
> >  	console_lock();
> >  	idx = con2fb_map[fg_console];
> >  
> > @@ -3668,9 +3646,6 @@ static void fbcon_exit(void)
> >  	struct fb_info *info;
> >  	int i, j, mapped;
> >  
> > -	if (fbcon_has_exited)
> > -		return;
> > -
> >  #ifdef CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER
> >  	if (deferred_takeover) {
> >  		dummycon_unregister_output_notifier(&fbcon_output_nb);
> > @@ -3695,7 +3670,7 @@ static void fbcon_exit(void)
> >  		for (j = first_fb_vc; j <= last_fb_vc; j++) {
> >  			if (con2fb_map[j] == i) {
> >  				mapped = 1;
> > -				break;
> > +				con2fb_map[j] = -1;
> >  			}
> >  		}
> >  
> > @@ -3718,8 +3693,6 @@ static void fbcon_exit(void)
> >  				info->queue.func = NULL;
> >  		}
> >  	}
> > -
> > -	fbcon_has_exited = 1;
> >  }
> >  
> >  void __init fb_console_init(void)
> > -- 
> > 2.20.1
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Sam Ravnborg May 27, 2019, 6:51 a.m. UTC | #3
Hi Daniel.

> But I indeed forgot
> to delete the initial assignment of info at the function start. Is that
> what you mean here?

Yes.

	Sam

Patch
diff mbox series

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index 5424051c8e1a..622d336cfc81 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -112,7 +112,6 @@  static int softback_lines;
 static int first_fb_vc;
 static int last_fb_vc = MAX_NR_CONSOLES - 1;
 static int fbcon_is_default = 1; 
-static int fbcon_has_exited;
 static int primary_device = -1;
 static int fbcon_has_console_bind;
 
@@ -1050,7 +1049,6 @@  static const char *fbcon_startup(void)
 		info->var.bits_per_pixel);
 
 	fbcon_add_cursor_timer(info);
-	fbcon_has_exited = 0;
 	return display_desc;
 }
 
@@ -1064,9 +1062,13 @@  static void fbcon_init(struct vc_data *vc, int init)
 	int logo = 1, new_rows, new_cols, rows, cols, charcnt = 256;
 	int cap, ret;
 
-	if (info_idx == -1 || info == NULL)
+	if (WARN_ON(info_idx == -1))
 	    return;
 
+	if (con2fb_map[vc->vc_num] == -1)
+		con2fb_map[vc->vc_num] = info_idx;
+
+	info = registered_fb[con2fb_map[vc->vc_num]];
 	cap = info->flags;
 
 	if (logo_shown < 0 && console_loglevel <= CONSOLE_LOGLEVEL_QUIET)
@@ -3336,14 +3338,6 @@  static int fbcon_event_notify(struct notifier_block *self,
 	struct fb_blit_caps *caps;
 	int idx, ret = 0;
 
-	/*
-	 * ignore all events except driver registration and deregistration
-	 * if fbcon is not active
-	 */
-	if (fbcon_has_exited && !(action == FB_EVENT_FB_REGISTERED ||
-				  action == FB_EVENT_FB_UNREGISTERED))
-		goto done;
-
 	switch(action) {
 	case FB_EVENT_SUSPEND:
 		fbcon_suspended(info);
@@ -3396,7 +3390,6 @@  static int fbcon_event_notify(struct notifier_block *self,
 		fbcon_remap_all(idx);
 		break;
 	}
-done:
 	return ret;
 }
 
@@ -3443,9 +3436,6 @@  static ssize_t store_rotate(struct device *device,
 	int rotate, idx;
 	char **last = NULL;
 
-	if (fbcon_has_exited)
-		return count;
-
 	console_lock();
 	idx = con2fb_map[fg_console];
 
@@ -3468,9 +3458,6 @@  static ssize_t store_rotate_all(struct device *device,
 	int rotate, idx;
 	char **last = NULL;
 
-	if (fbcon_has_exited)
-		return count;
-
 	console_lock();
 	idx = con2fb_map[fg_console];
 
@@ -3491,9 +3478,6 @@  static ssize_t show_rotate(struct device *device,
 	struct fb_info *info;
 	int rotate = 0, idx;
 
-	if (fbcon_has_exited)
-		return 0;
-
 	console_lock();
 	idx = con2fb_map[fg_console];
 
@@ -3514,9 +3498,6 @@  static ssize_t show_cursor_blink(struct device *device,
 	struct fbcon_ops *ops;
 	int idx, blink = -1;
 
-	if (fbcon_has_exited)
-		return 0;
-
 	console_lock();
 	idx = con2fb_map[fg_console];
 
@@ -3543,9 +3524,6 @@  static ssize_t store_cursor_blink(struct device *device,
 	int blink, idx;
 	char **last = NULL;
 
-	if (fbcon_has_exited)
-		return count;
-
 	console_lock();
 	idx = con2fb_map[fg_console];
 
@@ -3668,9 +3646,6 @@  static void fbcon_exit(void)
 	struct fb_info *info;
 	int i, j, mapped;
 
-	if (fbcon_has_exited)
-		return;
-
 #ifdef CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER
 	if (deferred_takeover) {
 		dummycon_unregister_output_notifier(&fbcon_output_nb);
@@ -3695,7 +3670,7 @@  static void fbcon_exit(void)
 		for (j = first_fb_vc; j <= last_fb_vc; j++) {
 			if (con2fb_map[j] == i) {
 				mapped = 1;
-				break;
+				con2fb_map[j] = -1;
 			}
 		}
 
@@ -3718,8 +3693,6 @@  static void fbcon_exit(void)
 				info->queue.func = NULL;
 		}
 	}
-
-	fbcon_has_exited = 1;
 }
 
 void __init fb_console_init(void)