Console unregistration questions

Console unregistration questions

[Jesse Barnes]

In doing the DRM modesetting work, we've been using the FB layer to manage 
regular FB devices and provide a DRM based console to the user (iow DRM 
owns the PCI device but the FB layer is used as a console and to provide 
the familiar /dev/fb interface to existing software).

However, in doing development we noticed that when we remove the new DRM 
based fb driver, our machines tend to hang.  This appears to be due to 
bugs in console unregistration.

The DRM fb driver's cleanup routine is normal, I think:

int drmfb_remove(struct drm_device *dev, struct drm_framebuffer *fb)
{
	struct fb_info *info = fb->fbdev;
	
	unregister_framebuffer(info);
	framebuffer_release(info);
	/* Unmap framebuffer */
	drm_mem_reg_iounmap(dev, &fb->bo->mem, fb->virtual_base);

	return 0;
}

But since we're using drmfb as a console, the unregister_framebuffer call 
silently fails.  fbcon is builtin to the kernel, so when drmfb exits, 
unregister_framebuffer will end up calling fbcon's fbcon_fb_unregistered() 
function via the notifier chain.  However, when fbcon_fb_unregistered 
calls unregister_con_driver(&fb_con) it ignores the return value, which is 
bad since unregister_con_driver left fbcon's routines active (due to fbcon 
still being bound to the console) even while unregister_framebuffer freed 
their underlying structures.  So when unregister_framebuffer returns, the 
very next console operation causes an oops or worse (I usually see 
hide_cursor->fbcon_cursor die when it tries to get at info->fbcon_par).

However, it doesn't appear that there's a way for fb drivers to unbind 
themselves from the console at unload time, so we have to do it manually.  
Should there be a way to unbind it from driver code?  Maybe by exporting a 
wrapper to unbind_con_driver?  Also, shouldn't callers of 
unregister_con_driver be checking return values?  That would have made 
this bug a lot easier to find at least. :)

Thanks,
Jesse

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: Console unregistration questions

[Antonino A. Daplas]

On Tue, 2007-04-24 at 14:59 -0700, Jesse Barnes wrote:
> In doing the DRM modesetting work, we've been using the FB layer to manage 
> regular FB devices and provide a DRM based console to the user (iow DRM 
> owns the PCI device but the FB layer is used as a console and to provide 
> the familiar /dev/fb interface to existing software).
> 
> However, in doing development we noticed that when we remove the new DRM 
> based fb driver, our machines tend to hang.  This appears to be due to 
> bugs in console unregistration.
> 
> The DRM fb driver's cleanup routine is normal, I think:
> 
> int drmfb_remove(struct drm_device *dev, struct drm_framebuffer *fb)
> {
> 	struct fb_info *info = fb->fbdev;
> 	
> 	unregister_framebuffer(info);
> 	framebuffer_release(info);
> 	/* Unmap framebuffer */
> 	drm_mem_reg_iounmap(dev, &fb->bo->mem, fb->virtual_base);
> 
> 	return 0;
> }
> 
> But since we're using drmfb as a console, the unregister_framebuffer call 
> silently fails.  fbcon is builtin to the kernel, so when drmfb exits, 
> unregister_framebuffer will end up calling fbcon's fbcon_fb_unregistered() 
> function via the notifier chain.  However, when fbcon_fb_unregistered 
> calls unregister_con_driver(&fb_con) it ignores the return value, which is 
> bad since unregister_con_driver left fbcon's routines active (due to fbcon 
> still being bound to the console) even while unregister_framebuffer freed 
> their underlying structures.

Okay, we can do that. This was not done before because
unregister_framebuffer was only called on rmmod <module>.  But checking
for the return error and have it propagate to the driver should not be a
problem.

I'll send a patch to you and to akpm soon.

>   So when unregister_framebuffer returns, the 
> very next console operation causes an oops or worse (I usually see 
> hide_cursor->fbcon_cursor die when it tries to get at info->fbcon_par).
> 
> However, it doesn't appear that there's a way for fb drivers to unbind 
> themselves from the console at unload time, so we have to do it manually.  

You cannot unbind framebuffer drivers independently from the console
layer because the console is holding a reference count on them. 

FYI: this was what I did initially, but akpm objected :-).

> Should there be a way to unbind it from driver code?  Maybe by exporting a 
> wrapper to unbind_con_driver? 

Yes, you can expose unbind_con_driver(), I don't mind. You can do it
first in your tree, then when you need it for mainline, let me know.

(The kernel has someone who hunts for unused EXPORT_SYMBOLs and removes
them without remorse :-)


>  Also, shouldn't callers of 
> unregister_con_driver be checking return values?  That would have made 
> this bug a lot easier to find at least. :)
> 

Okay.

Tony




-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: Console unregistration questions

[Antonino A. Daplas]

On Wed, 2007-04-25 at 07:19 +0800, Antonino A. Daplas wrote:
> On Tue, 2007-04-24 at 14:59 -0700, Jesse Barnes wrote:
> > In doing the DRM modesetting work, we've been using the FB layer to manage 
> > regular FB devices and provide a DRM based console to the user (iow DRM 
> > owns the PCI device but the FB layer is used as a console and to provide 
> > the familiar /dev/fb interface to existing software).
> > 
> > However, in doing development we noticed that when we remove the new DRM 
> > based fb driver, our machines tend to hang.  This appears to be due to 
> > bugs in console unregistration.
> > 
> > The DRM fb driver's cleanup routine is normal, I think:
> > 
> > int drmfb_remove(struct drm_device *dev, struct drm_framebuffer *fb)
> > {
> > 	struct fb_info *info = fb->fbdev;
> > 	
> > 	unregister_framebuffer(info);
> > 	framebuffer_release(info);
> > 	/* Unmap framebuffer */
> > 	drm_mem_reg_iounmap(dev, &fb->bo->mem, fb->virtual_base);
> > 
> > 	return 0;
> > }
> > 
> > But since we're using drmfb as a console, the unregister_framebuffer call 
> > silently fails.  fbcon is builtin to the kernel, so when drmfb exits, 
> > unregister_framebuffer will end up calling fbcon's fbcon_fb_unregistered() 
> > function via the notifier chain.  However, when fbcon_fb_unregistered 
> > calls unregister_con_driver(&fb_con) it ignores the return value, which is 
> > bad since unregister_con_driver left fbcon's routines active (due to fbcon 
> > still being bound to the console) even while unregister_framebuffer freed 
> > their underlying structures.
> 
> Okay, we can do that. This was not done before because
> unregister_framebuffer was only called on rmmod <module>.  But checking
> for the return error and have it propagate to the driver should not be a
> problem.
> 

Additionally, you can implement fb_open() and fb_release().  Then only
call unregister_framebuffer() if there are no other modules that opened
your driver.

> I'll send a patch to you and to akpm soon.
> 
> >   So when unregister_framebuffer returns, the 
> > very next console operation causes an oops or worse (I usually see 
> > hide_cursor->fbcon_cursor die when it tries to get at info->fbcon_par).
> > 
> > However, it doesn't appear that there's a way for fb drivers to unbind 
> > themselves from the console at unload time, so we have to do it manually.  
> 
> You cannot unbind framebuffer drivers independently from the console
> layer because the console is holding a reference count on them. 
> 
> FYI: this was what I did initially, but akpm objected :-).
> 
> > Should there be a way to unbind it from driver code?  Maybe by exporting a 
> > wrapper to unbind_con_driver? 
> 
> Yes, you can expose unbind_con_driver(), I don't mind. You can do it
> first in your tree, then when you need it for mainline, let me know.
> 
> (The kernel has someone who hunts for unused EXPORT_SYMBOLs and removes
> them without remorse :-)
> 
> 
> >  Also, shouldn't callers of 
> > unregister_con_driver be checking return values?  That would have made 
> > this bug a lot easier to find at least. :)
> > 

Replying to myself:

At this point, if unregister_con_driver() fails, it is too late for
fbcon, the best thing to do is issue a BUG().

However, I'll add extra code to unregister_framebuffer() that will check
if it's safe for the driver to unload.  The drivers may then choose to
check it or not.

Tony



-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: Console unregistration questions

[Jesse Barnes]

On Tuesday, April 24, 2007, Antonino A. Daplas wrote:
> > >  Also, shouldn't callers of
> > > unregister_con_driver be checking return values?  That would have
> > > made this bug a lot easier to find at least. :)
>
> Replying to myself:
>
> At this point, if unregister_con_driver() fails, it is too late for
> fbcon, the best thing to do is issue a BUG().

I guess there's no way to propagate the error back to 
unregister_framebuffer?  If we did, then it could return -EBUSY or 
something, keeping the module loaded instead of unsafely allowing it to be 
removed...

> However, I'll add extra code to unregister_framebuffer() that will check
> if it's safe for the driver to unload.  The drivers may then choose to
> check it or not.

Sounds good.

Jesse

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: Console unregistration questions

[Antonino A. Daplas]

On Tue, 2007-04-24 at 16:46 -0700, Jesse Barnes wrote:
> On Tuesday, April 24, 2007, Antonino A. Daplas wrote:
> > > >  Also, shouldn't callers of
> > > > unregister_con_driver be checking return values?  That would have
> > > > made this bug a lot easier to find at least. :)
> >
> > Replying to myself:
> >
> > At this point, if unregister_con_driver() fails, it is too late for
> > fbcon, the best thing to do is issue a BUG().
> 
> I guess there's no way to propagate the error back to 
> unregister_framebuffer?  If we did, then it could return -EBUSY or 
> something, keeping the module loaded instead of unsafely allowing it to be 
> removed...

We can, by doing the method mentioned below.  By the time fbcon is
notified of the unregistration, it's too late and the error is
unrecoverable.

> 
> > However, I'll add extra code to unregister_framebuffer() that will check
> > if it's safe for the driver to unload.  The drivers may then choose to
> > check it or not.

Yes, this should not be too hard to implement.

Tony



-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: Console unregistration questions

[Antonino A. Daplas]

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

On Tue, 2007-04-24 at 16:46 -0700, Jesse Barnes wrote:
> On Tuesday, April 24, 2007, Antonino A. Daplas wrote:
> > > >  Also, shouldn't callers of
> > > > unregister_con_driver be checking return values?  That would have
> > > > made this bug a lot easier to find at least. :)
> >
> > Replying to myself:
> >
> > At this point, if unregister_con_driver() fails, it is too late for
> > fbcon, the best thing to do is issue a BUG().
> 
> I guess there's no way to propagate the error back to 
> unregister_framebuffer?  If we did, then it could return -EBUSY or 
> something, keeping the module loaded instead of unsafely allowing it to be 
> removed...
> 
> > However, I'll add extra code to unregister_framebuffer() that will check
> > if it's safe for the driver to unload.  The drivers may then choose to
> > check it or not.
> 
> Sounds good.
> 

Here's a test patch.  Basically, it checks if the driver is still mapped
to the console, and if it is, unregister_framebuffer() will exit
immediately.  As mentioned, it's untested, so let me know if there are
problems.

Note, the driver must also do its part, by checking if the driver is
opened by something in userspace.

Tony


[-- Attachment #2: safe_unregister.diff --]
[-- Type: text/x-patch, Size: 1340 bytes --]

diff --git a/drivers/video/console/fbcon.c b/drivers/video/console/fbcon.c
index 34ec0ef..f424adb 100644
--- a/drivers/video/console/fbcon.c
+++ b/drivers/video/console/fbcon.c
@@ -3100,6 +3100,8 @@ static int fbcon_event_notify(struct not
 	case FB_EVENT_NEW_MODELIST:
 		fbcon_new_modelist(info);
 		break;
+	case FB_EVENT_FB_UNREGISTER_SAFE:
+		ret = search_fb_in_map(info->node);
 	}
 
 done:
diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index cd14079..8e878e6 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -1362,6 +1362,10 @@ unregister_framebuffer(struct fb_info *f
 	if (!registered_fb[i])
 		return -EINVAL;
 
+	event.info = fb_info;
+	if (fb_notifier_call_chain(FB_EVENT_FB_UNREGISTER_SAFE, &event))
+		return -EINVAL;
+	
 	if (fb_info->pixmap.addr &&
 	    (fb_info->pixmap.flags & FB_PIXMAP_DEFAULT))
 		kfree(fb_info->pixmap.addr);
diff --git a/include/linux/fb.h b/include/linux/fb.h
index ecb8bc6..e955865 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -527,6 +527,8 @@ #define FB_EVENT_NEW_MODELIST           
 #define FB_EVENT_MODE_CHANGE_ALL	0x0B
 /*	A software display blank change occured */
 #define FB_EVENT_CONBLANK               0x0C
+/*      Check if it's safe to unregister */
+#define FB_EVENT_FB_UNREGISTER_SAFE     0x0D
 
 struct fb_event {
 	struct fb_info *info;

[-- Attachment #3: Type: text/plain, Size: 286 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

[-- Attachment #4: Type: text/plain, Size: 182 bytes --]

_______________________________________________
Linux-fbdev-devel mailing list
Linux-fbdev-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-fbdev-devel

Re: Console unregistration questions

[Jesse Barnes]

On Tuesday, April 24, 2007 6:17 pm Antonino A. Daplas wrote:
> Here's a test patch.  Basically, it checks if the driver is still
> mapped to the console, and if it is, unregister_framebuffer() will
> exit immediately.  As mentioned, it's untested, so let me know if
> there are problems.
>
> Note, the driver must also do its part, by checking if the driver is
> opened by something in userspace.

Right.  Looks good to me.

Acked-by: Jesse Barnes <jesse.barnes@intel.com>

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: Console unregistration questions

[Jesse Barnes]

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

On Tuesday, April 24, 2007, Antonino A. Daplas wrote:
> Here's a test patch.  Basically, it checks if the driver is still mapped
> to the console, and if it is, unregister_framebuffer() will exit
> immediately.  As mentioned, it's untested, so let me know if there are
> problems.
>
> Note, the driver must also do its part, by checking if the driver is
> opened by something in userspace.

Here's a hacked up version of an unregistration patch.  It exports 
unbind_con_driver from vt.c and makes fbcon and fbmem use it when a driver 
goes away...

Thoughts?

Thanks,
Jesse

[-- Attachment #2: console-unregister.patch --]
[-- Type: text/x-diff, Size: 4172 bytes --]

--- linux-2.6.21-rc4/drivers/video/fbmem.c	2007-03-15 17:20:01.000000000 -0700
+++ linux-2.6.21-rc4-modesetting/drivers/video/fbmem.c	2007-04-26 18:16:52.000000000 -0700
@@ -1349,6 +1349,8 @@
 	if (!registered_fb[i])
 		return -EINVAL;
 
+	event.info = fb_info;
+	fb_notifier_call_chain(FB_EVENT_FB_UNBIND, &event);
 	if (fb_info->pixmap.addr &&
 	    (fb_info->pixmap.flags & FB_PIXMAP_DEFAULT))
 		kfree(fb_info->pixmap.addr);
--- linux-2.6.21-rc4/include/linux/fb.h	2007-03-15 17:20:01.000000000 -0700
+++ linux-2.6.21-rc4-modesetting/include/linux/fb.h	2007-04-26 17:33:07.000000000 -0700
@@ -525,6 +525,8 @@
 #define FB_EVENT_MODE_CHANGE_ALL	0x0B
 /*	A software display blank change occured */
 #define FB_EVENT_CONBLANK               0x0C
+/*      Unbind from the console if possible */
+#define FB_EVENT_FB_UNBIND              0x0E
 
 struct fb_event {
 	struct fb_info *info;
--- linux-2.6.21-rc4/include/linux/console.h	2007-03-15 17:20:01.000000000 -0700
+++ linux-2.6.21-rc4-modesetting/include/linux/console.h	2007-04-26 17:56:31.000000000 -0700
@@ -64,6 +64,7 @@
 extern const struct consw newport_con;	/* SGI Newport console  */
 extern const struct consw prom_con;	/* SPARC PROM console */
 
+int unbind_con_driver(const struct consw *csw, int first, int last, int deflt);
 int con_is_bound(const struct consw *csw);
 int register_con_driver(const struct consw *csw, int first, int last);
 int unregister_con_driver(const struct consw *csw);
--- linux-2.6.21-rc4/drivers/video/console/fbcon.c	2007-03-15 17:20:01.000000000 -0700
+++ linux-2.6.21-rc4-modesetting/drivers/video/console/fbcon.c	2007-04-26 18:15:49.000000000 -0700
@@ -563,8 +563,10 @@
 	for (i = first_fb_vc; i <= last_fb_vc; i++)
 		con2fb_map[i] = info_idx;
 
+	printk(KERN_ERR "calling take_over_console, return value: ");
 	err = take_over_console(&fb_con, first_fb_vc, last_fb_vc,
 				fbcon_is_default);
+	printk(KERN_ERR "%d\n", err);
 
 	if (err) {
 		for (i = first_fb_vc; i <= last_fb_vc; i++) {
@@ -2896,6 +2898,13 @@
 	return found;
 }
 
+static int fbcon_fb_unbind(int idx)
+{
+	unbind_con_driver(&fb_con, 0, MAX_NR_CONSOLES - 1, 0);
+
+	return 0;
+}
+
 static int fbcon_fb_unregistered(int idx)
 {
 	int i;
@@ -2933,6 +2942,7 @@
 {
 	int ret = 0, i;
 
+	printk(KERN_ERR "fbcon registration called\n");
 	if (info_idx == -1) {
 		for (i = first_fb_vc; i <= last_fb_vc; i++) {
 			if (con2fb_map_boot[i] == idx) {
@@ -2941,8 +2951,10 @@
 			}
 		}
 
-		if (info_idx != -1)
+		if (info_idx != -1) {
+			printk(KERN_ERR "fb taking over console\n");
 			ret = fbcon_takeover(1);
+		}
 	} else {
 		for (i = first_fb_vc; i <= last_fb_vc; i++) {
 			if (con2fb_map_boot[i] == idx &&
@@ -3036,6 +3048,9 @@
 		mode = event->data;
 		ret = fbcon_mode_deleted(info, mode);
 		break;
+	case FB_EVENT_FB_UNBIND:
+		ret = fbcon_fb_unbind(info->node);
+		break;
 	case FB_EVENT_FB_REGISTERED:
 		ret = fbcon_fb_registered(info->node);
 		break;
--- linux-2.6.21-rc4/drivers/char/vt.c	2007-03-15 17:20:01.000000000 -0700
+++ linux-2.6.21-rc4-modesetting/drivers/char/vt.c	2007-04-26 18:22:42.000000000 -0700
@@ -2832,8 +2832,24 @@
 	return retval;
 }
 
-static int unbind_con_driver(const struct consw *csw, int first, int last,
-			     int deflt)
+/**
+ * unbind_con_driver - unbind a console driver
+ * @csw: pointer to console driver to unregister
+ * @first: first in range of consoles that @csw should be unbound from
+ * @last: last in range of consoles that @csw should be unbound from
+ * @deflt: should next bound console driver be default after @csw is unbound?
+ * 
+ * To unbind a driver from all possible consoles, pass 0 as @first and
+ * %MAX_NR_CONSOLES as @last.
+ *
+ * @deflt controls whether the console that ends up replacing @csw should be
+ * the default console.
+ *
+ * RETURNS:
+ * -ENODEV if @csw isn't a registered console driver or can't be unregistered
+ * or 0 on success.
+ */
+int unbind_con_driver(const struct consw *csw, int first, int last, int deflt)
 {
 	struct module *owner = csw->owner;
 	const struct consw *defcsw = NULL;
@@ -2918,6 +2934,7 @@
 	return retval;
 
 }
+EXPORT_SYMBOL(unbind_con_driver);
 
 static int vt_bind(struct con_driver *con)
 {

[-- Attachment #3: Type: text/plain, Size: 286 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

[-- Attachment #4: Type: text/plain, Size: 182 bytes --]

_______________________________________________
Linux-fbdev-devel mailing list
Linux-fbdev-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-fbdev-devel

Re: Console unregistration questions

[Antonino A. Daplas]

On Thu, 2007-04-26 at 18:30 -0700, Jesse Barnes wrote:
> On Tuesday, April 24, 2007, Antonino A. Daplas wrote:
> > Here's a test patch.  Basically, it checks if the driver is still mapped
> > to the console, and if it is, unregister_framebuffer() will exit
> > immediately.  As mentioned, it's untested, so let me know if there are
> > problems.
> >
> > Note, the driver must also do its part, by checking if the driver is
> > opened by something in userspace.
> 
> Here's a hacked up version of an unregistration patch.  It exports 
> unbind_con_driver from vt.c and makes fbcon and fbmem use it when a driver 
> goes away...
> 

I have no problems with the purpose of this patch, but I would like what
other kernel developers, both fbdev and non-fbdev, think about this.

The main problem with this version is it's possible to have more than 1
fb driver mapped to each console (and people do that).  So if one of
them unregisters, it will also unbind the other one. Code-wise, it's not
a problem.  But users will not expect that behavior.

It's fixable though, as unbind_con_driver() also accepts the first and
last parameter.  fbcon_fb_unbind() will have to walk con2fbmap[], so
something like this untested code instead:

static int fbcon_fb_unbind(int idx) {
	for (i = 0; i < MAX_NR_CONSOLES; i++) {
		/* Assure we do not unbind other drivers */
		if (idx == con2fbmap[i])
			/* can be optimize to minimize multiple calls to
			 	unbind_con_driver() */
			unbind_con_driver(&fb_con, i, i, 0); 
	}
]	

Besides the above, we still have to make adjustments to fbcon so it can
handle one driver going away and while still managing other loaded
drivers.

Tony


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: Console unregistration questions

[Jesse Barnes]

On Thursday, April 26, 2007, Antonino A. Daplas wrote:
> I have no problems with the purpose of this patch, but I would like what
> other kernel developers, both fbdev and non-fbdev, think about this.
>
> The main problem with this version is it's possible to have more than 1
> fb driver mapped to each console (and people do that).  So if one of
> them unregisters, it will also unbind the other one. Code-wise, it's not
> a problem.  But users will not expect that behavior.
>
> It's fixable though, as unbind_con_driver() also accepts the first and
> last parameter.  fbcon_fb_unbind() will have to walk con2fbmap[], so
> something like this untested code instead:
>
> static int fbcon_fb_unbind(int idx) {
> 	for (i = 0; i < MAX_NR_CONSOLES; i++) {
> 		/* Assure we do not unbind other drivers */
> 		if (idx == con2fbmap[i])
> 			/* can be optimize to minimize multiple calls to
> 			 	unbind_con_driver() */
> 			unbind_con_driver(&fb_con, i, i, 0);
> 	}
> ]
>
> Besides the above, we still have to make adjustments to fbcon so it can
> handle one driver going away and while still managing other loaded
> drivers.

Unbinding from specific consoles would indeed be much better, and provide 
more intuitive behavior.  I just wasn't sure how to do it offhand in 
fbcon; I'll try your snippet, looks like it should work nicely (at and 
worst we'll have 64 calls to unbind_con_driver, so I don't think batching 
is an issue).

Thanks,
Jesse

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: Console unregistration questions

[Antonino A. Daplas]

On Fri, 2007-04-27 at 08:14 -0700, Jesse Barnes wrote:
> On Thursday, April 26, 2007, Antonino A. Daplas wrote:
> > I have no problems with the purpose of this patch, but I would like what
> > other kernel developers, both fbdev and non-fbdev, think about this.
> >
> > The main problem with this version is it's possible to have more than 1
> > fb driver mapped to each console (and people do that).  So if one of
> > them unregisters, it will also unbind the other one. Code-wise, it's not
> > a problem.  But users will not expect that behavior.
> >
> > It's fixable though, as unbind_con_driver() also accepts the first and
> > last parameter.  fbcon_fb_unbind() will have to walk con2fbmap[], so
> > something like this untested code instead:
> >
> > static int fbcon_fb_unbind(int idx) {
> > 	for (i = 0; i < MAX_NR_CONSOLES; i++) {
> > 		/* Assure we do not unbind other drivers */
> > 		if (idx == con2fbmap[i])
> > 			/* can be optimize to minimize multiple calls to
> > 			 	unbind_con_driver() */
> > 			unbind_con_driver(&fb_con, i, i, 0);
> > 	}
> > ]
> >
> > Besides the above, we still have to make adjustments to fbcon so it can
> > handle one driver going away and while still managing other loaded
> > drivers.
> 
> Unbinding from specific consoles would indeed be much better, and provide 
> more intuitive behavior.  I just wasn't sure how to do it offhand in 
> fbcon; I'll try your snippet, looks like it should work nicely (at and 
> worst we'll have 64 calls to unbind_con_driver, so I don't think batching 
> is an issue).

I'll try modifying fbcon + your patch this weekend so we can selectively
unbind framebuffers.

Tony



-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: Console unregistration questions

[Jesse Barnes]

On Tuesday, April 24, 2007, Antonino A. Daplas wrote:
> > But since we're using drmfb as a console, the unregister_framebuffer
> > call silently fails.  fbcon is builtin to the kernel, so when drmfb
> > exits, unregister_framebuffer will end up calling fbcon's
> > fbcon_fb_unregistered() function via the notifier chain.  However,
> > when fbcon_fb_unregistered calls unregister_con_driver(&fb_con) it
> > ignores the return value, which is bad since unregister_con_driver
> > left fbcon's routines active (due to fbcon still being bound to the
> > console) even while unregister_framebuffer freed their underlying
> > structures.
>
> Okay, we can do that. This was not done before because
> unregister_framebuffer was only called on rmmod <module>.  But checking
> for the return error and have it propagate to the driver should not be a
> problem.
>
> I'll send a patch to you and to akpm soon.

Great!  Thanks.

> >   So when unregister_framebuffer returns, the
> > very next console operation causes an oops or worse (I usually see
> > hide_cursor->fbcon_cursor die when it tries to get at
> > info->fbcon_par).
> >
> > However, it doesn't appear that there's a way for fb drivers to unbind
> > themselves from the console at unload time, so we have to do it
> > manually.
>
> You cannot unbind framebuffer drivers independently from the console
> layer because the console is holding a reference count on them.
>
> FYI: this was what I did initially, but akpm objected :-).
>
> > Should there be a way to unbind it from driver code?  Maybe by
> > exporting a wrapper to unbind_con_driver?
>
> Yes, you can expose unbind_con_driver(), I don't mind. You can do it
> first in your tree, then when you need it for mainline, let me know.

So using unbind_con_driver is ok since we go through the console layer and 
all the refcounting is kosher?  The only other thing with 
unbind_con_driver is I'm not sure about the other arguments (first, last, 
& default) should be if called from a driver context.

Thanks,
Jesse



-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: Console unregistration questions

[Antonino A. Daplas]

On Tue, 2007-04-24 at 16:42 -0700, Jesse Barnes wrote:
> On Tuesday, April 24, 2007, Antonino A. Daplas wrote:
> > > But since we're using drmfb as a console, the unregister_framebuffer
> > > call silently fails.  fbcon is builtin to the kernel, so when drmfb
> > > exits, unregister_framebuffer will end up calling fbcon's
> > > fbcon_fb_unregistered() function via the notifier chain.  However,
> > > when fbcon_fb_unregistered calls unregister_con_driver(&fb_con) it
> > > ignores the return value, which is bad since unregister_con_driver
> > > left fbcon's routines active (due to fbcon still being bound to the
> > > console) even while unregister_framebuffer freed their underlying
> > > structures.
> >
> > Okay, we can do that. This was not done before because
> > unregister_framebuffer was only called on rmmod <module>.  But checking
> > for the return error and have it propagate to the driver should not be a
> > problem.
> >
> > I'll send a patch to you and to akpm soon.
> 
> Great!  Thanks.
> 
> > >   So when unregister_framebuffer returns, the
> > > very next console operation causes an oops or worse (I usually see
> > > hide_cursor->fbcon_cursor die when it tries to get at
> > > info->fbcon_par).
> > >
> > > However, it doesn't appear that there's a way for fb drivers to unbind
> > > themselves from the console at unload time, so we have to do it
> > > manually.
> >
> > You cannot unbind framebuffer drivers independently from the console
> > layer because the console is holding a reference count on them.
> >
> > FYI: this was what I did initially, but akpm objected :-).
> >
> > > Should there be a way to unbind it from driver code?  Maybe by
> > > exporting a wrapper to unbind_con_driver?
> >
> > Yes, you can expose unbind_con_driver(), I don't mind. You can do it
> > first in your tree, then when you need it for mainline, let me know.
> 
> So using unbind_con_driver is ok since we go through the console layer and 
> all the refcounting is kosher?

Yes.

>   The only other thing with 
> unbind_con_driver is I'm not sure about the other arguments (first, last, 
> & default) should be if called from a driver context.

No, they're only for the console.  So, if you're writing a pure fbdev
driver, then the user has to unbind the console from the VT layer
manually.  If you are writing an fbdev/console-combo driver, then you
can do that by calling unbind_con_driver() first before
unregister_framebuffer().

Tony


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/