From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753043Ab1GTSQZ (ORCPT ); Wed, 20 Jul 2011 14:16:25 -0400 Received: from mailout-de.gmx.net ([213.165.64.23]:60906 "HELO mailout-de.gmx.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1752649Ab1GTSQX (ORCPT ); Wed, 20 Jul 2011 14:16:23 -0400 X-Authenticated: #10250065 X-Provags-ID: V01U2FsdGVkX193SM8L48B3CJ23/4cvIZ8EfU8d5cdm4eSfu+FPeg z31SAcMpKNGM9V Message-ID: <4E271B72.3070909@gmx.de> Date: Wed, 20 Jul 2011 18:16:18 +0000 From: Florian Tobias Schandinat User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.16) Gecko/20110702 Icedove/3.0.11 MIME-Version: 1.0 To: lethal@linux-sh.org CC: linux-fbdev@vger.kernel.org, francis.moro@gmail.com, torvalds@linux-foundation.org, bonbons@linux-vserver.org, linux-kernel@vger.kernel.org, Herton Ronaldo Krzesinski , stable@kernel.org Subject: Re: [PATCH] fb: avoid possible deadlock caused by fb_set_suspend References: <4DF7B0B4.4060002@gmx.de> <1308337359-3480-1-git-send-email-FlorianSchandinat@gmx.de> In-Reply-To: <1308337359-3480-1-git-send-email-FlorianSchandinat@gmx.de> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-Y-GMX-Trusted: 0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > > 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: > [] __mutex_lock_slowpath+0x11a/0x1e0 > [] ? _raw_spin_lock_irq+0x22/0x40 > [] mutex_lock+0x23/0x50 > [] lock_fb_info+0x25/0x60 > [] fb_set_suspend+0x20/0x80 > [] store_fbstate+0x4f/0x70 > [] dev_attr_store+0x20/0x30 > [] sysfs_write_file+0xd4/0x160 > [] vfs_write+0xc6/0x190 > [] sys_write+0x51/0x90 > [] 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: > [] schedule_timeout+0x215/0x310 > [] ? get_parent_ip+0x11/0x50 > [] __down+0x6d/0xb0 > [] down+0x41/0x50 > [] acquire_console_sem+0x2c/0x50 > [] unbind_con_driver+0xad/0x2d0 > [] fbcon_event_notify+0x457/0x890 > [] ? _raw_spin_unlock_irqrestore+0x1f/0x50 > [] ? get_parent_ip+0x11/0x50 > [] notifier_call_chain+0x4d/0x70 > [] __blocking_notifier_call_chain+0x58/0x80 > [] blocking_notifier_call_chain+0x16/0x20 > [] fb_notifier_call_chain+0x1b/0x20 > [] unregister_framebuffer+0x7c/0x130 > [] remove_conflicting_framebuffers+0x153/0x180 > [] register_framebuffer+0x93/0x2c0 > [] drm_fb_helper_single_fb_probe+0x252/0x2f0 [drm_kms_helper] > [] drm_fb_helper_initial_config+0x2f3/0x6d0 [drm_kms_helper] > [] ? drm_fb_helper_single_add_all_connectors+0x5d/0x1c0 [drm_kms_helper] > [] intel_fbdev_init+0xa8/0x160 [i915] > [] i915_driver_load+0x854/0x12b0 [i915] > [] drm_get_pci_dev+0x19e/0x360 [drm] > [] ? sub_preempt_count+0x9d/0xd0 > [] i915_pci_probe+0x15/0x17 [i915] > [] local_pci_probe+0x5f/0xd0 > [] pci_device_probe+0x119/0x120 > [] ? driver_sysfs_add+0x7a/0xb0 > [] driver_probe_device+0xa3/0x290 > [] ? __driver_attach+0x0/0xb0 > [] __driver_attach+0xab/0xb0 > [] ? __driver_attach+0x0/0xb0 > [] bus_for_each_dev+0x5e/0x90 > [] driver_attach+0x1e/0x20 > [] bus_add_driver+0xe2/0x320 > [] ? i915_init+0x0/0x96 [i915] > [] driver_register+0x76/0x140 > [] ? i915_init+0x0/0x96 [i915] > [] __pci_register_driver+0x56/0xd0 > [] drm_pci_init+0xe4/0xf0 [drm] > [] ? i915_init+0x0/0x96 [i915] > [] drm_init+0x58/0x70 [drm] > [] i915_init+0x94/0x96 [i915] > [] do_one_initcall+0x44/0x190 > [] sys_init_module+0xcb/0x210 > [] 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 > Signed-off-by: Florian Tobias Schandinat > 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; > }