Possible deadlock when suspending framebuffer

Possible deadlock when suspending framebuffer

[Francis Moreau]

Hello,

I noticed that a possible deadlock can happen when the current frame
buffering is being suspended and a new frambuffer device is being
registred at the same time.

When suspending the current frambuffer by doing : echo 1
>/sys/class/graphics/fb0/state, the kernel actually takes the
following locks in that order: console_lock, lock_fb_info (see
store_fbstate()).

However when a new framebuffer is coming in, the lock sequence is:
lock_fb_info (taken by do_remove_conflicting_framebuffer()),
console_lock() (taken by unbind_console).

I don't know how this should be fixed though...

Thanks
-- 
Francis

Re: Possible deadlock when suspending framebuffer

[Linus Torvalds]

Paul, fbdev people.. Comments? This was sent to me and lkml, the right
people probably didn't see it.

I doubt it's a big problem in practice, but..

                      Linus

On Tue, Jun 14, 2011 at 6:10 AM, Francis Moreau <francis.moro@gmail.com> wrote:
> Hello,
>
> I noticed that a possible deadlock can happen when the current frame
> buffering is being suspended and a new frambuffer device is being
> registred at the same time.
>
> When suspending the current frambuffer by doing : echo 1
>>/sys/class/graphics/fb0/state, the kernel actually takes the
> following locks in that order: console_lock, lock_fb_info (see
> store_fbstate()).
>
> However when a new framebuffer is coming in, the lock sequence is:
> lock_fb_info (taken by do_remove_conflicting_framebuffer()),
> console_lock() (taken by unbind_console).
>
> I don't know how this should be fixed though...
>
> Thanks
> --
> Francis
>

Re: Possible deadlock when suspending framebuffer

[Florian Tobias Schandinat]

Hi Linus,

On 06/14/2011 06:15 PM, Linus Torvalds wrote:
> Paul, fbdev people.. Comments? This was sent to me and lkml, the right
> people probably didn't see it.

Sounds very familiar. Indeed a quick glance at my archive revealed 2 
approaches/patches dealing with similar issues
http://marc.info/?l=linux-fbdev&m=129539210207429&w=2
http://marc.info/?l=linux-fbdev&m=129700789632450&w=2

I did not have time to verify that those do actually get those things right 
everywhere but the first one looks good and should also solve this issue I think.


Regards,

Florian Tobias Schandinat

>
> I doubt it's a big problem in practice, but..
>
>                        Linus
>
> On Tue, Jun 14, 2011 at 6:10 AM, Francis Moreau<francis.moro@gmail.com>  wrote:
>> Hello,
>>
>> I noticed that a possible deadlock can happen when the current frame
>> buffering is being suspended and a new frambuffer device is being
>> registred at the same time.
>>
>> When suspending the current frambuffer by doing : echo 1
>>> /sys/class/graphics/fb0/state, the kernel actually takes the
>> following locks in that order: console_lock, lock_fb_info (see
>> store_fbstate()).
>>
>> However when a new framebuffer is coming in, the lock sequence is:
>> lock_fb_info (taken by do_remove_conflicting_framebuffer()),
>> console_lock() (taken by unbind_console).
>>
>> I don't know how this should be fixed though...
>>
>> Thanks
>> --
>> Francis
>>
> --
> 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
>

[PATCH] fb: avoid possible deadlock caused by fb_set_suspend

[Florian Tobias Schandinat]

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      D 0000000000000000     0   237    228 0x00000000
 ffff8800774f3d68 0000000000000082 00000000000135c0 00000000000135c0
 ffff880000000000 ffff8800774f3fd8 ffff8800774f3fd8 ffff880076ee4530
 00000000000135c0 ffff8800774f3fd8 ffff8800774f2000 00000000000135c0
Call Trace:
 [<ffffffff8141287a>] __mutex_lock_slowpath+0x11a/0x1e0
 [<ffffffff814142f2>] ? _raw_spin_lock_irq+0x22/0x40
 [<ffffffff814123d3>] mutex_lock+0x23/0x50
 [<ffffffff8125dfc5>] lock_fb_info+0x25/0x60
 [<ffffffff8125e3f0>] fb_set_suspend+0x20/0x80
 [<ffffffff81263e2f>] store_fbstate+0x4f/0x70
 [<ffffffff812e7f70>] dev_attr_store+0x20/0x30
 [<ffffffff811c46b4>] sysfs_write_file+0xd4/0x160
 [<ffffffff81155a26>] vfs_write+0xc6/0x190
 [<ffffffff81155d51>] sys_write+0x51/0x90
 [<ffffffff8100c012>] system_call_fastpath+0x16/0x1b
..
modprobe process stalled because has the fb_info lock (got inside
unregister_framebuffer) but waiting for the semaphore held by the
test process which is waiting to get the fb_info lock:
..
modprobe      D 0000000000000000     0   230    218 0x00000000
 ffff880077a4d618 0000000000000082 0000000000000001 0000000000000001
 ffff880000000000 ffff880077a4dfd8 ffff880077a4dfd8 ffff8800775a2e20
 00000000000135c0 ffff880077a4dfd8 ffff880077a4c000 00000000000135c0
Call Trace:
 [<ffffffff81411fe5>] schedule_timeout+0x215/0x310
 [<ffffffff81058051>] ? get_parent_ip+0x11/0x50
 [<ffffffff814130dd>] __down+0x6d/0xb0
 [<ffffffff81089f71>] down+0x41/0x50
 [<ffffffff810629ac>] acquire_console_sem+0x2c/0x50
 [<ffffffff812ca53d>] unbind_con_driver+0xad/0x2d0
 [<ffffffff8126f5f7>] fbcon_event_notify+0x457/0x890
 [<ffffffff814144ff>] ? _raw_spin_unlock_irqrestore+0x1f/0x50
 [<ffffffff81058051>] ? get_parent_ip+0x11/0x50
 [<ffffffff8141836d>] notifier_call_chain+0x4d/0x70
 [<ffffffff8108a3b8>] __blocking_notifier_call_chain+0x58/0x80
 [<ffffffff8108a3f6>] blocking_notifier_call_chain+0x16/0x20
 [<ffffffff8125dabb>] fb_notifier_call_chain+0x1b/0x20
 [<ffffffff8125e6ac>] unregister_framebuffer+0x7c/0x130
 [<ffffffff8125e8b3>] remove_conflicting_framebuffers+0x153/0x180
 [<ffffffff8125eef3>] register_framebuffer+0x93/0x2c0
 [<ffffffffa0331112>] drm_fb_helper_single_fb_probe+0x252/0x2f0 [drm_kms_helper]
 [<ffffffffa03314a3>] drm_fb_helper_initial_config+0x2f3/0x6d0 [drm_kms_helper]
 [<ffffffffa03318dd>] ? drm_fb_helper_single_add_all_connectors+0x5d/0x1c0 [drm_kms_helper]
 [<ffffffffa037b588>] intel_fbdev_init+0xa8/0x160 [i915]
 [<ffffffffa0343d74>] i915_driver_load+0x854/0x12b0 [i915]
 [<ffffffffa02f0e7e>] drm_get_pci_dev+0x19e/0x360 [drm]
 [<ffffffff8141821d>] ? sub_preempt_count+0x9d/0xd0
 [<ffffffffa0386f91>] i915_pci_probe+0x15/0x17 [i915]
 [<ffffffff8124481f>] local_pci_probe+0x5f/0xd0
 [<ffffffff81244f89>] pci_device_probe+0x119/0x120
 [<ffffffff812eccaa>] ? driver_sysfs_add+0x7a/0xb0
 [<ffffffff812ed003>] driver_probe_device+0xa3/0x290
 [<ffffffff812ed1f0>] ? __driver_attach+0x0/0xb0
 [<ffffffff812ed29b>] __driver_attach+0xab/0xb0
 [<ffffffff812ed1f0>] ? __driver_attach+0x0/0xb0
 [<ffffffff812ebd3e>] bus_for_each_dev+0x5e/0x90
 [<ffffffff812ecc2e>] driver_attach+0x1e/0x20
 [<ffffffff812ec6f2>] bus_add_driver+0xe2/0x320
 [<ffffffffa03aa000>] ? i915_init+0x0/0x96 [i915]
 [<ffffffff812ed536>] driver_register+0x76/0x140
 [<ffffffffa03aa000>] ? i915_init+0x0/0x96 [i915]
 [<ffffffff81245216>] __pci_register_driver+0x56/0xd0
 [<ffffffffa02f1264>] drm_pci_init+0xe4/0xf0 [drm]
 [<ffffffffa03aa000>] ? i915_init+0x0/0x96 [i915]
 [<ffffffffa02e84a8>] drm_init+0x58/0x70 [drm]
 [<ffffffffa03aa094>] i915_init+0x94/0x96 [i915]
 [<ffffffff81002194>] do_one_initcall+0x44/0x190
 [<ffffffff810a066b>] sys_init_module+0xcb/0x210
 [<ffffffff8100c012>] system_call_fastpath+0x16/0x1b
..

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.

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;
 }
-- 
1.6.3.2

Re: [PATCH] fb: avoid possible deadlock caused by fb_set_suspend

[Bruno Prémont]

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;
>  }

Re: [PATCH] fb: avoid possible deadlock caused by fb_set_suspend

[Bruno Prémont]

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);
 	}
 

Re: [PATCH] fb: avoid possible deadlock caused by fb_set_suspend

[Florian Tobias Schandinat]

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@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
> 

Re: [PATCH] fb: avoid possible deadlock caused by fb_set_suspend

[Guennadi Liakhovetski]

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...

Sorry, somehow this patch excaped my attention. Will test tomorrow.

Thanks
Guennadi

> 
> 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@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/

Re: [PATCH] fb: avoid possible deadlock caused by fb_set_suspend

[Guennadi Liakhovetski]

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/

Re: [PATCH] fb: avoid possible deadlock caused by fb_set_suspend

[Florian Tobias Schandinat]

Hi Bruno, Guennadi,

On 09/02/2011 04:06 PM, Guennadi Liakhovetski wrote:
> 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>

So I guess we can finally solve this issue, thanks.

On the other hand I just noticed the original patch didn't have Bruno's
Signed-off and a commit message was missing. So may I ask you, Bruno, to add
your Signed-off or preferably resend the version tested by Guennadi in the
appropriate patch format.


Thanks,

Florian Tobias Schandinat

> 
> 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/
> 

[PATCH] fb: sh-mobile: Fix deadlock risk between lock_fb_info() and console_lock()

[Bruno Prémont]

Following on Herton's patch "fb: avoid possible deadlock caused by
fb_set_suspend" which moves lock_fb_info() out of fb_set_suspend()
to its callers, correct sh-mobile's locking around call to
fb_set_suspend() and the same sort of deaklocks with console_lock()
due to order of taking the lock.

console_lock() must be taken while fb_info is already locked and fb_info
must be locked while calling fb_set_suspend().

Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---

Hi Florian,

> On the other hand I just noticed the original patch didn't have
> Bruno's Signed-off and a commit message was missing. So may I ask you,
> Bruno, to add your Signed-off or preferably resend the version tested
> by Guennadi in the appropriate patch format.

Here it is with commit message and the Signed-off-by's.

Thanks,
Bruno




 drivers/video/sh_mobile_hdmi.c |   47 ++++++++++++++++++++++-----------------
 1 files changed, 26 insertions(+), 21 deletions(-)

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:

Re: [PATCH] fb: sh-mobile: Fix deadlock risk between lock_fb_info() and console_lock()

[Florian Tobias Schandinat]

On 09/02/2011 05:24 PM, Bruno Prémont wrote:
> Following on Herton's patch "fb: avoid possible deadlock caused by
> fb_set_suspend" which moves lock_fb_info() out of fb_set_suspend()
> to its callers, correct sh-mobile's locking around call to
> fb_set_suspend() and the same sort of deaklocks with console_lock()
> due to order of taking the lock.
> 
> console_lock() must be taken while fb_info is already locked and fb_info
> must be locked while calling fb_set_suspend().
> 
> Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
> 
> Hi Florian,
> 
>> On the other hand I just noticed the original patch didn't have
>> Bruno's Signed-off and a commit message was missing. So may I ask you,
>> Bruno, to add your Signed-off or preferably resend the version tested
>> by Guennadi in the appropriate patch format.
> 
> Here it is with commit message and the Signed-off-by's.

Applied and added stable as Cc as I think this should be applied as well
whenever Herton's patch is applied.


Thanks,

Florian Tobias Schandinat

> 
> Thanks,
> Bruno
> 
> 
> 
> 
>  drivers/video/sh_mobile_hdmi.c |   47 ++++++++++++++++++++++-----------------
>  1 files changed, 26 insertions(+), 21 deletions(-)
> 
> 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:
> 

Re: [PATCH] fb: avoid possible deadlock caused by fb_set_suspend

[Florian Tobias Schandinat]

Hi Paul,

what are your plans for the patch below? Do you queue it for 3.1?
The only feedback so far was that one driver (sh_mobile_hdmi) would require more 
work with a patch for that proposed. Without any objections to the patch I think 
we should take it as fixing the subsystem is more important than causing 
potential issues with a single driver.


Thanks,

Florian Tobias Schandinat

On 06/17/2011 07:02 PM, Florian Tobias Schandinat 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      D 0000000000000000     0   237    228 0x00000000
>   ffff8800774f3d68 0000000000000082 00000000000135c0 00000000000135c0
>   ffff880000000000 ffff8800774f3fd8 ffff8800774f3fd8 ffff880076ee4530
>   00000000000135c0 ffff8800774f3fd8 ffff8800774f2000 00000000000135c0
> Call Trace:
>   [<ffffffff8141287a>] __mutex_lock_slowpath+0x11a/0x1e0
>   [<ffffffff814142f2>] ? _raw_spin_lock_irq+0x22/0x40
>   [<ffffffff814123d3>] mutex_lock+0x23/0x50
>   [<ffffffff8125dfc5>] lock_fb_info+0x25/0x60
>   [<ffffffff8125e3f0>] fb_set_suspend+0x20/0x80
>   [<ffffffff81263e2f>] store_fbstate+0x4f/0x70
>   [<ffffffff812e7f70>] dev_attr_store+0x20/0x30
>   [<ffffffff811c46b4>] sysfs_write_file+0xd4/0x160
>   [<ffffffff81155a26>] vfs_write+0xc6/0x190
>   [<ffffffff81155d51>] sys_write+0x51/0x90
>   [<ffffffff8100c012>] system_call_fastpath+0x16/0x1b
> ..
> modprobe process stalled because has the fb_info lock (got inside
> unregister_framebuffer) but waiting for the semaphore held by the
> test process which is waiting to get the fb_info lock:
> ..
> modprobe      D 0000000000000000     0   230    218 0x00000000
>   ffff880077a4d618 0000000000000082 0000000000000001 0000000000000001
>   ffff880000000000 ffff880077a4dfd8 ffff880077a4dfd8 ffff8800775a2e20
>   00000000000135c0 ffff880077a4dfd8 ffff880077a4c000 00000000000135c0
> Call Trace:
>   [<ffffffff81411fe5>] schedule_timeout+0x215/0x310
>   [<ffffffff81058051>] ? get_parent_ip+0x11/0x50
>   [<ffffffff814130dd>] __down+0x6d/0xb0
>   [<ffffffff81089f71>] down+0x41/0x50
>   [<ffffffff810629ac>] acquire_console_sem+0x2c/0x50
>   [<ffffffff812ca53d>] unbind_con_driver+0xad/0x2d0
>   [<ffffffff8126f5f7>] fbcon_event_notify+0x457/0x890
>   [<ffffffff814144ff>] ? _raw_spin_unlock_irqrestore+0x1f/0x50
>   [<ffffffff81058051>] ? get_parent_ip+0x11/0x50
>   [<ffffffff8141836d>] notifier_call_chain+0x4d/0x70
>   [<ffffffff8108a3b8>] __blocking_notifier_call_chain+0x58/0x80
>   [<ffffffff8108a3f6>] blocking_notifier_call_chain+0x16/0x20
>   [<ffffffff8125dabb>] fb_notifier_call_chain+0x1b/0x20
>   [<ffffffff8125e6ac>] unregister_framebuffer+0x7c/0x130
>   [<ffffffff8125e8b3>] remove_conflicting_framebuffers+0x153/0x180
>   [<ffffffff8125eef3>] register_framebuffer+0x93/0x2c0
>   [<ffffffffa0331112>] drm_fb_helper_single_fb_probe+0x252/0x2f0 [drm_kms_helper]
>   [<ffffffffa03314a3>] drm_fb_helper_initial_config+0x2f3/0x6d0 [drm_kms_helper]
>   [<ffffffffa03318dd>] ? drm_fb_helper_single_add_all_connectors+0x5d/0x1c0 [drm_kms_helper]
>   [<ffffffffa037b588>] intel_fbdev_init+0xa8/0x160 [i915]
>   [<ffffffffa0343d74>] i915_driver_load+0x854/0x12b0 [i915]
>   [<ffffffffa02f0e7e>] drm_get_pci_dev+0x19e/0x360 [drm]
>   [<ffffffff8141821d>] ? sub_preempt_count+0x9d/0xd0
>   [<ffffffffa0386f91>] i915_pci_probe+0x15/0x17 [i915]
>   [<ffffffff8124481f>] local_pci_probe+0x5f/0xd0
>   [<ffffffff81244f89>] pci_device_probe+0x119/0x120
>   [<ffffffff812eccaa>] ? driver_sysfs_add+0x7a/0xb0
>   [<ffffffff812ed003>] driver_probe_device+0xa3/0x290
>   [<ffffffff812ed1f0>] ? __driver_attach+0x0/0xb0
>   [<ffffffff812ed29b>] __driver_attach+0xab/0xb0
>   [<ffffffff812ed1f0>] ? __driver_attach+0x0/0xb0
>   [<ffffffff812ebd3e>] bus_for_each_dev+0x5e/0x90
>   [<ffffffff812ecc2e>] driver_attach+0x1e/0x20
>   [<ffffffff812ec6f2>] bus_add_driver+0xe2/0x320
>   [<ffffffffa03aa000>] ? i915_init+0x0/0x96 [i915]
>   [<ffffffff812ed536>] driver_register+0x76/0x140
>   [<ffffffffa03aa000>] ? i915_init+0x0/0x96 [i915]
>   [<ffffffff81245216>] __pci_register_driver+0x56/0xd0
>   [<ffffffffa02f1264>] drm_pci_init+0xe4/0xf0 [drm]
>   [<ffffffffa03aa000>] ? i915_init+0x0/0x96 [i915]
>   [<ffffffffa02e84a8>] drm_init+0x58/0x70 [drm]
>   [<ffffffffa03aa094>] i915_init+0x94/0x96 [i915]
>   [<ffffffff81002194>] do_one_initcall+0x44/0x190
>   [<ffffffff810a066b>] sys_init_module+0xcb/0x210
>   [<ffffffff8100c012>] system_call_fastpath+0x16/0x1b
> ..
>
> 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.
>
> 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;
>   }

Re: [PATCH] fb: avoid possible deadlock caused by fb_set_suspend

[Francis Moreau]

On Wed, Jul 20, 2011 at 8:16 PM, Florian Tobias Schandinat
<FlorianSchandinat@gmx.de> wrote:
> what are your plans for the patch below? Do you queue it for 3.1?

Looks like someone else fixed it slightly differently:

http://permalink.gmane.org/gmane.linux.kernel.stable/15187

-- 
Francis