On Fri, 17 June 2011 Florian Tobias Schandinat <FlorianSchandinat@xxxxxx> wrote: > From: Herton Ronaldo Krzesinski <herton@xxxxxxxxxxxxxxx> > > 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@xxxxxxxxxxxxxxx> > Signed-off-by: Florian Tobias Schandinat <FlorianSchandinat@xxxxxx> > Cc: stable@xxxxxxxxxx > --- > 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
