lkml.org 
[lkml]   [2011]   [Sep]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH] fb: avoid possible deadlock caused by fb_set_suspend
    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 <g.liakhovetski@gmx.de>

    to the set and use this updated version:

    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:
    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 <bonbons@linux-vserver.org> wrote:
    > >> On Fri, 17 June 2011 Florian Tobias Schandinat <FlorianSchandinat@gmx.de> wrote:
    > >>> From: Herton Ronaldo Krzesinski <herton@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 <herton@mandriva.com.br>
    > >>> Signed-off-by: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
    > >>> 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-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/
    \
     
     \ /
      Last update: 2011-09-02 18:09    [W:0.042 / U:0.768 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site