[PATCH] drm/managed: Define drmm_mutex_init() as a macro to fix lockdep

[PATCH] drm/managed: Define drmm_mutex_init() as a macro to fix lockdep

[Boris Brezillon]

drmm_mutex_init() needs to be defined as a macro if we want
lockdep to classify locks properly. If we don't do that, all locks
will be considered as belonging to the same lock class, leading to
false positive deadlock reports.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Reported-by: Sarah Walker <sarah.walker@imgtec.com>
---
 drivers/gpu/drm/drm_managed.c | 26 --------------------------
 include/drm/drm_managed.h     | 30 +++++++++++++++++++++++++++++-
 2 files changed, 29 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/drm_managed.c b/drivers/gpu/drm/drm_managed.c
index 4cf214de50c4..71c49819a7a2 100644
--- a/drivers/gpu/drm/drm_managed.c
+++ b/drivers/gpu/drm/drm_managed.c
@@ -263,29 +263,3 @@ void drmm_kfree(struct drm_device *dev, void *data)
 	free_dr(dr_match);
 }
 EXPORT_SYMBOL(drmm_kfree);
-
-static void drmm_mutex_release(struct drm_device *dev, void *res)
-{
-	struct mutex *lock = res;
-
-	mutex_destroy(lock);
-}
-
-/**
- * drmm_mutex_init - &drm_device-managed mutex_init()
- * @dev: DRM device
- * @lock: lock to be initialized
- *
- * Returns:
- * 0 on success, or a negative errno code otherwise.
- *
- * This is a &drm_device-managed version of mutex_init(). The initialized
- * lock is automatically destroyed on the final drm_dev_put().
- */
-int drmm_mutex_init(struct drm_device *dev, struct mutex *lock)
-{
-	mutex_init(lock);
-
-	return drmm_add_action_or_reset(dev, drmm_mutex_release, lock);
-}
-EXPORT_SYMBOL(drmm_mutex_init);
diff --git a/include/drm/drm_managed.h b/include/drm/drm_managed.h
index 359883942612..87ffb92a16ba 100644
--- a/include/drm/drm_managed.h
+++ b/include/drm/drm_managed.h
@@ -105,6 +105,34 @@ char *drmm_kstrdup(struct drm_device *dev, const char *s, gfp_t gfp);
 
 void drmm_kfree(struct drm_device *dev, void *data);
 
-int drmm_mutex_init(struct drm_device *dev, struct mutex *lock);
+/* Private function, don't use. */
+static inline void __drmm_mutex_release(struct drm_device *dev, void *res)
+{
+	struct mutex *lock = res;
+
+	mutex_destroy(lock);
+}
+
+/**
+ * drmm_mutex_init - &drm_device-managed mutex_init()
+ * @dev: DRM device
+ * @lock: lock to be initialized
+ *
+ * Returns:
+ * 0 on success, or a negative errno code otherwise.
+ *
+ * This is a &drm_device-managed version of mutex_init(). The initialized
+ * lock is automatically destroyed on the final drm_dev_put().
+ *
+ * This needs to be defined as a macro to let lockdep classify locks
+ * properly. If we don't do that, all locks will be considered as
+ * belonging to the same lock class, leading to false positive lockdep
+ * reports.
+ */
+#define drmm_mutex_init(dev, lock) \
+	({\
+		mutex_init(lock); \
+		drmm_add_action_or_reset(dev, __drmm_mutex_release, lock); \
+	})
 
 #endif
-- 
2.40.1

Re: [PATCH] drm/managed: Define drmm_mutex_init() as a macro to fix lockdep

[Matthew Auld]

On Fri, 19 May 2023 at 09:55, Boris Brezillon
<boris.brezillon@collabora.com> wrote:
>
> drmm_mutex_init() needs to be defined as a macro if we want
> lockdep to classify locks properly. If we don't do that, all locks
> will be considered as belonging to the same lock class, leading to
> false positive deadlock reports.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> Reported-by: Sarah Walker <sarah.walker@imgtec.com>

Yeah, we also encountered the same issue. Patch is here:
https://patchwork.freedesktop.org/patch/537605/?series=117891&rev=2

> ---
>  drivers/gpu/drm/drm_managed.c | 26 --------------------------
>  include/drm/drm_managed.h     | 30 +++++++++++++++++++++++++++++-
>  2 files changed, 29 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_managed.c b/drivers/gpu/drm/drm_managed.c
> index 4cf214de50c4..71c49819a7a2 100644
> --- a/drivers/gpu/drm/drm_managed.c
> +++ b/drivers/gpu/drm/drm_managed.c
> @@ -263,29 +263,3 @@ void drmm_kfree(struct drm_device *dev, void *data)
>         free_dr(dr_match);
>  }
>  EXPORT_SYMBOL(drmm_kfree);
> -
> -static void drmm_mutex_release(struct drm_device *dev, void *res)
> -{
> -       struct mutex *lock = res;
> -
> -       mutex_destroy(lock);
> -}
> -
> -/**
> - * drmm_mutex_init - &drm_device-managed mutex_init()
> - * @dev: DRM device
> - * @lock: lock to be initialized
> - *
> - * Returns:
> - * 0 on success, or a negative errno code otherwise.
> - *
> - * This is a &drm_device-managed version of mutex_init(). The initialized
> - * lock is automatically destroyed on the final drm_dev_put().
> - */
> -int drmm_mutex_init(struct drm_device *dev, struct mutex *lock)
> -{
> -       mutex_init(lock);
> -
> -       return drmm_add_action_or_reset(dev, drmm_mutex_release, lock);
> -}
> -EXPORT_SYMBOL(drmm_mutex_init);
> diff --git a/include/drm/drm_managed.h b/include/drm/drm_managed.h
> index 359883942612..87ffb92a16ba 100644
> --- a/include/drm/drm_managed.h
> +++ b/include/drm/drm_managed.h
> @@ -105,6 +105,34 @@ char *drmm_kstrdup(struct drm_device *dev, const char *s, gfp_t gfp);
>
>  void drmm_kfree(struct drm_device *dev, void *data);
>
> -int drmm_mutex_init(struct drm_device *dev, struct mutex *lock);
> +/* Private function, don't use. */
> +static inline void __drmm_mutex_release(struct drm_device *dev, void *res)
> +{
> +       struct mutex *lock = res;
> +
> +       mutex_destroy(lock);
> +}
> +
> +/**
> + * drmm_mutex_init - &drm_device-managed mutex_init()
> + * @dev: DRM device
> + * @lock: lock to be initialized
> + *
> + * Returns:
> + * 0 on success, or a negative errno code otherwise.
> + *
> + * This is a &drm_device-managed version of mutex_init(). The initialized
> + * lock is automatically destroyed on the final drm_dev_put().
> + *
> + * This needs to be defined as a macro to let lockdep classify locks
> + * properly. If we don't do that, all locks will be considered as
> + * belonging to the same lock class, leading to false positive lockdep
> + * reports.
> + */
> +#define drmm_mutex_init(dev, lock) \
> +       ({\
> +               mutex_init(lock); \
> +               drmm_add_action_or_reset(dev, __drmm_mutex_release, lock); \
> +       })
>
>  #endif
> --
> 2.40.1
>

Re: [PATCH] drm/managed: Define drmm_mutex_init() as a macro to fix lockdep

[Boris Brezillon]

On Fri, 19 May 2023 10:05:27 +0100
Matthew Auld <matthew.william.auld@gmail.com> wrote:

> On Fri, 19 May 2023 at 09:55, Boris Brezillon
> <boris.brezillon@collabora.com> wrote:
> >
> > drmm_mutex_init() needs to be defined as a macro if we want
> > lockdep to classify locks properly. If we don't do that, all locks
> > will be considered as belonging to the same lock class, leading to
> > false positive deadlock reports.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > Reported-by: Sarah Walker <sarah.walker@imgtec.com>  
> 
> Yeah, we also encountered the same issue. Patch is here:
> https://patchwork.freedesktop.org/patch/537605/?series=117891&rev=2

Cool! Added my R-b to this patch.

Re: [PATCH] drm/managed: Define drmm_mutex_init() as a macro to fix lockdep

[Stanislaw Gruszka]

Hi

On Fri, May 19, 2023 at 10:55:25AM +0200, Boris Brezillon wrote:
> drmm_mutex_init() needs to be defined as a macro if we want
> lockdep to classify locks properly. If we don't do that, all locks
> will be considered as belonging to the same lock class, leading to
> false positive deadlock reports.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> Reported-by: Sarah Walker <sarah.walker@imgtec.com>

FYI: we have similar patch already posted:
https://lore.kernel.org/dri-devel/20230517152244.348171-1-matthew.auld@intel.com/

Regards
Stanislaw

> ---
>  drivers/gpu/drm/drm_managed.c | 26 --------------------------
>  include/drm/drm_managed.h     | 30 +++++++++++++++++++++++++++++-
>  2 files changed, 29 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_managed.c b/drivers/gpu/drm/drm_managed.c
> index 4cf214de50c4..71c49819a7a2 100644
> --- a/drivers/gpu/drm/drm_managed.c
> +++ b/drivers/gpu/drm/drm_managed.c
> @@ -263,29 +263,3 @@ void drmm_kfree(struct drm_device *dev, void *data)
>  	free_dr(dr_match);
>  }
>  EXPORT_SYMBOL(drmm_kfree);
> -
> -static void drmm_mutex_release(struct drm_device *dev, void *res)
> -{
> -	struct mutex *lock = res;
> -
> -	mutex_destroy(lock);
> -}
> -
> -/**
> - * drmm_mutex_init - &drm_device-managed mutex_init()
> - * @dev: DRM device
> - * @lock: lock to be initialized
> - *
> - * Returns:
> - * 0 on success, or a negative errno code otherwise.
> - *
> - * This is a &drm_device-managed version of mutex_init(). The initialized
> - * lock is automatically destroyed on the final drm_dev_put().
> - */
> -int drmm_mutex_init(struct drm_device *dev, struct mutex *lock)
> -{
> -	mutex_init(lock);
> -
> -	return drmm_add_action_or_reset(dev, drmm_mutex_release, lock);
> -}
> -EXPORT_SYMBOL(drmm_mutex_init);
> diff --git a/include/drm/drm_managed.h b/include/drm/drm_managed.h
> index 359883942612..87ffb92a16ba 100644
> --- a/include/drm/drm_managed.h
> +++ b/include/drm/drm_managed.h
> @@ -105,6 +105,34 @@ char *drmm_kstrdup(struct drm_device *dev, const char *s, gfp_t gfp);
>  
>  void drmm_kfree(struct drm_device *dev, void *data);
>  
> -int drmm_mutex_init(struct drm_device *dev, struct mutex *lock);
> +/* Private function, don't use. */
> +static inline void __drmm_mutex_release(struct drm_device *dev, void *res)
> +{
> +	struct mutex *lock = res;
> +
> +	mutex_destroy(lock);
> +}
> +
> +/**
> + * drmm_mutex_init - &drm_device-managed mutex_init()
> + * @dev: DRM device
> + * @lock: lock to be initialized
> + *
> + * Returns:
> + * 0 on success, or a negative errno code otherwise.
> + *
> + * This is a &drm_device-managed version of mutex_init(). The initialized
> + * lock is automatically destroyed on the final drm_dev_put().
> + *
> + * This needs to be defined as a macro to let lockdep classify locks
> + * properly. If we don't do that, all locks will be considered as
> + * belonging to the same lock class, leading to false positive lockdep
> + * reports.
> + */
> +#define drmm_mutex_init(dev, lock) \
> +	({\
> +		mutex_init(lock); \
> +		drmm_add_action_or_reset(dev, __drmm_mutex_release, lock); \
> +	})
>  
>  #endif
> -- 
> 2.40.1
>