fb: Rework locking to fix lock ordering on takeover

[The fb maintainer appears to be absent at the moment].

This is needed to fix a pile of lockdep splats that now show up because console_lock() is being properly audited. Hugh Dickins and Sasha Levin have tested it and both reports all looks good. This is probably not the whole story - the entire fb layer has locking confusion problems that were previously hidden but it seems to get the ones people hit in testing. This hopefully explains a few of the weird fb hangs that have been floating around forever.

From: Alan Cox <[email protected]>

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.

---
 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(-)

index f87d7e8..77bf205 100644
@@ -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;

index fdefa8f..c75f8ce 100644
@@ -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)

index 3ff0105..564ebe9 100644
@@ -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;

index a55e366..ef476b0 100644
@@ -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;
 }

index dedb082..4ef4307 100644
@@ -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);

Should this eventually get into the stable trees?

josh

Thats a question I'm not sure about at this point. I think the bug is real but not caught by the lock checker in older trees but I've not investigated.

Alan

So... this patch seems to still be twisting in the wind. It should probably be headed into 3.8 at this point, shouldn't it?

josh

Indeed it should. I'm seeing the original warnings in 3.8-rc1 and have to carry this patch to avoid them.

Thanks,
Sasha

This patch can fix the following warning we saw?
http://lkml.org/lkml/2012/12/22/53

I will give it a try.

Yup, that's the same error I've reported couple of months ago.

It looks like the fb maintains are still absent, so it'll probably need a different way to get upstream.

Thanks,
Sasha

Adding to the bug pressure: just got a very similar splat on -rc1 (see below). Alan, I'll run your patch to verify.

Thanks.

[33946.663968] ======================================================
[33946.663970] [ INFO: possible circular locking dependency detected ]
[33946.663978] 3.8.0-rc1+ #1 Not tainted
[33946.663980] -------------------------------------------------------
[33946.663986] kworker/1:2/15780 is trying to acquire lock:
[33946.664010]  ((fb_notifier_list).rwsem){++++.+}, at: [<ffffffff810776a3>] __blocking_notifier_call_chain+0x33/0x60
[33946.664013] 
[33946.664013] but task is already holding lock:
[33946.664029]  (console_lock){+.+.+.}, at: [<ffffffff812ecda3>] console_callback+0x13/0x160
[33946.664032] 
[33946.664032] which lock already depends on the new lock.
[33946.664032] 
[33946.664034] 
[33946.664034] the existing dependency chain (in reverse order) is:
[33946.664042] 
[33946.664042] -> #1 (console_lock){+.+.+.}:
[33946.664054]        [<ffffffff810a821a>] lock_acquire+0x8a/0x140
[33946.664063]        [<ffffffff8104bf5f>] console_lock+0x5f/0x70
[33946.664072]        [<ffffffff812e94f9>] register_con_driver+0x39/0x150
[33946.664080]        [<ffffffff812eafde>] take_over_console+0x2e/0x70
[33946.664088]        [<ffffffff8128409a>] fbcon_takeover+0x5a/0xb0
[33946.664096]        [<ffffffff81287c5b>] fbcon_event_notify+0x5eb/0x6f0
[33946.664103]        [<ffffffff8107758c>] notifier_call_chain+0x4c/0x70
[33946.664111]        [<ffffffff810776bb>] __blocking_notifier_call_chain+0x4b/0x60
[33946.664119]        [<ffffffff810776e6>] blocking_notifier_call_chain+0x16/0x20
[33946.664127]        [<ffffffff8127903b>] fb_notifier_call_chain+0x1b/0x20
[33946.664136]        [<ffffffff8127a76c>] register_framebuffer+0x1bc/0x2f0
[33946.664169]        [<ffffffffa00e0283>] drm_fb_helper_single_fb_probe+0x1e3/0x310 [drm_kms_helper]
[33946.664183]        [<ffffffffa00e0581>] drm_fb_helper_initial_config+0x1d1/0x230 [drm_kms_helper]
[33946.664239]        [<ffffffffa04a74a1>] radeon_fbdev_init+0xc1/0x120 [radeon]
[33946.664290]        [<ffffffffa04a22b8>] radeon_modeset_init+0x3a8/0xb90 [radeon]
[33946.664333]        [<ffffffffa047f0e0>] radeon_driver_load_kms+0xf0/0x180 [radeon]
[33946.664344]        [<ffffffff81314bd6>] drm_get_pci_dev+0x186/0x2d0
[33946.664379]        [<ffffffffa0465183>] radeon_pci_probe+0xb3/0xf0 [radeon]
[33946.664390]        [<ffffffff8126687c>] pci_device_probe+0x9c/0xe0
[33946.664400]        [<ffffffff8132cf6b>] driver_probe_device+0x8b/0x3a0
[33946.664408]        [<ffffffff8132d32b>] __driver_attach+0xab/0xb0
[33946.664415]        [<ffffffff8132aea5>] bus_for_each_dev+0x55/0x90
[33946.664422]        [<ffffffff8132c9ae>] driver_attach+0x1e/0x20
[33946.664429]        [<ffffffff8132c500>] bus_add_driver+0x1b0/0x2a0
[33946.664437]        [<ffffffff8132da17>] driver_register+0x77/0x160
[33946.664445]        [<ffffffff81265724>] __pci_register_driver+0x64/0x70
[33946.664452]        [<ffffffff81314e2c>] drm_pci_init+0x10c/0x120
[33946.664480]        [<ffffffffa05470e7>] inet6_ioctl+0x7/0xb0 [ipv6]
[33946.664491]        [<ffffffff810002f2>] do_one_initcall+0x122/0x170
[33946.664500]        [<ffffffff810b520f>] load_module+0x185f/0x2160
[33946.664507]        [<ffffffff810b5bbe>] sys_init_module+0xae/0x110
[33946.664516]        [<ffffffff814c7f42>] system_call_fastpath+0x16/0x1b
[33946.664526] 
[33946.664526] -> #0 ((fb_notifier_list).rwsem){++++.+}:
[33946.664534]        [<ffffffff810a7c88>] __lock_acquire+0x1ae8/0x1b10
[33946.664542]        [<ffffffff810a821a>] lock_acquire+0x8a/0x140
[33946.664549]        [<ffffffff814c4ba4>] down_read+0x34/0x49
[33946.664557]        [<ffffffff810776a3>] __blocking_notifier_call_chain+0x33/0x60
[33946.664564]        [<ffffffff810776e6>] blocking_notifier_call_chain+0x16/0x20
[33946.664572]        [<ffffffff8127903b>] fb_notifier_call_chain+0x1b/0x20
[33946.664579]        [<ffffffff812797cb>] fb_blank+0x3b/0xc0
[33946.664586]        [<ffffffff81287f83>] fbcon_blank+0x223/0x2d0
[33946.664595]        [<ffffffff812ebd6b>] do_blank_screen+0x1cb/0x270
[33946.664603]        [<ffffffff812ecdfa>] console_callback+0x6a/0x160
[33946.664612]        [<ffffffff81068f0d>] process_one_work+0x19d/0x5e0
[33946.664620]        [<ffffffff8106a7dd>] worker_thread+0x15d/0x450
[33946.664628]        [<ffffffff81070d5a>] kthread+0xea/0xf0
[33946.664636]        [<ffffffff814c7e9c>] ret_from_fork+0x7c/0xb0
[33946.664638] 
[33946.664638] other info that might help us debug this:
[33946.664638] 
[33946.664641]  Possible unsafe locking scenario:
[33946.664641] 
[33946.664643]        CPU0                    CPU1
[33946.664645]        ----                    ----
[33946.664650]   lock(console_lock);
[33946.664656]                                lock((fb_notifier_list).rwsem);
[33946.664661]                                lock(console_lock);
[33946.664666]   lock((fb_notifier_list).rwsem);
[33946.664667] 
[33946.664667]  *** DEADLOCK ***
[33946.664667] 
[33946.664671] 3 locks held by kworker/1:2/15780:
[33946.664686]  #0:  (events){.+.+.+}, at: [<ffffffff81068ea0>] process_one_work+0x130/0x5e0
[33946.664701]  #1:  (console_work){+.+.+.}, at: [<ffffffff81068ea0>] process_one_work+0x130/0x5e0
[33946.664715]  #2:  (console_lock){+.+.+.}, at: [<ffffffff812ecda3>] console_callback+0x13/0x160
[33946.664717] 
[33946.664717] stack backtrace:
[33946.664723] Pid: 15780, comm: kworker/1:2 Not tainted 3.8.0-rc1+ #1
[33946.664726] Call Trace:
[33946.664736]  [<ffffffff814bd52d>] print_circular_bug+0x1fe/0x20f
[33946.664745]  [<ffffffff810a7c88>] __lock_acquire+0x1ae8/0x1b10
[33946.664756]  [<ffffffff81005cc7>] ? print_context_stack+0x87/0xf0
[33946.664766]  [<ffffffff810a821a>] lock_acquire+0x8a/0x140
[33946.664773]  [<ffffffff810776a3>] ? __blocking_notifier_call_chain+0x33/0x60
[33946.664781]  [<ffffffff814c4ba4>] down_read+0x34/0x49
[33946.664788]  [<ffffffff810776a3>] ? __blocking_notifier_call_chain+0x33/0x60
[33946.664796]  [<ffffffff810a73b8>] ? __lock_acquire+0x1218/0x1b10
[33946.664803]  [<ffffffff810776a3>] __blocking_notifier_call_chain+0x33/0x60
[33946.664811]  [<ffffffff810776e6>] blocking_notifier_call_chain+0x16/0x20
[33946.664819]  [<ffffffff8127903b>] fb_notifier_call_chain+0x1b/0x20
[33946.664826]  [<ffffffff812797cb>] fb_blank+0x3b/0xc0
[33946.664833]  [<ffffffff81287f83>] fbcon_blank+0x223/0x2d0
[33946.664841]  [<ffffffff814c6f65>] ? _raw_spin_unlock_irqrestore+0x65/0x80
[33946.664848]  [<ffffffff81080581>] ? get_parent_ip+0x11/0x50
[33946.664855]  [<ffffffff81080639>] ? sub_preempt_count+0x79/0xd0
[33946.664862]  [<ffffffff814c6f42>] ? _raw_spin_unlock_irqrestore+0x42/0x80
[33946.664872]  [<ffffffff8105c71f>] ? try_to_del_timer_sync+0x4f/0x70
[33946.664880]  [<ffffffff8105c7ea>] ? del_timer_sync+0xaa/0xd0
[33946.664888]  [<ffffffff8105c745>] ? del_timer_sync+0x5/0xd0
[33946.664896]  [<ffffffff812ebd6b>] do_blank_screen+0x1cb/0x270
[33946.664905]  [<ffffffff812ecdfa>] console_callback+0x6a/0x160
[33946.664913]  [<ffffffff81068f0d>] process_one_work+0x19d/0x5e0
[33946.664921]  [<ffffffff81068ea0>] ? process_one_work+0x130/0x5e0
[33946.664927]  [<ffffffff814c6667>] ? _raw_spin_lock_irq+0x17/0x50
[33946.664935]  [<ffffffff812ecd90>] ? poke_blanked_console+0xd0/0xd0
[33946.664945]  [<ffffffff8106a7dd>] worker_thread+0x15d/0x450
[33946.664954]  [<ffffffff8106a680>] ? busy_worker_rebind_fn+0x100/0x100
[33946.664961]  [<ffffffff81070d5a>] kthread+0xea/0xf0
[33946.664972]  [<ffffffff81070c70>] ? kthread_create_on_node+0x160/0x160
[33946.664979]  [<ffffffff814c7e9c>] ret_from_fork+0x7c/0xb0
[33946.664987]  [<ffffffff81070c70>] ? kthread_create_on_node+0x160/0x160

Regards/Gruss,
Boris.

+1

http://thread.gmane.org/gmane.linux.kernel/1413953/focus=1415070

Shawn

Cool, works fine here too. Is Linus on CC? (/me checks.. ) Yes he is, good.

Linus, Alan's patch works at least in 2 cases, you might consider picking it up directly since the fb maintainer is absent, reportedly.

Thanks.

Regards/Gruss,
Boris.

Am 28.12.2012 13:40, schrieb Borislav Petkov:

Btw. I think all the usb-fb's (udlfb, smscufx and udl) are broken, at least on ARM(v5). When I have linked in udlfb the following happens on boot (with an attached USB-LCD and with or without the "Rework locking patch):

dlfb_init_framebuffer_work() (work)
register_framebuffer() (lock mutex registration_lock)
(vt_console_print) (spinlock printing_lock) (fbcon_scroll)
(fbcon_redraw)
(fbcon_putcs)
(bit_putcs)
dlfb_ops_imageblit()
dlfb_handle_damage()
dlfb_get_urb()
down_timeout(semaphore)
BUG: scheduling while atomic
(vt_console_print) (release spinlock printing_lock)
register_framebuffer() (unlock mutex registration_lock)

The above is without the "Rework locking" patch. But I get the same BUG with the patch (so the patch doesn't do any harm), I just haven't looked in detail what changed with the patch.

I don't get the BUG when I try the same on a x86_64 machine, not sure why. I've just started to read me through the code, documentation and the "Rework locking" patch. And I'm slow in doing so (spare time). But maybe someone else with more knowledge about the inner workings of this stuff is faster than I.

Regards,

Alexander

They are broken if used as the system console (has been known for years). Perhaps your x86 test has the system console still on another device ?

For the udl layer it shouldn't matter as Dave Airlie wrote a DRM driver for udl which obsoletes the old fb layer one and works much better (although the error handling is still totally broken and leaks like a sieve if it fails)

Fixing the console isn't that difficult - you just need to make your device queue the console I/O to a worker thread of some kind. We don't want to do that by default because we want to get the messages out reliably and immediately on saner hardware. Given there are several such cases a general helper and a console "I am crap" flag might be better than hacking each driver.

Alan

Am 04.01.2013 14:25, schrieb Alan Cox:

Ah. Thats why I didn't see it before. Usually I've used the serial as system console. So thats why it worked before. ;)

Exactly thats the case. Thanks for pointing it out.

That is what I wanted to try next. ;)

All those drivers look very similiar. I will see if I'm successfull in writing such an IamCrapHelper. Might need some time, but I will post a patch for review, if I've done and tested it. I'm only using the USB-LCD on occasion, so it doesn't have high priority for me because I don't really need it.

Thanks for the hints.

Regards,

Alexander

Am 04.01.2013 14:36, schrieb Alexander Holler:

...

I've just added a work queue for dlfb_handle_damage. Up to now I've only tested the console, seems to work without any problems (even with lock checking on).

In regard to that "I am crap" handler, I'm not sure how to do that.

Just queuing the ops wherever they are used outside the drivers doesn't work, because e.g.

if(i_am_crap)
queue_work(ops.fb_imageblit(..., image))
else
ops.fb_imageblit(..., image)

doesn't work, because e.g. image will become destroyed before the work gets executed. And copying the whole image doesn't make sense. handle_damage() in contrast just needs the coordinates for a rectangle (x, y, w, h).

So to add such an "I am crap" flag my idea would be to add an .fb_handle_damage to struct fb_ops and then call that (if exists) whenever something was changed.

But I don't like that very much. I think that might end up in more changes than just changing those 3 very similiar drivers (I'm not sure if the queuing is needed for udl at all).

Maybe it would make sense, to unify the stuff in those 3 similiar drivers moving the shared functions to one file which is used by them all. udl seems to have already split some stuff into different files.

My patch (for udlfb) follows as an reply to this message. If that patch is ok, it should be applied to smscufx too (I would make it). In regard to udl I don't know, I haven't had a deeper look at it nor used it up to now.

Regards,

Alexander

The console functions are using spinlocks while calling fb-driver ops but udlfb waits for a semaphore in many ops. This results in the BUG "scheduling while atomic". One of those call flows is e.g.

vt_console_print() (spinlock printing_lock)

	(...)
	dlfb_ops_imageblit()
                        dlfb_handle_damage()
                                dlfb_get_urb()
					down_timeout(semaphore)
BUG: scheduling while atomic

(...)
vt_console_print() (release spinlock printing_lock)

Fix this through a workqueue for dlfb_handle_damage().

---
 drivers/video/udlfb.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 51 insertions(+), 2 deletions(-)

index 86d449e..5aadcb2 100644
@@ -569,7 +569,7 @@ static int dlfb_render_hline(struct dlfb_data *dev, struct urb **urb_ptr,
 	return 0;
 }
 
-int dlfb_handle_damage(struct dlfb_data *dev, int x, int y,
+int dlfb_handle_damage_queued(struct dlfb_data *dev, int x, int y,
 	       int width, int height, char *data)
 {
 	int i, ret;

@@ -630,6 +630,44 @@ error:
 	return 0;
 }
 
+static struct workqueue_struct *dlfb_handle_damage_wq;
+
+struct dlfb_handle_damage_work {
+	struct work_struct my_work;
+	struct dlfb_data *dev;
+	char *data;
+	int x, y, width, height;
+};
+
+static void dlfb_handle_damage_work(struct work_struct *work)
+{
+	struct dlfb_handle_damage_work *my_work +		(struct dlfb_handle_damage_work *)work;
+
+	dlfb_handle_damage_queued(my_work->dev, my_work->x, my_work->y,
+			my_work->width, my_work->height, my_work->data);
+	kfree(work);
+	return;
+}
+
+void dlfb_handle_damage(struct dlfb_data *dev, int x, int y,
+	       int width, int height, char *data)
+{
+	struct dlfb_handle_damage_work *work +		kmalloc(sizeof(struct dlfb_handle_damage_work), GFP_KERNEL);
+
+	if (work) {
+		INIT_WORK((struct work_struct *)work, dlfb_handle_damage_work);
+		work->dev = dev;
+		work->x = x;
+		work->y = y;
+		work->width = width;
+		work->height = height;
+		work->data = data;
+		queue_work(dlfb_handle_damage_wq, (struct work_struct *)work);
+	}
+}
+
 /*
  * Path triggered by usermode clients who write to filesystem
  * e.g. cat filename > /dev/fb1

@@ -945,6 +983,9 @@ static void dlfb_free_framebuffer(struct dlfb_data *dev)
 
 		unregister_framebuffer(info);
 
+		if (dlfb_handle_damage_wq)
+			destroy_workqueue(dlfb_handle_damage_wq);
+
 		if (info->cmap.len != 0)
 			fb_dealloc_cmap(&info->cmap);
 		if (info->monspecs.modedb)

@@ -1626,13 +1667,13 @@ static int dlfb_usb_probe(struct usb_interface *interface,
 		dev->sku_pixel_limit = pixel_limit;
 	}
 
-
 	if (!dlfb_alloc_urb_list(dev, WRITES_IN_FLIGHT, MAX_TRANSFER)) {
 		retval = -ENOMEM;
 		pr_err("dlfb_alloc_urb_list failed\n");
 		goto error;
 	}
 
+
 	kref_get(&dev->kref); /* matching kref_put in free_framebuffer_work */
 
 	/* We don't register a new USB class. Our client interface is fbdev */

@@ -1694,6 +1735,13 @@ static void dlfb_init_framebuffer_work(struct work_struct *work)
 		goto error;
 	}
 
+	dlfb_handle_damage_wq = alloc_workqueue("udlfb_damage",
+						WQ_MEM_RECLAIM, 0);
+	if (dlfb_handle_damage_wq = NULL) {
+		pr_err("unable to allocate workqueue\n");
+		goto error;
+	}
+
 	/* ready to begin using device */
 
 	atomic_set(&dev->usb_active, 1);

@@ -1702,6 +1750,7 @@ static void dlfb_init_framebuffer_work(struct work_struct *work)
 	dlfb_ops_check_var(&info->var, info);
 	dlfb_ops_set_par(info);
 
+
 	retval = register_framebuffer(info);
 	if (retval < 0) {
 		pr_err("register_framebuffer failed %d\n", retval);

1.7.11.7

Am 05.01.2013 12:42, schrieb Alexander Holler:

Having had a second look at my patch for udlfb, I'm not sure it will work with more than one of those devices attached. I think my approach to just add one (static) workqueue might not work in such a case, at least it looks so to me. But I'm unable to test it, as I only have one of those devices.

Having had a look at udl, I wonder why udlfb still has to be around. But because udl currently doesn't work here too, I'm not sure what functionality udl misses which udlfb still has.

So to conclude, my patch works as a workaround if only one of those devices will be attached, but currently should not be included into the kernel.

I don't know if I will make another version of that patch, as I will first have a deeper look at udl (if I find the time).

Regards,

Alexander

The console functions are using spinlocks while calling fb-driver ops but udlfb waits for a semaphore in many ops. This results in the BUG "scheduling while atomic". One of those call flows is e.g.

vt_console_print() (spinlock printing_lock)

	(...)
	dlfb_ops_imageblit()
                        dlfb_handle_damage()
                                dlfb_get_urb()
					down_timeout(semaphore)
BUG: scheduling while atomic

(...)
vt_console_print() (release spinlock printing_lock)

Fix this through a workqueue for dlfb_handle_damage().

---
 drivers/video/udlfb.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++-
 include/video/udlfb.h |  1 +
 2 files changed, 50 insertions(+), 1 deletion(-)

index 86d449e..4a90784 100644
@@ -569,7 +569,7 @@ static int dlfb_render_hline(struct dlfb_data *dev, struct urb **urb_ptr,
 	return 0;
 }
 
-int dlfb_handle_damage(struct dlfb_data *dev, int x, int y,
+int dlfb_handle_damage_queued(struct dlfb_data *dev, int x, int y,
 	       int width, int height, char *data)
 {
 	int i, ret;

@@ -630,6 +630,44 @@ error:
 	return 0;
 }
 
+struct dlfb_handle_damage_work {
+	struct work_struct my_work;
+	struct dlfb_data *dev;
+	char *data;
+	int x, y, width, height;
+};
+
+static void dlfb_handle_damage_work(struct work_struct *work)
+{
+	struct dlfb_handle_damage_work *my_work +		(struct dlfb_handle_damage_work *)work;
+
+	dlfb_handle_damage_queued(my_work->dev, my_work->x, my_work->y,
+			my_work->width, my_work->height, my_work->data);
+	kfree(work);
+	return;
+}
+
+void dlfb_handle_damage(struct dlfb_data *dev, int x, int y,
+	       int width, int height, char *data)
+{
+	struct dlfb_handle_damage_work *work +		kmalloc(sizeof(struct dlfb_handle_damage_work), GFP_KERNEL);
+
+	if (!work) {
+		pr_err("unable to allocate work\n");
+		return;
+	}
+	INIT_WORK((struct work_struct *)work, dlfb_handle_damage_work);
+	work->dev = dev;
+	work->x = x;
+	work->y = y;
+	work->width = width;
+	work->height = height;
+	work->data = data;
+	queue_work(dev->handle_damage_wq, (struct work_struct *)work);
+}
+
 /*
  * Path triggered by usermode clients who write to filesystem
  * e.g. cat filename > /dev/fb1

@@ -945,6 +983,9 @@ static void dlfb_free_framebuffer(struct dlfb_data *dev)
 
 		unregister_framebuffer(info);
 
+		if (dev->handle_damage_wq)
+			destroy_workqueue(dev->handle_damage_wq);
+
 		if (info->cmap.len != 0)
 			fb_dealloc_cmap(&info->cmap);
 		if (info->monspecs.modedb)

@@ -1694,6 +1735,13 @@ static void dlfb_init_framebuffer_work(struct work_struct *work)
 		goto error;
 	}
 
+	dev->handle_damage_wq = alloc_workqueue("udlfb_damage",
+						WQ_MEM_RECLAIM, 0);
+	if (dev->handle_damage_wq = NULL) {
+		pr_err("unable to allocate workqueue\n");
+		goto error;
+	}
+
 	/* ready to begin using device */
 
 	atomic_set(&dev->usb_active, 1);

index f9466fa..1e765f9 100644
@@ -43,6 +43,7 @@ struct dlfb_data {
 	bool virtualized; /* true when physical usb device not present */
 	struct delayed_work init_framebuffer_work;
 	struct delayed_work free_framebuffer_work;
+	struct workqueue_struct *handle_damage_wq;
 	atomic_t usb_active; /* 0 = update virtual buffer, but no usb traffic */
 	atomic_t lost_pixels; /* 1 = a render op failed. Need screen refresh */
 	char *edid; /* null until we read edid from hw or get from sysfs */

1.7.11.7

I was thinking much higher level - ie at the printk kind of level

Looks pretty clean as a solution to me.

Am 05.01.2013 13:07, schrieb Alan Cox:

Thanks and sorry for the two empty lines in the patch. I swear I had a look at the patch before sending it out, but haven't seen them.

So should I make the same patch for smscufx and while beeing there, send out at v2 without those 2 empty lines?

Regards,

Alexander

Still seeing it with current Linus' tree.

Anyone applying this, please?

Jiri Kosina
SUSE Labs

Yeah, I'm still seeing it in -rc3. Linus, can you pick up Alan's patch from https://patchwork.kernel.org/patch/1757061/ please?

Thanks.

Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Christ people.

I already reported that it DOES NOT EVEN COMPILE. See the other thread here on lkml ("Re: 3.8-rc2: EFI framebuffer lock inversion...").

Alan apparently doesn't care about the patch he wrote to even bother fixing that, and the only person who does seem to care enough to carry two fixes around (Andrew) apparently doesn't feel that he's comfortable forwarding it to me (he's been sending other patches, so it's not like Andrew is offline either)..

I'm not picking up random patches from people who don't care enough about those patches to even bother fixing compile errors when reportyed and didn't even send them to me to begin with.

So I'm trusting that Andrew is right, and is waiting for something.

Linus

Alan is dealing with family stuff that is far more important. Someone who has the time needs to own this and the fb layer. Not a don't care - a don't have time.

Alan

Florian has been taking a month or two's downtime (now expired, I think) so I've been waiting for him to reappear to process this one for 3.8.

Meanwhile, I guess we could put it into mainline ;) It has been in -next for a month.

From: Alan Cox <[email protected]>
Subject: 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 is partly a band aid, the fb layer is terminally confused about the locking rules it uses for its notifiers it seems.

---

 drivers/tty/vt/vt.c           |   91 ++++++++++++++++++++++++--------
 drivers/video/console/fbcon.c |   30 ++++++++++
 drivers/video/fbmem.c         |    5 -
 drivers/video/fbsysfs.c       |    3 +
 include/linux/console.h       |    1 
 5 files changed, 104 insertions(+), 26 deletions(-)

diff -puN drivers/tty/vt/vt.c~fb-rework-locking-to-fix-lock-ordering-on-takeover drivers/tty/vt/vt.c
@@ -2987,7 +2987,7 @@ int __init vty_init(const struct file_op
 
 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 
 	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 
 
 	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
 	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 con
 	}
 
 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,15 +3645,42 @@ 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;
+}
+EXPORT_SYMBOL_GPL(do_take_over_console);
+
+/*
+ *	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;
 
 	err = register_con_driver(csw, first, last);
-	/* if we get an busy error we still want to bind the console driver
+	/*
+	 * 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.
-	*/
+	_* but not unregistered it.
+	 */
 	if (err == -EBUSY)
 		err = 0;
 	if (!err)

diff -puN drivers/video/console/fbcon.c~fb-rework-locking-to-fix-lock-ordering-on-takeover 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
 		}
 
 		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 -puN drivers/video/fbmem.c~fb-rework-locking-to-fix-lock-ordering-on-takeover drivers/video/fbmem.c
@@ -1650,7 +1650,9 @@ static int do_register_framebuffer(struc
 	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 -puN drivers/video/fbsysfs.c~fb-rework-locking-to-fix-lock-ordering-on-takeover drivers/video/fbsysfs.c
@@ -177,6 +177,8 @@ static ssize_t store_modes(struct 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
 		fb_destroy_modelist(&old_list);
 
 	console_unlock();
+	unlock_fb_info(fb_info);
 
 	return 0;
 }

diff -puN include/linux/console.h~fb-rework-locking-to-fix-lock-ordering-on-takeover 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);

_

Yes, that's the one, modulo Andrew's fixes, which I've been running. Irregardless, I'll run this one tomorrow just in case, because it triggers here so easily.

Thanks.

Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Hey,

Op 13-01-13 01:02, Borislav Petkov schreef:

Just tested this patch on a macbook pro with i915 and radeon. First I get a nasty lockdep splat, then it locks up completely:

[   13.507373] fb: conflicting fb hw usage radeondrmfb vs EFI VGA - removing generic driver
[   13.507412] 
[   13.507413] ======================================================
[   13.507413] [ INFO: possible circular locking dependency detected ]
[   13.507414] 3.8.0-rc3-patser+ #910 Not tainted
[   13.507415] -------------------------------------------------------
[   13.507415] modprobe/554 is trying to acquire lock:
[   13.507421]  (console_lock){+.+.+.}, at: [<ffffffff813f6ca7>] unbind_con_driver+0x37/0x210
[   13.507422] 
[   13.507422] but task is already holding lock:
[   13.507425]  ((fb_notifier_list).rwsem){.+.+.+}, at: [<ffffffff8106d96d>] __blocking_notifier_call_chain+0x3d/0x80
[   13.507426] 
[   13.507426] which lock already depends on the new lock.
[   13.507426] 
[   13.507426] 
[   13.507426] the existing dependency chain (in reverse order) is:
[   13.507428] 
[   13.507428] -> #1 ((fb_notifier_list).rwsem){.+.+.+}:
[   13.507431]        [<ffffffff8109e946>] lock_acquire+0x96/0xc0
[   13.507434]        [<ffffffff816f8442>] down_read+0x42/0x57
[   13.507435]        [<ffffffff8106d96d>] __blocking_notifier_call_chain+0x3d/0x80
[   13.507437]        [<ffffffff8106d9c1>] blocking_notifier_call_chain+0x11/0x20
[   13.507439]        [<ffffffff8138e816>] fb_notifier_call_chain+0x16/0x20
[   13.507441]        [<ffffffff8138fd23>] register_framebuffer+0x1c3/0x2e0
[   13.507444]        [<ffffffff81cdfabf>] efifb_probe+0x402/0x489
[   13.507446]        [<ffffffff81412e0e>] platform_drv_probe+0x3e/0x70
[   13.507448]        [<ffffffff81410e96>] driver_probe_device+0x76/0x240
[   13.507450]        [<ffffffff81411103>] __driver_attach+0xa3/0xb0
[   13.507451]        [<ffffffff8140f2f6>] bus_for_each_dev+0x56/0x90
[   13.507452]        [<ffffffff814109f9>] driver_attach+0x19/0x20
[   13.507454]        [<ffffffff81410598>] bus_add_driver+0x188/0x270
[   13.507455]        [<ffffffff81411635>] driver_register+0x75/0x150
[   13.507457]        [<ffffffff814126a1>] platform_driver_register+0x41/0x50
[   13.507458]        [<ffffffff814126c6>] platform_driver_probe+0x16/0xa0
[   13.507460]        [<ffffffff81cdfdbd>] efifb_init+0x277/0x292
[   13.507462]        [<ffffffff810001fa>] do_one_initcall+0x3a/0x170
[   13.507464]        [<ffffffff816df85d>] kernel_init+0x11d/0x290
[   13.507466]        [<ffffffff817008ac>] ret_from_fork+0x7c/0xb0
[   13.507467] 
[   13.507467] -> #0 (console_lock){+.+.+.}:
[   13.507469]        [<ffffffff8109dccf>] __lock_acquire+0x168f/0x1d90
[   13.507471]        [<ffffffff8109e946>] lock_acquire+0x96/0xc0
[   13.507474]        [<ffffffff81048dff>] console_lock+0x6f/0x80
[   13.507475]        [<ffffffff813f6ca7>] unbind_con_driver+0x37/0x210
[   13.507477]        [<ffffffff8139dbe7>] fbcon_event_notify+0x477/0x920
[   13.507478]        [<ffffffff816fe318>] notifier_call_chain+0x58/0xb0
[   13.507479]        [<ffffffff8106d983>] __blocking_notifier_call_chain+0x53/0x80
[   13.507481]        [<ffffffff8106d9c1>] blocking_notifier_call_chain+0x11/0x20
[   13.507482]        [<ffffffff8138e816>] fb_notifier_call_chain+0x16/0x20
[   13.507484]        [<ffffffff8138f902>] do_unregister_framebuffer+0x62/0x100
[   13.507485]        [<ffffffff8138fb34>] do_remove_conflicting_framebuffers+0x154/0x180
[   13.507487]        [<ffffffff8138fe7a>] remove_conflicting_framebuffers+0x3a/0x60
[   13.507501]        [<ffffffffa02111a4>] radeon_pci_probe+0x84/0xc0 [radeon]
[   13.507503]        [<ffffffff813745c6>] local_pci_probe+0x46/0x80
[   13.507505]        [<ffffffff81375e19>] pci_device_probe+0xf9/0x120
[   13.507506]        [<ffffffff81410e96>] driver_probe_device+0x76/0x240
[   13.507507]        [<ffffffff81411103>] __driver_attach+0xa3/0xb0
[   13.507508]        [<ffffffff8140f2f6>] bus_for_each_dev+0x56/0x90
[   13.507510]        [<ffffffff814109f9>] driver_attach+0x19/0x20
[   13.507511]        [<ffffffff81410598>] bus_add_driver+0x188/0x270
[   13.507512]        [<ffffffff81411635>] driver_register+0x75/0x150
[   13.507514]        [<ffffffff81374e2f>] __pci_register_driver+0x5f/0x70
[   13.507520]        [<ffffffffa009773a>] drm_pci_init+0x11a/0x130 [drm]
[   13.507528]        [<ffffffffa02f30ec>] radeon_init+0xec/0x1000 [radeon]
[   13.507530]        [<ffffffff810001fa>] do_one_initcall+0x3a/0x170
[   13.507532]        [<ffffffff810a9fd2>] load_module+0x1a52/0x2020
[   13.507533]        [<ffffffff810aa671>] sys_init_module+0xd1/0x100
[   13.507535]        [<ffffffff81700952>] system_call_fastpath+0x16/0x1b
[   13.507535] 
[   13.507535] other info that might help us debug this:
[   13.507535] 
[   13.507535]  Possible unsafe locking scenario:
[   13.507535] 
[   13.507536]        CPU0                    CPU1
[   13.507536]        ----                    ----
[   13.507537]   lock((fb_notifier_list).rwsem);
[   13.507538]                                lock(console_lock);
[   13.507538]                                lock((fb_notifier_list).rwsem);
[   13.507539]   lock(console_lock);
[   13.507539] 
[   13.507539]  *** DEADLOCK ***
[   13.507539] 
[   13.507540] 5 locks held by modprobe/554:
[   13.507543]  #0:  (&__lockdep_no_validate__){......}, at: [<ffffffff814110b3>] __driver_attach+0x53/0xb0
[   13.507545]  #1:  (&__lockdep_no_validate__){......}, at: [<ffffffff814110c1>] __driver_attach+0x61/0xb0
[   13.507547]  #2:  (registration_lock){+.+.+.}, at: [<ffffffff8138fe6b>] remove_conflicting_framebuffers+0x2b/0x60
[   13.507550]  #3:  (&fb_info->lock){+.+.+.}, at: [<ffffffff8138ecd1>] lock_fb_info+0x21/0x60
[   13.507552]  #4:  ((fb_notifier_list).rwsem){.+.+.+}, at: [<ffffffff8106d96d>] __blocking_notifier_call_chain+0x3d/0x80
[   13.507552] 
[   13.507552] stack backtrace:
[   13.507554] Pid: 554, comm: modprobe Not tainted 3.8.0-rc3-patser+ #910
[   13.507554] Call Trace:
[   13.507557]  [<ffffffff816efd4e>] print_circular_bug+0x1fb/0x20c
[   13.507559]  [<ffffffff8109dccf>] __lock_acquire+0x168f/0x1d90
[   13.507561]  [<ffffffff8109b532>] ? mark_held_locks+0x82/0x130
[   13.507563]  [<ffffffff8109e946>] lock_acquire+0x96/0xc0
[   13.507565]  [<ffffffff813f6ca7>] ? unbind_con_driver+0x37/0x210
[   13.507566]  [<ffffffff8109b7cd>] ? trace_hardirqs_on+0xd/0x10
[   13.507568]  [<ffffffff81048dff>] console_lock+0x6f/0x80
[   13.507570]  [<ffffffff813f6ca7>] ? unbind_con_driver+0x37/0x210
[   13.507571]  [<ffffffff813f6ca7>] unbind_con_driver+0x37/0x210
[   13.507573]  [<ffffffff8109e953>] ? lock_acquire+0xa3/0xc0
[   13.507574]  [<ffffffff8139dbe7>] fbcon_event_notify+0x477/0x920
[   13.507576]  [<ffffffff816fe318>] notifier_call_chain+0x58/0xb0
[   13.507577]  [<ffffffff8106d983>] __blocking_notifier_call_chain+0x53/0x80
[   13.507579]  [<ffffffff8106d9c1>] blocking_notifier_call_chain+0x11/0x20
[   13.507580]  [<ffffffff8138e816>] fb_notifier_call_chain+0x16/0x20
[   13.507582]  [<ffffffff8138f902>] do_unregister_framebuffer+0x62/0x100
[   13.507584]  [<ffffffff8138fb34>] do_remove_conflicting_framebuffers+0x154/0x180
[   13.507586]  [<ffffffff8138fe7a>] remove_conflicting_framebuffers+0x3a/0x60
[   13.507597]  [<ffffffffa02111a4>] radeon_pci_probe+0x84/0xc0 [radeon]
[   13.507598]  [<ffffffff813745c6>] local_pci_probe+0x46/0x80
[   13.507600]  [<ffffffff81375e19>] pci_device_probe+0xf9/0x120
[   13.507601]  [<ffffffff81410e96>] driver_probe_device+0x76/0x240
[   13.507603]  [<ffffffff81411103>] __driver_attach+0xa3/0xb0
[   13.507604]  [<ffffffff81411060>] ? driver_probe_device+0x240/0x240
[   13.507606]  [<ffffffff8140f2f6>] bus_for_each_dev+0x56/0x90
[   13.507607]  [<ffffffff814109f9>] driver_attach+0x19/0x20
[   13.507608]  [<ffffffff81410598>] bus_add_driver+0x188/0x270
[   13.507610]  [<ffffffff81411635>] driver_register+0x75/0x150
[   13.507612]  [<ffffffff81374e2f>] __pci_register_driver+0x5f/0x70
[   13.507617]  [<ffffffffa009773a>] drm_pci_init+0x11a/0x130 [drm]
[   13.507619]  [<ffffffffa02f3000>] ? 0xffffffffa02f2fff
[   13.507620]  [<ffffffffa02f3000>] ? 0xffffffffa02f2fff
[   13.507628]  [<ffffffffa02f30ec>] radeon_init+0xec/0x1000 [radeon]
[   13.507630]  [<ffffffff810001fa>] do_one_initcall+0x3a/0x170
[   13.507632]  [<ffffffff810a9fd2>] load_module+0x1a52/0x2020
[   13.507633]  [<ffffffff810a71b0>] ? get_modinfo.isra.30+0xc0/0xc0
[   13.507635]  [<ffffffff8135166e>] ? trace_hardirqs_on_thunk+0x3a/0x3f
[   13.507637]  [<ffffffff810aa671>] sys_init_module+0xd1/0x100
[   13.507639]  [<ffffffff81700952>] system_call_fastpath+0x16/0x1b

<...>

[   62.505111] INFO: task plymouthd:293 blocked for more than 30 seconds.
[   62.505113] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[   62.505122] plymouthd       D 0000000000000246     0   293      1 0x00000000
[   62.505130]  ffff88016a293b18 0000000000000046 0000000000000000 0000000000000000
[   62.505135]  ffff88015ce12040 ffff88016a293fd8 ffff88016a293fd8 ffff88016a293fd8
[   62.505141]  ffff880153c66040 ffff88015ce12040 ffffffff81c30360 ffffffffa00bb1e0
[   62.505142] Call Trace:
[   62.505162]  [<ffffffff816f9224>] schedule+0x24/0x70
[   62.505178]  [<ffffffff816f9543>] schedule_preempt_disabled+0x13/0x20
[   62.505180]  [<ffffffff816f5de9>] mutex_lock_nested+0x179/0x390
[   62.505187]  [<ffffffffa00907f4>] ? drm_stub_open+0x54/0x170 [drm]
[   62.505193]  [<ffffffffa00907f4>] drm_stub_open+0x54/0x170 [drm]
[   62.505195]  [<ffffffff8113739c>] chrdev_open+0x9c/0x1a0
[   62.505197]  [<ffffffff81137300>] ? cdev_put+0x30/0x30
[   62.505200]  [<ffffffff81130cae>] do_dentry_open+0x26e/0x310
[   62.505201]  [<ffffffff81130eb0>] finish_open+0x30/0x40
[   62.505204]  [<ffffffff81140fee>] do_last+0x74e/0xea0
[   62.505206]  [<ffffffff8113e00d>] ? link_path_walk+0x23d/0x920
[   62.505208]  [<ffffffff811417ee>] path_openat+0xae/0x4c0
[   62.505210]  [<ffffffff8114237d>] do_filp_open+0x3d/0xa0
[   62.505213]  [<ffffffff8114fd0f>] ? __alloc_fd+0xcf/0x120
[   62.505215]  [<ffffffff811321f9>] do_sys_open+0xf9/0x1e0
[   62.505216]  [<ffffffff811322fc>] sys_open+0x1c/0x20
[   62.505218]  [<ffffffff81700952>] system_call_fastpath+0x16/0x1b
[   62.505219] INFO: lockdep is turned off.
[   62.505221] INFO: task modprobe:554 blocked for more than 30 seconds.
[   62.505221] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[   62.505223] modprobe        D 0000000000000000     0   554    539 0x00000002
[   62.505225]  ffff88015cf155a8 0000000000000046 0000000000000000 ffffffff81070dad
[   62.505227]  ffff88016a284040 ffff88015cf15fd8 ffff88015cf15fd8 ffff88015cf15fd8
[   62.505228]  ffffffff81c13440 ffff88016a284040 ffff88016fdfdf48 ffffffff81c220e0
[   62.505228] Call Trace:
[   62.505231]  [<ffffffff81070dad>] ? __wake_up+0x2d/0x70
[   62.505233]  [<ffffffff816f9224>] schedule+0x24/0x70
[   62.505234]  [<ffffffff816f57e5>] schedule_timeout+0x175/0x1b0
[   62.505237]  [<ffffffff81009996>] ? native_sched_clock+0x26/0x90
[   62.505239]  [<ffffffff816f84ee>] __down_common+0x97/0xe8
[   62.505241]  [<ffffffff816f859e>] __down+0x18/0x1a
[   62.505242]  [<ffffffff8106d81c>] down+0x3c/0x50
[   62.505246]  [<ffffffff81048db7>] console_lock+0x27/0x80
[   62.505248]  [<ffffffff8139b196>] set_con2fb_map+0x116/0x4c0
[   62.505250]  [<ffffffff8139d9bb>] fbcon_event_notify+0x24b/0x920
[   62.505252]  [<ffffffff816fe318>] notifier_call_chain+0x58/0xb0
[   62.505254]  [<ffffffff8106d983>] __blocking_notifier_call_chain+0x53/0x80
[   62.505255]  [<ffffffff8106d9c1>] blocking_notifier_call_chain+0x11/0x20
[   62.505257]  [<ffffffff8138e816>] fb_notifier_call_chain+0x16/0x20
[   62.505259]  [<ffffffff8138fd23>] register_framebuffer+0x1c3/0x2e0
[   62.505262]  [<ffffffffa0115e7b>] drm_fb_helper_single_fb_probe+0x1eb/0x320 [drm_kms_helper]
[   62.505265]  [<ffffffffa0116183>] drm_fb_helper_initial_config+0x1d3/0x260 [drm_kms_helper]
[   62.505268]  [<ffffffff8109b7cd>] ? trace_hardirqs_on+0xd/0x10
[   62.505288]  [<ffffffffa0253e1a>] radeon_fbdev_init+0xaa/0xf0 [radeon]
[   62.505301]  [<ffffffffa024e95b>] radeon_modeset_init+0x37b/0xa50 [radeon]
[   62.505313]  [<ffffffffa022b5c8>] radeon_driver_load_kms+0xe8/0x150 [radeon]
[   62.505319]  [<ffffffffa00974f9>] drm_get_pci_dev+0x179/0x2a0 [drm]
[   62.505331]  [<ffffffffa02111be>] radeon_pci_probe+0x9e/0xc0 [radeon]
[   62.505333]  [<ffffffff813745c6>] local_pci_probe+0x46/0x80
[   62.505335]  [<ffffffff81375e19>] pci_device_probe+0xf9/0x120
[   62.505337]  [<ffffffff81410e96>] driver_probe_device+0x76/0x240
[   62.505339]  [<ffffffff81411103>] __driver_attach+0xa3/0xb0
[   62.505341]  [<ffffffff81411060>] ? driver_probe_device+0x240/0x240
[   62.505342]  [<ffffffff8140f2f6>] bus_for_each_dev+0x56/0x90
[   62.505344]  [<ffffffff814109f9>] driver_attach+0x19/0x20
[   62.505345]  [<ffffffff81410598>] bus_add_driver+0x188/0x270
[   62.505347]  [<ffffffff81411635>] driver_register+0x75/0x150
[   62.505349]  [<ffffffff81374e2f>] __pci_register_driver+0x5f/0x70
[   62.505354]  [<ffffffffa009773a>] drm_pci_init+0x11a/0x130 [drm]
[   62.505356]  [<ffffffffa02f3000>] ? 0xffffffffa02f2fff
[   62.505358]  [<ffffffffa02f3000>] ? 0xffffffffa02f2fff
[   62.505367]  [<ffffffffa02f30ec>] radeon_init+0xec/0x1000 [radeon]
[   62.505369]  [<ffffffff810001fa>] do_one_initcall+0x3a/0x170
[   62.505372]  [<ffffffff810a9fd2>] load_module+0x1a52/0x2020
[   62.505373]  [<ffffffff810a71b0>] ? get_modinfo.isra.30+0xc0/0xc0
[   62.505375]  [<ffffffff8135166e>] ? trace_hardirqs_on_thunk+0x3a/0x3f
[   62.505377]  [<ffffffff810aa671>] sys_init_module+0xd1/0x100
[   62.505379]  [<ffffffff81700952>] system_call_fastpath+0x16/0x1b

The EFI driver is known to need other work of its own.

Alan