Re: Possible deadlock when suspending framebuffer

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\x129539210207429&w=2
http://marc.info/?l=linux-fbdev&m\x129700789632450&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&232
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&232
> 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&232
> > 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&232
>>> 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&232
> >>> 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\x130833638508657&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&232
> >>> 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\x130833638508657&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&232
>>>>> 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

[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()

[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&232
> 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

re:Possible deadlock when suspending framebuffer

[Wanlong Gao]

[-- Attachment #1: Type: text/plain, Size: 884 bytes --]

<snip>
Hi Francis:
can you test this patch?
Thanks

From fe026c42af4cbdce053460a428a445e99071586a Mon Sep 17 00:00:00 2001
From: Wanlong Gao <wanlong.gao@gmail.com>
Date: Wed, 15 Jun 2011 09:03:41 +0800
Subject: [PATCH] test



Signed-off-by: Wanlong Gao <wanlong.gao@gmail.com>
---
 drivers/video/fbmem.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index 5aac00e..6e6cef3 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -1642,11 +1642,8 @@ static int do_unregister_framebuffer(struct
fb_info *fb_info)
 	if (i < 0 || i >= FB_MAX || registered_fb[i] != fb_info)
 		return -EINVAL;
 
-	if (!lock_fb_info(fb_info))
-		return -ENODEV;
 	event.info = fb_info;
 	ret = fb_notifier_call_chain(FB_EVENT_FB_UNBIND, &event);
-	unlock_fb_info(fb_info);
 
 	if (ret)
 		return -EINVAL;
-- 
1.7.4.1



[-- Attachment #2: 0001-test.patch --]
[-- Type: text/x-patch, Size: 835 bytes --]

From fe026c42af4cbdce053460a428a445e99071586a Mon Sep 17 00:00:00 2001
From: Wanlong Gao <wanlong.gao@gmail.com>
Date: Wed, 15 Jun 2011 09:03:41 +0800
Subject: [PATCH] test

test

Signed-off-by: Wanlong Gao <wanlong.gao@gmail.com>
---
 drivers/video/fbmem.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index 5aac00e..6e6cef3 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -1642,11 +1642,8 @@ static int do_unregister_framebuffer(struct fb_info *fb_info)
 	if (i < 0 || i >= FB_MAX || registered_fb[i] != fb_info)
 		return -EINVAL;
 
-	if (!lock_fb_info(fb_info))
-		return -ENODEV;
 	event.info = fb_info;
 	ret = fb_notifier_call_chain(FB_EVENT_FB_UNBIND, &event);
-	unlock_fb_info(fb_info);
 
 	if (ret)
 		return -EINVAL;
-- 
1.7.4.1

Re: Possible deadlock when suspending framebuffer

[Bruno Prémont]

Hi,

On Wed, 15 Jun 2011 09:09:24 Wanlong Gao <wanlong.gao@gmail.com> wrote:
> <snip>
> Hi Francis:
> can you test this patch?

Do you have a deadlock trace which you are trying to fix?

It's either the caller of unregister_framebuffer() which must be
changed to not call unregister_framebuffer with info's lock held or
the code reacting on the notification that must not try to acquire the
lock again.

The interesting par is if console semaphore has some relation to this
deadlock as the order for taking both varies... It could be
lock_fb_info(); console_lock()  versus console_lock(); lock_fb_info()

Bruno


> Thanks
> 
> From fe026c42af4cbdce053460a428a445e99071586a Mon Sep 17 00:00:00 2001
> From: Wanlong Gao <wanlong.gao@gmail.com>
> Date: Wed, 15 Jun 2011 09:03:41 +0800
> Subject: [PATCH] test
> 
> 
> 
> Signed-off-by: Wanlong Gao <wanlong.gao@gmail.com>
> ---
>  drivers/video/fbmem.c |    3 ---
>  1 files changed, 0 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
> index 5aac00e..6e6cef3 100644
> --- a/drivers/video/fbmem.c
> +++ b/drivers/video/fbmem.c
> @@ -1642,11 +1642,8 @@ static int do_unregister_framebuffer(struct
> fb_info *fb_info)
>  	if (i < 0 || i >= FB_MAX || registered_fb[i] != fb_info)
>  		return -EINVAL;
>  
> -	if (!lock_fb_info(fb_info))
> -		return -ENODEV;
>  	event.info = fb_info;
>  	ret = fb_notifier_call_chain(FB_EVENT_FB_UNBIND, &event);
> -	unlock_fb_info(fb_info);

Not a good idea to stop taking fb_lock here.
Pretty all calls of fb_notifier_call_chain are protected by info's
lock, except the one for FB_EVENT_FB_UNREGISTERED a few lines further.

IMHO it wou make sense to add the lock around that last one so all
notifier chain calls are handled the same.

>  	if (ret)
>  		return -EINVAL;

Re: Possible deadlock when suspending framebuffer

[Wanlong Gao]

On Wed, Jun 15, 2011 at 1:58 PM, Bruno Prémont
<bonbons@linux-vserver.org> wrote:
>
> Hi,
>
> On Wed, 15 Jun 2011 09:09:24 Wanlong Gao <wanlong.gao@gmail.com> wrote:
> > <snip>
> > Hi Francis:
> > can you test this patch?
>
> Do you have a deadlock trace which you are trying to fix?
No, I just look at the code and try to fix this but I'm not sure.
Can you teach me how to have a deadlock trace here?
Thanks
>
> It's either the caller of unregister_framebuffer() which must be
> changed to not call unregister_framebuffer with info's lock held or
> the code reacting on the notification that must not try to acquire the
> lock again.
>
> The interesting par is if console semaphore has some relation to this
> deadlock as the order for taking both varies... It could be
> lock_fb_info(); console_lock()  versus console_lock(); lock_fb_info()
>
I see, thanks
> Bruno
>
>
> > Thanks
> >
><snip>
>
> Not a good idea to stop taking fb_lock here.
> Pretty all calls of fb_notifier_call_chain are protected by info's
> lock, except the one for FB_EVENT_FB_UNREGISTERED a few lines further.
Yup, thanks
>
> IMHO it wou make sense to add the lock around that last one so all
> notifier chain calls are handled the same.
>
> <snip>
>



--
Best regards
Wanlong Gao

Re: Possible deadlock when suspending framebuffer

[Américo Wang]

On Wed, Jun 15, 2011 at 2:22 PM, Wanlong Gao <wanlong.gao@gmail.com> wrote:
> On Wed, Jun 15, 2011 at 1:58 PM, Bruno Prémont
> <bonbons@linux-vserver.org> wrote:
>>
>> Hi,
>>
>> On Wed, 15 Jun 2011 09:09:24 Wanlong Gao <wanlong.gao@gmail.com> wrote:
>> > <snip>
>> > Hi Francis:
>> > can you test this patch?
>>
>> Do you have a deadlock trace which you are trying to fix?
> No, I just look at the code and try to fix this but I'm not sure.
> Can you teach me how to have a deadlock trace here?

CONFIG_LOCKDEP=y

Re: Possible deadlock when suspending framebuffer

[Francis Moreau]

On Wed, Jun 15, 2011 at 7:58 AM, Bruno Prémont
<bonbons@linux-vserver.org> wrote:
> Hi,
>
> On Wed, 15 Jun 2011 09:09:24 Wanlong Gao <wanlong.gao@gmail.com> wrote:
>> <snip>
>> Hi Francis:
>> can you test this patch?
>
> Do you have a deadlock trace which you are trying to fix?
>
> It's either the caller of unregister_framebuffer() which must be
> changed to not call unregister_framebuffer with info's lock held or
> the code reacting on the notification that must not try to acquire the
> lock again.
>
> The interesting par is if console semaphore has some relation to this
> deadlock as the order for taking both varies... It could be
> lock_fb_info(); console_lock()  versus console_lock(); lock_fb_info()
>
> Bruno
>
>
>> Thanks
>>
>> From fe026c42af4cbdce053460a428a445e99071586a Mon Sep 17 00:00:00 2001
>> From: Wanlong Gao <wanlong.gao@gmail.com>
>> Date: Wed, 15 Jun 2011 09:03:41 +0800
>> Subject: [PATCH] test
>>
>>
>>
>> Signed-off-by: Wanlong Gao <wanlong.gao@gmail.com>
>> ---
>>  drivers/video/fbmem.c |    3 ---
>>  1 files changed, 0 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
>> index 5aac00e..6e6cef3 100644
>> --- a/drivers/video/fbmem.c
>> +++ b/drivers/video/fbmem.c
>> @@ -1642,11 +1642,8 @@ static int do_unregister_framebuffer(struct
>> fb_info *fb_info)
>>       if (i < 0 || i >= FB_MAX || registered_fb[i] != fb_info)
>>               return -EINVAL;
>>
>> -     if (!lock_fb_info(fb_info))
>> -             return -ENODEV;
>>       event.info = fb_info;
>>       ret = fb_notifier_call_chain(FB_EVENT_FB_UNBIND, &event);
>> -     unlock_fb_info(fb_info);
>
> Not a good idea to stop taking fb_lock here.
> Pretty all calls of fb_notifier_call_chain are protected by info's
> lock, except the one for FB_EVENT_FB_UNREGISTERED a few lines further.
>
> IMHO it wou make sense to add the lock around that last one so all
> notifier chain calls are handled the same.
>
>>       if (ret)
>>               return -EINVAL;
>
>

Well, sorry for the dumb question but the fb/fbcon code is pretty hard
to follow for me.

Why does store_fbstate() and any fb driver's suspsend methods acquire
the console lock at all ?

Thanks
-- 
Francis

Re: Possible deadlock when suspending framebuffer

[Bruno Prémont]

On Wed, 15 Jun 2011 09:12:46 Francis Moreau wrote:
> On Wed, Jun 15, 2011 at 7:58 AM, Bruno Prémont wrote:
> Well, sorry for the dumb question but the fb/fbcon code is pretty hard
> to follow for me.

Certainly not just for you

> Why does store_fbstate() and any fb driver's suspsend methods acquire
> the console lock at all ?

From my understanding, fbcon currently has very loose binding with
framebuffers in general instead of just with those few framebuffers
it is effectively mapped to (and active on!).
James Simmons started a complete rework of fbcon/tty code which is
expected to get things more fine-grained, until then console semaphore
(console_lock) remains a kind of big kernel lock in the
console/framebuffer area.

As such any state change of framebuffer may influence/race against
fbcon.
e.g. for the suspend state you want to avoid fbcon to fiddle with
your framebuffer while it is suspending (and have fbcon know about the
suspended state). This way fbcon can unsuspend framebuffer if needed
but also stop accessing it when it should not.

Bruno