[PATCH] vga16fb: refuse to load in face of other driver controlling primary card

Subsystems: framebuffer layer, the rest

STALE5923d

6 messages, 4 authors, 2010-07-25 · open the first message on its own page

[PATCH] vga16fb: refuse to load in face of other driver controlling primary card

From: Marcin Slusarz <hidden>
Date: 2010-07-20 19:22:14

We don't want vga16fb to mess with hardware initialized by other driver.
Detect it and refuse to load.
It fixes nouveau interrupt storm on some machines.

Signed-off-by: Marcin Slusarz <redacted>
---
 drivers/video/vga16fb.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/drivers/video/vga16fb.c b/drivers/video/vga16fb.c
index 28ccab4..4505446 100644
--- a/drivers/video/vga16fb.c
+++ b/drivers/video/vga16fb.c

@@ -22,6 +22,7 @@
 #include <linux/platform_device.h>
 #include <linux/screen_info.h>
 
+#include <asm/fb.h>
 #include <asm/io.h>
 #include <video/vga.h>

@@ -1415,7 +1416,7 @@ static struct platform_device *vga16fb_device;
 
 static int __init vga16fb_init(void)
 {
-	int ret;
+	int ret, i;
 #ifndef MODULE
 	char *option = NULL;

@@ -1424,6 +1425,16 @@ static int __init vga16fb_init(void)
 
 	vga16fb_setup(option);
 #endif
+	for (i = 0 ; i < FB_MAX; i++) {
+		if (!registered_fb[i])
+			continue;
+		if (fb_is_primary_device(registered_fb[i])) {
+			printk(KERN_INFO "vga16fb: %s is driving the primary card, refusing to load\n",
+				registered_fb[i]->fix.id);
+			return -EBUSY;
+		}
+	}
+
 	ret = platform_driver_register(&vga16fb_driver);
 
 	if (!ret) {

-- 
1.7.1

Re: [PATCH] vga16fb: refuse to load in face of other driver controlling primary card

From: Andrew Morton <akpm@linux-foundation.org>
Date: 2010-07-23 00:21:17

On Tue, 20 Jul 2010 21:19:23 +0200
Marcin Slusarz [off-list ref] wrote:

quoted hunk

We don't want vga16fb to mess with hardware initialized by other driver.
Detect it and refuse to load.
It fixes nouveau interrupt storm on some machines.

Signed-off-by: Marcin Slusarz <redacted>
---
 drivers/video/vga16fb.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/drivers/video/vga16fb.c b/drivers/video/vga16fb.c
index 28ccab4..4505446 100644
--- a/drivers/video/vga16fb.c
+++ b/drivers/video/vga16fb.c

@@ -22,6 +22,7 @@
 #include <linux/platform_device.h>
 #include <linux/screen_info.h>
 
+#include <asm/fb.h>
 #include <asm/io.h>
 #include <video/vga.h>

@@ -1415,7 +1416,7 @@ static struct platform_device *vga16fb_device;
 
 static int __init vga16fb_init(void)
 {
-	int ret;
+	int ret, i;
 #ifndef MODULE
 	char *option = NULL;

@@ -1424,6 +1425,16 @@ static int __init vga16fb_init(void)
 
 	vga16fb_setup(option);
 #endif
+	for (i = 0 ; i < FB_MAX; i++) {
+		if (!registered_fb[i])
+			continue;
+		if (fb_is_primary_device(registered_fb[i])) {
+			printk(KERN_INFO "vga16fb: %s is driving the primary card, refusing to load\n",
+				registered_fb[i]->fix.id);
+			return -EBUSY;
+		}
+	}
+
 	ret = platform_driver_register(&vga16fb_driver);
 
 	if (!ret) {

This seems pretty hacky, and specific to vga16fb.

Is vga16fb.c the only fbdev driver (now and forever more) which can
grab the hardware from under a different driver's feet?


This is the basic and very common problem of two drivers diddling the
same hardware, yes?  A common way of preventing such collisions is to
perform a suitably-checked request_region() in both drivers.  Whichever
driver gets there second will get a failure and will bale out cleanly. 
Can that mechanism be made to work in this case?

[PATCH v2] vga16fb: refuse to load in face of other driver controlling primary card

From: Marcin Slusarz <hidden>
Date: 2010-07-23 13:13:28

On Thu, Jul 22, 2010 at 05:20:39PM -0700, Andrew Morton wrote:

On Tue, 20 Jul 2010 21:19:23 +0200
Marcin Slusarz [off-list ref] wrote:

quoted

We don't want vga16fb to mess with hardware initialized by other driver.
Detect it and refuse to load.
It fixes nouveau interrupt storm on some machines.

Signed-off-by: Marcin Slusarz <redacted>
---
 drivers/video/vga16fb.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/drivers/video/vga16fb.c b/drivers/video/vga16fb.c
index 28ccab4..4505446 100644
--- a/drivers/video/vga16fb.c
+++ b/drivers/video/vga16fb.c

@@ -22,6 +22,7 @@
 #include <linux/platform_device.h>
 #include <linux/screen_info.h>
 
+#include <asm/fb.h>
 #include <asm/io.h>
 #include <video/vga.h>

@@ -1415,7 +1416,7 @@ static struct platform_device *vga16fb_device;
 
 static int __init vga16fb_init(void)
 {
-	int ret;
+	int ret, i;
 #ifndef MODULE
 	char *option = NULL;

@@ -1424,6 +1425,16 @@ static int __init vga16fb_init(void)
 
 	vga16fb_setup(option);
 #endif
+	for (i = 0 ; i < FB_MAX; i++) {
+		if (!registered_fb[i])
+			continue;
+		if (fb_is_primary_device(registered_fb[i])) {
+			printk(KERN_INFO "vga16fb: %s is driving the primary card, refusing to load\n",
+				registered_fb[i]->fix.id);
+			return -EBUSY;
+		}
+	}
+
 	ret = platform_driver_register(&vga16fb_driver);
 
 	if (!ret) {

This seems pretty hacky, and specific to vga16fb.

Is vga16fb.c the only fbdev driver (now and forever more) which can
grab the hardware from under a different driver's feet?

It's the only unaccelerated framebuffer which can be loaded _after_
real framebuffer (at least on x86). efifb and vesafb can only be compiled
statically and they are kicked off when accelerated driver is loaded - there's
no way to reload them later.

This is the basic and very common problem of two drivers diddling the
same hardware, yes?  A common way of preventing such collisions is to
perform a suitably-checked request_region() in both drivers.  Whichever
driver gets there second will get a failure and will bale out cleanly. 
Can that mechanism be made to work in this case?

I implemented this approach (patch below), but I still needed to modify
vga16fb - it really is a special framebuffer.

---
From: Marcin Slusarz <redacted>
Subject: [PATCH] vga16fb: refuse to load in face of other driver controlling primary card

We don't want vga16fb to mess with hardware initialized by other driver.
Detect it and refuse to load.
It fixes nouveau interrupt storm on some machines.

Signed-off-by: Marcin Slusarz <redacted>
---
 drivers/video/fbmem.c   |   33 ++++++++++++++++++++++++++++++++-
 drivers/video/vga16fb.c |   12 ++++++++++++
 include/linux/fb.h      |    3 +++
 3 files changed, 47 insertions(+), 1 deletions(-)

diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index 731fce6..cb19cae 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c

@@ -1504,6 +1504,7 @@ static bool fb_do_apertures_overlap(struct apertures_struct *gena,
 }
 
 #define VGA_FB_PHYS 0xA0000
+#define VGA_FB_PHYS_LEN 65536
 void remove_conflicting_framebuffers(struct apertures_struct *a,
 				     const char *name, bool primary)
 {

@@ -1532,6 +1533,17 @@ void remove_conflicting_framebuffers(struct apertures_struct *a,
 }
 EXPORT_SYMBOL(remove_conflicting_framebuffers);
 
+struct resource *request_vga_mem_region(const char *name)
+{
+	struct resource *res = request_mem_region(VGA_FB_PHYS,
+						VGA_FB_PHYS_LEN, name);
+	if (res = NULL)
+		printk(KERN_INFO "%s: other driver owns the primary card, refusing to load\n",
+				  name);
+	return res;
+}
+EXPORT_SYMBOL(request_vga_mem_region);
+
 /**
  *	register_framebuffer - registers a frame buffer device
  *	@fb_info: frame buffer info structure

@@ -1548,6 +1560,8 @@ register_framebuffer(struct fb_info *fb_info)
 	int i;
 	struct fb_event event;
 	struct fb_videomode mode;
+	bool primary;
+	struct resource *res = fb_info->resource;
 
 	if (num_registered_fb = FB_MAX)
 		return -ENXIO;

@@ -1555,8 +1569,19 @@ register_framebuffer(struct fb_info *fb_info)
 	if (fb_check_foreignness(fb_info))
 		return -ENOSYS;
 
+	primary = fb_is_primary_device(fb_info);
 	remove_conflicting_framebuffers(fb_info->apertures, fb_info->fix.id,
-					 fb_is_primary_device(fb_info));
+					primary);
+
+	/*
+	 * if the card is primary and the resource was not already allocated
+	 * by framebuffer driver then lock vga memory region
+	 */
+	if (primary && res = NULL) {
+		res = request_vga_mem_region(fb_info->fix.id);
+		if (res = NULL)
+			return -EBUSY;
+	}
 
 	num_registered_fb++;
 	for (i = 0 ; i < FB_MAX; i++)

@@ -1574,6 +1599,7 @@ register_framebuffer(struct fb_info *fb_info)
 		fb_info->dev = NULL;
 	} else
 		fb_init_device(fb_info);
+	fb_info->resource = res;
 
 	if (fb_info->pixmap.addr = NULL) {
 		fb_info->pixmap.addr = kmalloc(FBPIXMAPSIZE, GFP_KERNEL);

@@ -1658,6 +1684,11 @@ unregister_framebuffer(struct fb_info *fb_info)
 	num_registered_fb--;
 	fb_cleanup_device(fb_info);
 	device_destroy(fb_class, MKDEV(FB_MAJOR, i));
+	if (fb_info->resource) {
+		release_mem_region(fb_info->resource->start,
+				   resource_size(fb_info->resource));
+		fb_info->resource = NULL;
+	}
 	event.info = fb_info;
 	fb_notifier_call_chain(FB_EVENT_FB_UNREGISTERED, &event);

diff --git a/drivers/video/vga16fb.c b/drivers/video/vga16fb.c
index 28ccab4..952ae43 100644
--- a/drivers/video/vga16fb.c
+++ b/drivers/video/vga16fb.c

@@ -1307,6 +1307,16 @@ static int __devinit vga16fb_probe(struct platform_device *dev)
 	struct vga16fb_par *par;
 	int i;
 	int ret = 0;
+	struct resource *res;
+
+	/*
+	 * We have to do it here instead of relying on register_framebuffer - it
+	 * won't request that region because vga16fb won't be detected as primary
+	 * driver, because it's not directly connected to pci layer.
+	 */
+	res = request_vga_mem_region("vga16fb");
+	if (!res)
+		return -EBUSY;
 
 	printk(KERN_DEBUG "vga16fb: initializing\n");
 	info = framebuffer_alloc(sizeof(struct vga16fb_par), &dev->dev);

@@ -1321,6 +1331,7 @@ static int __devinit vga16fb_probe(struct platform_device *dev)
 		goto err_ioremap;
 	}
 
+	info->resource = res;
 	/* XXX share VGA_FB_PHYS and I/O region with vgacon and others */
 	info->screen_base = (void __iomem *)VGA_MAP_MEM(VGA_FB_PHYS, 0);

@@ -1390,6 +1401,7 @@ static int __devinit vga16fb_probe(struct platform_device *dev)
  err_ioremap:
 	framebuffer_release(info);
  err_fb_alloc:
+	release_mem_region(res->start, resource_size(res));
 	return ret;
 }

diff --git a/include/linux/fb.h b/include/linux/fb.h
index 8e5a9df..fc90804 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h

@@ -868,6 +868,8 @@ struct fb_info {
 			resource_size_t size;
 		} ranges[0];
 	} *apertures;
+
+	struct resource *resource;
 };
 
 static inline struct apertures_struct *alloc_apertures(unsigned int max_num) {

@@ -971,6 +973,7 @@ extern int register_framebuffer(struct fb_info *fb_info);
 extern int unregister_framebuffer(struct fb_info *fb_info);
 extern void remove_conflicting_framebuffers(struct apertures_struct *a,
 				const char *name, bool primary);
+extern struct resource *request_vga_mem_region(const char *name);
 extern int fb_prepare_logo(struct fb_info *fb_info, int rotate);
 extern int fb_show_logo(struct fb_info *fb_info, int rotate);
 extern char* fb_get_buffer_offset(struct fb_info *info, struct fb_pixmap *buf, u32 size);

-- 
1.7.1.1

Re: [PATCH v2] vga16fb: refuse to load in face of other driver controlling primary card

From: Geert Uytterhoeven <geert@linux-m68k.org>
Date: 2010-07-23 15:00:17

On Fri, Jul 23, 2010 at 15:10, Marcin Slusarz [off-list ref] wrote:

quoted hunk

diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index 731fce6..cb19cae 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c

@@ -1504,6 +1504,7 @@ static bool fb_do_apertures_overlap(struct apertures_struct *gena,

 }

 #define VGA_FB_PHYS 0xA0000
+#define VGA_FB_PHYS_LEN 65536

Does this really belong here?

quoted hunk

 void remove_conflicting_framebuffers(struct apertures_struct *a,
                                    const char *name, bool primary)
 {

@@ -1532,6 +1533,17 @@ void remove_conflicting_framebuffers(struct apertures_struct *a,

 }
 EXPORT_SYMBOL(remove_conflicting_framebuffers);

+struct resource *request_vga_mem_region(const char *name)
+{
+       struct resource *res = request_mem_region(VGA_FB_PHYS,
+                                               VGA_FB_PHYS_LEN, name);
+       if (res = NULL)
+               printk(KERN_INFO "%s: other driver owns the primary card, refusing to load\n",
+                                 name);
+       return res;
+}
+EXPORT_SYMBOL(request_vga_mem_region);
+
 /**
 *     register_framebuffer - registers a frame buffer device
 *     @fb_info: frame buffer info structure

@@ -1548,6 +1560,8 @@ register_framebuffer(struct fb_info *fb_info)

       int i;
       struct fb_event event;
       struct fb_videomode mode;
+       bool primary;
+       struct resource *res = fb_info->resource;

       if (num_registered_fb = FB_MAX)
               return -ENXIO;

@@ -1555,8 +1569,19 @@ register_framebuffer(struct fb_info *fb_info)

       if (fb_check_foreignness(fb_info))
               return -ENOSYS;

+       primary = fb_is_primary_device(fb_info);
       remove_conflicting_framebuffers(fb_info->apertures, fb_info->fix.id,
-                                        fb_is_primary_device(fb_info));
+                                       primary);
+
+       /*
+        * if the card is primary and the resource was not already allocated
+        * by framebuffer driver then lock vga memory region
+        */
+       if (primary && res = NULL) {
+               res = request_vga_mem_region(fb_info->fix.id);
+               if (res = NULL)
+                       return -EBUSY;
+       }

More VGA-specific stuff in drivers/video/fbmem.c.
Not all platforms have VGA memory.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [Nouveau] [PATCH v2] vga16fb: refuse to load in face of other driver controlling primary card

From: Dave Airlie <airlied@gmail.com>
Date: 2010-07-25 08:42:50

On Fri, Jul 23, 2010 at 11:10 PM, Marcin Slusarz
[off-list ref] wrote:

On Thu, Jul 22, 2010 at 05:20:39PM -0700, Andrew Morton wrote:

quoted

On Tue, 20 Jul 2010 21:19:23 +0200
Marcin Slusarz [off-list ref] wrote:

quoted

We don't want vga16fb to mess with hardware initialized by other driver.
Detect it and refuse to load.
It fixes nouveau interrupt storm on some machines.

Can't we keep this all inside vga16fb? just have it walk PCI tree
looking for a primary VGA card, and if it has a driver bound, just
refuse to bind.

Dave.

Re: [Nouveau] [PATCH v2] vga16fb: refuse to load in face of other driver controlling primary card

From: Marcin Slusarz <hidden>
Date: 2010-07-25 11:57:25

On Sun, Jul 25, 2010 at 06:42:48PM +1000, Dave Airlie wrote:

On Fri, Jul 23, 2010 at 11:10 PM, Marcin Slusarz
[off-list ref] wrote:

quoted

On Thu, Jul 22, 2010 at 05:20:39PM -0700, Andrew Morton wrote:

quoted

On Tue, 20 Jul 2010 21:19:23 +0200
Marcin Slusarz [off-list ref] wrote:

quoted

We don't want vga16fb to mess with hardware initialized by other driver.
Detect it and refuse to load.
It fixes nouveau interrupt storm on some machines.

Can't we keep this all inside vga16fb?

That's what v1 did (*), but Andrew didn't like it.

just have it walk PCI tree
looking for a primary VGA card, and if it has a driver bound, just
refuse to bind.

Dave.

(*) http://lkml.org/lkml/2010/7/20/244

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help