From patchwork Sat Jun 18 09:19:34 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Bruno_Pr=C3=A9mont?= X-Patchwork-Id: 892972 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by demeter2.kernel.org (8.14.4/8.14.4) with ESMTP id p5I9KC84031392 for ; Sat, 18 Jun 2011 09:20:12 GMT Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752875Ab1FRJT7 (ORCPT ); Sat, 18 Jun 2011 05:19:59 -0400 Received: from smtprelay.restena.lu ([158.64.1.62]:60261 "EHLO smtprelay.restena.lu" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751359Ab1FRJT6 convert rfc822-to-8bit (ORCPT ); Sat, 18 Jun 2011 05:19:58 -0400 Received: from smtprelay.restena.lu (localhost [127.0.0.1]) by smtprelay.restena.lu (Postfix) with ESMTP id 579F610582; Sat, 18 Jun 2011 11:19:57 +0200 (CEST) Received: from neptune.home (unknown [IPv6:2001:a18:1:1402:2c0:9fff:fe2d:39d]) by smtprelay.restena.lu (Postfix) with ESMTP id D980610581; Sat, 18 Jun 2011 11:19:56 +0200 (CEST) Date: Sat, 18 Jun 2011 11:19:34 +0200 From: Bruno =?UTF-8?B?UHLDqW1vbnQ=?= To: Guennadi Liakhovetski Cc: Florian Tobias Schandinat , lethal@linux-sh.org, linux-fbdev@vger.kernel.org, francis.moro@gmail.com, torvalds@linux-foundation.org, linux-kernel@vger.kernel.org, Herton Ronaldo Krzesinski , stable@kernel.org Subject: Re: [PATCH] fb: avoid possible deadlock caused by fb_set_suspend Message-ID: <20110618111934.26203dbf@neptune.home> In-Reply-To: <20110618104311.6d80ba50@neptune.home> References: <4DF7B0B4.4060002@gmx.de> <1308337359-3480-1-git-send-email-FlorianSchandinat@gmx.de> <20110618104311.6d80ba50@neptune.home> X-Mailer: Claws Mail 3.7.9 (GTK+ 2.22.1; i686-pc-linux-gnu) Mime-Version: 1.0 X-Virus-Scanned: ClamAV Sender: linux-fbdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fbdev@vger.kernel.org X-Greylist: IP, sender and recipient auto-whitelisted, not delayed by milter-greylist-4.2.6 (demeter2.kernel.org [140.211.167.43]); Sat, 18 Jun 2011 09:20:12 +0000 (UTC) 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 wrote: > On Fri, 17 June 2011 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 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 > > 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; > > } --- 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 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); }