fbcon: fix locking harder

PATCH In review Dave Airlie 3 participants 7 messages Jan 25, 2013

Subsystems (5)

DRM drivers DRM misc Framebuffer layer Console +1 more subsystems

lore

Share & tools

lore

Dave Airlie

author maintainer

2013-01-25 01:43 UTC #0 lore raw

PATCH 1/1 fbcon: fix locking harder

Okay so Alan's patch handled the case where there was no registered fbcon, however the other path entered in set_con2fb_map pit.

In there we called fbcon_takeover, but we also took the console lock in a couple of places. So push the console lock out to the callers of set_con2fb_map,

this means fbmem and switcheroo needed to take the lock around the fb notifier entry points that lead to this.

This should fix the efifb regression seen by Maarten.

Signed-off-by Dave Airlie <[email protected]>

---
 drivers/gpu/vga/vga_switcheroo.c |  3 +++
 drivers/video/console/fbcon.c    | 11 ++++++++---
 drivers/video/fbmem.c            |  2 ++
 3 files changed, 13 insertions(+), 3 deletions(-)

drivers/gpu/vga/vga_switcheroo.c +3 -0

index fa60add..cf787e1 100644
@@ -25,6 +25,7 @@
 #include <linux/fb.h>
 
 #include <linux/pci.h>
+#include <linux/console.h>
 #include <linux/vga_switcheroo.h>
 
 #include <linux/vgaarb.h>

@@ -337,8 +338,10 @@ static int vga_switchto_stage2(struct vga_switcheroo_client *new_client)
 
 	if (new_client->fb_info) {
 		struct fb_event event;
+		console_lock();
 		event.info = new_client->fb_info;
 		fb_notifier_call_chain(FB_EVENT_REMAP_ALL_CONSOLE, &event);
+		console_unlock();
 	}
 
 	ret = vgasr_priv.handler->switchto(new_client->id);

drivers/video/console/fbcon.c +8 -3

index 2aef9ca..2e2d959 100644
@@ -842,6 +842,8 @@ static void con2fb_init_display(struct vc_data *vc, struct fb_info *info,
  *
  *	Maps a virtual console @unit to a frame buffer device
  *	@newidx.
+ *
+ *	This should be called with the console lock held.
  */
 static int set_con2fb_map(int unit, int newidx, int user)
 {

@@ -859,7 +861,7 @@ static int set_con2fb_map(int unit, int newidx, int user)
 
 	if (!search_for_mapped_con() || !con_is_bound(&fb_con)) {
 		info_idx = newidx;
-		return fbcon_takeover(0);
+		return do_fbcon_takeover(0);
 	}
 
 	if (oldidx != -1)

@@ -867,7 +869,6 @@ static int set_con2fb_map(int unit, int newidx, int user)
 
 	found = search_fb_in_map(newidx);
 
-	console_lock();
 	con2fb_map[unit] = newidx;
 	if (!err && !found)
  		err = con2fb_acquire_newinfo(vc, info, unit, oldidx);

@@ -894,7 +895,6 @@ static int set_con2fb_map(int unit, int newidx, int user)
 	if (!search_fb_in_map(info_idx))
 		info_idx = newidx;
 
-	console_unlock();
  	return err;
 }

@@ -3019,6 +3019,7 @@ static inline int fbcon_unbind(void)
 }
 #endif /* CONFIG_VT_HW_CONSOLE_BINDING */
 
+/* called with console_lock held */
 static int fbcon_fb_unbind(int idx)
 {
 	int i, new_idx = -1, ret = 0;

@@ -3045,6 +3046,7 @@ static int fbcon_fb_unbind(int idx)
 	return ret;
 }
 
+/* called with console_lock held */
 static int fbcon_fb_unregistered(struct fb_info *info)
 {
 	int i, idx;

@@ -3082,6 +3084,7 @@ static int fbcon_fb_unregistered(struct fb_info *info)
 	return 0;
 }
 
+/* called with console_lock held */
 static void fbcon_remap_all(int idx)
 {
 	int i;

@@ -3126,6 +3129,7 @@ static inline void fbcon_select_primary(struct fb_info *info)
 }
 #endif /* CONFIG_FRAMEBUFFER_DETECT_PRIMARY */
 
+/* called with console_lock held */
 static int fbcon_fb_registered(struct fb_info *info)
 {
 	int ret = 0, i, idx;

@@ -3278,6 +3282,7 @@ static int fbcon_event_notify(struct notifier_block *self,
 		ret = fbcon_fb_unregistered(info);
 		break;
 	case FB_EVENT_SET_CONSOLE_MAP:
+		/* called with console lock held */
 		con2fb = event->data;
 		ret = set_con2fb_map(con2fb->console - 1,
 				     con2fb->framebuffer, 1);

drivers/video/fbmem.c +2 -0

index 070b9a1..dc61c12 100644
@@ -1177,8 +1177,10 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
 		event.data = &con2fb;
 		if (!lock_fb_info(info))
 			return -ENODEV;
+		console_lock();
 		event.info = info;
 		ret = fb_notifier_call_chain(FB_EVENT_SET_CONSOLE_MAP, &event);
+		console_unlock();
 		unlock_fb_info(info);
 		break;
 	case FBIOBLANK:

Signature · 1 line

1.8.1

Josh Boyer

2013-01-25 14:08 UTC #1 lore raw

Quoted 6 lines · Dave Airlie

Okay so Alan's patch handled the case where there was no registered fbcon, however the other path entered in set_con2fb_map pit.
In there we called fbcon_takeover, but we also took the console lock in a couple of places. So push the console lock out to the callers of set_con2fb_map,

Quoted 6 lines

this means fbmem and switcheroo needed to take the lock around the fb notifier entry points that lead to this.
This should fix the efifb regression seen by Maarten.
Signed-off-by Dave Airlie <[email protected]>

Linus might not be taking this for 3.8 because of lack of testing, but that doesn't mean the problems don't exist in 3.8 (and older). Maybe throw a CC for stable on the patch? That way it works its way back when it does get merged.

josh

Josh Boyer

2013-01-25 14:08 UTC #2 lore raw

In reply to Dave Airlie

Quoted 6 lines · Dave Airlie

Okay so Alan's patch handled the case where there was no registered fbcon, however the other path entered in set_con2fb_map pit.
In there we called fbcon_takeover, but we also took the console lock in a couple of places. So push the console lock out to the callers of set_con2fb_map,

Quoted 6 lines

this means fbmem and switcheroo needed to take the lock around the fb notifier entry points that lead to this.
This should fix the efifb regression seen by Maarten.
Signed-off-by Dave Airlie <[email protected]>

josh

------------------------------------------------------------------------------
Master Visual Studio, SharePoint, SQL, ASP.NET, C# 2012, HTML5, CSS, MVC, Windows 8 Apps, JavaScript and much more. Keep your skills current with LearnDevNow - 3,200 step-by-step video tutorials by Microsoft MVPs and experts. ON SALE this month only -- learn more at:
http://p.sf.net/sfu/learnnow-d2d
--
_______________________________________________
Dri-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/dri-devel

Josh Boyer

2013-01-25 14:08 UTC #3 lore raw

In reply to Dave Airlie

Quoted 6 lines · Dave Airlie

Okay so Alan's patch handled the case where there was no registered fbcon, however the other path entered in set_con2fb_map pit.
In there we called fbcon_takeover, but we also took the console lock in a couple of places. So push the console lock out to the callers of set_con2fb_map,

Quoted 6 lines

this means fbmem and switcheroo needed to take the lock around the fb notifier entry points that lead to this.
This should fix the efifb regression seen by Maarten.
Signed-off-by Dave Airlie <[email protected]>

josh

Daniel Vetter

2013-01-25 15:55 UTC #4 lore raw

In reply to Dave Airlie

Quoted 18 lines · Dave Airlie

Okay so Alan's patch handled the case where there was no registered fbcon, however the other path entered in set_con2fb_map pit.
In there we called fbcon_takeover, but we also took the console lock in a couple of places. So push the console lock out to the callers of set_con2fb_map,
this means fbmem and switcheroo needed to take the lock around the fb notifier entry points that lead to this.
This should fix the efifb regression seen by Maarten.
Signed-off-by Dave Airlie <[email protected]>
---
 drivers/gpu/vga/vga_switcheroo.c |  3 +++
 drivers/video/console/fbcon.c    | 11 ++++++++---
 drivers/video/fbmem.c            |  2 ++
 3 files changed, 13 insertions(+), 3 deletions(-)

[cut]

Quoted 6 lines

        ret = vgasr_priv.handler->switchto(new_client->id);

drivers/video/console/fbcon.c +0 -0

index 2aef9ca..2e2d959 100644

drivers/video/console/fbcon.c static void con2fb_init_display(struct vc_data *vc, struct fb_info *info, · line 849

Quoted 8 lines

drivers/video/console/fbcon.c +2 -0

@@ -842,6 +842,8 @@ static void con2fb_init_display(struct vc_data *vc, struct fb_info *info,
  *
  *     Maps a virtual console @unit to a frame buffer device
  *     @newidx.
+ *
+ *     This should be called with the console lock held.
  */
 static int set_con2fb_map(int unit, int newidx, int user)
 {

What about throwing a WARN_CONSOLE_UNLOCKED(); in here to make sure this new rule is obeyed?
-Daniel

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Daniel Vetter

2013-01-25 15:55 UTC #5 lore raw

In reply to Dave Airlie

Quoted 18 lines · Dave Airlie

Okay so Alan's patch handled the case where there was no registered fbcon, however the other path entered in set_con2fb_map pit.
In there we called fbcon_takeover, but we also took the console lock in a couple of places. So push the console lock out to the callers of set_con2fb_map,
this means fbmem and switcheroo needed to take the lock around the fb notifier entry points that lead to this.
This should fix the efifb regression seen by Maarten.
Signed-off-by Dave Airlie <[email protected]>
---
 drivers/gpu/vga/vga_switcheroo.c |  3 +++
 drivers/video/console/fbcon.c    | 11 ++++++++---
 drivers/video/fbmem.c            |  2 ++
 3 files changed, 13 insertions(+), 3 deletions(-)

[cut]

Quoted 6 lines

        ret = vgasr_priv.handler->switchto(new_client->id);

drivers/video/console/fbcon.c +0 -0

index 2aef9ca..2e2d959 100644

drivers/video/console/fbcon.c static void con2fb_init_display(struct vc_data *vc, struct fb_info *info, · line 849

Quoted 8 lines

drivers/video/console/fbcon.c +2 -0

@@ -842,6 +842,8 @@ static void con2fb_init_display(struct vc_data *vc, struct fb_info *info,
  *
  *     Maps a virtual console @unit to a frame buffer device
  *     @newidx.
+ *
+ *     This should be called with the console lock held.
  */
 static int set_con2fb_map(int unit, int newidx, int user)
 {

What about throwing a WARN_CONSOLE_UNLOCKED(); in here to make sure this new rule is obeyed?
-Daniel

Signature · 15 lines

Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

------------------------------------------------------------------------------
Master Visual Studio, SharePoint, SQL, ASP.NET, C# 2012, HTML5, CSS,
MVC, Windows 8 Apps, JavaScript and much more. Keep your skills current
with LearnDevNow - 3,200 step-by-step video tutorials by Microsoft
MVPs and experts. ON SALE this month only -- learn more at:
http://p.sf.net/sfu/learnnow-d2d
--
_______________________________________________
Dri-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/dri-devel

Daniel Vetter

2013-01-25 15:55 UTC #6 lore raw

In reply to Dave Airlie

Quoted 18 lines · Dave Airlie

Okay so Alan's patch handled the case where there was no registered fbcon, however the other path entered in set_con2fb_map pit.
In there we called fbcon_takeover, but we also took the console lock in a couple of places. So push the console lock out to the callers of set_con2fb_map,
this means fbmem and switcheroo needed to take the lock around the fb notifier entry points that lead to this.
This should fix the efifb regression seen by Maarten.
Signed-off-by Dave Airlie <[email protected]>
---
 drivers/gpu/vga/vga_switcheroo.c |  3 +++
 drivers/video/console/fbcon.c    | 11 ++++++++---
 drivers/video/fbmem.c            |  2 ++
 3 files changed, 13 insertions(+), 3 deletions(-)

[cut]

Quoted 6 lines

        ret = vgasr_priv.handler->switchto(new_client->id);

drivers/video/console/fbcon.c +0 -0

index 2aef9ca..2e2d959 100644

drivers/video/console/fbcon.c static void con2fb_init_display(struct vc_data *vc, struct fb_info *info, · line 849

Quoted 8 lines

drivers/video/console/fbcon.c +2 -0

@@ -842,6 +842,8 @@ static void con2fb_init_display(struct vc_data *vc, struct fb_info *info,
  *
  *     Maps a virtual console @unit to a frame buffer device
  *     @newidx.
+ *
+ *     This should be called with the console lock held.
  */
 static int set_con2fb_map(int unit, int newidx, int user)
 {

What about throwing a WARN_CONSOLE_UNLOCKED(); in here to make sure this new rule is obeyed?
-Daniel

Signature · 11 lines

Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

------------------------------------------------------------------------------
Master Visual Studio, SharePoint, SQL, ASP.NET, C# 2012, HTML5, CSS,
MVC, Windows 8 Apps, JavaScript and much more. Keep your skills current
with LearnDevNow - 3,200 step-by-step video tutorials by Microsoft
MVPs and experts. ON SALE this month only -- learn more at:
http://p.sf.net/sfu/learnnow-d2d
--