From patchwork Fri Sep 2 16:06:49 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Guennadi Liakhovetski X-Patchwork-Id: 1122632 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 p82G7Ten013798 for ; Fri, 2 Sep 2011 16:07:29 GMT Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752754Ab1IBQH2 (ORCPT ); Fri, 2 Sep 2011 12:07:28 -0400 Received: from moutng.kundenserver.de ([212.227.17.9]:61117 "EHLO moutng.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752645Ab1IBQH1 convert rfc822-to-8bit (ORCPT ); Fri, 2 Sep 2011 12:07:27 -0400 Received: from axis700.grange (dslb-188-109-038-002.pools.arcor-ip.net [188.109.38.2]) by mrelayeu.kundenserver.de (node=mrbap1) with ESMTP (Nemesis) id 0LgvGY-1RVlxE0Ot4-00o5TZ; Fri, 02 Sep 2011 18:06:50 +0200 Received: by axis700.grange (Postfix, from userid 1000) id B6DFD189B83; Fri, 2 Sep 2011 18:06:49 +0200 (CEST) Received: from localhost (localhost [127.0.0.1]) by axis700.grange (Postfix) with ESMTP id AA8F3189B6F; Fri, 2 Sep 2011 18:06:49 +0200 (CEST) Date: Fri, 2 Sep 2011 18:06:49 +0200 (CEST) From: Guennadi Liakhovetski X-X-Sender: lyakh@axis700.grange To: Florian Tobias Schandinat cc: =?UTF-8?B?QnJ1bm8gUHLDqW1vbnQ=?= , 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 In-Reply-To: <4E5FA7F9.9030802@gmx.de> Message-ID: References: <4DF7B0B4.4060002@gmx.de> <1308337359-3480-1-git-send-email-FlorianSchandinat@gmx.de> <20110618104311.6d80ba50@neptune.home> <20110618111934.26203dbf@neptune.home> <4E5FA7F9.9030802@gmx.de> MIME-Version: 1.0 X-Provags-ID: V02:K0:ftGpDj5Vgqn5oQosNiqiSuIek5FIl7sao20lhYTbPB5 XprC141pnCSpKcTkeZROs3kLmv0KCpKLoZ+iPgrUfv1SVKDJFj PrvuM7lQCn1HLJj36ojOlfTWkHxLBvi2SDMqsYXLa0N8XZbxID 1ecZrmt5Pbxv6/WqzTp8fe/J1IZN6y0L9ke9QlNVgiDIekCJqQ g+TiG2Wtv8s/bBRWDfISdqfS+AZ/L4529PczZlR881lE1Y4c02 YxsQNl50SnapwuKy1swPzHaQTzrwx94awJAwYQ4bO5/QGXkSKl 2QG1Bf4lKG7ZWKWx1t3gnW82UqyCtXLqxj1LNuTtnAo3e+3UFx U0uglKkTuQ5GfN1R3dyplFqjjIQIvKN/wVzqu8US+YVDubcWTV GgOBuof88H25g== 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]); Fri, 02 Sep 2011 16:07:29 +0000 (UTC) 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 to the set and use this updated version: 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 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; > >>> } > > > > > > 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/ -- 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 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: