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. 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 <bonbons@xxxxxxxxxxxxxxxxx> wrote: >> 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; >>> } > > > 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@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- 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
