[2/2] drm/udl: add a release method and delay modeset teardown

Submitted by Dave Airlie on March 15, 2019, 5:13 a.m.

Details

Message ID 20190315051330.18871-3-airlied@gmail.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in DRI devel

Not browsing as part of any series.

Commit Message

Dave Airlie March 15, 2019, 5:13 a.m.
From: Dave Airlie <airlied@redhat.com>

If we unplug a udl device, the usb callback with deinit the
mode_config struct, however userspace will still have an open
file descriptor and a framebuffer on that device. When userspace
closes the fd, we'll oops because it'll try and look stuff up
in the object idr which we've destroyed.

This punts destroying the mode objects until release time instead.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/udl/udl_drv.c  | 1 +
 drivers/gpu/drm/udl/udl_drv.h  | 1 +
 drivers/gpu/drm/udl/udl_main.c | 8 +++++++-
 3 files changed, 9 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
index 22cd2d13e272..ff47f890e6ad 100644
--- a/drivers/gpu/drm/udl/udl_drv.c
+++ b/drivers/gpu/drm/udl/udl_drv.c
@@ -52,6 +52,7 @@  static struct drm_driver driver = {
 	.driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME,
 	.load = udl_driver_load,
 	.unload = udl_driver_unload,
+	.release = udl_driver_release,
 
 	/* gem hooks */
 	.gem_free_object_unlocked = udl_gem_free_object,
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index e9e9b1ff678e..4ae67d882eae 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -104,6 +104,7 @@  void udl_urb_completion(struct urb *urb);
 
 int udl_driver_load(struct drm_device *dev, unsigned long flags);
 void udl_driver_unload(struct drm_device *dev);
+void udl_driver_release(struct drm_device *dev);
 
 int udl_fbdev_init(struct drm_device *dev);
 void udl_fbdev_cleanup(struct drm_device *dev);
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index 9086d0d1b880..5626e1f11852 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -379,6 +379,12 @@  void udl_driver_unload(struct drm_device *dev)
 		udl_free_urb_list(dev);
 
 	udl_fbdev_cleanup(dev);
-	udl_modeset_cleanup(dev);
 	kfree(udl);
 }
+
+void udl_driver_release(struct drm_device *dev)
+{
+	drm_mode_config_cleanup(dev);
+	drm_dev_fini(dev);
+	kfree(dev);
+}

Comments

On Fri, Mar 15, 2019 at 03:13:30PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> If we unplug a udl device, the usb callback with deinit the
> mode_config struct, however userspace will still have an open
> file descriptor and a framebuffer on that device. When userspace
> closes the fd, we'll oops because it'll try and look stuff up
> in the object idr which we've destroyed.
> 
> This punts destroying the mode objects until release time instead.
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

For the first one pls ping Noralf to make sure it all aligns with the
Grand Plan.
-Daniel

> ---
>  drivers/gpu/drm/udl/udl_drv.c  | 1 +
>  drivers/gpu/drm/udl/udl_drv.h  | 1 +
>  drivers/gpu/drm/udl/udl_main.c | 8 +++++++-
>  3 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
> index 22cd2d13e272..ff47f890e6ad 100644
> --- a/drivers/gpu/drm/udl/udl_drv.c
> +++ b/drivers/gpu/drm/udl/udl_drv.c
> @@ -52,6 +52,7 @@ static struct drm_driver driver = {
>  	.driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME,
>  	.load = udl_driver_load,
>  	.unload = udl_driver_unload,
> +	.release = udl_driver_release,
>  
>  	/* gem hooks */
>  	.gem_free_object_unlocked = udl_gem_free_object,
> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
> index e9e9b1ff678e..4ae67d882eae 100644
> --- a/drivers/gpu/drm/udl/udl_drv.h
> +++ b/drivers/gpu/drm/udl/udl_drv.h
> @@ -104,6 +104,7 @@ void udl_urb_completion(struct urb *urb);
>  
>  int udl_driver_load(struct drm_device *dev, unsigned long flags);
>  void udl_driver_unload(struct drm_device *dev);
> +void udl_driver_release(struct drm_device *dev);
>  
>  int udl_fbdev_init(struct drm_device *dev);
>  void udl_fbdev_cleanup(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
> index 9086d0d1b880..5626e1f11852 100644
> --- a/drivers/gpu/drm/udl/udl_main.c
> +++ b/drivers/gpu/drm/udl/udl_main.c
> @@ -379,6 +379,12 @@ void udl_driver_unload(struct drm_device *dev)
>  		udl_free_urb_list(dev);
>  
>  	udl_fbdev_cleanup(dev);
> -	udl_modeset_cleanup(dev);
>  	kfree(udl);
>  }
> +
> +void udl_driver_release(struct drm_device *dev)
> +{
> +	drm_mode_config_cleanup(dev);
> +	drm_dev_fini(dev);
> +	kfree(dev);
> +}
> -- 
> 2.20.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Den 15.03.2019 06.13, skrev Dave Airlie:
> From: Dave Airlie <airlied@redhat.com>
> 
> If we unplug a udl device, the usb callback with deinit the
> mode_config struct, however userspace will still have an open
> file descriptor and a framebuffer on that device. When userspace
> closes the fd, we'll oops because it'll try and look stuff up
> in the object idr which we've destroyed.
> 
> This punts destroying the mode objects until release time instead.
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
>  drivers/gpu/drm/udl/udl_drv.c  | 1 +
>  drivers/gpu/drm/udl/udl_drv.h  | 1 +
>  drivers/gpu/drm/udl/udl_main.c | 8 +++++++-
>  3 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
> index 22cd2d13e272..ff47f890e6ad 100644
> --- a/drivers/gpu/drm/udl/udl_drv.c
> +++ b/drivers/gpu/drm/udl/udl_drv.c
> @@ -52,6 +52,7 @@ static struct drm_driver driver = {
>  	.driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME,
>  	.load = udl_driver_load,
>  	.unload = udl_driver_unload,
> +	.release = udl_driver_release,
>  
>  	/* gem hooks */
>  	.gem_free_object_unlocked = udl_gem_free_object,
> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
> index e9e9b1ff678e..4ae67d882eae 100644
> --- a/drivers/gpu/drm/udl/udl_drv.h
> +++ b/drivers/gpu/drm/udl/udl_drv.h
> @@ -104,6 +104,7 @@ void udl_urb_completion(struct urb *urb);
>  
>  int udl_driver_load(struct drm_device *dev, unsigned long flags);
>  void udl_driver_unload(struct drm_device *dev);
> +void udl_driver_release(struct drm_device *dev);
>  
>  int udl_fbdev_init(struct drm_device *dev);
>  void udl_fbdev_cleanup(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
> index 9086d0d1b880..5626e1f11852 100644
> --- a/drivers/gpu/drm/udl/udl_main.c
> +++ b/drivers/gpu/drm/udl/udl_main.c
> @@ -379,6 +379,12 @@ void udl_driver_unload(struct drm_device *dev)
>  		udl_free_urb_list(dev);
>  
>  	udl_fbdev_cleanup(dev);
> -	udl_modeset_cleanup(dev);
>  	kfree(udl);
>  }
> +
> +void udl_driver_release(struct drm_device *dev)
> +{
> +	drm_mode_config_cleanup(dev);
> +	drm_dev_fini(dev);
> +	kfree(dev);
> +}
> 

You can remove udl_modeset_cleanup() now.

There's still a problem here and that is if udl_driver_load() errors out
before drm_mode_config_init() is called, drm_mode_config_cleanup() will
crash on uninitialized lists etc.

I have solved this by calling drm_mode_config_init() right after
drm_device has been initialized.

Noralf.