* Possible deadlock when suspending framebuffer
@ 2011-06-14 13:10 Francis Moreau
2011-06-14 18:15 ` Linus Torvalds
0 siblings, 1 reply; 39+ messages in thread
From: Francis Moreau @ 2011-06-14 13:10 UTC (permalink / raw)
To: Linux Kernel Mailing List, Linus Torvalds
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] 39+ messages in thread* Re: Possible deadlock when suspending framebuffer 2011-06-14 13:10 Possible deadlock when suspending framebuffer Francis Moreau @ 2011-06-14 18:15 ` Linus Torvalds 0 siblings, 0 replies; 39+ 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] 39+ messages in thread
* Re: Possible deadlock when suspending framebuffer @ 2011-06-14 18:15 ` Linus Torvalds 0 siblings, 0 replies; 39+ 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] 39+ messages in thread
* Re: Possible deadlock when suspending framebuffer 2011-06-14 18:15 ` Linus Torvalds @ 2011-06-14 19:04 ` Florian Tobias Schandinat -1 siblings, 0 replies; 39+ 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] 39+ messages in thread
* Re: Possible deadlock when suspending framebuffer @ 2011-06-14 19:04 ` Florian Tobias Schandinat 0 siblings, 0 replies; 39+ 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=129539210207429&w=2 http://marc.info/?l=linux-fbdev&m=129700789632450&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] 39+ messages in thread
* [PATCH] fb: avoid possible deadlock caused by fb_set_suspend 2011-06-14 19:04 ` Florian Tobias Schandinat @ 2011-06-17 19:02 ` Florian Tobias Schandinat -1 siblings, 0 replies; 39+ 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] 39+ messages in thread
* [PATCH] fb: avoid possible deadlock caused by fb_set_suspend @ 2011-06-17 19:02 ` Florian Tobias Schandinat 0 siblings, 0 replies; 39+ messages in thread From: Florian Tobias Schandinat @ 2011-06-17 19:02 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=26232 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] 39+ messages in thread
* Re: [PATCH] fb: avoid possible deadlock caused by fb_set_suspend 2011-06-17 19:02 ` Florian Tobias Schandinat @ 2011-06-18 8:43 ` Bruno Prémont -1 siblings, 0 replies; 39+ 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] 39+ messages in thread
* Re: [PATCH] fb: avoid possible deadlock caused by fb_set_suspend @ 2011-06-18 8:43 ` Bruno Prémont 0 siblings, 0 replies; 39+ 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=26232 > 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] 39+ 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 -1 siblings, 0 replies; 39+ 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] 39+ messages in thread
* Re: [PATCH] fb: avoid possible deadlock caused by fb_set_suspend @ 2011-06-18 9:19 ` Bruno Prémont 0 siblings, 0 replies; 39+ 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=26232 > > 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] 39+ 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 -1 siblings, 0 replies; 39+ 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] 39+ messages in thread
* Re: [PATCH] fb: avoid possible deadlock caused by fb_set_suspend @ 2011-09-01 15:42 ` Florian Tobias Schandinat 0 siblings, 0 replies; 39+ 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=26232 >>> 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] 39+ 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 -1 siblings, 0 replies; 39+ 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] 39+ messages in thread
* Re: [PATCH] fb: avoid possible deadlock caused by fb_set_suspend @ 2011-09-01 16:28 ` Guennadi Liakhovetski 0 siblings, 0 replies; 39+ 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=26232 > >>> 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] 39+ messages in thread
* Re: [PATCH] fb: avoid possible deadlock caused by fb_set_suspend 2011-09-01 15:42 ` Florian Tobias Schandinat @ 2011-09-02 16:06 ` Guennadi Liakhovetski -1 siblings, 0 replies; 39+ 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] 39+ messages in thread
* Re: [PATCH] fb: avoid possible deadlock caused by fb_set_suspend @ 2011-09-02 16:06 ` Guennadi Liakhovetski 0 siblings, 0 replies; 39+ 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=130833638508657&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=26232 > >>> 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] 39+ 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 -1 siblings, 0 replies; 39+ 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] 39+ messages in thread
* Re: [PATCH] fb: avoid possible deadlock caused by fb_set_suspend @ 2011-09-02 16:46 ` Florian Tobias Schandinat 0 siblings, 0 replies; 39+ 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=130833638508657&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=26232 >>>>> 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] 39+ 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 -1 siblings, 0 replies; 39+ 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] 39+ messages in thread
* [PATCH] fb: sh-mobile: Fix deadlock risk between lock_fb_info() and console_lock() @ 2011-09-02 17:24 ` Bruno Prémont 0 siblings, 0 replies; 39+ 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] 39+ 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 console_lock() Bruno Prémont @ 2011-09-02 20:54 ` Florian Tobias Schandinat -1 siblings, 0 replies; 39+ 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] 39+ messages in thread
* Re: [PATCH] fb: sh-mobile: Fix deadlock risk between lock_fb_info() and console_lock() @ 2011-09-02 20:54 ` Florian Tobias Schandinat 0 siblings, 0 replies; 39+ 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] 39+ messages in thread
* Re: [PATCH] fb: avoid possible deadlock caused by fb_set_suspend 2011-06-17 19:02 ` Florian Tobias Schandinat @ 2011-07-20 18:16 ` Florian Tobias Schandinat -1 siblings, 0 replies; 39+ 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] 39+ messages in thread
* Re: [PATCH] fb: avoid possible deadlock caused by fb_set_suspend @ 2011-07-20 18:16 ` Florian Tobias Schandinat 0 siblings, 0 replies; 39+ 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=26232 > 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] 39+ messages in thread
* Re: [PATCH] fb: avoid possible deadlock caused by fb_set_suspend 2011-07-20 18:16 ` Florian Tobias Schandinat @ 2011-07-28 22:10 ` Francis Moreau -1 siblings, 0 replies; 39+ 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] 39+ messages in thread
* Re: [PATCH] fb: avoid possible deadlock caused by fb_set_suspend @ 2011-07-28 22:10 ` Francis Moreau 0 siblings, 0 replies; 39+ 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] 39+ messages in thread
* re:Possible deadlock when suspending framebuffer @ 2011-06-15 1:09 ` Wanlong Gao 0 siblings, 0 replies; 39+ 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] 39+ messages in thread
* re:Possible deadlock when suspending framebuffer @ 2011-06-15 1:09 ` Wanlong Gao 0 siblings, 0 replies; 39+ 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, Francis Moreau, Linus Torvalds [-- Attachment #1: Type: text/plain, Size: 885 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: 836 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] 39+ messages in thread
* Re: Possible deadlock when suspending framebuffer 2011-06-15 1:09 ` Wanlong Gao @ 2011-06-15 5:58 ` Bruno Prémont -1 siblings, 0 replies; 39+ 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] 39+ messages in thread
* Re: Possible deadlock when suspending framebuffer @ 2011-06-15 5:58 ` Bruno Prémont 0 siblings, 0 replies; 39+ 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] 39+ messages in thread
* Re: Possible deadlock when suspending framebuffer 2011-06-15 5:58 ` Bruno Prémont @ 2011-06-15 6:22 ` Wanlong Gao -1 siblings, 0 replies; 39+ 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] 39+ messages in thread
* Re: Possible deadlock when suspending framebuffer @ 2011-06-15 6:22 ` Wanlong Gao 0 siblings, 0 replies; 39+ 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] 39+ messages in thread
* Re: Possible deadlock when suspending framebuffer 2011-06-15 6:22 ` Wanlong Gao @ 2011-06-15 7:04 ` Américo Wang -1 siblings, 0 replies; 39+ 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] 39+ messages in thread
* Re: Possible deadlock when suspending framebuffer @ 2011-06-15 7:04 ` Américo Wang 0 siblings, 0 replies; 39+ 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] 39+ messages in thread
* Re: Possible deadlock when suspending framebuffer 2011-06-15 5:58 ` Bruno Prémont @ 2011-06-15 7:12 ` Francis Moreau -1 siblings, 0 replies; 39+ 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] 39+ messages in thread
* Re: Possible deadlock when suspending framebuffer @ 2011-06-15 7:12 ` Francis Moreau 0 siblings, 0 replies; 39+ 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] 39+ messages in thread
* Re: Possible deadlock when suspending framebuffer 2011-06-15 7:12 ` Francis Moreau @ 2011-06-15 10:20 ` Bruno Prémont -1 siblings, 0 replies; 39+ 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] 39+ messages in thread
* Re: Possible deadlock when suspending framebuffer @ 2011-06-15 10:20 ` Bruno Prémont 0 siblings, 0 replies; 39+ 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] 39+ messages in thread
end of thread, other threads:[~2011-09-02 20:54 UTC | newest] Thread overview: 39+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-06-14 13:10 Possible deadlock when suspending framebuffer Francis Moreau 2011-06-14 18:15 ` Linus Torvalds 2011-06-14 18:15 ` Linus Torvalds 2011-06-14 19:04 ` Florian Tobias Schandinat 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-17 19:02 ` Florian Tobias Schandinat 2011-06-18 8:43 ` Bruno Prémont 2011-06-18 8:43 ` Bruno Prémont 2011-06-18 9:19 ` Bruno Prémont 2011-06-18 9:19 ` Bruno Prémont 2011-09-01 15:42 ` Florian Tobias Schandinat 2011-09-01 15:42 ` Florian Tobias Schandinat 2011-09-01 16:28 ` Guennadi Liakhovetski 2011-09-01 16:28 ` Guennadi Liakhovetski 2011-09-02 16:06 ` Guennadi Liakhovetski 2011-09-02 16:06 ` Guennadi Liakhovetski 2011-09-02 16:46 ` Florian Tobias Schandinat 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 17:24 ` [PATCH] fb: sh-mobile: Fix deadlock risk between lock_fb_info() and console_lock() Bruno Prémont 2011-09-02 20:54 ` [PATCH] fb: sh-mobile: Fix deadlock risk between lock_fb_info() Florian Tobias Schandinat 2011-09-02 20:54 ` [PATCH] fb: sh-mobile: Fix deadlock risk between lock_fb_info() and console_lock() Florian Tobias Schandinat 2011-07-20 18:16 ` [PATCH] fb: avoid possible deadlock caused by fb_set_suspend Florian Tobias Schandinat 2011-07-20 18:16 ` Florian Tobias Schandinat 2011-07-28 22:10 ` Francis Moreau 2011-07-28 22:10 ` Francis Moreau 2011-06-15 1:09 ` re:Possible deadlock when suspending framebuffer Wanlong Gao 2011-06-15 1:09 ` Wanlong Gao 2011-06-15 5:58 ` Possible " Bruno Prémont 2011-06-15 5:58 ` Bruno Prémont 2011-06-15 6:22 ` Wanlong Gao 2011-06-15 6:22 ` Wanlong Gao 2011-06-15 7:04 ` Américo Wang 2011-06-15 7:04 ` Américo Wang 2011-06-15 7:12 ` Francis Moreau 2011-06-15 7:12 ` Francis Moreau 2011-06-15 10:20 ` Bruno Prémont 2011-06-15 10:20 ` Bruno Prémont
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.