linux-fbdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* udlfb: remove sysfs framebuffer device with USB .disconnect()
@ 2012-01-20 20:18 Kay Sievers
  2012-01-21 13:44 ` Florian Tobias Schandinat
                   ` (5 more replies)
  0 siblings, 6 replies; 7+ messages in thread
From: Kay Sievers @ 2012-01-20 20:18 UTC (permalink / raw)
  To: linux-fbdev

From: Kay Sievers <kay.sievers@vrfy.org>
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: stable@vger.kernel.org
Tested-By: Bernie Thompson <bernie@plugable.com>
Acked-By: Bernie Thompson <bernie@plugable.com>
Signed-off-by: Kay Sievers <kay.sievers@vrfy.org>
---
 drivers/video/fbmem.c |   18 +++++++++++++++++-
 drivers/video/udlfb.c |    2 +-
 include/linux/fb.h    |    1 +
 3 files changed, 19 insertions(+), 2 deletions(-)

--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -1672,7 +1672,7 @@ 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));
+	unlink_framebuffer(fb_info);
 	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)
 {
--- a/drivers/video/udlfb.c
+++ b/drivers/video/udlfb.c
@@ -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 */
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -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);



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: udlfb: remove sysfs framebuffer device with USB .disconnect()
  2012-01-20 20:18 udlfb: remove sysfs framebuffer device with USB .disconnect() Kay Sievers
@ 2012-01-21 13:44 ` Florian Tobias Schandinat
  2012-01-21 13:57 ` Kay Sievers
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Florian Tobias Schandinat @ 2012-01-21 13:44 UTC (permalink / raw)
  To: linux-fbdev

Hi Kay,

On 01/20/2012 08:18 PM, Kay Sievers wrote:
> From: Kay Sievers <kay.sievers@vrfy.org>
> 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: stable@vger.kernel.org
> Tested-By: Bernie Thompson <bernie@plugable.com>
> Acked-By: Bernie Thompson <bernie@plugable.com>
> Signed-off-by: Kay Sievers <kay.sievers@vrfy.org>
> ---
>  drivers/video/fbmem.c |   18 +++++++++++++++++-
>  drivers/video/udlfb.c |    2 +-
>  include/linux/fb.h    |    1 +
>  3 files changed, 19 insertions(+), 2 deletions(-)
> 
> --- a/drivers/video/fbmem.c
> +++ b/drivers/video/fbmem.c
> @@ -1672,7 +1672,7 @@ static int do_unregister_framebuffer(str
>  	registered_fb[i] = NULL;

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

>  	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);
>  
> @@ -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;

...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.

> +
> +	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)
>  {
> --- a/drivers/video/udlfb.c
> +++ b/drivers/video/udlfb.c
> @@ -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 */
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -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);
> 
> 
> 


Best regards,

Florian Tobias Schandinat



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: udlfb: remove sysfs framebuffer device with USB .disconnect()
  2012-01-20 20:18 udlfb: remove sysfs framebuffer device with USB .disconnect() Kay Sievers
  2012-01-21 13:44 ` Florian Tobias Schandinat
@ 2012-01-21 13:57 ` Kay Sievers
  2012-01-21 14:13 ` Florian Tobias Schandinat
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Kay Sievers @ 2012-01-21 13:57 UTC (permalink / raw)
  To: linux-fbdev

On Sat, Jan 21, 2012 at 14:44, Florian Tobias Schandinat
<FlorianSchandinat@gmx.de> wrote:
> On 01/20/2012 08:18 PM, Kay Sievers wrote:

>> +++ b/drivers/video/fbmem.c
>> @@ -1672,7 +1672,7 @@ static int do_unregister_framebuffer(str
>>       registered_fb[i] = NULL;
>
> Here registered_fb[fb_info->node] is set to NULL...

>> +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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: udlfb: remove sysfs framebuffer device with USB .disconnect()
  2012-01-20 20:18 udlfb: remove sysfs framebuffer device with USB .disconnect() Kay Sievers
  2012-01-21 13:44 ` Florian Tobias Schandinat
  2012-01-21 13:57 ` Kay Sievers
@ 2012-01-21 14:13 ` Florian Tobias Schandinat
  2012-01-21 14:32 ` Kay Sievers
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Florian Tobias Schandinat @ 2012-01-21 14:13 UTC (permalink / raw)
  To: linux-fbdev

On 01/21/2012 01:57 PM, Kay Sievers wrote:
> On Sat, Jan 21, 2012 at 14:44, Florian Tobias Schandinat
> <FlorianSchandinat@gmx.de> wrote:
>> On 01/20/2012 08:18 PM, Kay Sievers wrote:
> 
>>> +++ b/drivers/video/fbmem.c
>>> @@ -1672,7 +1672,7 @@ static int do_unregister_framebuffer(str
>>>       registered_fb[i] = NULL;
>>
>> Here registered_fb[fb_info->node] is set to NULL...
> 
>>> +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.

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?


Best regards,

Florian Tobias Schandinat

> 
> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: udlfb: remove sysfs framebuffer device with USB .disconnect()
  2012-01-20 20:18 udlfb: remove sysfs framebuffer device with USB .disconnect() Kay Sievers
                   ` (2 preceding siblings ...)
  2012-01-21 14:13 ` Florian Tobias Schandinat
@ 2012-01-21 14:32 ` Kay Sievers
  2012-01-27 16:32 ` Kay Sievers
  2012-01-30  5:16 ` Florian Tobias Schandinat
  5 siblings, 0 replies; 7+ messages in thread
From: Kay Sievers @ 2012-01-21 14:32 UTC (permalink / raw)
  To: linux-fbdev

On Sat, 2012-01-21 at 14:13 +0000, Florian Tobias Schandinat wrote:
> On 01/21/2012 01:57 PM, Kay Sievers wrote:

> > 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?

Yes, I absolutely agree. :)

Thanks,
Kay


From: Kay Sievers <kay.sievers@vrfy.org>
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: stable@vger.kernel.org
Tested-By: Bernie Thompson <bernie@plugable.com>
Acked-By: Bernie Thompson <bernie@plugable.com>
Signed-off-by: Kay Sievers <kay.sievers@vrfy.org>
---
 drivers/video/fbmem.c |   18 +++++++++++++++++-
 drivers/video/udlfb.c |    2 +-
 include/linux/fb.h    |    1 +
 3 files changed, 19 insertions(+), 2 deletions(-)

--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -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)
 {
--- a/drivers/video/udlfb.c
+++ b/drivers/video/udlfb.c
@@ -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 */
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -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);



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: udlfb: remove sysfs framebuffer device with USB .disconnect()
  2012-01-20 20:18 udlfb: remove sysfs framebuffer device with USB .disconnect() Kay Sievers
                   ` (3 preceding siblings ...)
  2012-01-21 14:32 ` Kay Sievers
@ 2012-01-27 16:32 ` Kay Sievers
  2012-01-30  5:16 ` Florian Tobias Schandinat
  5 siblings, 0 replies; 7+ messages in thread
From: Kay Sievers @ 2012-01-27 16:32 UTC (permalink / raw)
  To: linux-fbdev

On Sat, Jan 21, 2012 at 15:32, Kay Sievers <kay.sievers@vrfy.org> wrote:
> On Sat, 2012-01-21 at 14:13 +0000, Florian Tobias Schandinat wrote:
>> On 01/21/2012 01:57 PM, Kay Sievers wrote:
>
>> > 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?
>
> Yes, I absolutely agree. :)

Hey Florian,

any updates?

We like to get that into the Fedora kernel, as we promised
out-of-the-box multi-seat hotplug support, but the framebuffer events
are broken at the moment.

It would be nice if it would show up in -next somehow, and gets
testing, so I don't need to convince anybody at the Fedora kernel
side, that this will land upstream ...

Thanks,
Kay

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: udlfb: remove sysfs framebuffer device with USB .disconnect()
  2012-01-20 20:18 udlfb: remove sysfs framebuffer device with USB .disconnect() Kay Sievers
                   ` (4 preceding siblings ...)
  2012-01-27 16:32 ` Kay Sievers
@ 2012-01-30  5:16 ` Florian Tobias Schandinat
  5 siblings, 0 replies; 7+ messages in thread
From: Florian Tobias Schandinat @ 2012-01-30  5:16 UTC (permalink / raw)
  To: linux-fbdev

On 01/21/2012 02:32 PM, Kay Sievers wrote:
[...]
> From: Kay Sievers <kay.sievers@vrfy.org>
> 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: stable@vger.kernel.org
> Tested-By: Bernie Thompson <bernie@plugable.com>
> Acked-By: Bernie Thompson <bernie@plugable.com>
> Signed-off-by: Kay Sievers <kay.sievers@vrfy.org>

Applied.


Thanks,

Florian Tobias Schandinat

> ---
>  drivers/video/fbmem.c |   18 +++++++++++++++++-
>  drivers/video/udlfb.c |    2 +-
>  include/linux/fb.h    |    1 +
>  3 files changed, 19 insertions(+), 2 deletions(-)
> 
> --- a/drivers/video/fbmem.c
> +++ b/drivers/video/fbmem.c
> @@ -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)
>  {
> --- a/drivers/video/udlfb.c
> +++ b/drivers/video/udlfb.c
> @@ -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 */
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -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);
> 
> 
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2012-01-30  5:16 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-01-20 20:18 udlfb: remove sysfs framebuffer device with USB .disconnect() Kay Sievers
2012-01-21 13:44 ` Florian Tobias Schandinat
2012-01-21 13:57 ` Kay Sievers
2012-01-21 14:13 ` Florian Tobias Schandinat
2012-01-21 14:32 ` Kay Sievers
2012-01-27 16:32 ` Kay Sievers
2012-01-30  5:16 ` Florian Tobias Schandinat

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).