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 <bonb...@linux-vserver.org> wrote: > On Fri, 17 June 2011 Florian Tobias Schandinat <florianschandi...@gmx.de> > wrote: > > From: Herton Ronaldo Krzesinski <her...@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 <her...@mandriva.com.br> > > Signed-off-by: Florian Tobias Schandinat <florianschandi...@gmx.de> > > Cc: sta...@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); } _______________________________________________ stable mailing list stable@linux.kernel.org http://linux.kernel.org/mailman/listinfo/stable
