fb: Rework locking to fix lock ordering on takeover
diff mbox series

Message ID 20121116192606.11799.35711.stgit@localhost.localdomain
State New, archived
Headers show
Series
  • fb: Rework locking to fix lock ordering on takeover
Related show

Commit Message

Alan Cox Nov. 16, 2012, 7:27 p.m. UTC
[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 <alan@linux.intel.com>

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


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

Comments

Josh Boyer Nov. 21, 2012, 12:45 p.m. UTC | #1
On Fri, Nov 16, 2012 at 2:27 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>
> [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 <alan@linux.intel.com>
>
> 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>

Should this eventually get into the stable trees?

josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Alan Cox Nov. 21, 2012, 12:53 p.m. UTC | #2
On Wed, 21 Nov 2012 07:45:45 -0500
Josh Boyer <jwboyer@gmail.com> wrote:

> On Fri, Nov 16, 2012 at 2:27 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> >
> > [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 <alan@linux.intel.com>
> >
> > 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>
> 
> Should this eventually get into the stable trees?

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Josh Boyer Dec. 18, 2012, 3:20 p.m. UTC | #3
On Wed, Nov 21, 2012 at 7:53 AM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> On Wed, 21 Nov 2012 07:45:45 -0500
> Josh Boyer <jwboyer@gmail.com> wrote:
>
>> On Fri, Nov 16, 2012 at 2:27 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>> >
>> > [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 <alan@linux.intel.com>
>> >
>> > 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>
>>
>> Should this eventually get into the stable trees?
>
> 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.

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Sasha Levin Dec. 25, 2012, 4:08 p.m. UTC | #4
On Tue, Dec 18, 2012 at 10:20 AM, Josh Boyer <jwboyer@gmail.com> wrote:
> On Wed, Nov 21, 2012 at 7:53 AM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>> On Wed, 21 Nov 2012 07:45:45 -0500
>> Josh Boyer <jwboyer@gmail.com> wrote:
>>
>>> On Fri, Nov 16, 2012 at 2:27 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>>> >
>>> > [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 <alan@linux.intel.com>
>>> >
>>> > 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>
>>>
>>> Should this eventually get into the stable trees?
>>
>> 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.
>
> 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?

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


Thanks,
Sasha
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Cong Wang Dec. 26, 2012, 2:41 a.m. UTC | #5
On Wed, Dec 26, 2012 at 12:08 AM, Sasha Levin <levinsasha928@gmail.com> wrote:
> On Tue, Dec 18, 2012 at 10:20 AM, Josh Boyer <jwboyer@gmail.com> wrote:
>> On Wed, Nov 21, 2012 at 7:53 AM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>>> On Wed, 21 Nov 2012 07:45:45 -0500
>>> Josh Boyer <jwboyer@gmail.com> wrote:
>>>
>>>> On Fri, Nov 16, 2012 at 2:27 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>>>> >
>>>> > [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 <alan@linux.intel.com>
>>>> >
>>>> > 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>
>>>>
>>>> Should this eventually get into the stable trees?
>>>
>>> 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.
>>
>> 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?
>
> Indeed it should. I'm seeing the original warnings in 3.8-rc1 and have
> to carry this patch to avoid them.

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

I will give it a try.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Sasha Levin Dec. 26, 2012, 6:09 p.m. UTC | #6
On Tue, Dec 25, 2012 at 9:41 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Wed, Dec 26, 2012 at 12:08 AM, Sasha Levin <levinsasha928@gmail.com> wrote:
>> On Tue, Dec 18, 2012 at 10:20 AM, Josh Boyer <jwboyer@gmail.com> wrote:
>>> On Wed, Nov 21, 2012 at 7:53 AM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>>>> On Wed, 21 Nov 2012 07:45:45 -0500
>>>> Josh Boyer <jwboyer@gmail.com> wrote:
>>>>
>>>>> On Fri, Nov 16, 2012 at 2:27 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>>>>> >
>>>>> > [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 <alan@linux.intel.com>
>>>>> >
>>>>> > 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>
>>>>>
>>>>> Should this eventually get into the stable trees?
>>>>
>>>> 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.
>>>
>>> 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?
>>
>> Indeed it should. I'm seeing the original warnings in 3.8-rc1 and have
>> to carry this patch to avoid them.
>
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Borislav Petkov Dec. 27, 2012, 4:53 a.m. UTC | #7
On Wed, Dec 26, 2012 at 01:09:51PM -0500, Sasha Levin wrote:
> > 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.

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
Shawn Guo Dec. 28, 2012, 11:50 a.m. UTC | #8
On Thu, Dec 27, 2012 at 05:53:01AM +0100, Borislav Petkov wrote:
> On Wed, Dec 26, 2012 at 01:09:51PM -0500, Sasha Levin wrote:
> > > 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.
> 
> Adding to the bug pressure: just got a very similar splat on -rc1 (see
> below). Alan, I'll run your patch to verify.
> 
+1

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

Shawn

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Borislav Petkov Dec. 28, 2012, 12:40 p.m. UTC | #9
On Fri, Dec 28, 2012 at 07:50:27PM +0800, Shawn Guo wrote:
> +1
> 
> http://thread.gmane.org/gmane.linux.kernel/1413953/focus=1415070

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.

Tested-by: Borislav Petkov <bp@alien8.de>

Thanks.
Alexander Holler Jan. 4, 2013, 12:50 p.m. UTC | #10
Am 28.12.2012 13:40, schrieb Borislav Petkov:
> On Fri, Dec 28, 2012 at 07:50:27PM +0800, Shawn Guo wrote:
>> +1
>>
>> http://thread.gmane.org/gmane.linux.kernel/1413953/focus=1415070
>
> 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.


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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Alan Cox Jan. 4, 2013, 1:25 p.m. UTC | #11
On Fri, 04 Jan 2013 13:50:37 +0100
Alexander Holler <holler@ahsoftware.de> wrote:

> Am 28.12.2012 13:40, schrieb Borislav Petkov:
> > On Fri, Dec 28, 2012 at 07:50:27PM +0800, Shawn Guo wrote:
> >> +1
> >>
> >> http://thread.gmane.org/gmane.linux.kernel/1413953/focus=1415070
> >
> > 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.
> 
> 
> 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):

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Alexander Holler Jan. 4, 2013, 1:36 p.m. UTC | #12
Am 04.01.2013 14:25, schrieb Alan Cox:
> On Fri, 04 Jan 2013 13:50:37 +0100
> Alexander Holler <holler@ahsoftware.de> wrote:
>
>> Am 28.12.2012 13:40, schrieb Borislav Petkov:
>>> On Fri, Dec 28, 2012 at 07:50:27PM +0800, Shawn Guo wrote:
>>>> +1
>>>>
>>>> http://thread.gmane.org/gmane.linux.kernel/1413953/focus=1415070
>>>
>>> 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.
>>
>>
>> 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):
>
> They are broken if used as the system console (has been known for years).

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

> Perhaps your x86 test has the system console still on another device ?

Exactly thats the case. Thanks for pointing it out.

> 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

That is what I wanted to try next. ;)

> 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.

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Alexander Holler Jan. 5, 2013, 11:41 a.m. UTC | #13
Am 04.01.2013 14:36, schrieb Alexander Holler:
> Am 04.01.2013 14:25, schrieb Alan Cox:
>> On Fri, 04 Jan 2013 13:50:37 +0100
>> Alexander Holler <holler@ahsoftware.de> wrote:

...
>>> 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):
>>
>> They are broken if used as the system console (has been known for years).

>> 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
>
> That is what I wanted to try next. ;)
>
>> 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.
>
> 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.

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Alexander Holler Jan. 5, 2013, 12:06 p.m. UTC | #14
Am 05.01.2013 13:07, schrieb Alan Cox:
>> 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.
>
> I was thinking much higher level - ie at the printk kind of level
>
>> 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.
>
> Looks pretty clean as a solution to me.

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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Alan Cox Jan. 5, 2013, 12:07 p.m. UTC | #15
> 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.

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

> 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.

Looks pretty clean as a solution to me.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Jiri Kosina Jan. 7, 2013, 9:37 a.m. UTC | #16
On Fri, 28 Dec 2012, Borislav Petkov wrote:

> > http://thread.gmane.org/gmane.linux.kernel/1413953/focus=1415070
> 
> 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.
> 
> Tested-by: Borislav Petkov <bp@alien8.de>

Still seeing it with current Linus' tree.
  
        Tested-by: Jiri Kosina <jkosina@suse.cz>
  
Anyone applying this, please?
Borislav Petkov Jan. 12, 2013, 6:36 p.m. UTC | #17
On Mon, Jan 07, 2013 at 10:37:15AM +0100, Jiri Kosina wrote:
> Still seeing it with current Linus' tree.
>   
>         Tested-by: Jiri Kosina <jkosina@suse.cz>
>   
> Anyone applying this, please?

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.
Linus Torvalds Jan. 12, 2013, 8:51 p.m. UTC | #18
On Sat, Jan 12, 2013 at 10:36 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Mon, Jan 07, 2013 at 10:37:15AM +0100, Jiri Kosina wrote:
>> Still seeing it with current Linus' tree.
>>
>>         Tested-by: Jiri Kosina <jkosina@suse.cz>
>>
>> Anyone applying this, please?
>
> Yeah, I'm still seeing it in -rc3. Linus, can you pick up Alan's patch
> from https://patchwork.kernel.org/patch/1757061/ please?

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Alan Cox Jan. 12, 2013, 9:05 p.m. UTC | #19
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Andrew Morton Jan. 12, 2013, 9:13 p.m. UTC | #20
On Sat, 12 Jan 2013 12:51:49 -0800 Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Sat, Jan 12, 2013 at 10:36 AM, Borislav Petkov <bp@alien8.de> wrote:
> > On Mon, Jan 07, 2013 at 10:37:15AM +0100, Jiri Kosina wrote:
> >> Still seeing it with current Linus' tree.
> >>
> >>         Tested-by: Jiri Kosina <jkosina@suse.cz>
> >>
> >> Anyone applying this, please?
> >
> > Yeah, I'm still seeing it in -rc3. Linus, can you pick up Alan's patch
> > from https://patchwork.kernel.org/patch/1757061/ please?
> 
> 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.
> 

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 <alan@linux.intel.com>
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.

[akpm@linux-foundation.org: remove stray non-ascii char, tidy comment]
[akpm@linux-foundation.org: export do_take_over_console()]
Signed-off-by: Alan Cox <alan@linux.intel.com>
Cc: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
Cc: Stephen Rothwell <sfr@canb.auug.org.au>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 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
--- a/drivers/tty/vt/vt.c~fb-rework-locking-to-fix-lock-ordering-on-takeover
+++ a/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
--- a/drivers/video/console/fbcon.c~fb-rework-locking-to-fix-lock-ordering-on-takeover
+++ a/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
--- a/drivers/video/fbmem.c~fb-rework-locking-to-fix-lock-ordering-on-takeover
+++ a/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
--- a/drivers/video/fbsysfs.c~fb-rework-locking-to-fix-lock-ordering-on-takeover
+++ a/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
--- a/include/linux/console.h~fb-rework-locking-to-fix-lock-ordering-on-takeover
+++ a/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);
Borislav Petkov Jan. 13, 2013, 12:02 a.m. UTC | #21
On Sat, Jan 12, 2013 at 01:13:23PM -0800, Andrew Morton wrote:
> 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 <alan@linux.intel.com>
> 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.
> 
> [akpm@linux-foundation.org: remove stray non-ascii char, tidy comment]
> [akpm@linux-foundation.org: export do_take_over_console()]
> Signed-off-by: Alan Cox <alan@linux.intel.com>
> Cc: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
> Cc: Stephen Rothwell <sfr@canb.auug.org.au>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

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.
Maarten Lankhorst Jan. 15, 2013, 12:06 p.m. UTC | #22
Hey,

Op 13-01-13 01:02, Borislav Petkov schreef:
> On Sat, Jan 12, 2013 at 01:13:23PM -0800, Andrew Morton wrote:
>> 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 <alan@linux.intel.com>
>> 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.
>>
>> [akpm@linux-foundation.org: remove stray non-ascii char, tidy comment]
>> [akpm@linux-foundation.org: export do_take_over_console()]
>> Signed-off-by: Alan Cox <alan@linux.intel.com>
>> Cc: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
>> Cc: Stephen Rothwell <sfr@canb.auug.org.au>
>> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> 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.
>
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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Alan Cox Jan. 15, 2013, 3:12 p.m. UTC | #23
> 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

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

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Patch
diff mbox series

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