[PATCH 0/2] fbcon+i915 locking fixes

[PATCH 0/2] fbcon+i915 locking fixes

[Daniel Vetter]

Hi Dave,

First patch is a resend of Alan's fbcon vs. fb notifier deadlock fix.
Without this lockdep is pretty much useless for debugging drm locking
issues, and it looks like quite a few bootup hangs could be blamed on this
one, too.

Since the fbdev maintainer seems to be awol, please merge this through
your drm tree. I've looked through the patch and the new locking seems to
be sane so feel free to smash a

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

onto it.

2nd patch is a locking bug in the i915 driver with the new kms locking.
Since the bug only happens in your drm-next tree, please apply the patch
there directly.

Thanks, Daniel

Alan Cox (1):
  fb: Rework locking to fix lock ordering on takeover

Daniel Vetter (1):
  drm/i915: fixup crtc locking in intel_release_load_detect_pipe

 drivers/gpu/drm/i915/intel_display.c |    1 +
 drivers/tty/vt/vt.c                  |   81 ++++++++++++++++++++++++++--------
 drivers/video/console/fbcon.c        |   30 ++++++++++++-
 drivers/video/fbmem.c                |    5 +--
 drivers/video/fbsysfs.c              |    3 ++
 include/linux/console.h              |    1 +
 6 files changed, 98 insertions(+), 23 deletions(-)

-- 
1.7.10.4

[PATCH 1/2] fb: Rework locking to fix lock ordering on takeover

[Daniel Vetter]

From: Alan Cox <alan@lxorguk.ukuu.org.uk>

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>
[danvet: Tiny whitespace cleanup.]
Reported-and-tested-by: Hugh Dickins <hughd@google.com>
Reported-and-tested-by: Sasha Levin <levinsasha928@gmail.com>
References: https://lkml.org/lkml/2012/10/25/516
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/tty/vt/vt.c           |   81 +++++++++++++++++++++++++++++++----------
 drivers/video/console/fbcon.c |   30 ++++++++++++++-
 drivers/video/fbmem.c         |    5 +--
 drivers/video/fbsysfs.c       |    3 ++
 include/linux/console.h       |    1 +
 5 files changed, 97 insertions(+), 23 deletions(-)

diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 8fd8968..a38d9d1 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -2987,7 +2987,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;
@@ -2998,7 +2998,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++) {
@@ -3083,11 +3083,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)
 {
@@ -3199,9 +3210,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;
@@ -3492,28 +3503,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];
 
@@ -3566,10 +3567,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);
 
 /**
@@ -3625,6 +3645,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..d8d9831 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);
-- 
1.7.10.4

[PATCH 2/2] drm/i915: fixup per-crtc locking in intel_release_load_detect_pipe

[Daniel Vetter]

One of the early return cases missed the mutex unlocking. Hilarity
ensued.

This regression has been introduced in

commit 7b24056be6db7ce907baffdd4cf142ab774ea60c
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Wed Dec 12 00:35:33 2012 +0100

    drm: don't hold crtc mutexes for connector ->detect callbacks

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=59750
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 26df9e3..ece4afd 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6522,6 +6522,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
 			drm_framebuffer_unreference(old->release_fb);
 		}
 
+		mutex_unlock(&crtc->mutex);
 		return;
 	}
 
-- 
1.7.10.4

Re: [PATCH 1/2] fb: Rework locking to fix lock ordering on takeover

[Takashi Iwai]

At Wed, 23 Jan 2013 17:25:08 +0100,
Daniel Vetter wrote:
> 
> From: Alan Cox <alan@lxorguk.ukuu.org.uk>
> 
> 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>
> [danvet: Tiny whitespace cleanup.]
> Reported-and-tested-by: Hugh Dickins <hughd@google.com>
> Reported-and-tested-by: Sasha Levin <levinsasha928@gmail.com>
> References: https://lkml.org/lkml/2012/10/25/516
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

FYI, the latest patch of this is found in mm tree:
  http://ozlabs.org/~akpm/mmots/broken-out/fb-rework-locking-to-fix-lock-ordering-on-takeover.patch

Also I hit the same problem in another code paths (for unbind and
unregister):
  http://marc.info/?t=135309396400003&r=1&w=2

My additional patch is found in mm tree, too:
  http://ozlabs.org/~akpm/mmots/broken-out/fb-yet-another-band-aid-for-fixing-lockdep-mess.patch


Takashi

Re: [PATCH 1/2] fb: Rework locking to fix lock ordering on takeover

[Daniel Vetter]

On Wed, Jan 23, 2013 at 5:38 PM, Takashi Iwai <tiwai@suse.de> wrote:
> At Wed, 23 Jan 2013 17:25:08 +0100,
> Daniel Vetter wrote:
>>
>> From: Alan Cox <alan@lxorguk.ukuu.org.uk>
>>
>> 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>
>> [danvet: Tiny whitespace cleanup.]
>> Reported-and-tested-by: Hugh Dickins <hughd@google.com>
>> Reported-and-tested-by: Sasha Levin <levinsasha928@gmail.com>
>> References: https://lkml.org/lkml/2012/10/25/516
>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> FYI, the latest patch of this is found in mm tree:
>   http://ozlabs.org/~akpm/mmots/broken-out/fb-rework-locking-to-fix-lock-ordering-on-takeover.patch
>
> Also I hit the same problem in another code paths (for unbind and
> unregister):
>   http://marc.info/?t=135309396400003&r=1&w=2
>
> My additional patch is found in mm tree, too:
>   http://ozlabs.org/~akpm/mmots/broken-out/fb-yet-another-band-aid-for-fixing-lockdep-mess.patch

Indeed, there's more stuff :( And I've missed the export_symbol fix
for the first patch. Still, is there any chance we can at least merge
the first one (I don't think that many people unload framebuffers)
into 3.8 with cc: stable? I'd like to get at least rid of the known
deadlocks at kms takeover time, since we have quite a few unexplained
such bug reports floating around ... Andrew, Dave?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/2] drm/i915: fixup per-crtc locking in intel_release_load_detect_pipe

[Daniel Vetter]

On Wed, Jan 23, 2013 at 05:25:09PM +0100, Daniel Vetter wrote:
> One of the early return cases missed the mutex unlocking. Hilarity
> ensued.
> 
> This regression has been introduced in
> 
> commit 7b24056be6db7ce907baffdd4cf142ab774ea60c
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Wed Dec 12 00:35:33 2012 +0100
> 
>     drm: don't hold crtc mutexes for connector ->detect callbacks
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=59750
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Tested-by: Cancan Feng <cancan.feng@intel.com>

Dave, please pick this one up for your drm-next tree since the issue only
happens there due to the modeset locking rework.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch