Re: [PATCH v7 1/2] drm: Unplug drm device when unregistering it
From: jeffy
Date: Wed Apr 12 2017 - 04:18:15 EST
Hi Daniel,
On 04/12/2017 02:33 PM, Daniel Vetter wrote:
On Tue, Apr 11, 2017 at 11:31:41AM +0800, Jeffy Chen wrote:
After unbinding drm, the user space may still owns the drm dev fd,
and may still be able to call drm ioctl.
We're using an unplugged state to prevent something like that, so
let's reuse it here.
Signed-off-by: Jeffy Chen <jeffy.chen@xxxxxxxxxxxxxx>
Reviewed-by: Sean Paul <seanpaul@xxxxxxxxxxxx>
---
Changes in v7:
Address Sean Paul <seanpaul@xxxxxxxxxxxx>'s comments.
Changes in v6:
Address Daniel Vetter <daniel@xxxxxxxx>'s comments.
Please don't just list the names, but what you've changed. As-is this
isn't very informative ... Also, in drm we prefer the patch changelog
above the --- line, for more credit to the review process.
A few questions below.
-Daniel
Changes in v5:
Fix wrong git account.
Changes in v2:
Fix some commit messages.
drivers/gpu/drm/drm_drv.c | 8 ++++----
drivers/gpu/drm/udl/udl_drv.c | 2 +-
include/drm/drmP.h | 5 +++--
3 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index b5c6bb4..ad13e20 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -357,12 +357,9 @@ EXPORT_SYMBOL(drm_put_dev);
void drm_unplug_dev(struct drm_device *dev)
I think this function can now be unexported and made static?
{
- /* for a USB device */
- drm_dev_unregister(dev);
-
mutex_lock(&drm_global_mutex);
- drm_device_set_unplugged(dev);
+ drm_device_set_plug_state(dev, false);
if (dev->open_count == 0) {
drm_put_dev(dev);
@@ -787,6 +784,8 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
if (drm_core_check_feature(dev, DRIVER_MODESET))
drm_modeset_register_all(dev);
+ drm_device_set_plug_state(dev, true);
+
ret = 0;
DRM_INFO("Initialized %s %d.%d.%d %s for %s on minor %d\n",
@@ -826,6 +825,7 @@ void drm_dev_unregister(struct drm_device *dev)
drm_lastclose(dev);
dev->registered = false;
+ drm_unplug_dev(dev);
if (drm_core_check_feature(dev, DRIVER_MODESET))
drm_modeset_unregister_all(dev);
diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
index cd8b017..5dbd916 100644
--- a/drivers/gpu/drm/udl/udl_drv.c
+++ b/drivers/gpu/drm/udl/udl_drv.c
@@ -108,7 +108,7 @@ static void udl_usb_disconnect(struct usb_interface *interface)
drm_kms_helper_poll_disable(dev);
udl_fbdev_unplug(dev);
udl_drop_usb(dev);
- drm_unplug_dev(dev);
+ drm_dev_unregister(dev);
}
/*
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 3bfafcd..a9a5a64 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -488,10 +488,11 @@ static __inline__ int drm_core_check_feature(struct drm_device *dev,
return ((dev->driver->driver_features & feature) ? 1 : 0);
}
-static inline void drm_device_set_unplugged(struct drm_device *dev)
+static inline void drm_device_set_plug_state(struct drm_device *dev,
+ bool plugged)
Why this change here? I thought the plan was to just unplug _all_ devices
at unregister time? Also, I think we want to make this into a static
function in drm_drv.c
calling drm_unplug_dev when unregistering drm device may cause
hang(drm_unplug_dev will try to put drm dev when open_count is 0).
so i think we only need to update the unplug state at that time?
maybe put drm_device_set_plug_state into drm_drv.c(because no one else
would use it)?
{
smp_wmb();
- atomic_set(&dev->unplugged, 1);
+ atomic_set(&dev->unplugged, !plugged);
}
static inline int drm_device_is_unplugged(struct drm_device *dev)
--
2.1.4
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel