diff mbox

tty, vt: lockdep warnings (Patch v3)

Message ID 20121108143408.77033416@pyramind.ukuu.org.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Alan Cox Nov. 8, 2012, 2:34 p.m. UTC 
commit f35b3fbf24c4e4debb6a7a864b09854ccc2a22e7
Author: Alan Cox <alan@linux.intel.com>
Date:   Wed Nov 7 16:35:08 2012 +0000

    fb: Rework locking to fix lock ordering on takeover
    
    Adjust the console layer to allow a take over call where the caller already
    holds the locks. Make the fb layer lock in order.
    
    This s partly a band aid, the fb layer is terminally confused about the
    locking rules it uses for its notifiers it seems.
    
    Signed-off-by: Alan Cox <alan@linux.intel.com>

--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Hugh Dickins Nov. 9, 2012, 7:34 p.m. UTC | #1 
On Thu, 8 Nov 2012, Alan Cox wrote:
> commit f35b3fbf24c4e4debb6a7a864b09854ccc2a22e7
> Author: Alan Cox <alan@linux.intel.com>
> Date:   Wed Nov 7 16:35:08 2012 +0000
> 
>     fb: Rework locking to fix lock ordering on takeover
>     
>     Adjust the console layer to allow a take over call where the caller already
>     holds the locks. Make the fb layer lock in order.
>     
>     This s partly a band aid, the fb layer is terminally confused about the
>     locking rules it uses for its notifiers it seems.
>     
>     Signed-off-by: Alan Cox <alan@linux.intel.com>

This version works for me too - thanks.
Hugh

> 
> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> index f87d7e8..77bf205 100644
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> @@ -2984,7 +2984,7 @@ int __init vty_init(const struct file_operations *console_fops)
>  
>  static struct class *vtconsole_class;
>  
> -static int bind_con_driver(const struct consw *csw, int first, int last,
> +static int do_bind_con_driver(const struct consw *csw, int first, int last,
>  			   int deflt)
>  {
>  	struct module *owner = csw->owner;
> @@ -2995,7 +2995,7 @@ static int bind_con_driver(const struct consw *csw, int first, int last,
>  	if (!try_module_get(owner))
>  		return -ENODEV;
>  
> -	console_lock();
> +	WARN_CONSOLE_UNLOCKED();
>  
>  	/* check if driver is registered */
>  	for (i = 0; i < MAX_NR_CON_DRIVER; i++) {
> @@ -3080,11 +3080,22 @@ static int bind_con_driver(const struct consw *csw, int first, int last,
>  
>  	retval = 0;
>  err:
> -	console_unlock();
>  	module_put(owner);
>  	return retval;
>  };
>  
> +
> +static int bind_con_driver(const struct consw *csw, int first, int last,
> +			   int deflt)
> +{
> +	int ret;
> +	
> +	console_lock();
> +	ret = do_bind_con_driver(csw, first, last, deflt);
> +	console_unlock();
> +	return ret;
> +}
> +	
>  #ifdef CONFIG_VT_HW_CONSOLE_BINDING
>  static int con_is_graphics(const struct consw *csw, int first, int last)
>  {
> @@ -3196,9 +3207,9 @@ int unbind_con_driver(const struct consw *csw, int first, int last, int deflt)
>  	if (!con_is_bound(csw))
>  		con_driver->flag &= ~CON_DRIVER_FLAG_INIT;
>  
> -	console_unlock();
>  	/* ignore return value, binding should not fail */
> -	bind_con_driver(defcsw, first, last, deflt);
> +	do_bind_con_driver(defcsw, first, last, deflt);
> +	console_unlock();
>  err:
>  	module_put(owner);
>  	return retval;
> @@ -3489,28 +3500,18 @@ int con_debug_leave(void)
>  }
>  EXPORT_SYMBOL_GPL(con_debug_leave);
>  
> -/**
> - * register_con_driver - register console driver to console layer
> - * @csw: console driver
> - * @first: the first console to take over, minimum value is 0
> - * @last: the last console to take over, maximum value is MAX_NR_CONSOLES -1
> - *
> - * DESCRIPTION: This function registers a console driver which can later
> - * bind to a range of consoles specified by @first and @last. It will
> - * also initialize the console driver by calling con_startup().
> - */
> -int register_con_driver(const struct consw *csw, int first, int last)
> +static int do_register_con_driver(const struct consw *csw, int first, int last)
>  {
>  	struct module *owner = csw->owner;
>  	struct con_driver *con_driver;
>  	const char *desc;
>  	int i, retval = 0;
>  
> +	WARN_CONSOLE_UNLOCKED();
> +
>  	if (!try_module_get(owner))
>  		return -ENODEV;
>  
> -	console_lock();
> -
>  	for (i = 0; i < MAX_NR_CON_DRIVER; i++) {
>  		con_driver = &registered_con_driver[i];
>  
> @@ -3563,10 +3564,29 @@ int register_con_driver(const struct consw *csw, int first, int last)
>  	}
>  
>  err:
> -	console_unlock();
>  	module_put(owner);
>  	return retval;
>  }
> +
> +/**
> + * register_con_driver - register console driver to console layer
> + * @csw: console driver
> + * @first: the first console to take over, minimum value is 0
> + * @last: the last console to take over, maximum value is MAX_NR_CONSOLES -1
> + *
> + * DESCRIPTION: This function registers a console driver which can later
> + * bind to a range of consoles specified by @first and @last. It will
> + * also initialize the console driver by calling con_startup().
> + */
> +int register_con_driver(const struct consw *csw, int first, int last)
> +{
> +	int retval;
> +	
> +	console_lock();
> +	retval = do_register_con_driver(csw, first, last);
> +	console_unlock();
> +	return retval;
> +}
>  EXPORT_SYMBOL(register_con_driver);
>  
>  /**
> @@ -3622,6 +3642,29 @@ EXPORT_SYMBOL(unregister_con_driver);
>   *
>   *      take_over_console is basically a register followed by unbind
>   */
> +int do_take_over_console(const struct consw *csw, int first, int last, int deflt)
> +{
> +	int err;
> +
> +	err = do_register_con_driver(csw, first, last);
> +	/* if we get an busy error we still want to bind the console driver
> +	 * and return success, as we may have unbound the console driver
> +	 * but not unregistered it.
> +	*/
> +	if (err == -EBUSY)
> +		err = 0;
> +	if (!err)
> +		do_bind_con_driver(csw, first, last, deflt);
> +
> +	return err;
> +}
> +/*
> + *	If we support more console drivers, this function is used
> + *	when a driver wants to take over some existing consoles
> + *	and become default driver for newly opened ones.
> + *
> + *      take_over_console is basically a register followed by unbind
> + */
>  int take_over_console(const struct consw *csw, int first, int last, int deflt)
>  {
>  	int err;
> diff --git a/drivers/video/console/fbcon.c b/drivers/video/console/fbcon.c
> index fdefa8f..c75f8ce 100644
> --- a/drivers/video/console/fbcon.c
> +++ b/drivers/video/console/fbcon.c
> @@ -529,6 +529,34 @@ static int search_for_mapped_con(void)
>  	return retval;
>  }
>  
> +static int do_fbcon_takeover(int show_logo)
> +{
> +	int err, i;
> +
> +	if (!num_registered_fb)
> +		return -ENODEV;
> +
> +	if (!show_logo)
> +		logo_shown = FBCON_LOGO_DONTSHOW;
> +
> +	for (i = first_fb_vc; i <= last_fb_vc; i++)
> +		con2fb_map[i] = info_idx;
> +
> +	err = do_take_over_console(&fb_con, first_fb_vc, last_fb_vc,
> +				fbcon_is_default);
> +
> +	if (err) {
> +		for (i = first_fb_vc; i <= last_fb_vc; i++) {
> +			con2fb_map[i] = -1;
> +		}
> +		info_idx = -1;
> +	} else {
> +		fbcon_has_console_bind = 1;
> +	}
> +
> +	return err;
> +}
> +
>  static int fbcon_takeover(int show_logo)
>  {
>  	int err, i;
> @@ -3115,7 +3143,7 @@ static int fbcon_fb_registered(struct fb_info *info)
>  		}
>  
>  		if (info_idx != -1)
> -			ret = fbcon_takeover(1);
> +			ret = do_fbcon_takeover(1);
>  	} else {
>  		for (i = first_fb_vc; i <= last_fb_vc; i++) {
>  			if (con2fb_map_boot[i] == idx)
> diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
> index 3ff0105..564ebe9 100644
> --- a/drivers/video/fbmem.c
> +++ b/drivers/video/fbmem.c
> @@ -1650,7 +1650,9 @@ static int do_register_framebuffer(struct fb_info *fb_info)
>  	event.info = fb_info;
>  	if (!lock_fb_info(fb_info))
>  		return -ENODEV;
> +        console_lock();
>  	fb_notifier_call_chain(FB_EVENT_FB_REGISTERED, &event);
> +        console_unlock();
>  	unlock_fb_info(fb_info);
>  	return 0;
>  }
> @@ -1853,11 +1855,8 @@ int fb_new_modelist(struct fb_info *info)
>  	err = 1;
>  
>  	if (!list_empty(&info->modelist)) {
> -		if (!lock_fb_info(info))
> -			return -ENODEV;
>  		event.info = info;
>  		err = fb_notifier_call_chain(FB_EVENT_NEW_MODELIST, &event);
> -		unlock_fb_info(info);
>  	}
>  
>  	return err;
> diff --git a/drivers/video/fbsysfs.c b/drivers/video/fbsysfs.c
> index a55e366..ef476b0 100644
> --- a/drivers/video/fbsysfs.c
> +++ b/drivers/video/fbsysfs.c
> @@ -177,6 +177,8 @@ static ssize_t store_modes(struct device *device,
>  	if (i * sizeof(struct fb_videomode) != count)
>  		return -EINVAL;
>  
> +	if (!lock_fb_info(fb_info))
> +		return -ENODEV;
>  	console_lock();
>  	list_splice(&fb_info->modelist, &old_list);
>  	fb_videomode_to_modelist((const struct fb_videomode *)buf, i,
> @@ -188,6 +190,7 @@ static ssize_t store_modes(struct device *device,
>  		fb_destroy_modelist(&old_list);
>  
>  	console_unlock();
> +	unlock_fb_info(fb_info);
>  
>  	return 0;
>  }
> diff --git a/include/linux/console.h b/include/linux/console.h
> index dedb082..4ef4307 100644
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -78,6 +78,7 @@ int con_is_bound(const struct consw *csw);
>  int register_con_driver(const struct consw *csw, int first, int last);
>  int unregister_con_driver(const struct consw *csw);
>  int take_over_console(const struct consw *sw, int first, int last, int deflt);
> +int do_take_over_console(const struct consw *sw, int first, int last, int deflt);
>  void give_up_console(const struct consw *sw);
>  #ifdef CONFIG_HW_CONSOLE
>  int con_debug_enter(struct vc_data *vc);
>
Sasha Levin Nov. 9, 2012, 7:42 p.m. UTC | #2 
On 11/09/2012 02:34 PM, Hugh Dickins wrote:
> On Thu, 8 Nov 2012, Alan Cox wrote:
>> commit f35b3fbf24c4e4debb6a7a864b09854ccc2a22e7
>> Author: Alan Cox <alan@linux.intel.com>
>> Date:   Wed Nov 7 16:35:08 2012 +0000
>>
>>     fb: Rework locking to fix lock ordering on takeover
>>     
>>     Adjust the console layer to allow a take over call where the caller already
>>     holds the locks. Make the fb layer lock in order.
>>     
>>     This s partly a band aid, the fb layer is terminally confused about the
>>     locking rules it uses for its notifiers it seems.
>>     
>>     Signed-off-by: Alan Cox <alan@linux.intel.com>
> 
> This version works for me too - thanks.
> Hugh

I was planning to test it last night, but new code in mm/ failed horribly and
was BUG()ing all over the place, so I didn't get any significant testing
of this patch done.

Will update...


Thanks,
Sasha

--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sasha Levin Nov. 13, 2012, 4:24 p.m. UTC | #3 
On Fri, Nov 9, 2012 at 2:42 PM, Sasha Levin <sasha.levin@oracle.com> wrote:
> On 11/09/2012 02:34 PM, Hugh Dickins wrote:
>> On Thu, 8 Nov 2012, Alan Cox wrote:
>>> commit f35b3fbf24c4e4debb6a7a864b09854ccc2a22e7
>>> Author: Alan Cox <alan@linux.intel.com>
>>> Date:   Wed Nov 7 16:35:08 2012 +0000
>>>
>>>     fb: Rework locking to fix lock ordering on takeover
>>>
>>>     Adjust the console layer to allow a take over call where the caller already
>>>     holds the locks. Make the fb layer lock in order.
>>>
>>>     This s partly a band aid, the fb layer is terminally confused about the
>>>     locking rules it uses for its notifiers it seems.
>>>
>>>     Signed-off-by: Alan Cox <alan@linux.intel.com>
>>
>> This version works for me too - thanks.
>> Hugh
>
> I was planning to test it last night, but new code in mm/ failed horribly and
> was BUG()ing all over the place, so I didn't get any significant testing
> of this patch done.
>
> Will update...

Nothing complains anywhere, looks great.


Thanks,
Sasha
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index f87d7e8..77bf205 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -2984,7 +2984,7 @@  int __init vty_init(const struct file_operations *console_fops)
 
 static struct class *vtconsole_class;
 
-static int bind_con_driver(const struct consw *csw, int first, int last,
+static int do_bind_con_driver(const struct consw *csw, int first, int last,
 			   int deflt)
 {
 	struct module *owner = csw->owner;
@@ -2995,7 +2995,7 @@  static int bind_con_driver(const struct consw *csw, int first, int last,
 	if (!try_module_get(owner))
 		return -ENODEV;
 
-	console_lock();
+	WARN_CONSOLE_UNLOCKED();
 
 	/* check if driver is registered */
 	for (i = 0; i < MAX_NR_CON_DRIVER; i++) {
@@ -3080,11 +3080,22 @@  static int bind_con_driver(const struct consw *csw, int first, int last,
 
 	retval = 0;
 err:
-	console_unlock();
 	module_put(owner);
 	return retval;
 };
 
+
+static int bind_con_driver(const struct consw *csw, int first, int last,
+			   int deflt)
+{
+	int ret;
+	
+	console_lock();
+	ret = do_bind_con_driver(csw, first, last, deflt);
+	console_unlock();
+	return ret;
+}
+	
 #ifdef CONFIG_VT_HW_CONSOLE_BINDING
 static int con_is_graphics(const struct consw *csw, int first, int last)
 {
@@ -3196,9 +3207,9 @@  int unbind_con_driver(const struct consw *csw, int first, int last, int deflt)
 	if (!con_is_bound(csw))
 		con_driver->flag &= ~CON_DRIVER_FLAG_INIT;
 
-	console_unlock();
 	/* ignore return value, binding should not fail */
-	bind_con_driver(defcsw, first, last, deflt);
+	do_bind_con_driver(defcsw, first, last, deflt);
+	console_unlock();
 err:
 	module_put(owner);
 	return retval;
@@ -3489,28 +3500,18 @@  int con_debug_leave(void)
 }
 EXPORT_SYMBOL_GPL(con_debug_leave);
 
-/**
- * register_con_driver - register console driver to console layer
- * @csw: console driver
- * @first: the first console to take over, minimum value is 0
- * @last: the last console to take over, maximum value is MAX_NR_CONSOLES -1
- *
- * DESCRIPTION: This function registers a console driver which can later
- * bind to a range of consoles specified by @first and @last. It will
- * also initialize the console driver by calling con_startup().
- */
-int register_con_driver(const struct consw *csw, int first, int last)
+static int do_register_con_driver(const struct consw *csw, int first, int last)
 {
 	struct module *owner = csw->owner;
 	struct con_driver *con_driver;
 	const char *desc;
 	int i, retval = 0;
 
+	WARN_CONSOLE_UNLOCKED();
+
 	if (!try_module_get(owner))
 		return -ENODEV;
 
-	console_lock();
-
 	for (i = 0; i < MAX_NR_CON_DRIVER; i++) {
 		con_driver = &registered_con_driver[i];
 
@@ -3563,10 +3564,29 @@  int register_con_driver(const struct consw *csw, int first, int last)
 	}
 
 err:
-	console_unlock();
 	module_put(owner);
 	return retval;
 }
+
+/**
+ * register_con_driver - register console driver to console layer
+ * @csw: console driver
+ * @first: the first console to take over, minimum value is 0
+ * @last: the last console to take over, maximum value is MAX_NR_CONSOLES -1
+ *
+ * DESCRIPTION: This function registers a console driver which can later
+ * bind to a range of consoles specified by @first and @last. It will
+ * also initialize the console driver by calling con_startup().
+ */
+int register_con_driver(const struct consw *csw, int first, int last)
+{
+	int retval;
+	
+	console_lock();
+	retval = do_register_con_driver(csw, first, last);
+	console_unlock();
+	return retval;
+}
 EXPORT_SYMBOL(register_con_driver);
 
 /**
@@ -3622,6 +3642,29 @@  EXPORT_SYMBOL(unregister_con_driver);
  *
  *      take_over_console is basically a register followed by unbind
  */
+int do_take_over_console(const struct consw *csw, int first, int last, int deflt)
+{
+	int err;
+
+	err = do_register_con_driver(csw, first, last);
+	/* if we get an busy error we still want to bind the console driver
+	 * and return success, as we may have unbound the console driver
+	 * but not unregistered it.
+	*/
+	if (err == -EBUSY)
+		err = 0;
+	if (!err)
+		do_bind_con_driver(csw, first, last, deflt);
+
+	return err;
+}
+/*
+ *	If we support more console drivers, this function is used
+ *	when a driver wants to take over some existing consoles
+ *	and become default driver for newly opened ones.
+ *
+ *      take_over_console is basically a register followed by unbind
+ */
 int take_over_console(const struct consw *csw, int first, int last, int deflt)
 {
 	int err;
diff --git a/drivers/video/console/fbcon.c b/drivers/video/console/fbcon.c
index fdefa8f..c75f8ce 100644
--- a/drivers/video/console/fbcon.c
+++ b/drivers/video/console/fbcon.c
@@ -529,6 +529,34 @@  static int search_for_mapped_con(void)
 	return retval;
 }
 
+static int do_fbcon_takeover(int show_logo)
+{
+	int err, i;
+
+	if (!num_registered_fb)
+		return -ENODEV;
+
+	if (!show_logo)
+		logo_shown = FBCON_LOGO_DONTSHOW;
+
+	for (i = first_fb_vc; i <= last_fb_vc; i++)
+		con2fb_map[i] = info_idx;
+
+	err = do_take_over_console(&fb_con, first_fb_vc, last_fb_vc,
+				fbcon_is_default);
+
+	if (err) {
+		for (i = first_fb_vc; i <= last_fb_vc; i++) {
+			con2fb_map[i] = -1;
+		}
+		info_idx = -1;
+	} else {
+		fbcon_has_console_bind = 1;
+	}
+
+	return err;
+}
+
 static int fbcon_takeover(int show_logo)
 {
 	int err, i;
@@ -3115,7 +3143,7 @@  static int fbcon_fb_registered(struct fb_info *info)
 		}
 
 		if (info_idx != -1)
-			ret = fbcon_takeover(1);
+			ret = do_fbcon_takeover(1);
 	} else {
 		for (i = first_fb_vc; i <= last_fb_vc; i++) {
 			if (con2fb_map_boot[i] == idx)
diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index 3ff0105..564ebe9 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -1650,7 +1650,9 @@  static int do_register_framebuffer(struct fb_info *fb_info)
 	event.info = fb_info;
 	if (!lock_fb_info(fb_info))
 		return -ENODEV;
+        console_lock();
 	fb_notifier_call_chain(FB_EVENT_FB_REGISTERED, &event);
+        console_unlock();
 	unlock_fb_info(fb_info);
 	return 0;
 }
@@ -1853,11 +1855,8 @@  int fb_new_modelist(struct fb_info *info)
 	err = 1;
 
 	if (!list_empty(&info->modelist)) {
-		if (!lock_fb_info(info))
-			return -ENODEV;
 		event.info = info;
 		err = fb_notifier_call_chain(FB_EVENT_NEW_MODELIST, &event);
-		unlock_fb_info(info);
 	}
 
 	return err;
diff --git a/drivers/video/fbsysfs.c b/drivers/video/fbsysfs.c
index a55e366..ef476b0 100644
--- a/drivers/video/fbsysfs.c
+++ b/drivers/video/fbsysfs.c
@@ -177,6 +177,8 @@  static ssize_t store_modes(struct device *device,
 	if (i * sizeof(struct fb_videomode) != count)
 		return -EINVAL;
 
+	if (!lock_fb_info(fb_info))
+		return -ENODEV;
 	console_lock();
 	list_splice(&fb_info->modelist, &old_list);
 	fb_videomode_to_modelist((const struct fb_videomode *)buf, i,
@@ -188,6 +190,7 @@  static ssize_t store_modes(struct device *device,
 		fb_destroy_modelist(&old_list);
 
 	console_unlock();
+	unlock_fb_info(fb_info);
 
 	return 0;
 }
diff --git a/include/linux/console.h b/include/linux/console.h
index dedb082..4ef4307 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -78,6 +78,7 @@  int con_is_bound(const struct consw *csw);
 int register_con_driver(const struct consw *csw, int first, int last);
 int unregister_con_driver(const struct consw *csw);
 int take_over_console(const struct consw *sw, int first, int last, int deflt);
+int do_take_over_console(const struct consw *sw, int first, int last, int deflt);
 void give_up_console(const struct consw *sw);
 #ifdef CONFIG_HW_CONSOLE
 int con_debug_enter(struct vc_data *vc);