Re: [Intel-gfx] [PATCH v4 1/2] drm: Use srcu to protect drm_device.unplugged
On Wed, Mar 28, 2018 at 09:47:40AM +0300, Oleksandr Andrushchenko wrote:
> From: Noralf Trønnes
>
> Use srcu to protect drm_device.unplugged in a race free manner.
> Drivers can use drm_dev_enter()/drm_dev_exit() to protect and mark
> sections preventing access to device resources that are not available
> after the device is gone.
>
> Suggested-by: Daniel Vetter
> Signed-off-by: Noralf Trønnes
> Reviewed-by: Oleksandr Andrushchenko
> Tested-by: Oleksandr Andrushchenko
> Cc: intel-...@lists.freedesktop.org
When you apply/forward a patch we also need your s-o-b line, even if you
changed nothing. sob needs to reflect the full record of everyone who
handled a patch from author to when it finally lands in git.
-Daniel
> ---
> drivers/gpu/drm/drm_drv.c | 54
> ++-
> include/drm/drm_device.h | 9 +++-
> include/drm/drm_drv.h | 15 +
> 3 files changed, 68 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index a1b9338736e3..32a83b41ab61 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -32,6 +32,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -75,6 +76,8 @@ static bool drm_core_init_complete = false;
>
> static struct dentry *drm_debugfs_root;
>
> +DEFINE_STATIC_SRCU(drm_unplug_srcu);
> +
> /*
> * DRM Minors
> * A DRM device can provide several char-dev interfaces on the DRM-Major.
> Each
> @@ -318,18 +321,51 @@ void drm_put_dev(struct drm_device *dev)
> }
> EXPORT_SYMBOL(drm_put_dev);
>
> -static void drm_device_set_unplugged(struct drm_device *dev)
> +/**
> + * drm_dev_enter - Enter device critical section
> + * @dev: DRM device
> + * @idx: Pointer to index that will be passed to the matching drm_dev_exit()
> + *
> + * This function marks and protects the beginning of a section that should
> not
> + * be entered after the device has been unplugged. The section end is marked
> + * with drm_dev_exit(). Calls to this function can be nested.
> + *
> + * Returns:
> + * True if it is OK to enter the section, false otherwise.
> + */
> +bool drm_dev_enter(struct drm_device *dev, int *idx)
> +{
> + *idx = srcu_read_lock(&drm_unplug_srcu);
> +
> + if (dev->unplugged) {
> + srcu_read_unlock(&drm_unplug_srcu, *idx);
> + return false;
> + }
> +
> + return true;
> +}
> +EXPORT_SYMBOL(drm_dev_enter);
> +
> +/**
> + * drm_dev_exit - Exit device critical section
> + * @idx: index returned from drm_dev_enter()
> + *
> + * This function marks the end of a section that should not be entered after
> + * the device has been unplugged.
> + */
> +void drm_dev_exit(int idx)
> {
> - smp_wmb();
> - atomic_set(&dev->unplugged, 1);
> + srcu_read_unlock(&drm_unplug_srcu, idx);
> }
> +EXPORT_SYMBOL(drm_dev_exit);
>
> /**
> * drm_dev_unplug - unplug a DRM device
> * @dev: DRM device
> *
> * This unplugs a hotpluggable DRM device, which makes it inaccessible to
> - * userspace operations. Entry-points can use drm_dev_is_unplugged(). This
> + * userspace operations. Entry-points can use drm_dev_enter() and
> + * drm_dev_exit() to protect device resources in a race free manner. This
> * essentially unregisters the device like drm_dev_unregister(), but can be
> * called while there are still open users of @dev.
> */
> @@ -338,10 +374,18 @@ void drm_dev_unplug(struct drm_device *dev)
> drm_dev_unregister(dev);
>
> mutex_lock(&drm_global_mutex);
> - drm_device_set_unplugged(dev);
> if (dev->open_count == 0)
> drm_dev_put(dev);
> mutex_unlock(&drm_global_mutex);
> +
> + /*
> + * After synchronizing any critical read section is guaranteed to see
> + * the new value of ->unplugged, and any critical section which might
> + * still have seen the old value of ->unplugged is guaranteed to have
> + * finished.
> + */
> + dev->unplugged = true;
> + synchronize_srcu(&drm_unplug_srcu);
> }
> EXPORT_SYMBOL(drm_dev_unplug);
>
> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
> index 7c4fa32f3fc6..3a0eac2885b7 100644
> --- a/include/drm/drm_device.h
> +++ b/include/drm/drm_device.h
> @@ -46,7 +46,14 @@ struct drm_device {
> /* currently active master for this device. Protected by master_mutex */
> struct drm_master *master;
>
> - atomic_t unplugged; /**< Flag whether dev is dead */
> + /**
> + * @unplugged:
> + *
> + * Flag to tell if the device has been unplugged.
> + * See drm_dev_enter() and drm_dev_is_unplugged().
> + */
> + bool unplugged;
> +
> struct inode *anon_inode; /**< inode for private
> address-space */
> char *unique; /**< unique name of the device
> */
> /*@} */
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index d2
[PATCH v4 1/2] drm: Use srcu to protect drm_device.unplugged
From: Noralf Trønnes
Use srcu to protect drm_device.unplugged in a race free manner.
Drivers can use drm_dev_enter()/drm_dev_exit() to protect and mark
sections preventing access to device resources that are not available
after the device is gone.
Suggested-by: Daniel Vetter
Signed-off-by: Noralf Trønnes
Reviewed-by: Oleksandr Andrushchenko
Tested-by: Oleksandr Andrushchenko
Cc: intel-...@lists.freedesktop.org
---
drivers/gpu/drm/drm_drv.c | 54 ++-
include/drm/drm_device.h | 9 +++-
include/drm/drm_drv.h | 15 +
3 files changed, 68 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index a1b9338736e3..32a83b41ab61 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -32,6 +32,7 @@
#include
#include
#include
+#include
#include
#include
@@ -75,6 +76,8 @@ static bool drm_core_init_complete = false;
static struct dentry *drm_debugfs_root;
+DEFINE_STATIC_SRCU(drm_unplug_srcu);
+
/*
* DRM Minors
* A DRM device can provide several char-dev interfaces on the DRM-Major. Each
@@ -318,18 +321,51 @@ void drm_put_dev(struct drm_device *dev)
}
EXPORT_SYMBOL(drm_put_dev);
-static void drm_device_set_unplugged(struct drm_device *dev)
+/**
+ * drm_dev_enter - Enter device critical section
+ * @dev: DRM device
+ * @idx: Pointer to index that will be passed to the matching drm_dev_exit()
+ *
+ * This function marks and protects the beginning of a section that should not
+ * be entered after the device has been unplugged. The section end is marked
+ * with drm_dev_exit(). Calls to this function can be nested.
+ *
+ * Returns:
+ * True if it is OK to enter the section, false otherwise.
+ */
+bool drm_dev_enter(struct drm_device *dev, int *idx)
+{
+ *idx = srcu_read_lock(&drm_unplug_srcu);
+
+ if (dev->unplugged) {
+ srcu_read_unlock(&drm_unplug_srcu, *idx);
+ return false;
+ }
+
+ return true;
+}
+EXPORT_SYMBOL(drm_dev_enter);
+
+/**
+ * drm_dev_exit - Exit device critical section
+ * @idx: index returned from drm_dev_enter()
+ *
+ * This function marks the end of a section that should not be entered after
+ * the device has been unplugged.
+ */
+void drm_dev_exit(int idx)
{
- smp_wmb();
- atomic_set(&dev->unplugged, 1);
+ srcu_read_unlock(&drm_unplug_srcu, idx);
}
+EXPORT_SYMBOL(drm_dev_exit);
/**
* drm_dev_unplug - unplug a DRM device
* @dev: DRM device
*
* This unplugs a hotpluggable DRM device, which makes it inaccessible to
- * userspace operations. Entry-points can use drm_dev_is_unplugged(). This
+ * userspace operations. Entry-points can use drm_dev_enter() and
+ * drm_dev_exit() to protect device resources in a race free manner. This
* essentially unregisters the device like drm_dev_unregister(), but can be
* called while there are still open users of @dev.
*/
@@ -338,10 +374,18 @@ void drm_dev_unplug(struct drm_device *dev)
drm_dev_unregister(dev);
mutex_lock(&drm_global_mutex);
- drm_device_set_unplugged(dev);
if (dev->open_count == 0)
drm_dev_put(dev);
mutex_unlock(&drm_global_mutex);
+
+ /*
+* After synchronizing any critical read section is guaranteed to see
+* the new value of ->unplugged, and any critical section which might
+* still have seen the old value of ->unplugged is guaranteed to have
+* finished.
+*/
+ dev->unplugged = true;
+ synchronize_srcu(&drm_unplug_srcu);
}
EXPORT_SYMBOL(drm_dev_unplug);
diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
index 7c4fa32f3fc6..3a0eac2885b7 100644
--- a/include/drm/drm_device.h
+++ b/include/drm/drm_device.h
@@ -46,7 +46,14 @@ struct drm_device {
/* currently active master for this device. Protected by master_mutex */
struct drm_master *master;
- atomic_t unplugged; /**< Flag whether dev is dead */
+ /**
+* @unplugged:
+*
+* Flag to tell if the device has been unplugged.
+* See drm_dev_enter() and drm_dev_is_unplugged().
+*/
+ bool unplugged;
+
struct inode *anon_inode; /**< inode for private
address-space */
char *unique; /**< unique name of the device
*/
/*@} */
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index d23dcdd1bd95..7e545f5f94d3 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -624,6 +624,8 @@ void drm_dev_get(struct drm_device *dev);
void drm_dev_put(struct drm_device *dev);
void drm_dev_unref(struct drm_device *dev);
void drm_put_dev(struct drm_device *dev);
+bool drm_dev_enter(struct drm_device *dev, int *idx);
+void drm_dev_exit(int idx);
void drm_dev_unplug(struct drm_device *dev);
/**
@@ -635,11 +637,16 @@ void drm_dev_unplug(struct drm_device *dev);
* unplugged, t
