udlfb: remove sysfs framebuffer device with USB .disconnect()

From: Kay Sievers <[email protected]>
Subject: udlfb: remove sysfs framebuffer device with USB .disconnect()

The USB graphics card driver delays the unregistering of the framebuffer device to a workqueue, which breaks the userspace visible remove uevent sequence. Recent userspace tools started to support USB graphics card hotplug out-of-the-box and rely on proper events sent by the kernel.

The framebuffer device is a direct child of the USB interface which is removed immediately after the USB .disconnect() callback. But the fb device in /sys stays around until its final cleanup, at a time where all the parent devices have been removed already.

To work around that, we remove the sysfs fb device directly in the USB .disconnect() callback and leave only the cleanup of the internal fb data to the delayed work.

Before:

  add      /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.2 (usb)
  add      /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.2/2-1.2:1.0 (usb)
  add      /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.2/2-1.2:1.0/graphics/fb0 (graphics)
  remove   /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.2/2-1.2:1.0 (usb)
  remove   /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.2 (usb)
  remove   /2-1.2:1.0/graphics/fb0 (graphics)

After:

  add      /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.2 (usb)
  add      /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.2/2-1.2:1.0 (usb)
  add      /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.2/2-1.2:1.0/graphics/fb1 (graphics)
  remove   /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.2/2-1.2:1.0/graphics/fb1 (graphics)
  remove   /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.2/2-1.2:1.0 (usb)
  remove   /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.2 (usb)

Cc [email protected]

Tested-by Bernie Thompson <[email protected]>

Acked-by Bernie Thompson <[email protected]>

Signed-off-by Kay Sievers <[email protected]>

---
 drivers/video/fbmem.c |   18 +++++++++++++++++-
 drivers/video/udlfb.c |    2 +-
 include/linux/fb.h    |    1 +
 3 files changed, 19 insertions(+), 2 deletions(-)

drivers/video/fbmem.c +1 -1

 	num_registered_fb--;
 	fb_cleanup_device(fb_info);
-	device_destroy(fb_class, MKDEV(FB_MAJOR, i));
+	unlink_framebuffer(fb_info);
 	event.info = fb_info;
 	fb_notifier_call_chain(FB_EVENT_FB_UNREGISTERED, &event);
 

drivers/video/fbmem.c +9 -0

+
+	if (fb_info->dev) {
+		device_destroy(fb_class, MKDEV(FB_MAJOR, i));
+		fb_info->dev = NULL;
+	}
+	return 0;
+}
+EXPORT_SYMBOL(unlink_framebuffer);
+
 void remove_conflicting_framebuffers(struct apertures_struct *a,
 				     const char *name, bool primary)
 {

drivers/video/udlfb.c +1 -1

@@ -1739,7 +1739,7 @@ static void dlfb_usb_disconnect(struct u
 	for (i = 0; i < ARRAY_SIZE(fb_device_attrs); i++)
 		device_remove_file(info->dev, &fb_device_attrs[i]);
 	device_remove_bin_file(info->dev, &edid_attr);
-
+	unlink_framebuffer(info);
 	usb_set_intfdata(interface, NULL);
 
 	/* if clients still have us open, will be freed on last close */

drivers/video/fbmem.c static int do_unregister_framebuffer(str · line 1672

Quoted 3 lines · Kay Sievers

+++ b/drivers/video/fbmem.c

drivers/video/fbmem.c +0 -0

@@ -1672,7 +1672,7 @@ static int do_unregister_framebuffer(str
       registered_fb[i] = NULL;

Here registered_fb[fb_info->node] is set to NULL...

drivers/video/fbmem.c static int do_unregister_framebuffer(str · line 1690

Quoted 7 lines

drivers/video/fbmem.c +7 -0

+int unlink_framebuffer(struct fb_info *fb_info)
+{
+     int i;
+
+     i = fb_info->node;
+     if (i < 0 || i >= FB_MAX || registered_fb[i] != fb_info)
+             return -EINVAL;

...and here you check whether it is still valid (did you copy the check from the do_unregister_framebuffer?). So the code below would be never executed, when called in this context.

Quoted 8 lines · Florian Tobias Schandinat

drivers/video/fbmem.c static int do_unregister_framebuffer(str · line 1672

Quoted 3 lines · Kay Sievers

+++ b/drivers/video/fbmem.c

drivers/video/fbmem.c +0 -0

@@ -1672,7 +1672,7 @@ static int do_unregister_framebuffer(str
      registered_fb[i] = NULL;

Here registered_fb[fb_info->node] is set to NULL...

Quoted 11 lines

drivers/video/fbmem.c static int do_unregister_framebuffer(str · line 1690

Quoted 7 lines

drivers/video/fbmem.c +7 -0

+int unlink_framebuffer(struct fb_info *fb_info)
+{
+     int i;
+
+     i = fb_info->node;
+     if (i < 0 || i >= FB_MAX || registered_fb[i] != fb_info)
+             return -EINVAL;

...and here you check whether it is still valid (did you copy the check from the do_unregister_framebuffer?). So the code below would be never executed, when called in this context.

That's true. If one calls the current _unregister() *before* calling _unlink(), the _unlink() is a NOP. But that's intentional.

The _unlink() is only needed for stuff needs to be called when we are required to 'orphan' the device before cleaning up. Normal, non-hotpluggable devices do not need this, they can continue what they do today.

The _unlink() call *can* be called before _unregister() if needed for disconnecting it from the driver core parent and remove its userspace visibility. If _unlink() is called before _unregister(), it passes the check, the later _unregister() will also pass the check but get rid of the entire device.

Kay
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 2 lines · Kay Sievers

That's true. If one calls the current _unregister() *before* calling _unlink(), the _unlink() is a NOP. But that's intentional.

I don't think you got me right. My complaint was that after your patch do_unregister_framebuffer itself calls unlink_framebuffer _after_ it set registered_fb[i] = NULL;
So for any framebuffer that does not call unlink_framebuffer directly the line device_destroy(fb_class, MKDEV(FB_MAJOR, i));
will no longer be executed. Do you agree?

Quoted 5 lines · Florian Tobias Schandinat

Quoted 2 lines · Kay Sievers

That's true. If one calls the current _unregister() *before* calling _unlink(), the _unlink() is a NOP. But that's intentional.

Quoted 6 lines

I don't think you got me right. My complaint was that after your patch do_unregister_framebuffer itself calls unlink_framebuffer _after_ it set registered_fb[i] = NULL;
So for any framebuffer that does not call unlink_framebuffer directly the line device_destroy(fb_class, MKDEV(FB_MAJOR, i));
will no longer be executed. Do you agree?

Yes, I absolutely agree. :)

From: Kay Sievers <[email protected]>
Subject: udlfb: remove sysfs framebuffer device with USB .disconnect()

The USB graphics card driver delays the unregistering of the framebuffer device to a workqueue, which breaks the userspace visible remove uevent sequence. Recent userspace tools started to support USB graphics card hotplug out-of-the-box and rely on proper events sent by the kernel.

The framebuffer device is a direct child of the USB interface which is removed immediately after the USB .disconnect() callback. But the fb device in /sys stays around until its final cleanup, at a time where all the parent devices have been removed already.

To work around that, we remove the sysfs fb device directly in the USB .disconnect() callback and leave only the cleanup of the internal fb data to the delayed work.

Before:

 add      /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.2 (usb)
 add      /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.2/2-1.2:1.0 (usb)
 add      /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.2/2-1.2:1.0/graphics/fb0 (graphics)
 remove   /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.2/2-1.2:1.0 (usb)
 remove   /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.2 (usb)
 remove   /2-1.2:1.0/graphics/fb0 (graphics)

After:

 add      /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.2 (usb)
 add      /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.2/2-1.2:1.0 (usb)
 add      /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.2/2-1.2:1.0/graphics/fb1 (graphics)
 remove   /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.2/2-1.2:1.0/graphics/fb1 (graphics)
 remove   /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.2/2-1.2:1.0 (usb)
 remove   /devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.2 (usb)

Cc [email protected]

Tested-by Bernie Thompson <[email protected]>

Acked-by Bernie Thompson <[email protected]>

Signed-off-by Kay Sievers <[email protected]>

---
 drivers/video/fbmem.c |   18 +++++++++++++++++-
 drivers/video/udlfb.c |    2 +-
 include/linux/fb.h    |    1 +
 3 files changed, 19 insertions(+), 2 deletions(-)

drivers/video/fbmem.c +17 -1

@@ -1665,6 +1665,7 @@ static int do_unregister_framebuffer(str
 	if (ret)
 		return -EINVAL;
 
+	unlink_framebuffer(fb_info);
 	if (fb_info->pixmap.addr &&
 	    (fb_info->pixmap.flags & FB_PIXMAP_DEFAULT))
 		kfree(fb_info->pixmap.addr);

@@ -1672,7 +1673,6 @@ static int do_unregister_framebuffer(str
 	registered_fb[i] = NULL;
 	num_registered_fb--;
 	fb_cleanup_device(fb_info);
-	device_destroy(fb_class, MKDEV(FB_MAJOR, i));
 	event.info = fb_info;
 	fb_notifier_call_chain(FB_EVENT_FB_UNREGISTERED, &event);
 

@@ -1681,6 +1681,22 @@ static int do_unregister_framebuffer(str
 	return 0;
 }
 
+int unlink_framebuffer(struct fb_info *fb_info)
+{
+	int i;
+
+	i = fb_info->node;
+	if (i < 0 || i >= FB_MAX || registered_fb[i] != fb_info)
+		return -EINVAL;
+
+	if (fb_info->dev) {
+		device_destroy(fb_class, MKDEV(FB_MAJOR, i));
+		fb_info->dev = NULL;
+	}
+	return 0;
+}
+EXPORT_SYMBOL(unlink_framebuffer);
+
 void remove_conflicting_framebuffers(struct apertures_struct *a,
 				     const char *name, bool primary)
 {

drivers/video/udlfb.c +1 -1

@@ -1739,7 +1739,7 @@ static void dlfb_usb_disconnect(struct u
 	for (i = 0; i < ARRAY_SIZE(fb_device_attrs); i++)
 		device_remove_file(info->dev, &fb_device_attrs[i]);
 	device_remove_bin_file(info->dev, &edid_attr);
-
+	unlink_framebuffer(info);
 	usb_set_intfdata(interface, NULL);
 
 	/* if clients still have us open, will be freed on last close */

include/linux/fb.h +1 -0

@@ -1003,6 +1003,7 @@ extern ssize_t fb_sys_write(struct fb_in
 /* drivers/video/fbmem.c */
 extern int register_framebuffer(struct fb_info *fb_info);
 extern int unregister_framebuffer(struct fb_info *fb_info);
+extern int unlink_framebuffer(struct fb_info *fb_info);
 extern void remove_conflicting_framebuffers(struct apertures_struct *a,
 				const char *name, bool primary);
 extern int fb_prepare_logo(struct fb_info *fb_info, int rotate);