* Re: Possible deadlock when suspending framebuffer
[not found] <BANLkTiko+2AM8dq8jP5xjbTcCM63hBAK=A@mail.gmail.com>
@ 2011-06-14 18:15 ` Linus Torvalds
2011-06-14 19:04 ` Florian Tobias Schandinat
2011-06-15 1:09 ` re:Possible deadlock when suspending framebuffer Wanlong Gao
0 siblings, 2 replies; 19+ messages in thread
From: Linus Torvalds @ 2011-06-14 18:15 UTC (permalink / raw)
To: Paul Mundt, linux-fbdev; +Cc: Linux Kernel Mailing List, Francis Moreau
Paul, fbdev people.. Comments? This was sent to me and lkml, the right
people probably didn't see it.
I doubt it's a big problem in practice, but..
Linus
On Tue, Jun 14, 2011 at 6:10 AM, Francis Moreau <francis.moro@gmail.com> wrote:
> Hello,
>
> I noticed that a possible deadlock can happen when the current frame
> buffering is being suspended and a new frambuffer device is being
> registred at the same time.
>
> When suspending the current frambuffer by doing : echo 1
>>/sys/class/graphics/fb0/state, the kernel actually takes the
> following locks in that order: console_lock, lock_fb_info (see
> store_fbstate()).
>
> However when a new framebuffer is coming in, the lock sequence is:
> lock_fb_info (taken by do_remove_conflicting_framebuffer()),
> console_lock() (taken by unbind_console).
>
> I don't know how this should be fixed though...
>
> Thanks
> --
> Francis
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: Possible deadlock when suspending framebuffer
2011-06-14 18:15 ` Possible deadlock when suspending framebuffer Linus Torvalds
@ 2011-06-14 19:04 ` Florian Tobias Schandinat
2011-06-17 18:46 ` [PATCH] fb: avoid possible deadlock caused by fb_set_suspend Florian Tobias Schandinat
2011-06-15 1:09 ` re:Possible deadlock when suspending framebuffer Wanlong Gao
1 sibling, 1 reply; 19+ messages in thread
From: Florian Tobias Schandinat @ 2011-06-14 19:04 UTC (permalink / raw)
To: Linus Torvalds
Cc: Paul Mundt, linux-fbdev, Linux Kernel Mailing List,
Francis Moreau
Hi Linus,
On 06/14/2011 06:15 PM, Linus Torvalds wrote:
> Paul, fbdev people.. Comments? This was sent to me and lkml, the right
> people probably didn't see it.
Sounds very familiar. Indeed a quick glance at my archive revealed 2
approaches/patches dealing with similar issues
http://marc.info/?l=linux-fbdev&m\x129539210207429&w=2
http://marc.info/?l=linux-fbdev&m\x129700789632450&w=2
I did not have time to verify that those do actually get those things right
everywhere but the first one looks good and should also solve this issue I think.
Regards,
Florian Tobias Schandinat
>
> I doubt it's a big problem in practice, but..
>
> Linus
>
> On Tue, Jun 14, 2011 at 6:10 AM, Francis Moreau<francis.moro@gmail.com> wrote:
>> Hello,
>>
>> I noticed that a possible deadlock can happen when the current frame
>> buffering is being suspended and a new frambuffer device is being
>> registred at the same time.
>>
>> When suspending the current frambuffer by doing : echo 1
>>> /sys/class/graphics/fb0/state, the kernel actually takes the
>> following locks in that order: console_lock, lock_fb_info (see
>> store_fbstate()).
>>
>> However when a new framebuffer is coming in, the lock sequence is:
>> lock_fb_info (taken by do_remove_conflicting_framebuffer()),
>> console_lock() (taken by unbind_console).
>>
>> I don't know how this should be fixed though...
>>
>> Thanks
>> --
>> Francis
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* re:Possible deadlock when suspending framebuffer
2011-06-14 18:15 ` Possible deadlock when suspending framebuffer Linus Torvalds
2011-06-14 19:04 ` Florian Tobias Schandinat
@ 2011-06-15 1:09 ` Wanlong Gao
2011-06-15 5:58 ` Possible " Bruno Prémont
1 sibling, 1 reply; 19+ messages in thread
From: Wanlong Gao @ 2011-06-15 1:09 UTC (permalink / raw)
To: Francis Moreau
Cc: Paul Mundt, linux-fbdev, Linux Kernel Mailing List,
Linus Torvalds
[-- Attachment #1: Type: text/plain, Size: 884 bytes --]
<snip>
Hi Francis:
can you test this patch?
Thanks
From fe026c42af4cbdce053460a428a445e99071586a Mon Sep 17 00:00:00 2001
From: Wanlong Gao <wanlong.gao@gmail.com>
Date: Wed, 15 Jun 2011 09:03:41 +0800
Subject: [PATCH] test
Signed-off-by: Wanlong Gao <wanlong.gao@gmail.com>
---
drivers/video/fbmem.c | 3 ---
1 files changed, 0 insertions(+), 3 deletions(-)
diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index 5aac00e..6e6cef3 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -1642,11 +1642,8 @@ static int do_unregister_framebuffer(struct
fb_info *fb_info)
if (i < 0 || i >= FB_MAX || registered_fb[i] != fb_info)
return -EINVAL;
- if (!lock_fb_info(fb_info))
- return -ENODEV;
event.info = fb_info;
ret = fb_notifier_call_chain(FB_EVENT_FB_UNBIND, &event);
- unlock_fb_info(fb_info);
if (ret)
return -EINVAL;
--
1.7.4.1
[-- Attachment #2: 0001-test.patch --]
[-- Type: text/x-patch, Size: 835 bytes --]
From fe026c42af4cbdce053460a428a445e99071586a Mon Sep 17 00:00:00 2001
From: Wanlong Gao <wanlong.gao@gmail.com>
Date: Wed, 15 Jun 2011 09:03:41 +0800
Subject: [PATCH] test
test
Signed-off-by: Wanlong Gao <wanlong.gao@gmail.com>
---
drivers/video/fbmem.c | 3 ---
1 files changed, 0 insertions(+), 3 deletions(-)
diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index 5aac00e..6e6cef3 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -1642,11 +1642,8 @@ static int do_unregister_framebuffer(struct fb_info *fb_info)
if (i < 0 || i >= FB_MAX || registered_fb[i] != fb_info)
return -EINVAL;
- if (!lock_fb_info(fb_info))
- return -ENODEV;
event.info = fb_info;
ret = fb_notifier_call_chain(FB_EVENT_FB_UNBIND, &event);
- unlock_fb_info(fb_info);
if (ret)
return -EINVAL;
--
1.7.4.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: Possible deadlock when suspending framebuffer
2011-06-15 1:09 ` re:Possible deadlock when suspending framebuffer Wanlong Gao
@ 2011-06-15 5:58 ` Bruno Prémont
2011-06-15 6:22 ` Wanlong Gao
2011-06-15 7:12 ` Francis Moreau
0 siblings, 2 replies; 19+ messages in thread
From: Bruno Prémont @ 2011-06-15 5:58 UTC (permalink / raw)
To: Wanlong Gao
Cc: Francis Moreau, Paul Mundt, linux-fbdev,
Linux Kernel Mailing List, Linus Torvalds
Hi,
On Wed, 15 Jun 2011 09:09:24 Wanlong Gao <wanlong.gao@gmail.com> wrote:
> <snip>
> Hi Francis:
> can you test this patch?
Do you have a deadlock trace which you are trying to fix?
It's either the caller of unregister_framebuffer() which must be
changed to not call unregister_framebuffer with info's lock held or
the code reacting on the notification that must not try to acquire the
lock again.
The interesting par is if console semaphore has some relation to this
deadlock as the order for taking both varies... It could be
lock_fb_info(); console_lock() versus console_lock(); lock_fb_info()
Bruno
> Thanks
>
> From fe026c42af4cbdce053460a428a445e99071586a Mon Sep 17 00:00:00 2001
> From: Wanlong Gao <wanlong.gao@gmail.com>
> Date: Wed, 15 Jun 2011 09:03:41 +0800
> Subject: [PATCH] test
>
>
>
> Signed-off-by: Wanlong Gao <wanlong.gao@gmail.com>
> ---
> drivers/video/fbmem.c | 3 ---
> 1 files changed, 0 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
> index 5aac00e..6e6cef3 100644
> --- a/drivers/video/fbmem.c
> +++ b/drivers/video/fbmem.c
> @@ -1642,11 +1642,8 @@ static int do_unregister_framebuffer(struct
> fb_info *fb_info)
> if (i < 0 || i >= FB_MAX || registered_fb[i] != fb_info)
> return -EINVAL;
>
> - if (!lock_fb_info(fb_info))
> - return -ENODEV;
> event.info = fb_info;
> ret = fb_notifier_call_chain(FB_EVENT_FB_UNBIND, &event);
> - unlock_fb_info(fb_info);
Not a good idea to stop taking fb_lock here.
Pretty all calls of fb_notifier_call_chain are protected by info's
lock, except the one for FB_EVENT_FB_UNREGISTERED a few lines further.
IMHO it wou make sense to add the lock around that last one so all
notifier chain calls are handled the same.
> if (ret)
> return -EINVAL;
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: Possible deadlock when suspending framebuffer
2011-06-15 5:58 ` Possible " Bruno Prémont
@ 2011-06-15 6:22 ` Wanlong Gao
2011-06-15 7:04 ` Américo Wang
2011-06-15 7:12 ` Francis Moreau
1 sibling, 1 reply; 19+ messages in thread
From: Wanlong Gao @ 2011-06-15 6:22 UTC (permalink / raw)
To: Bruno Prémont
Cc: Francis Moreau, Paul Mundt, linux-fbdev,
Linux Kernel Mailing List, Linus Torvalds
On Wed, Jun 15, 2011 at 1:58 PM, Bruno Prémont
<bonbons@linux-vserver.org> wrote:
>
> Hi,
>
> On Wed, 15 Jun 2011 09:09:24 Wanlong Gao <wanlong.gao@gmail.com> wrote:
> > <snip>
> > Hi Francis:
> > can you test this patch?
>
> Do you have a deadlock trace which you are trying to fix?
No, I just look at the code and try to fix this but I'm not sure.
Can you teach me how to have a deadlock trace here?
Thanks
>
> It's either the caller of unregister_framebuffer() which must be
> changed to not call unregister_framebuffer with info's lock held or
> the code reacting on the notification that must not try to acquire the
> lock again.
>
> The interesting par is if console semaphore has some relation to this
> deadlock as the order for taking both varies... It could be
> lock_fb_info(); console_lock() versus console_lock(); lock_fb_info()
>
I see, thanks
> Bruno
>
>
> > Thanks
> >
><snip>
>
> Not a good idea to stop taking fb_lock here.
> Pretty all calls of fb_notifier_call_chain are protected by info's
> lock, except the one for FB_EVENT_FB_UNREGISTERED a few lines further.
Yup, thanks
>
> IMHO it wou make sense to add the lock around that last one so all
> notifier chain calls are handled the same.
>
> <snip>
>
--
Best regards
Wanlong Gao
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: Possible deadlock when suspending framebuffer
2011-06-15 6:22 ` Wanlong Gao
@ 2011-06-15 7:04 ` Américo Wang
0 siblings, 0 replies; 19+ messages in thread
From: Américo Wang @ 2011-06-15 7:04 UTC (permalink / raw)
To: wanlong.gao
Cc: Bruno Prémont, Francis Moreau, Paul Mundt, linux-fbdev,
Linux Kernel Mailing List, Linus Torvalds
On Wed, Jun 15, 2011 at 2:22 PM, Wanlong Gao <wanlong.gao@gmail.com> wrote:
> On Wed, Jun 15, 2011 at 1:58 PM, Bruno Prémont
> <bonbons@linux-vserver.org> wrote:
>>
>> Hi,
>>
>> On Wed, 15 Jun 2011 09:09:24 Wanlong Gao <wanlong.gao@gmail.com> wrote:
>> > <snip>
>> > Hi Francis:
>> > can you test this patch?
>>
>> Do you have a deadlock trace which you are trying to fix?
> No, I just look at the code and try to fix this but I'm not sure.
> Can you teach me how to have a deadlock trace here?
CONFIG_LOCKDEP=y
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: Possible deadlock when suspending framebuffer
2011-06-15 5:58 ` Possible " Bruno Prémont
2011-06-15 6:22 ` Wanlong Gao
@ 2011-06-15 7:12 ` Francis Moreau
2011-06-15 10:20 ` Bruno Prémont
1 sibling, 1 reply; 19+ messages in thread
From: Francis Moreau @ 2011-06-15 7:12 UTC (permalink / raw)
To: Bruno Prémont
Cc: Wanlong Gao, Paul Mundt, linux-fbdev, Linux Kernel Mailing List,
Linus Torvalds
On Wed, Jun 15, 2011 at 7:58 AM, Bruno Prémont
<bonbons@linux-vserver.org> wrote:
> Hi,
>
> On Wed, 15 Jun 2011 09:09:24 Wanlong Gao <wanlong.gao@gmail.com> wrote:
>> <snip>
>> Hi Francis:
>> can you test this patch?
>
> Do you have a deadlock trace which you are trying to fix?
>
> It's either the caller of unregister_framebuffer() which must be
> changed to not call unregister_framebuffer with info's lock held or
> the code reacting on the notification that must not try to acquire the
> lock again.
>
> The interesting par is if console semaphore has some relation to this
> deadlock as the order for taking both varies... It could be
> lock_fb_info(); console_lock() versus console_lock(); lock_fb_info()
>
> Bruno
>
>
>> Thanks
>>
>> From fe026c42af4cbdce053460a428a445e99071586a Mon Sep 17 00:00:00 2001
>> From: Wanlong Gao <wanlong.gao@gmail.com>
>> Date: Wed, 15 Jun 2011 09:03:41 +0800
>> Subject: [PATCH] test
>>
>>
>>
>> Signed-off-by: Wanlong Gao <wanlong.gao@gmail.com>
>> ---
>> drivers/video/fbmem.c | 3 ---
>> 1 files changed, 0 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
>> index 5aac00e..6e6cef3 100644
>> --- a/drivers/video/fbmem.c
>> +++ b/drivers/video/fbmem.c
>> @@ -1642,11 +1642,8 @@ static int do_unregister_framebuffer(struct
>> fb_info *fb_info)
>> if (i < 0 || i >= FB_MAX || registered_fb[i] != fb_info)
>> return -EINVAL;
>>
>> - if (!lock_fb_info(fb_info))
>> - return -ENODEV;
>> event.info = fb_info;
>> ret = fb_notifier_call_chain(FB_EVENT_FB_UNBIND, &event);
>> - unlock_fb_info(fb_info);
>
> Not a good idea to stop taking fb_lock here.
> Pretty all calls of fb_notifier_call_chain are protected by info's
> lock, except the one for FB_EVENT_FB_UNREGISTERED a few lines further.
>
> IMHO it wou make sense to add the lock around that last one so all
> notifier chain calls are handled the same.
>
>> if (ret)
>> return -EINVAL;
>
>
Well, sorry for the dumb question but the fb/fbcon code is pretty hard
to follow for me.
Why does store_fbstate() and any fb driver's suspsend methods acquire
the console lock at all ?
Thanks
--
Francis
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: Possible deadlock when suspending framebuffer
2011-06-15 7:12 ` Francis Moreau
@ 2011-06-15 10:20 ` Bruno Prémont
0 siblings, 0 replies; 19+ messages in thread
From: Bruno Prémont @ 2011-06-15 10:20 UTC (permalink / raw)
To: Francis Moreau
Cc: Wanlong Gao, Paul Mundt, linux-fbdev, Linux Kernel Mailing List,
Linus Torvalds
On Wed, 15 Jun 2011 09:12:46 Francis Moreau wrote:
> On Wed, Jun 15, 2011 at 7:58 AM, Bruno Prémont wrote:
> Well, sorry for the dumb question but the fb/fbcon code is pretty hard
> to follow for me.
Certainly not just for you
> Why does store_fbstate() and any fb driver's suspsend methods acquire
> the console lock at all ?
From my understanding, fbcon currently has very loose binding with
framebuffers in general instead of just with those few framebuffers
it is effectively mapped to (and active on!).
James Simmons started a complete rework of fbcon/tty code which is
expected to get things more fine-grained, until then console semaphore
(console_lock) remains a kind of big kernel lock in the
console/framebuffer area.
As such any state change of framebuffer may influence/race against
fbcon.
e.g. for the suspend state you want to avoid fbcon to fiddle with
your framebuffer while it is suspending (and have fbcon know about the
suspended state). This way fbcon can unsuspend framebuffer if needed
but also stop accessing it when it should not.
Bruno
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH] fb: avoid possible deadlock caused by fb_set_suspend
2011-06-14 19:04 ` Florian Tobias Schandinat
@ 2011-06-17 18:46 ` Florian Tobias Schandinat
2011-06-18 8:43 ` Bruno Prémont
2011-07-20 18:16 ` [PATCH] fb: avoid possible deadlock caused by fb_set_suspend Florian Tobias Schandinat
0 siblings, 2 replies; 19+ messages in thread
From: Florian Tobias Schandinat @ 2011-06-17 18:46 UTC (permalink / raw)
To: lethal, linux-fbdev
Cc: francis.moro, torvalds, bonbons, linux-kernel,
Herton Ronaldo Krzesinski, Florian Tobias Schandinat, stable
From: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
A lock ordering issue can cause deadlocks: in framebuffer/console code,
all needed struct fb_info locks are taken before acquire_console_sem(),
in places which need to take console semaphore.
But fb_set_suspend is always called with console semaphore held, and
inside it we call lock_fb_info which gets the fb_info lock, inverse
locking order of what the rest of the code does. This causes a real
deadlock issue, when we write to state fb sysfs attribute (which calls
fb_set_suspend) while a framebuffer is being unregistered by
remove_conflicting_framebuffers, as can be shown by following show
blocked state trace on a test program which loads i915 and runs another
forked processes writing to state attribute:
Test process with semaphore held and trying to get fb_info lock:
..
fb-test2 D 0000000000000000 0 237 228 0x00000000
ffff8800774f3d68 0000000000000082 00000000000135c0 00000000000135c0
ffff880000000000 ffff8800774f3fd8 ffff8800774f3fd8 ffff880076ee4530
00000000000135c0 ffff8800774f3fd8 ffff8800774f2000 00000000000135c0
Call Trace:
[<ffffffff8141287a>] __mutex_lock_slowpath+0x11a/0x1e0
[<ffffffff814142f2>] ? _raw_spin_lock_irq+0x22/0x40
[<ffffffff814123d3>] mutex_lock+0x23/0x50
[<ffffffff8125dfc5>] lock_fb_info+0x25/0x60
[<ffffffff8125e3f0>] fb_set_suspend+0x20/0x80
[<ffffffff81263e2f>] store_fbstate+0x4f/0x70
[<ffffffff812e7f70>] dev_attr_store+0x20/0x30
[<ffffffff811c46b4>] sysfs_write_file+0xd4/0x160
[<ffffffff81155a26>] vfs_write+0xc6/0x190
[<ffffffff81155d51>] sys_write+0x51/0x90
[<ffffffff8100c012>] system_call_fastpath+0x16/0x1b
..
modprobe process stalled because has the fb_info lock (got inside
unregister_framebuffer) but waiting for the semaphore held by the
test process which is waiting to get the fb_info lock:
..
modprobe D 0000000000000000 0 230 218 0x00000000
ffff880077a4d618 0000000000000082 0000000000000001 0000000000000001
ffff880000000000 ffff880077a4dfd8 ffff880077a4dfd8 ffff8800775a2e20
00000000000135c0 ffff880077a4dfd8 ffff880077a4c000 00000000000135c0
Call Trace:
[<ffffffff81411fe5>] schedule_timeout+0x215/0x310
[<ffffffff81058051>] ? get_parent_ip+0x11/0x50
[<ffffffff814130dd>] __down+0x6d/0xb0
[<ffffffff81089f71>] down+0x41/0x50
[<ffffffff810629ac>] acquire_console_sem+0x2c/0x50
[<ffffffff812ca53d>] unbind_con_driver+0xad/0x2d0
[<ffffffff8126f5f7>] fbcon_event_notify+0x457/0x890
[<ffffffff814144ff>] ? _raw_spin_unlock_irqrestore+0x1f/0x50
[<ffffffff81058051>] ? get_parent_ip+0x11/0x50
[<ffffffff8141836d>] notifier_call_chain+0x4d/0x70
[<ffffffff8108a3b8>] __blocking_notifier_call_chain+0x58/0x80
[<ffffffff8108a3f6>] blocking_notifier_call_chain+0x16/0x20
[<ffffffff8125dabb>] fb_notifier_call_chain+0x1b/0x20
[<ffffffff8125e6ac>] unregister_framebuffer+0x7c/0x130
[<ffffffff8125e8b3>] remove_conflicting_framebuffers+0x153/0x180
[<ffffffff8125eef3>] register_framebuffer+0x93/0x2c0
[<ffffffffa0331112>] drm_fb_helper_single_fb_probe+0x252/0x2f0 [drm_kms_helper]
[<ffffffffa03314a3>] drm_fb_helper_initial_config+0x2f3/0x6d0 [drm_kms_helper]
[<ffffffffa03318dd>] ? drm_fb_helper_single_add_all_connectors+0x5d/0x1c0 [drm_kms_helper]
[<ffffffffa037b588>] intel_fbdev_init+0xa8/0x160 [i915]
[<ffffffffa0343d74>] i915_driver_load+0x854/0x12b0 [i915]
[<ffffffffa02f0e7e>] drm_get_pci_dev+0x19e/0x360 [drm]
[<ffffffff8141821d>] ? sub_preempt_count+0x9d/0xd0
[<ffffffffa0386f91>] i915_pci_probe+0x15/0x17 [i915]
[<ffffffff8124481f>] local_pci_probe+0x5f/0xd0
[<ffffffff81244f89>] pci_device_probe+0x119/0x120
[<ffffffff812eccaa>] ? driver_sysfs_add+0x7a/0xb0
[<ffffffff812ed003>] driver_probe_device+0xa3/0x290
[<ffffffff812ed1f0>] ? __driver_attach+0x0/0xb0
[<ffffffff812ed29b>] __driver_attach+0xab/0xb0
[<ffffffff812ed1f0>] ? __driver_attach+0x0/0xb0
[<ffffffff812ebd3e>] bus_for_each_dev+0x5e/0x90
[<ffffffff812ecc2e>] driver_attach+0x1e/0x20
[<ffffffff812ec6f2>] bus_add_driver+0xe2/0x320
[<ffffffffa03aa000>] ? i915_init+0x0/0x96 [i915]
[<ffffffff812ed536>] driver_register+0x76/0x140
[<ffffffffa03aa000>] ? i915_init+0x0/0x96 [i915]
[<ffffffff81245216>] __pci_register_driver+0x56/0xd0
[<ffffffffa02f1264>] drm_pci_init+0xe4/0xf0 [drm]
[<ffffffffa03aa000>] ? i915_init+0x0/0x96 [i915]
[<ffffffffa02e84a8>] drm_init+0x58/0x70 [drm]
[<ffffffffa03aa094>] i915_init+0x94/0x96 [i915]
[<ffffffff81002194>] do_one_initcall+0x44/0x190
[<ffffffff810a066b>] sys_init_module+0xcb/0x210
[<ffffffff8100c012>] system_call_fastpath+0x16/0x1b
..
fb-test2 which reproduces above is available on kernel.org bug #26232.
To solve this issue, avoid calling lock_fb_info inside fb_set_suspend,
and move it out to where needed (callers of fb_set_suspend must call
lock_fb_info before if needed). So far, the only place which needs to
call lock_fb_info is store_fbstate, all other places which calls
fb_set_suspend are suspend/resume hooks that should not need the lock as
they should be run only when processes are already frozen in
suspend/resume.
References: https://bugzilla.kernel.org/show_bug.cgi?id&232
Signed-off-by: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
Signed-off-by: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
Cc: stable@kernel.org
---
drivers/video/fbmem.c | 3 ---
drivers/video/fbsysfs.c | 3 +++
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index 5aac00e..ad93629 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -1738,8 +1738,6 @@ void fb_set_suspend(struct fb_info *info, int state)
{
struct fb_event event;
- if (!lock_fb_info(info))
- return;
event.info = info;
if (state) {
fb_notifier_call_chain(FB_EVENT_SUSPEND, &event);
@@ -1748,7 +1746,6 @@ void fb_set_suspend(struct fb_info *info, int state)
info->state = FBINFO_STATE_RUNNING;
fb_notifier_call_chain(FB_EVENT_RESUME, &event);
}
- unlock_fb_info(info);
}
/**
diff --git a/drivers/video/fbsysfs.c b/drivers/video/fbsysfs.c
index 04251ce..67afa9c 100644
--- a/drivers/video/fbsysfs.c
+++ b/drivers/video/fbsysfs.c
@@ -399,9 +399,12 @@ static ssize_t store_fbstate(struct device *device,
state = simple_strtoul(buf, &last, 0);
+ if (!lock_fb_info(fb_info))
+ return -ENODEV;
console_lock();
fb_set_suspend(fb_info, (int)state);
console_unlock();
+ unlock_fb_info(fb_info);
return count;
}
--
1.6.3.2
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] fb: avoid possible deadlock caused by fb_set_suspend
2011-06-17 18:46 ` [PATCH] fb: avoid possible deadlock caused by fb_set_suspend Florian Tobias Schandinat
@ 2011-06-18 8:43 ` Bruno Prémont
2011-06-18 9:19 ` Bruno Prémont
2011-07-20 18:16 ` [PATCH] fb: avoid possible deadlock caused by fb_set_suspend Florian Tobias Schandinat
1 sibling, 1 reply; 19+ messages in thread
From: Bruno Prémont @ 2011-06-18 8:43 UTC (permalink / raw)
To: Florian Tobias Schandinat
Cc: lethal, linux-fbdev, francis.moro, torvalds, linux-kernel,
Herton Ronaldo Krzesinski, stable
On Fri, 17 June 2011 Florian Tobias Schandinat <FlorianSchandinat@gmx.de> wrote:
> From: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
>
> A lock ordering issue can cause deadlocks: in framebuffer/console code,
> all needed struct fb_info locks are taken before acquire_console_sem(),
> in places which need to take console semaphore.
>
> But fb_set_suspend is always called with console semaphore held, and
> inside it we call lock_fb_info which gets the fb_info lock, inverse
> locking order of what the rest of the code does. This causes a real
> deadlock issue, when we write to state fb sysfs attribute (which calls
> fb_set_suspend) while a framebuffer is being unregistered by
> remove_conflicting_framebuffers, as can be shown by following show
> blocked state trace on a test program which loads i915 and runs another
> forked processes writing to state attribute:
>
> Test process with semaphore held and trying to get fb_info lock:
...
> fb-test2 which reproduces above is available on kernel.org bug #26232.
> To solve this issue, avoid calling lock_fb_info inside fb_set_suspend,
> and move it out to where needed (callers of fb_set_suspend must call
> lock_fb_info before if needed). So far, the only place which needs to
> call lock_fb_info is store_fbstate, all other places which calls
> fb_set_suspend are suspend/resume hooks that should not need the lock as
> they should be run only when processes are already frozen in
> suspend/resume.
From a quick look through FB drivers in 2.6.39 I've found one that would need
more work:
- drivers/video/sh_mobile_hdmi.c: sh_hdmi_edid_work_fn()
Should get changed to
a) right locking order in case (hdmi->hp_state = HDMI_HOTPLUG_CONNECTED)
b) lock fb_info in the other case
For this one fb_set_suspend() does get call in a hotplug worker,
thus independently on suspend/resume process.
The rest does match the suspend/resume hook pattern mentioned.
Bruno
> References: https://bugzilla.kernel.org/show_bug.cgi?id&232
> Signed-off-by: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
> Signed-off-by: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
> Cc: stable@kernel.org
> ---
> drivers/video/fbmem.c | 3 ---
> drivers/video/fbsysfs.c | 3 +++
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
> index 5aac00e..ad93629 100644
> --- a/drivers/video/fbmem.c
> +++ b/drivers/video/fbmem.c
> @@ -1738,8 +1738,6 @@ void fb_set_suspend(struct fb_info *info, int state)
> {
> struct fb_event event;
>
> - if (!lock_fb_info(info))
> - return;
> event.info = info;
> if (state) {
> fb_notifier_call_chain(FB_EVENT_SUSPEND, &event);
> @@ -1748,7 +1746,6 @@ void fb_set_suspend(struct fb_info *info, int state)
> info->state = FBINFO_STATE_RUNNING;
> fb_notifier_call_chain(FB_EVENT_RESUME, &event);
> }
> - unlock_fb_info(info);
> }
>
> /**
> diff --git a/drivers/video/fbsysfs.c b/drivers/video/fbsysfs.c
> index 04251ce..67afa9c 100644
> --- a/drivers/video/fbsysfs.c
> +++ b/drivers/video/fbsysfs.c
> @@ -399,9 +399,12 @@ static ssize_t store_fbstate(struct device *device,
>
> state = simple_strtoul(buf, &last, 0);
>
> + if (!lock_fb_info(fb_info))
> + return -ENODEV;
> console_lock();
> fb_set_suspend(fb_info, (int)state);
> console_unlock();
> + unlock_fb_info(fb_info);
>
> return count;
> }
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] fb: avoid possible deadlock caused by fb_set_suspend
2011-06-18 8:43 ` Bruno Prémont
@ 2011-06-18 9:19 ` Bruno Prémont
2011-09-01 15:42 ` Florian Tobias Schandinat
0 siblings, 1 reply; 19+ messages in thread
From: Bruno Prémont @ 2011-06-18 9:19 UTC (permalink / raw)
To: Guennadi Liakhovetski
Cc: Florian Tobias Schandinat, lethal, linux-fbdev, francis.moro,
torvalds, linux-kernel, Herton Ronaldo Krzesinski, stable
Guennadi, could you have a look at (completely untested) patch which avoids
possible deadlock due to inverted lock taking order on hotplug as well
as "readding" lock_fb_info() for fb_set_suspend() call after Herton's
patch to fb_set_suspend().
Thanks,
Bruno
On Sat, 18 June 2011 Bruno Prémont <bonbons@linux-vserver.org> wrote:
> On Fri, 17 June 2011 Florian Tobias Schandinat <FlorianSchandinat@gmx.de> wrote:
> > From: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
> >
> > A lock ordering issue can cause deadlocks: in framebuffer/console code,
> > all needed struct fb_info locks are taken before acquire_console_sem(),
> > in places which need to take console semaphore.
> >
> > But fb_set_suspend is always called with console semaphore held, and
> > inside it we call lock_fb_info which gets the fb_info lock, inverse
> > locking order of what the rest of the code does. This causes a real
> > deadlock issue, when we write to state fb sysfs attribute (which calls
> > fb_set_suspend) while a framebuffer is being unregistered by
> > remove_conflicting_framebuffers, as can be shown by following show
> > blocked state trace on a test program which loads i915 and runs another
> > forked processes writing to state attribute:
> >
> > Test process with semaphore held and trying to get fb_info lock:
>
> ...
>
> > fb-test2 which reproduces above is available on kernel.org bug #26232.
> > To solve this issue, avoid calling lock_fb_info inside fb_set_suspend,
> > and move it out to where needed (callers of fb_set_suspend must call
> > lock_fb_info before if needed). So far, the only place which needs to
> > call lock_fb_info is store_fbstate, all other places which calls
> > fb_set_suspend are suspend/resume hooks that should not need the lock as
> > they should be run only when processes are already frozen in
> > suspend/resume.
>
> From a quick look through FB drivers in 2.6.39 I've found one that would need
> more work:
> - drivers/video/sh_mobile_hdmi.c: sh_hdmi_edid_work_fn()
> Should get changed to
> a) right locking order in case (hdmi->hp_state = HDMI_HOTPLUG_CONNECTED)
> b) lock fb_info in the other case
> For this one fb_set_suspend() does get call in a hotplug worker,
> thus independently on suspend/resume process.
>
> The rest does match the suspend/resume hook pattern mentioned.
>
> Bruno
>
>
> > References: https://bugzilla.kernel.org/show_bug.cgi?id&232
> > Signed-off-by: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
> > Signed-off-by: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
> > Cc: stable@kernel.org
> > ---
> > drivers/video/fbmem.c | 3 ---
> > drivers/video/fbsysfs.c | 3 +++
> > 2 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
> > index 5aac00e..ad93629 100644
> > --- a/drivers/video/fbmem.c
> > +++ b/drivers/video/fbmem.c
> > @@ -1738,8 +1738,6 @@ void fb_set_suspend(struct fb_info *info, int state)
> > {
> > struct fb_event event;
> >
> > - if (!lock_fb_info(info))
> > - return;
> > event.info = info;
> > if (state) {
> > fb_notifier_call_chain(FB_EVENT_SUSPEND, &event);
> > @@ -1748,7 +1746,6 @@ void fb_set_suspend(struct fb_info *info, int state)
> > info->state = FBINFO_STATE_RUNNING;
> > fb_notifier_call_chain(FB_EVENT_RESUME, &event);
> > }
> > - unlock_fb_info(info);
> > }
> >
> > /**
> > diff --git a/drivers/video/fbsysfs.c b/drivers/video/fbsysfs.c
> > index 04251ce..67afa9c 100644
> > --- a/drivers/video/fbsysfs.c
> > +++ b/drivers/video/fbsysfs.c
> > @@ -399,9 +399,12 @@ static ssize_t store_fbstate(struct device *device,
> >
> > state = simple_strtoul(buf, &last, 0);
> >
> > + if (!lock_fb_info(fb_info))
> > + return -ENODEV;
> > console_lock();
> > fb_set_suspend(fb_info, (int)state);
> > console_unlock();
> > + unlock_fb_info(fb_info);
> >
> > return count;
> > }
diff --git a/drivers/video/sh_mobile_hdmi.c b/drivers/video/sh_mobile_hdmi.c
index 2b9e56a..b1a13ab 100644
--- a/drivers/video/sh_mobile_hdmi.c
+++ b/drivers/video/sh_mobile_hdmi.c
@@ -1151,27 +1151,27 @@ static void sh_hdmi_edid_work_fn(struct work_struct *work)
ch = info->par;
- console_lock();
+ if (lock_fb_info(info)) {
+ console_lock();
- /* HDMI plug in */
- if (!sh_hdmi_must_reconfigure(hdmi) &&
- info->state = FBINFO_STATE_RUNNING) {
- /*
- * First activation with the default monitor - just turn
- * on, if we run a resume here, the logo disappears
- */
- if (lock_fb_info(info)) {
+ /* HDMI plug in */
+ if (!sh_hdmi_must_reconfigure(hdmi) &&
+ info->state = FBINFO_STATE_RUNNING) {
+ /*
+ * First activation with the default monitor - just turn
+ * on, if we run a resume here, the logo disappears
+ */
info->var.width = hdmi->var.width;
info->var.height = hdmi->var.height;
sh_hdmi_display_on(hdmi, info);
- unlock_fb_info(info);
+ } else {
+ /* New monitor or have to wake up */
+ fb_set_suspend(info, 0);
}
- } else {
- /* New monitor or have to wake up */
- fb_set_suspend(info, 0);
- }
- console_unlock();
+ console_unlock();
+ unlock_fb_info(info);
+ }
} else {
ret = 0;
if (!hdmi->info)
@@ -1181,12 +1181,15 @@ static void sh_hdmi_edid_work_fn(struct work_struct *work)
fb_destroy_modedb(hdmi->monspec.modedb);
hdmi->monspec.modedb = NULL;
- console_lock();
+ if (lock_fb_info(info)) {
+ console_lock();
- /* HDMI disconnect */
- fb_set_suspend(hdmi->info, 1);
+ /* HDMI disconnect */
+ fb_set_suspend(hdmi->info, 1);
- console_unlock();
+ console_unlock();
+ unlock_fb_info(info);
+ }
pm_runtime_put(hdmi->dev);
}
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] fb: avoid possible deadlock caused by fb_set_suspend
2011-06-17 18:46 ` [PATCH] fb: avoid possible deadlock caused by fb_set_suspend Florian Tobias Schandinat
2011-06-18 8:43 ` Bruno Prémont
@ 2011-07-20 18:16 ` Florian Tobias Schandinat
2011-07-28 22:10 ` Francis Moreau
1 sibling, 1 reply; 19+ messages in thread
From: Florian Tobias Schandinat @ 2011-07-20 18:16 UTC (permalink / raw)
To: lethal
Cc: linux-fbdev, francis.moro, torvalds, bonbons, linux-kernel,
Herton Ronaldo Krzesinski, stable
Hi Paul,
what are your plans for the patch below? Do you queue it for 3.1?
The only feedback so far was that one driver (sh_mobile_hdmi) would require more
work with a patch for that proposed. Without any objections to the patch I think
we should take it as fixing the subsystem is more important than causing
potential issues with a single driver.
Thanks,
Florian Tobias Schandinat
On 06/17/2011 07:02 PM, Florian Tobias Schandinat wrote:
> From: Herton Ronaldo Krzesinski<herton@mandriva.com.br>
>
> A lock ordering issue can cause deadlocks: in framebuffer/console code,
> all needed struct fb_info locks are taken before acquire_console_sem(),
> in places which need to take console semaphore.
>
> But fb_set_suspend is always called with console semaphore held, and
> inside it we call lock_fb_info which gets the fb_info lock, inverse
> locking order of what the rest of the code does. This causes a real
> deadlock issue, when we write to state fb sysfs attribute (which calls
> fb_set_suspend) while a framebuffer is being unregistered by
> remove_conflicting_framebuffers, as can be shown by following show
> blocked state trace on a test program which loads i915 and runs another
> forked processes writing to state attribute:
>
> Test process with semaphore held and trying to get fb_info lock:
> ..
> fb-test2 D 0000000000000000 0 237 228 0x00000000
> ffff8800774f3d68 0000000000000082 00000000000135c0 00000000000135c0
> ffff880000000000 ffff8800774f3fd8 ffff8800774f3fd8 ffff880076ee4530
> 00000000000135c0 ffff8800774f3fd8 ffff8800774f2000 00000000000135c0
> Call Trace:
> [<ffffffff8141287a>] __mutex_lock_slowpath+0x11a/0x1e0
> [<ffffffff814142f2>] ? _raw_spin_lock_irq+0x22/0x40
> [<ffffffff814123d3>] mutex_lock+0x23/0x50
> [<ffffffff8125dfc5>] lock_fb_info+0x25/0x60
> [<ffffffff8125e3f0>] fb_set_suspend+0x20/0x80
> [<ffffffff81263e2f>] store_fbstate+0x4f/0x70
> [<ffffffff812e7f70>] dev_attr_store+0x20/0x30
> [<ffffffff811c46b4>] sysfs_write_file+0xd4/0x160
> [<ffffffff81155a26>] vfs_write+0xc6/0x190
> [<ffffffff81155d51>] sys_write+0x51/0x90
> [<ffffffff8100c012>] system_call_fastpath+0x16/0x1b
> ..
> modprobe process stalled because has the fb_info lock (got inside
> unregister_framebuffer) but waiting for the semaphore held by the
> test process which is waiting to get the fb_info lock:
> ..
> modprobe D 0000000000000000 0 230 218 0x00000000
> ffff880077a4d618 0000000000000082 0000000000000001 0000000000000001
> ffff880000000000 ffff880077a4dfd8 ffff880077a4dfd8 ffff8800775a2e20
> 00000000000135c0 ffff880077a4dfd8 ffff880077a4c000 00000000000135c0
> Call Trace:
> [<ffffffff81411fe5>] schedule_timeout+0x215/0x310
> [<ffffffff81058051>] ? get_parent_ip+0x11/0x50
> [<ffffffff814130dd>] __down+0x6d/0xb0
> [<ffffffff81089f71>] down+0x41/0x50
> [<ffffffff810629ac>] acquire_console_sem+0x2c/0x50
> [<ffffffff812ca53d>] unbind_con_driver+0xad/0x2d0
> [<ffffffff8126f5f7>] fbcon_event_notify+0x457/0x890
> [<ffffffff814144ff>] ? _raw_spin_unlock_irqrestore+0x1f/0x50
> [<ffffffff81058051>] ? get_parent_ip+0x11/0x50
> [<ffffffff8141836d>] notifier_call_chain+0x4d/0x70
> [<ffffffff8108a3b8>] __blocking_notifier_call_chain+0x58/0x80
> [<ffffffff8108a3f6>] blocking_notifier_call_chain+0x16/0x20
> [<ffffffff8125dabb>] fb_notifier_call_chain+0x1b/0x20
> [<ffffffff8125e6ac>] unregister_framebuffer+0x7c/0x130
> [<ffffffff8125e8b3>] remove_conflicting_framebuffers+0x153/0x180
> [<ffffffff8125eef3>] register_framebuffer+0x93/0x2c0
> [<ffffffffa0331112>] drm_fb_helper_single_fb_probe+0x252/0x2f0 [drm_kms_helper]
> [<ffffffffa03314a3>] drm_fb_helper_initial_config+0x2f3/0x6d0 [drm_kms_helper]
> [<ffffffffa03318dd>] ? drm_fb_helper_single_add_all_connectors+0x5d/0x1c0 [drm_kms_helper]
> [<ffffffffa037b588>] intel_fbdev_init+0xa8/0x160 [i915]
> [<ffffffffa0343d74>] i915_driver_load+0x854/0x12b0 [i915]
> [<ffffffffa02f0e7e>] drm_get_pci_dev+0x19e/0x360 [drm]
> [<ffffffff8141821d>] ? sub_preempt_count+0x9d/0xd0
> [<ffffffffa0386f91>] i915_pci_probe+0x15/0x17 [i915]
> [<ffffffff8124481f>] local_pci_probe+0x5f/0xd0
> [<ffffffff81244f89>] pci_device_probe+0x119/0x120
> [<ffffffff812eccaa>] ? driver_sysfs_add+0x7a/0xb0
> [<ffffffff812ed003>] driver_probe_device+0xa3/0x290
> [<ffffffff812ed1f0>] ? __driver_attach+0x0/0xb0
> [<ffffffff812ed29b>] __driver_attach+0xab/0xb0
> [<ffffffff812ed1f0>] ? __driver_attach+0x0/0xb0
> [<ffffffff812ebd3e>] bus_for_each_dev+0x5e/0x90
> [<ffffffff812ecc2e>] driver_attach+0x1e/0x20
> [<ffffffff812ec6f2>] bus_add_driver+0xe2/0x320
> [<ffffffffa03aa000>] ? i915_init+0x0/0x96 [i915]
> [<ffffffff812ed536>] driver_register+0x76/0x140
> [<ffffffffa03aa000>] ? i915_init+0x0/0x96 [i915]
> [<ffffffff81245216>] __pci_register_driver+0x56/0xd0
> [<ffffffffa02f1264>] drm_pci_init+0xe4/0xf0 [drm]
> [<ffffffffa03aa000>] ? i915_init+0x0/0x96 [i915]
> [<ffffffffa02e84a8>] drm_init+0x58/0x70 [drm]
> [<ffffffffa03aa094>] i915_init+0x94/0x96 [i915]
> [<ffffffff81002194>] do_one_initcall+0x44/0x190
> [<ffffffff810a066b>] sys_init_module+0xcb/0x210
> [<ffffffff8100c012>] system_call_fastpath+0x16/0x1b
> ..
>
> fb-test2 which reproduces above is available on kernel.org bug #26232.
> To solve this issue, avoid calling lock_fb_info inside fb_set_suspend,
> and move it out to where needed (callers of fb_set_suspend must call
> lock_fb_info before if needed). So far, the only place which needs to
> call lock_fb_info is store_fbstate, all other places which calls
> fb_set_suspend are suspend/resume hooks that should not need the lock as
> they should be run only when processes are already frozen in
> suspend/resume.
>
> References: https://bugzilla.kernel.org/show_bug.cgi?id&232
> Signed-off-by: Herton Ronaldo Krzesinski<herton@mandriva.com.br>
> Signed-off-by: Florian Tobias Schandinat<FlorianSchandinat@gmx.de>
> Cc: stable@kernel.org
> ---
> drivers/video/fbmem.c | 3 ---
> drivers/video/fbsysfs.c | 3 +++
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
> index 5aac00e..ad93629 100644
> --- a/drivers/video/fbmem.c
> +++ b/drivers/video/fbmem.c
> @@ -1738,8 +1738,6 @@ void fb_set_suspend(struct fb_info *info, int state)
> {
> struct fb_event event;
>
> - if (!lock_fb_info(info))
> - return;
> event.info = info;
> if (state) {
> fb_notifier_call_chain(FB_EVENT_SUSPEND,&event);
> @@ -1748,7 +1746,6 @@ void fb_set_suspend(struct fb_info *info, int state)
> info->state = FBINFO_STATE_RUNNING;
> fb_notifier_call_chain(FB_EVENT_RESUME,&event);
> }
> - unlock_fb_info(info);
> }
>
> /**
> diff --git a/drivers/video/fbsysfs.c b/drivers/video/fbsysfs.c
> index 04251ce..67afa9c 100644
> --- a/drivers/video/fbsysfs.c
> +++ b/drivers/video/fbsysfs.c
> @@ -399,9 +399,12 @@ static ssize_t store_fbstate(struct device *device,
>
> state = simple_strtoul(buf,&last, 0);
>
> + if (!lock_fb_info(fb_info))
> + return -ENODEV;
> console_lock();
> fb_set_suspend(fb_info, (int)state);
> console_unlock();
> + unlock_fb_info(fb_info);
>
> return count;
> }
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] fb: avoid possible deadlock caused by fb_set_suspend
2011-07-20 18:16 ` [PATCH] fb: avoid possible deadlock caused by fb_set_suspend Florian Tobias Schandinat
@ 2011-07-28 22:10 ` Francis Moreau
0 siblings, 0 replies; 19+ messages in thread
From: Francis Moreau @ 2011-07-28 22:10 UTC (permalink / raw)
To: Florian Tobias Schandinat
Cc: lethal, linux-fbdev, torvalds, bonbons, linux-kernel,
Herton Ronaldo Krzesinski, stable
On Wed, Jul 20, 2011 at 8:16 PM, Florian Tobias Schandinat
<FlorianSchandinat@gmx.de> wrote:
> what are your plans for the patch below? Do you queue it for 3.1?
Looks like someone else fixed it slightly differently:
http://permalink.gmane.org/gmane.linux.kernel.stable/15187
--
Francis
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] fb: avoid possible deadlock caused by fb_set_suspend
2011-06-18 9:19 ` Bruno Prémont
@ 2011-09-01 15:42 ` Florian Tobias Schandinat
2011-09-01 16:28 ` Guennadi Liakhovetski
2011-09-02 16:06 ` Guennadi Liakhovetski
0 siblings, 2 replies; 19+ messages in thread
From: Florian Tobias Schandinat @ 2011-09-01 15:42 UTC (permalink / raw)
To: Guennadi Liakhovetski
Cc: Bruno Prémont, lethal, linux-fbdev, francis.moro, torvalds,
linux-kernel, Herton Ronaldo Krzesinski, stable
ping
Guennadi, I really want this issue fixed. Please have a look at Bruno's patch
otherwise your driver might remain or get even more broken...
I am scheduling Herton's patch for the next merge window.
Regards,
Florian Tobias Schandinat
On 06/18/2011 09:19 AM, Bruno Prémont wrote:
> Guennadi, could you have a look at (completely untested) patch which avoids
> possible deadlock due to inverted lock taking order on hotplug as well
> as "readding" lock_fb_info() for fb_set_suspend() call after Herton's
> patch to fb_set_suspend().
>
> Thanks,
> Bruno
>
>
> On Sat, 18 June 2011 Bruno Prémont <bonbons@linux-vserver.org> wrote:
>> On Fri, 17 June 2011 Florian Tobias Schandinat <FlorianSchandinat@gmx.de> wrote:
>>> From: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
>>>
>>> A lock ordering issue can cause deadlocks: in framebuffer/console code,
>>> all needed struct fb_info locks are taken before acquire_console_sem(),
>>> in places which need to take console semaphore.
>>>
>>> But fb_set_suspend is always called with console semaphore held, and
>>> inside it we call lock_fb_info which gets the fb_info lock, inverse
>>> locking order of what the rest of the code does. This causes a real
>>> deadlock issue, when we write to state fb sysfs attribute (which calls
>>> fb_set_suspend) while a framebuffer is being unregistered by
>>> remove_conflicting_framebuffers, as can be shown by following show
>>> blocked state trace on a test program which loads i915 and runs another
>>> forked processes writing to state attribute:
>>>
>>> Test process with semaphore held and trying to get fb_info lock:
>>
>> ...
>>
>>> fb-test2 which reproduces above is available on kernel.org bug #26232.
>>> To solve this issue, avoid calling lock_fb_info inside fb_set_suspend,
>>> and move it out to where needed (callers of fb_set_suspend must call
>>> lock_fb_info before if needed). So far, the only place which needs to
>>> call lock_fb_info is store_fbstate, all other places which calls
>>> fb_set_suspend are suspend/resume hooks that should not need the lock as
>>> they should be run only when processes are already frozen in
>>> suspend/resume.
>>
>> From a quick look through FB drivers in 2.6.39 I've found one that would need
>> more work:
>> - drivers/video/sh_mobile_hdmi.c: sh_hdmi_edid_work_fn()
>> Should get changed to
>> a) right locking order in case (hdmi->hp_state = HDMI_HOTPLUG_CONNECTED)
>> b) lock fb_info in the other case
>> For this one fb_set_suspend() does get call in a hotplug worker,
>> thus independently on suspend/resume process.
>>
>> The rest does match the suspend/resume hook pattern mentioned.
>>
>> Bruno
>>
>>
>>> References: https://bugzilla.kernel.org/show_bug.cgi?id&232
>>> Signed-off-by: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
>>> Signed-off-by: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
>>> Cc: stable@kernel.org
>>> ---
>>> drivers/video/fbmem.c | 3 ---
>>> drivers/video/fbsysfs.c | 3 +++
>>> 2 files changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
>>> index 5aac00e..ad93629 100644
>>> --- a/drivers/video/fbmem.c
>>> +++ b/drivers/video/fbmem.c
>>> @@ -1738,8 +1738,6 @@ void fb_set_suspend(struct fb_info *info, int state)
>>> {
>>> struct fb_event event;
>>>
>>> - if (!lock_fb_info(info))
>>> - return;
>>> event.info = info;
>>> if (state) {
>>> fb_notifier_call_chain(FB_EVENT_SUSPEND, &event);
>>> @@ -1748,7 +1746,6 @@ void fb_set_suspend(struct fb_info *info, int state)
>>> info->state = FBINFO_STATE_RUNNING;
>>> fb_notifier_call_chain(FB_EVENT_RESUME, &event);
>>> }
>>> - unlock_fb_info(info);
>>> }
>>>
>>> /**
>>> diff --git a/drivers/video/fbsysfs.c b/drivers/video/fbsysfs.c
>>> index 04251ce..67afa9c 100644
>>> --- a/drivers/video/fbsysfs.c
>>> +++ b/drivers/video/fbsysfs.c
>>> @@ -399,9 +399,12 @@ static ssize_t store_fbstate(struct device *device,
>>>
>>> state = simple_strtoul(buf, &last, 0);
>>>
>>> + if (!lock_fb_info(fb_info))
>>> + return -ENODEV;
>>> console_lock();
>>> fb_set_suspend(fb_info, (int)state);
>>> console_unlock();
>>> + unlock_fb_info(fb_info);
>>>
>>> return count;
>>> }
>
>
> diff --git a/drivers/video/sh_mobile_hdmi.c b/drivers/video/sh_mobile_hdmi.c
> index 2b9e56a..b1a13ab 100644
> --- a/drivers/video/sh_mobile_hdmi.c
> +++ b/drivers/video/sh_mobile_hdmi.c
> @@ -1151,27 +1151,27 @@ static void sh_hdmi_edid_work_fn(struct work_struct *work)
>
> ch = info->par;
>
> - console_lock();
> + if (lock_fb_info(info)) {
> + console_lock();
>
> - /* HDMI plug in */
> - if (!sh_hdmi_must_reconfigure(hdmi) &&
> - info->state = FBINFO_STATE_RUNNING) {
> - /*
> - * First activation with the default monitor - just turn
> - * on, if we run a resume here, the logo disappears
> - */
> - if (lock_fb_info(info)) {
> + /* HDMI plug in */
> + if (!sh_hdmi_must_reconfigure(hdmi) &&
> + info->state = FBINFO_STATE_RUNNING) {
> + /*
> + * First activation with the default monitor - just turn
> + * on, if we run a resume here, the logo disappears
> + */
> info->var.width = hdmi->var.width;
> info->var.height = hdmi->var.height;
> sh_hdmi_display_on(hdmi, info);
> - unlock_fb_info(info);
> + } else {
> + /* New monitor or have to wake up */
> + fb_set_suspend(info, 0);
> }
> - } else {
> - /* New monitor or have to wake up */
> - fb_set_suspend(info, 0);
> - }
>
> - console_unlock();
> + console_unlock();
> + unlock_fb_info(info);
> + }
> } else {
> ret = 0;
> if (!hdmi->info)
> @@ -1181,12 +1181,15 @@ static void sh_hdmi_edid_work_fn(struct work_struct *work)
> fb_destroy_modedb(hdmi->monspec.modedb);
> hdmi->monspec.modedb = NULL;
>
> - console_lock();
> + if (lock_fb_info(info)) {
> + console_lock();
>
> - /* HDMI disconnect */
> - fb_set_suspend(hdmi->info, 1);
> + /* HDMI disconnect */
> + fb_set_suspend(hdmi->info, 1);
>
> - console_unlock();
> + console_unlock();
> + unlock_fb_info(info);
> + }
> pm_runtime_put(hdmi->dev);
> }
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] fb: avoid possible deadlock caused by fb_set_suspend
2011-09-01 15:42 ` Florian Tobias Schandinat
@ 2011-09-01 16:28 ` Guennadi Liakhovetski
2011-09-02 16:06 ` Guennadi Liakhovetski
1 sibling, 0 replies; 19+ messages in thread
From: Guennadi Liakhovetski @ 2011-09-01 16:28 UTC (permalink / raw)
To: Florian Tobias Schandinat
Cc: Bruno Prémont, lethal, linux-fbdev, francis.moro, torvalds,
linux-kernel, Herton Ronaldo Krzesinski, stable
Hi Florian
On Thu, 1 Sep 2011, Florian Tobias Schandinat wrote:
> ping
>
> Guennadi, I really want this issue fixed. Please have a look at Bruno's patch
> otherwise your driver might remain or get even more broken...
Sorry, somehow this patch excaped my attention. Will test tomorrow.
Thanks
Guennadi
>
> I am scheduling Herton's patch for the next merge window.
>
>
> Regards,
>
> Florian Tobias Schandinat
>
>
> On 06/18/2011 09:19 AM, Bruno Prémont wrote:
> > Guennadi, could you have a look at (completely untested) patch which avoids
> > possible deadlock due to inverted lock taking order on hotplug as well
> > as "readding" lock_fb_info() for fb_set_suspend() call after Herton's
> > patch to fb_set_suspend().
> >
> > Thanks,
> > Bruno
> >
> >
> > On Sat, 18 June 2011 Bruno Prémont <bonbons@linux-vserver.org> wrote:
> >> On Fri, 17 June 2011 Florian Tobias Schandinat <FlorianSchandinat@gmx.de> wrote:
> >>> From: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
> >>>
> >>> A lock ordering issue can cause deadlocks: in framebuffer/console code,
> >>> all needed struct fb_info locks are taken before acquire_console_sem(),
> >>> in places which need to take console semaphore.
> >>>
> >>> But fb_set_suspend is always called with console semaphore held, and
> >>> inside it we call lock_fb_info which gets the fb_info lock, inverse
> >>> locking order of what the rest of the code does. This causes a real
> >>> deadlock issue, when we write to state fb sysfs attribute (which calls
> >>> fb_set_suspend) while a framebuffer is being unregistered by
> >>> remove_conflicting_framebuffers, as can be shown by following show
> >>> blocked state trace on a test program which loads i915 and runs another
> >>> forked processes writing to state attribute:
> >>>
> >>> Test process with semaphore held and trying to get fb_info lock:
> >>
> >> ...
> >>
> >>> fb-test2 which reproduces above is available on kernel.org bug #26232.
> >>> To solve this issue, avoid calling lock_fb_info inside fb_set_suspend,
> >>> and move it out to where needed (callers of fb_set_suspend must call
> >>> lock_fb_info before if needed). So far, the only place which needs to
> >>> call lock_fb_info is store_fbstate, all other places which calls
> >>> fb_set_suspend are suspend/resume hooks that should not need the lock as
> >>> they should be run only when processes are already frozen in
> >>> suspend/resume.
> >>
> >> From a quick look through FB drivers in 2.6.39 I've found one that would need
> >> more work:
> >> - drivers/video/sh_mobile_hdmi.c: sh_hdmi_edid_work_fn()
> >> Should get changed to
> >> a) right locking order in case (hdmi->hp_state = HDMI_HOTPLUG_CONNECTED)
> >> b) lock fb_info in the other case
> >> For this one fb_set_suspend() does get call in a hotplug worker,
> >> thus independently on suspend/resume process.
> >>
> >> The rest does match the suspend/resume hook pattern mentioned.
> >>
> >> Bruno
> >>
> >>
> >>> References: https://bugzilla.kernel.org/show_bug.cgi?id&232
> >>> Signed-off-by: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
> >>> Signed-off-by: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
> >>> Cc: stable@kernel.org
> >>> ---
> >>> drivers/video/fbmem.c | 3 ---
> >>> drivers/video/fbsysfs.c | 3 +++
> >>> 2 files changed, 3 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
> >>> index 5aac00e..ad93629 100644
> >>> --- a/drivers/video/fbmem.c
> >>> +++ b/drivers/video/fbmem.c
> >>> @@ -1738,8 +1738,6 @@ void fb_set_suspend(struct fb_info *info, int state)
> >>> {
> >>> struct fb_event event;
> >>>
> >>> - if (!lock_fb_info(info))
> >>> - return;
> >>> event.info = info;
> >>> if (state) {
> >>> fb_notifier_call_chain(FB_EVENT_SUSPEND, &event);
> >>> @@ -1748,7 +1746,6 @@ void fb_set_suspend(struct fb_info *info, int state)
> >>> info->state = FBINFO_STATE_RUNNING;
> >>> fb_notifier_call_chain(FB_EVENT_RESUME, &event);
> >>> }
> >>> - unlock_fb_info(info);
> >>> }
> >>>
> >>> /**
> >>> diff --git a/drivers/video/fbsysfs.c b/drivers/video/fbsysfs.c
> >>> index 04251ce..67afa9c 100644
> >>> --- a/drivers/video/fbsysfs.c
> >>> +++ b/drivers/video/fbsysfs.c
> >>> @@ -399,9 +399,12 @@ static ssize_t store_fbstate(struct device *device,
> >>>
> >>> state = simple_strtoul(buf, &last, 0);
> >>>
> >>> + if (!lock_fb_info(fb_info))
> >>> + return -ENODEV;
> >>> console_lock();
> >>> fb_set_suspend(fb_info, (int)state);
> >>> console_unlock();
> >>> + unlock_fb_info(fb_info);
> >>>
> >>> return count;
> >>> }
> >
> >
> > diff --git a/drivers/video/sh_mobile_hdmi.c b/drivers/video/sh_mobile_hdmi.c
> > index 2b9e56a..b1a13ab 100644
> > --- a/drivers/video/sh_mobile_hdmi.c
> > +++ b/drivers/video/sh_mobile_hdmi.c
> > @@ -1151,27 +1151,27 @@ static void sh_hdmi_edid_work_fn(struct work_struct *work)
> >
> > ch = info->par;
> >
> > - console_lock();
> > + if (lock_fb_info(info)) {
> > + console_lock();
> >
> > - /* HDMI plug in */
> > - if (!sh_hdmi_must_reconfigure(hdmi) &&
> > - info->state = FBINFO_STATE_RUNNING) {
> > - /*
> > - * First activation with the default monitor - just turn
> > - * on, if we run a resume here, the logo disappears
> > - */
> > - if (lock_fb_info(info)) {
> > + /* HDMI plug in */
> > + if (!sh_hdmi_must_reconfigure(hdmi) &&
> > + info->state = FBINFO_STATE_RUNNING) {
> > + /*
> > + * First activation with the default monitor - just turn
> > + * on, if we run a resume here, the logo disappears
> > + */
> > info->var.width = hdmi->var.width;
> > info->var.height = hdmi->var.height;
> > sh_hdmi_display_on(hdmi, info);
> > - unlock_fb_info(info);
> > + } else {
> > + /* New monitor or have to wake up */
> > + fb_set_suspend(info, 0);
> > }
> > - } else {
> > - /* New monitor or have to wake up */
> > - fb_set_suspend(info, 0);
> > - }
> >
> > - console_unlock();
> > + console_unlock();
> > + unlock_fb_info(info);
> > + }
> > } else {
> > ret = 0;
> > if (!hdmi->info)
> > @@ -1181,12 +1181,15 @@ static void sh_hdmi_edid_work_fn(struct work_struct *work)
> > fb_destroy_modedb(hdmi->monspec.modedb);
> > hdmi->monspec.modedb = NULL;
> >
> > - console_lock();
> > + if (lock_fb_info(info)) {
> > + console_lock();
> >
> > - /* HDMI disconnect */
> > - fb_set_suspend(hdmi->info, 1);
> > + /* HDMI disconnect */
> > + fb_set_suspend(hdmi->info, 1);
> >
> > - console_unlock();
> > + console_unlock();
> > + unlock_fb_info(info);
> > + }
> > pm_runtime_put(hdmi->dev);
> > }
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
>
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] fb: avoid possible deadlock caused by fb_set_suspend
2011-09-01 15:42 ` Florian Tobias Schandinat
2011-09-01 16:28 ` Guennadi Liakhovetski
@ 2011-09-02 16:06 ` Guennadi Liakhovetski
2011-09-02 16:46 ` Florian Tobias Schandinat
1 sibling, 1 reply; 19+ messages in thread
From: Guennadi Liakhovetski @ 2011-09-02 16:06 UTC (permalink / raw)
To: Florian Tobias Schandinat
Cc: Bruno Prémont, lethal, linux-fbdev, francis.moro, torvalds,
linux-kernel, Herton Ronaldo Krzesinski, stable
Hi Florian
On Thu, 1 Sep 2011, Florian Tobias Schandinat wrote:
> ping
>
> Guennadi, I really want this issue fixed. Please have a look at Bruno's patch
> otherwise your driver might remain or get even more broken...
>
> I am scheduling Herton's patch for the next merge window.
So, the patch looks simple and correct, when applied on top of
http://marc.info/?l=linux-kernel&m\x130833638508657&w=2
But it doesn't apply anymore and after fixing a trivial merge conflict, it
fails to build, which is also trivially fixed. Please, add my
Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
to the set and use this updated version:
diff --git a/drivers/video/sh_mobile_hdmi.c b/drivers/video/sh_mobile_hdmi.c
index 7d54e2c..647ba98 100644
--- a/drivers/video/sh_mobile_hdmi.c
+++ b/drivers/video/sh_mobile_hdmi.c
@@ -1111,6 +1111,7 @@ static long sh_hdmi_clk_configure(struct sh_hdmi *hdmi, unsigned long hdmi_rate,
static void sh_hdmi_edid_work_fn(struct work_struct *work)
{
struct sh_hdmi *hdmi = container_of(work, struct sh_hdmi, edid_work.work);
+ struct fb_info *info;
struct sh_mobile_hdmi_info *pdata = hdmi->dev->platform_data;
struct sh_mobile_lcdc_chan *ch;
int ret;
@@ -1123,8 +1124,9 @@ static void sh_hdmi_edid_work_fn(struct work_struct *work)
mutex_lock(&hdmi->mutex);
+ info = hdmi->info;
+
if (hdmi->hp_state = HDMI_HOTPLUG_CONNECTED) {
- struct fb_info *info = hdmi->info;
unsigned long parent_rate = 0, hdmi_rate;
ret = sh_hdmi_read_edid(hdmi, &hdmi_rate, &parent_rate);
@@ -1148,42 +1150,45 @@ static void sh_hdmi_edid_work_fn(struct work_struct *work)
ch = info->par;
- console_lock();
+ if (lock_fb_info(info)) {
+ console_lock();
- /* HDMI plug in */
- if (!sh_hdmi_must_reconfigure(hdmi) &&
- info->state = FBINFO_STATE_RUNNING) {
- /*
- * First activation with the default monitor - just turn
- * on, if we run a resume here, the logo disappears
- */
- if (lock_fb_info(info)) {
+ /* HDMI plug in */
+ if (!sh_hdmi_must_reconfigure(hdmi) &&
+ info->state = FBINFO_STATE_RUNNING) {
+ /*
+ * First activation with the default monitor - just turn
+ * on, if we run a resume here, the logo disappears
+ */
info->var.width = hdmi->var.width;
info->var.height = hdmi->var.height;
sh_hdmi_display_on(hdmi, info);
- unlock_fb_info(info);
+ } else {
+ /* New monitor or have to wake up */
+ fb_set_suspend(info, 0);
}
- } else {
- /* New monitor or have to wake up */
- fb_set_suspend(info, 0);
- }
- console_unlock();
+ console_unlock();
+ unlock_fb_info(info);
+ }
} else {
ret = 0;
- if (!hdmi->info)
+ if (!info)
goto out;
hdmi->monspec.modedb_len = 0;
fb_destroy_modedb(hdmi->monspec.modedb);
hdmi->monspec.modedb = NULL;
- console_lock();
+ if (lock_fb_info(info)) {
+ console_lock();
- /* HDMI disconnect */
- fb_set_suspend(hdmi->info, 1);
+ /* HDMI disconnect */
+ fb_set_suspend(info, 1);
- console_unlock();
+ console_unlock();
+ unlock_fb_info(info);
+ }
}
out:
Thanks
Guennadi
>
>
> Regards,
>
> Florian Tobias Schandinat
>
>
> On 06/18/2011 09:19 AM, Bruno Prémont wrote:
> > Guennadi, could you have a look at (completely untested) patch which avoids
> > possible deadlock due to inverted lock taking order on hotplug as well
> > as "readding" lock_fb_info() for fb_set_suspend() call after Herton's
> > patch to fb_set_suspend().
> >
> > Thanks,
> > Bruno
> >
> >
> > On Sat, 18 June 2011 Bruno Prémont <bonbons@linux-vserver.org> wrote:
> >> On Fri, 17 June 2011 Florian Tobias Schandinat <FlorianSchandinat@gmx.de> wrote:
> >>> From: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
> >>>
> >>> A lock ordering issue can cause deadlocks: in framebuffer/console code,
> >>> all needed struct fb_info locks are taken before acquire_console_sem(),
> >>> in places which need to take console semaphore.
> >>>
> >>> But fb_set_suspend is always called with console semaphore held, and
> >>> inside it we call lock_fb_info which gets the fb_info lock, inverse
> >>> locking order of what the rest of the code does. This causes a real
> >>> deadlock issue, when we write to state fb sysfs attribute (which calls
> >>> fb_set_suspend) while a framebuffer is being unregistered by
> >>> remove_conflicting_framebuffers, as can be shown by following show
> >>> blocked state trace on a test program which loads i915 and runs another
> >>> forked processes writing to state attribute:
> >>>
> >>> Test process with semaphore held and trying to get fb_info lock:
> >>
> >> ...
> >>
> >>> fb-test2 which reproduces above is available on kernel.org bug #26232.
> >>> To solve this issue, avoid calling lock_fb_info inside fb_set_suspend,
> >>> and move it out to where needed (callers of fb_set_suspend must call
> >>> lock_fb_info before if needed). So far, the only place which needs to
> >>> call lock_fb_info is store_fbstate, all other places which calls
> >>> fb_set_suspend are suspend/resume hooks that should not need the lock as
> >>> they should be run only when processes are already frozen in
> >>> suspend/resume.
> >>
> >> From a quick look through FB drivers in 2.6.39 I've found one that would need
> >> more work:
> >> - drivers/video/sh_mobile_hdmi.c: sh_hdmi_edid_work_fn()
> >> Should get changed to
> >> a) right locking order in case (hdmi->hp_state = HDMI_HOTPLUG_CONNECTED)
> >> b) lock fb_info in the other case
> >> For this one fb_set_suspend() does get call in a hotplug worker,
> >> thus independently on suspend/resume process.
> >>
> >> The rest does match the suspend/resume hook pattern mentioned.
> >>
> >> Bruno
> >>
> >>
> >>> References: https://bugzilla.kernel.org/show_bug.cgi?id&232
> >>> Signed-off-by: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
> >>> Signed-off-by: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
> >>> Cc: stable@kernel.org
> >>> ---
> >>> drivers/video/fbmem.c | 3 ---
> >>> drivers/video/fbsysfs.c | 3 +++
> >>> 2 files changed, 3 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
> >>> index 5aac00e..ad93629 100644
> >>> --- a/drivers/video/fbmem.c
> >>> +++ b/drivers/video/fbmem.c
> >>> @@ -1738,8 +1738,6 @@ void fb_set_suspend(struct fb_info *info, int state)
> >>> {
> >>> struct fb_event event;
> >>>
> >>> - if (!lock_fb_info(info))
> >>> - return;
> >>> event.info = info;
> >>> if (state) {
> >>> fb_notifier_call_chain(FB_EVENT_SUSPEND, &event);
> >>> @@ -1748,7 +1746,6 @@ void fb_set_suspend(struct fb_info *info, int state)
> >>> info->state = FBINFO_STATE_RUNNING;
> >>> fb_notifier_call_chain(FB_EVENT_RESUME, &event);
> >>> }
> >>> - unlock_fb_info(info);
> >>> }
> >>>
> >>> /**
> >>> diff --git a/drivers/video/fbsysfs.c b/drivers/video/fbsysfs.c
> >>> index 04251ce..67afa9c 100644
> >>> --- a/drivers/video/fbsysfs.c
> >>> +++ b/drivers/video/fbsysfs.c
> >>> @@ -399,9 +399,12 @@ static ssize_t store_fbstate(struct device *device,
> >>>
> >>> state = simple_strtoul(buf, &last, 0);
> >>>
> >>> + if (!lock_fb_info(fb_info))
> >>> + return -ENODEV;
> >>> console_lock();
> >>> fb_set_suspend(fb_info, (int)state);
> >>> console_unlock();
> >>> + unlock_fb_info(fb_info);
> >>>
> >>> return count;
> >>> }
> >
> >
> > diff --git a/drivers/video/sh_mobile_hdmi.c b/drivers/video/sh_mobile_hdmi.c
> > index 2b9e56a..b1a13ab 100644
> > --- a/drivers/video/sh_mobile_hdmi.c
> > +++ b/drivers/video/sh_mobile_hdmi.c
> > @@ -1151,27 +1151,27 @@ static void sh_hdmi_edid_work_fn(struct work_struct *work)
> >
> > ch = info->par;
> >
> > - console_lock();
> > + if (lock_fb_info(info)) {
> > + console_lock();
> >
> > - /* HDMI plug in */
> > - if (!sh_hdmi_must_reconfigure(hdmi) &&
> > - info->state = FBINFO_STATE_RUNNING) {
> > - /*
> > - * First activation with the default monitor - just turn
> > - * on, if we run a resume here, the logo disappears
> > - */
> > - if (lock_fb_info(info)) {
> > + /* HDMI plug in */
> > + if (!sh_hdmi_must_reconfigure(hdmi) &&
> > + info->state = FBINFO_STATE_RUNNING) {
> > + /*
> > + * First activation with the default monitor - just turn
> > + * on, if we run a resume here, the logo disappears
> > + */
> > info->var.width = hdmi->var.width;
> > info->var.height = hdmi->var.height;
> > sh_hdmi_display_on(hdmi, info);
> > - unlock_fb_info(info);
> > + } else {
> > + /* New monitor or have to wake up */
> > + fb_set_suspend(info, 0);
> > }
> > - } else {
> > - /* New monitor or have to wake up */
> > - fb_set_suspend(info, 0);
> > - }
> >
> > - console_unlock();
> > + console_unlock();
> > + unlock_fb_info(info);
> > + }
> > } else {
> > ret = 0;
> > if (!hdmi->info)
> > @@ -1181,12 +1181,15 @@ static void sh_hdmi_edid_work_fn(struct work_struct *work)
> > fb_destroy_modedb(hdmi->monspec.modedb);
> > hdmi->monspec.modedb = NULL;
> >
> > - console_lock();
> > + if (lock_fb_info(info)) {
> > + console_lock();
> >
> > - /* HDMI disconnect */
> > - fb_set_suspend(hdmi->info, 1);
> > + /* HDMI disconnect */
> > + fb_set_suspend(hdmi->info, 1);
> >
> > - console_unlock();
> > + console_unlock();
> > + unlock_fb_info(info);
> > + }
> > pm_runtime_put(hdmi->dev);
> > }
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
>
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] fb: avoid possible deadlock caused by fb_set_suspend
2011-09-02 16:06 ` Guennadi Liakhovetski
@ 2011-09-02 16:46 ` Florian Tobias Schandinat
2011-09-02 17:24 ` [PATCH] fb: sh-mobile: Fix deadlock risk between lock_fb_info() and Bruno Prémont
0 siblings, 1 reply; 19+ messages in thread
From: Florian Tobias Schandinat @ 2011-09-02 16:46 UTC (permalink / raw)
To: Bruno Prémont
Cc: Guennadi Liakhovetski, lethal, linux-fbdev, francis.moro,
torvalds, linux-kernel, Herton Ronaldo Krzesinski, stable
Hi Bruno, Guennadi,
On 09/02/2011 04:06 PM, Guennadi Liakhovetski wrote:
> Hi Florian
>
> On Thu, 1 Sep 2011, Florian Tobias Schandinat wrote:
>
>> ping
>>
>> Guennadi, I really want this issue fixed. Please have a look at Bruno's patch
>> otherwise your driver might remain or get even more broken...
>>
>> I am scheduling Herton's patch for the next merge window.
>
> So, the patch looks simple and correct, when applied on top of
> http://marc.info/?l=linux-kernel&m\x130833638508657&w=2
> But it doesn't apply anymore and after fixing a trivial merge conflict, it
> fails to build, which is also trivially fixed. Please, add my
>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
So I guess we can finally solve this issue, thanks.
On the other hand I just noticed the original patch didn't have Bruno's
Signed-off and a commit message was missing. So may I ask you, Bruno, to add
your Signed-off or preferably resend the version tested by Guennadi in the
appropriate patch format.
Thanks,
Florian Tobias Schandinat
>
> to the set and use this updated version:
>
> diff --git a/drivers/video/sh_mobile_hdmi.c b/drivers/video/sh_mobile_hdmi.c
> index 7d54e2c..647ba98 100644
> --- a/drivers/video/sh_mobile_hdmi.c
> +++ b/drivers/video/sh_mobile_hdmi.c
> @@ -1111,6 +1111,7 @@ static long sh_hdmi_clk_configure(struct sh_hdmi *hdmi, unsigned long hdmi_rate,
> static void sh_hdmi_edid_work_fn(struct work_struct *work)
> {
> struct sh_hdmi *hdmi = container_of(work, struct sh_hdmi, edid_work.work);
> + struct fb_info *info;
> struct sh_mobile_hdmi_info *pdata = hdmi->dev->platform_data;
> struct sh_mobile_lcdc_chan *ch;
> int ret;
> @@ -1123,8 +1124,9 @@ static void sh_hdmi_edid_work_fn(struct work_struct *work)
>
> mutex_lock(&hdmi->mutex);
>
> + info = hdmi->info;
> +
> if (hdmi->hp_state = HDMI_HOTPLUG_CONNECTED) {
> - struct fb_info *info = hdmi->info;
> unsigned long parent_rate = 0, hdmi_rate;
>
> ret = sh_hdmi_read_edid(hdmi, &hdmi_rate, &parent_rate);
> @@ -1148,42 +1150,45 @@ static void sh_hdmi_edid_work_fn(struct work_struct *work)
>
> ch = info->par;
>
> - console_lock();
> + if (lock_fb_info(info)) {
> + console_lock();
>
> - /* HDMI plug in */
> - if (!sh_hdmi_must_reconfigure(hdmi) &&
> - info->state = FBINFO_STATE_RUNNING) {
> - /*
> - * First activation with the default monitor - just turn
> - * on, if we run a resume here, the logo disappears
> - */
> - if (lock_fb_info(info)) {
> + /* HDMI plug in */
> + if (!sh_hdmi_must_reconfigure(hdmi) &&
> + info->state = FBINFO_STATE_RUNNING) {
> + /*
> + * First activation with the default monitor - just turn
> + * on, if we run a resume here, the logo disappears
> + */
> info->var.width = hdmi->var.width;
> info->var.height = hdmi->var.height;
> sh_hdmi_display_on(hdmi, info);
> - unlock_fb_info(info);
> + } else {
> + /* New monitor or have to wake up */
> + fb_set_suspend(info, 0);
> }
> - } else {
> - /* New monitor or have to wake up */
> - fb_set_suspend(info, 0);
> - }
>
> - console_unlock();
> + console_unlock();
> + unlock_fb_info(info);
> + }
> } else {
> ret = 0;
> - if (!hdmi->info)
> + if (!info)
> goto out;
>
> hdmi->monspec.modedb_len = 0;
> fb_destroy_modedb(hdmi->monspec.modedb);
> hdmi->monspec.modedb = NULL;
>
> - console_lock();
> + if (lock_fb_info(info)) {
> + console_lock();
>
> - /* HDMI disconnect */
> - fb_set_suspend(hdmi->info, 1);
> + /* HDMI disconnect */
> + fb_set_suspend(info, 1);
>
> - console_unlock();
> + console_unlock();
> + unlock_fb_info(info);
> + }
> }
>
> out:
>
> Thanks
> Guennadi
>
>>
>>
>> Regards,
>>
>> Florian Tobias Schandinat
>>
>>
>> On 06/18/2011 09:19 AM, Bruno Prémont wrote:
>>> Guennadi, could you have a look at (completely untested) patch which avoids
>>> possible deadlock due to inverted lock taking order on hotplug as well
>>> as "readding" lock_fb_info() for fb_set_suspend() call after Herton's
>>> patch to fb_set_suspend().
>>>
>>> Thanks,
>>> Bruno
>>>
>>>
>>> On Sat, 18 June 2011 Bruno Prémont <bonbons@linux-vserver.org> wrote:
>>>> On Fri, 17 June 2011 Florian Tobias Schandinat <FlorianSchandinat@gmx.de> wrote:
>>>>> From: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
>>>>>
>>>>> A lock ordering issue can cause deadlocks: in framebuffer/console code,
>>>>> all needed struct fb_info locks are taken before acquire_console_sem(),
>>>>> in places which need to take console semaphore.
>>>>>
>>>>> But fb_set_suspend is always called with console semaphore held, and
>>>>> inside it we call lock_fb_info which gets the fb_info lock, inverse
>>>>> locking order of what the rest of the code does. This causes a real
>>>>> deadlock issue, when we write to state fb sysfs attribute (which calls
>>>>> fb_set_suspend) while a framebuffer is being unregistered by
>>>>> remove_conflicting_framebuffers, as can be shown by following show
>>>>> blocked state trace on a test program which loads i915 and runs another
>>>>> forked processes writing to state attribute:
>>>>>
>>>>> Test process with semaphore held and trying to get fb_info lock:
>>>>
>>>> ...
>>>>
>>>>> fb-test2 which reproduces above is available on kernel.org bug #26232.
>>>>> To solve this issue, avoid calling lock_fb_info inside fb_set_suspend,
>>>>> and move it out to where needed (callers of fb_set_suspend must call
>>>>> lock_fb_info before if needed). So far, the only place which needs to
>>>>> call lock_fb_info is store_fbstate, all other places which calls
>>>>> fb_set_suspend are suspend/resume hooks that should not need the lock as
>>>>> they should be run only when processes are already frozen in
>>>>> suspend/resume.
>>>>
>>>> From a quick look through FB drivers in 2.6.39 I've found one that would need
>>>> more work:
>>>> - drivers/video/sh_mobile_hdmi.c: sh_hdmi_edid_work_fn()
>>>> Should get changed to
>>>> a) right locking order in case (hdmi->hp_state = HDMI_HOTPLUG_CONNECTED)
>>>> b) lock fb_info in the other case
>>>> For this one fb_set_suspend() does get call in a hotplug worker,
>>>> thus independently on suspend/resume process.
>>>>
>>>> The rest does match the suspend/resume hook pattern mentioned.
>>>>
>>>> Bruno
>>>>
>>>>
>>>>> References: https://bugzilla.kernel.org/show_bug.cgi?id&232
>>>>> Signed-off-by: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
>>>>> Signed-off-by: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
>>>>> Cc: stable@kernel.org
>>>>> ---
>>>>> drivers/video/fbmem.c | 3 ---
>>>>> drivers/video/fbsysfs.c | 3 +++
>>>>> 2 files changed, 3 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
>>>>> index 5aac00e..ad93629 100644
>>>>> --- a/drivers/video/fbmem.c
>>>>> +++ b/drivers/video/fbmem.c
>>>>> @@ -1738,8 +1738,6 @@ void fb_set_suspend(struct fb_info *info, int state)
>>>>> {
>>>>> struct fb_event event;
>>>>>
>>>>> - if (!lock_fb_info(info))
>>>>> - return;
>>>>> event.info = info;
>>>>> if (state) {
>>>>> fb_notifier_call_chain(FB_EVENT_SUSPEND, &event);
>>>>> @@ -1748,7 +1746,6 @@ void fb_set_suspend(struct fb_info *info, int state)
>>>>> info->state = FBINFO_STATE_RUNNING;
>>>>> fb_notifier_call_chain(FB_EVENT_RESUME, &event);
>>>>> }
>>>>> - unlock_fb_info(info);
>>>>> }
>>>>>
>>>>> /**
>>>>> diff --git a/drivers/video/fbsysfs.c b/drivers/video/fbsysfs.c
>>>>> index 04251ce..67afa9c 100644
>>>>> --- a/drivers/video/fbsysfs.c
>>>>> +++ b/drivers/video/fbsysfs.c
>>>>> @@ -399,9 +399,12 @@ static ssize_t store_fbstate(struct device *device,
>>>>>
>>>>> state = simple_strtoul(buf, &last, 0);
>>>>>
>>>>> + if (!lock_fb_info(fb_info))
>>>>> + return -ENODEV;
>>>>> console_lock();
>>>>> fb_set_suspend(fb_info, (int)state);
>>>>> console_unlock();
>>>>> + unlock_fb_info(fb_info);
>>>>>
>>>>> return count;
>>>>> }
>>>
>>>
>>> diff --git a/drivers/video/sh_mobile_hdmi.c b/drivers/video/sh_mobile_hdmi.c
>>> index 2b9e56a..b1a13ab 100644
>>> --- a/drivers/video/sh_mobile_hdmi.c
>>> +++ b/drivers/video/sh_mobile_hdmi.c
>>> @@ -1151,27 +1151,27 @@ static void sh_hdmi_edid_work_fn(struct work_struct *work)
>>>
>>> ch = info->par;
>>>
>>> - console_lock();
>>> + if (lock_fb_info(info)) {
>>> + console_lock();
>>>
>>> - /* HDMI plug in */
>>> - if (!sh_hdmi_must_reconfigure(hdmi) &&
>>> - info->state = FBINFO_STATE_RUNNING) {
>>> - /*
>>> - * First activation with the default monitor - just turn
>>> - * on, if we run a resume here, the logo disappears
>>> - */
>>> - if (lock_fb_info(info)) {
>>> + /* HDMI plug in */
>>> + if (!sh_hdmi_must_reconfigure(hdmi) &&
>>> + info->state = FBINFO_STATE_RUNNING) {
>>> + /*
>>> + * First activation with the default monitor - just turn
>>> + * on, if we run a resume here, the logo disappears
>>> + */
>>> info->var.width = hdmi->var.width;
>>> info->var.height = hdmi->var.height;
>>> sh_hdmi_display_on(hdmi, info);
>>> - unlock_fb_info(info);
>>> + } else {
>>> + /* New monitor or have to wake up */
>>> + fb_set_suspend(info, 0);
>>> }
>>> - } else {
>>> - /* New monitor or have to wake up */
>>> - fb_set_suspend(info, 0);
>>> - }
>>>
>>> - console_unlock();
>>> + console_unlock();
>>> + unlock_fb_info(info);
>>> + }
>>> } else {
>>> ret = 0;
>>> if (!hdmi->info)
>>> @@ -1181,12 +1181,15 @@ static void sh_hdmi_edid_work_fn(struct work_struct *work)
>>> fb_destroy_modedb(hdmi->monspec.modedb);
>>> hdmi->monspec.modedb = NULL;
>>>
>>> - console_lock();
>>> + if (lock_fb_info(info)) {
>>> + console_lock();
>>>
>>> - /* HDMI disconnect */
>>> - fb_set_suspend(hdmi->info, 1);
>>> + /* HDMI disconnect */
>>> + fb_set_suspend(hdmi->info, 1);
>>>
>>> - console_unlock();
>>> + console_unlock();
>>> + unlock_fb_info(info);
>>> + }
>>> pm_runtime_put(hdmi->dev);
>>> }
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>
>
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH] fb: sh-mobile: Fix deadlock risk between lock_fb_info() and
2011-09-02 16:46 ` Florian Tobias Schandinat
@ 2011-09-02 17:24 ` Bruno Prémont
2011-09-02 20:54 ` [PATCH] fb: sh-mobile: Fix deadlock risk between lock_fb_info() Florian Tobias Schandinat
0 siblings, 1 reply; 19+ messages in thread
From: Bruno Prémont @ 2011-09-02 17:24 UTC (permalink / raw)
To: Florian Tobias Schandinat
Cc: Guennadi Liakhovetski, lethal, linux-fbdev, francis.moro,
torvalds, linux-kernel, Herton Ronaldo Krzesinski, stable
Following on Herton's patch "fb: avoid possible deadlock caused by
fb_set_suspend" which moves lock_fb_info() out of fb_set_suspend()
to its callers, correct sh-mobile's locking around call to
fb_set_suspend() and the same sort of deaklocks with console_lock()
due to order of taking the lock.
console_lock() must be taken while fb_info is already locked and fb_info
must be locked while calling fb_set_suspend().
Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
Hi Florian,
> On the other hand I just noticed the original patch didn't have
> Bruno's Signed-off and a commit message was missing. So may I ask you,
> Bruno, to add your Signed-off or preferably resend the version tested
> by Guennadi in the appropriate patch format.
Here it is with commit message and the Signed-off-by's.
Thanks,
Bruno
drivers/video/sh_mobile_hdmi.c | 47 ++++++++++++++++++++++-----------------
1 files changed, 26 insertions(+), 21 deletions(-)
diff --git a/drivers/video/sh_mobile_hdmi.c b/drivers/video/sh_mobile_hdmi.c
index 7d54e2c..647ba98 100644
--- a/drivers/video/sh_mobile_hdmi.c
+++ b/drivers/video/sh_mobile_hdmi.c
@@ -1111,6 +1111,7 @@ static long sh_hdmi_clk_configure(struct sh_hdmi *hdmi, unsigned long hdmi_rate,
static void sh_hdmi_edid_work_fn(struct work_struct *work)
{
struct sh_hdmi *hdmi = container_of(work, struct sh_hdmi, edid_work.work);
+ struct fb_info *info;
struct sh_mobile_hdmi_info *pdata = hdmi->dev->platform_data;
struct sh_mobile_lcdc_chan *ch;
int ret;
@@ -1123,8 +1124,9 @@ static void sh_hdmi_edid_work_fn(struct work_struct *work)
mutex_lock(&hdmi->mutex);
+ info = hdmi->info;
+
if (hdmi->hp_state = HDMI_HOTPLUG_CONNECTED) {
- struct fb_info *info = hdmi->info;
unsigned long parent_rate = 0, hdmi_rate;
ret = sh_hdmi_read_edid(hdmi, &hdmi_rate, &parent_rate);
@@ -1148,42 +1150,45 @@ static void sh_hdmi_edid_work_fn(struct work_struct *work)
ch = info->par;
- console_lock();
+ if (lock_fb_info(info)) {
+ console_lock();
- /* HDMI plug in */
- if (!sh_hdmi_must_reconfigure(hdmi) &&
- info->state = FBINFO_STATE_RUNNING) {
- /*
- * First activation with the default monitor - just turn
- * on, if we run a resume here, the logo disappears
- */
- if (lock_fb_info(info)) {
+ /* HDMI plug in */
+ if (!sh_hdmi_must_reconfigure(hdmi) &&
+ info->state = FBINFO_STATE_RUNNING) {
+ /*
+ * First activation with the default monitor - just turn
+ * on, if we run a resume here, the logo disappears
+ */
info->var.width = hdmi->var.width;
info->var.height = hdmi->var.height;
sh_hdmi_display_on(hdmi, info);
- unlock_fb_info(info);
+ } else {
+ /* New monitor or have to wake up */
+ fb_set_suspend(info, 0);
}
- } else {
- /* New monitor or have to wake up */
- fb_set_suspend(info, 0);
- }
- console_unlock();
+ console_unlock();
+ unlock_fb_info(info);
+ }
} else {
ret = 0;
- if (!hdmi->info)
+ if (!info)
goto out;
hdmi->monspec.modedb_len = 0;
fb_destroy_modedb(hdmi->monspec.modedb);
hdmi->monspec.modedb = NULL;
- console_lock();
+ if (lock_fb_info(info)) {
+ console_lock();
- /* HDMI disconnect */
- fb_set_suspend(hdmi->info, 1);
+ /* HDMI disconnect */
+ fb_set_suspend(info, 1);
- console_unlock();
+ console_unlock();
+ unlock_fb_info(info);
+ }
}
out:
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] fb: sh-mobile: Fix deadlock risk between lock_fb_info()
2011-09-02 17:24 ` [PATCH] fb: sh-mobile: Fix deadlock risk between lock_fb_info() and Bruno Prémont
@ 2011-09-02 20:54 ` Florian Tobias Schandinat
0 siblings, 0 replies; 19+ messages in thread
From: Florian Tobias Schandinat @ 2011-09-02 20:54 UTC (permalink / raw)
To: Bruno Prémont
Cc: Guennadi Liakhovetski, lethal, linux-fbdev, francis.moro,
torvalds, linux-kernel, Herton Ronaldo Krzesinski, stable
On 09/02/2011 05:24 PM, Bruno Prémont wrote:
> Following on Herton's patch "fb: avoid possible deadlock caused by
> fb_set_suspend" which moves lock_fb_info() out of fb_set_suspend()
> to its callers, correct sh-mobile's locking around call to
> fb_set_suspend() and the same sort of deaklocks with console_lock()
> due to order of taking the lock.
>
> console_lock() must be taken while fb_info is already locked and fb_info
> must be locked while calling fb_set_suspend().
>
> Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
>
> Hi Florian,
>
>> On the other hand I just noticed the original patch didn't have
>> Bruno's Signed-off and a commit message was missing. So may I ask you,
>> Bruno, to add your Signed-off or preferably resend the version tested
>> by Guennadi in the appropriate patch format.
>
> Here it is with commit message and the Signed-off-by's.
Applied and added stable as Cc as I think this should be applied as well
whenever Herton's patch is applied.
Thanks,
Florian Tobias Schandinat
>
> Thanks,
> Bruno
>
>
>
>
> drivers/video/sh_mobile_hdmi.c | 47 ++++++++++++++++++++++-----------------
> 1 files changed, 26 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/video/sh_mobile_hdmi.c b/drivers/video/sh_mobile_hdmi.c
> index 7d54e2c..647ba98 100644
> --- a/drivers/video/sh_mobile_hdmi.c
> +++ b/drivers/video/sh_mobile_hdmi.c
> @@ -1111,6 +1111,7 @@ static long sh_hdmi_clk_configure(struct sh_hdmi *hdmi, unsigned long hdmi_rate,
> static void sh_hdmi_edid_work_fn(struct work_struct *work)
> {
> struct sh_hdmi *hdmi = container_of(work, struct sh_hdmi, edid_work.work);
> + struct fb_info *info;
> struct sh_mobile_hdmi_info *pdata = hdmi->dev->platform_data;
> struct sh_mobile_lcdc_chan *ch;
> int ret;
> @@ -1123,8 +1124,9 @@ static void sh_hdmi_edid_work_fn(struct work_struct *work)
>
> mutex_lock(&hdmi->mutex);
>
> + info = hdmi->info;
> +
> if (hdmi->hp_state = HDMI_HOTPLUG_CONNECTED) {
> - struct fb_info *info = hdmi->info;
> unsigned long parent_rate = 0, hdmi_rate;
>
> ret = sh_hdmi_read_edid(hdmi, &hdmi_rate, &parent_rate);
> @@ -1148,42 +1150,45 @@ static void sh_hdmi_edid_work_fn(struct work_struct *work)
>
> ch = info->par;
>
> - console_lock();
> + if (lock_fb_info(info)) {
> + console_lock();
>
> - /* HDMI plug in */
> - if (!sh_hdmi_must_reconfigure(hdmi) &&
> - info->state = FBINFO_STATE_RUNNING) {
> - /*
> - * First activation with the default monitor - just turn
> - * on, if we run a resume here, the logo disappears
> - */
> - if (lock_fb_info(info)) {
> + /* HDMI plug in */
> + if (!sh_hdmi_must_reconfigure(hdmi) &&
> + info->state = FBINFO_STATE_RUNNING) {
> + /*
> + * First activation with the default monitor - just turn
> + * on, if we run a resume here, the logo disappears
> + */
> info->var.width = hdmi->var.width;
> info->var.height = hdmi->var.height;
> sh_hdmi_display_on(hdmi, info);
> - unlock_fb_info(info);
> + } else {
> + /* New monitor or have to wake up */
> + fb_set_suspend(info, 0);
> }
> - } else {
> - /* New monitor or have to wake up */
> - fb_set_suspend(info, 0);
> - }
>
> - console_unlock();
> + console_unlock();
> + unlock_fb_info(info);
> + }
> } else {
> ret = 0;
> - if (!hdmi->info)
> + if (!info)
> goto out;
>
> hdmi->monspec.modedb_len = 0;
> fb_destroy_modedb(hdmi->monspec.modedb);
> hdmi->monspec.modedb = NULL;
>
> - console_lock();
> + if (lock_fb_info(info)) {
> + console_lock();
>
> - /* HDMI disconnect */
> - fb_set_suspend(hdmi->info, 1);
> + /* HDMI disconnect */
> + fb_set_suspend(info, 1);
>
> - console_unlock();
> + console_unlock();
> + unlock_fb_info(info);
> + }
> }
>
> out:
>
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2011-09-02 20:54 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <BANLkTiko+2AM8dq8jP5xjbTcCM63hBAK=A@mail.gmail.com>
2011-06-14 18:15 ` Possible deadlock when suspending framebuffer Linus Torvalds
2011-06-14 19:04 ` Florian Tobias Schandinat
2011-06-17 18:46 ` [PATCH] fb: avoid possible deadlock caused by fb_set_suspend Florian Tobias Schandinat
2011-06-18 8:43 ` Bruno Prémont
2011-06-18 9:19 ` Bruno Prémont
2011-09-01 15:42 ` Florian Tobias Schandinat
2011-09-01 16:28 ` Guennadi Liakhovetski
2011-09-02 16:06 ` Guennadi Liakhovetski
2011-09-02 16:46 ` Florian Tobias Schandinat
2011-09-02 17:24 ` [PATCH] fb: sh-mobile: Fix deadlock risk between lock_fb_info() and Bruno Prémont
2011-09-02 20:54 ` [PATCH] fb: sh-mobile: Fix deadlock risk between lock_fb_info() Florian Tobias Schandinat
2011-07-20 18:16 ` [PATCH] fb: avoid possible deadlock caused by fb_set_suspend Florian Tobias Schandinat
2011-07-28 22:10 ` Francis Moreau
2011-06-15 1:09 ` re:Possible deadlock when suspending framebuffer Wanlong Gao
2011-06-15 5:58 ` Possible " Bruno Prémont
2011-06-15 6:22 ` Wanlong Gao
2011-06-15 7:04 ` Américo Wang
2011-06-15 7:12 ` Francis Moreau
2011-06-15 10:20 ` Bruno Prémont
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).