* udl unplug fixes @ 2019-03-15 5:13 Dave Airlie 2019-03-15 5:13 ` [PATCH 1/2] drm: do lower level device put on unplugged releases Dave Airlie 2019-03-15 5:13 ` [PATCH 2/2] drm/udl: add a release method and delay modeset teardown Dave Airlie 0 siblings, 2 replies; 6+ messages in thread From: Dave Airlie @ 2019-03-15 5:13 UTC (permalink / raw) To: dri-devel I've sent two udl fixes for plugging in, these two fix the unplug scenario, The first avoids hitting the unregister path twice, once in device unplug and once in last open fd release. The second avoids uninit the modeset configs while userspace still has the open fd which can access them. Dave. _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] drm: do lower level device put on unplugged releases 2019-03-15 5:13 udl unplug fixes Dave Airlie @ 2019-03-15 5:13 ` Dave Airlie 2019-03-15 14:24 ` Noralf Trønnes 2019-03-15 5:13 ` [PATCH 2/2] drm/udl: add a release method and delay modeset teardown Dave Airlie 1 sibling, 1 reply; 6+ messages in thread From: Dave Airlie @ 2019-03-15 5:13 UTC (permalink / raw) To: dri-devel From: Dave Airlie <airlied@redhat.com> When we release the file handle on a device that has been unplugged it has already called the unregister path, which doesn't like being called again. We should just do the dev put version instead. This fixes some crashes unplugged in a udl device. Signed-off-by: Dave Airlie <airlied@redhat.com> --- drivers/gpu/drm/drm_file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c index 83a5bbca6e7e..900fe8228745 100644 --- a/drivers/gpu/drm/drm_file.c +++ b/drivers/gpu/drm/drm_file.c @@ -492,7 +492,7 @@ int drm_release(struct inode *inode, struct file *filp) if (!--dev->open_count) { drm_lastclose(dev); if (drm_dev_is_unplugged(dev)) - drm_put_dev(dev); + drm_dev_put(dev); } mutex_unlock(&drm_global_mutex); -- 2.20.1 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] drm: do lower level device put on unplugged releases 2019-03-15 5:13 ` [PATCH 1/2] drm: do lower level device put on unplugged releases Dave Airlie @ 2019-03-15 14:24 ` Noralf Trønnes 0 siblings, 0 replies; 6+ messages in thread From: Noralf Trønnes @ 2019-03-15 14:24 UTC (permalink / raw) To: Dave Airlie, dri-devel Den 15.03.2019 06.13, skrev Dave Airlie: > From: Dave Airlie <airlied@redhat.com> > > When we release the file handle on a device that has been unplugged > it has already called the unregister path, which doesn't like being > called again. We should just do the dev put version instead. > > This fixes some crashes unplugged in a udl device. > > Signed-off-by: Dave Airlie <airlied@redhat.com> > --- I believe this is fixed already in drm-misc-next: drm: Fix drm_release() and device unplug https://cgit.freedesktop.org/drm/drm-misc/commit/?id=1ee57d4d75fbc74bb2ae601c8f334219165ef276 Noralf. > drivers/gpu/drm/drm_file.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c > index 83a5bbca6e7e..900fe8228745 100644 > --- a/drivers/gpu/drm/drm_file.c > +++ b/drivers/gpu/drm/drm_file.c > @@ -492,7 +492,7 @@ int drm_release(struct inode *inode, struct file *filp) > if (!--dev->open_count) { > drm_lastclose(dev); > if (drm_dev_is_unplugged(dev)) > - drm_put_dev(dev); > + drm_dev_put(dev); > } > mutex_unlock(&drm_global_mutex); > > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] drm/udl: add a release method and delay modeset teardown 2019-03-15 5:13 udl unplug fixes Dave Airlie 2019-03-15 5:13 ` [PATCH 1/2] drm: do lower level device put on unplugged releases Dave Airlie @ 2019-03-15 5:13 ` Dave Airlie 2019-03-15 11:02 ` Daniel Vetter 2019-03-15 14:34 ` Noralf Trønnes 1 sibling, 2 replies; 6+ messages in thread From: Dave Airlie @ 2019-03-15 5:13 UTC (permalink / raw) To: dri-devel 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); +} -- 2.20.1 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] drm/udl: add a release method and delay modeset teardown 2019-03-15 5:13 ` [PATCH 2/2] drm/udl: add a release method and delay modeset teardown Dave Airlie @ 2019-03-15 11:02 ` Daniel Vetter 2019-03-15 14:34 ` Noralf Trønnes 1 sibling, 0 replies; 6+ messages in thread From: Daniel Vetter @ 2019-03-15 11:02 UTC (permalink / raw) To: Dave Airlie; +Cc: dri-devel 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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] drm/udl: add a release method and delay modeset teardown 2019-03-15 5:13 ` [PATCH 2/2] drm/udl: add a release method and delay modeset teardown Dave Airlie 2019-03-15 11:02 ` Daniel Vetter @ 2019-03-15 14:34 ` Noralf Trønnes 1 sibling, 0 replies; 6+ messages in thread From: Noralf Trønnes @ 2019-03-15 14:34 UTC (permalink / raw) To: Dave Airlie, 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. _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-03-15 14:35 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-03-15 5:13 udl unplug fixes Dave Airlie 2019-03-15 5:13 ` [PATCH 1/2] drm: do lower level device put on unplugged releases Dave Airlie 2019-03-15 14:24 ` Noralf Trønnes 2019-03-15 5:13 ` [PATCH 2/2] drm/udl: add a release method and delay modeset teardown Dave Airlie 2019-03-15 11:02 ` Daniel Vetter 2019-03-15 14:34 ` Noralf Trønnes
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).