Using of devm API leads to certain order of releasing resources.
So all dependent resources which are not devm-wrapped should be deleted
with respect to devm-release order. Mutex is one of such objects that
often is bound to other resources and has no own devm wrapping.
Since mutex_destroy() actually does nothing in non-debug builds
frequently calling mutex_destroy() is just ignored which is safe for now
but wrong formally and can lead to a problem if mutex_destroy() will be
extended so introduce devm_mutex_init() and devm_mutex_destroy().
Signed-off-by: George Stark <[email protected]>
---
include/linux/mutex.h | 3 +++
kernel/locking/mutex.c | 37 +++++++++++++++++++++++++++++++++++++
2 files changed, 40 insertions(+)
diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index a33aa9eb9fc3..7f60cd842322 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -119,6 +119,9 @@ do { \
extern void __mutex_init(struct mutex *lock, const char *name,
struct lock_class_key *key);
+int devm_mutex_init(struct device *dev, struct mutex *lock);
+void devm_mutex_destroy(struct device *dev, struct mutex *lock);
+
/**
* mutex_is_locked - is the mutex locked
* @lock: the mutex to be queried
diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
index d973fe6041bf..a73124719dcb 100644
--- a/kernel/locking/mutex.c
+++ b/kernel/locking/mutex.c
@@ -56,6 +56,43 @@ __mutex_init(struct mutex *lock, const char *name, struct lock_class_key *key)
}
EXPORT_SYMBOL(__mutex_init);
+static void devm_mutex_release(struct device *dev, void *res)
+{
+ mutex_destroy(*(struct mutex **)res);
+}
+
+static int devm_mutex_match(struct device *dev, void *res, void *data)
+{
+ struct mutex **r = res;
+
+ if (WARN_ON(!r || !*r))
+ return 0;
+
+ return *r == data;
+}
+
+int devm_mutex_init(struct device *dev, struct mutex *lock)
+{
+ struct mutex **ptr;
+
+ ptr = devres_alloc(devm_mutex_release, sizeof(*ptr), GFP_KERNEL);
+ if (!ptr)
+ return -ENOMEM;
+
+ mutex_init(lock);
+
+ *ptr = lock;
+ devres_add(dev, ptr);
+ return 0;
+}
+EXPORT_SYMBOL(devm_mutex_init);
+
+void devm_mutex_destroy(struct device *dev, struct mutex *lock)
+{
+ devres_release(dev, devm_mutex_release, devm_mutex_match, lock);
+}
+EXPORT_SYMBOL(devm_mutex_destroy);
+
/*
* @owner: contains: 'struct task_struct *' to the current lock owner,
* NULL means not owned. Since task_struct pointers are aligned at
--
2.38.4
On 11/27/23 19:09, George Stark wrote:
> Using of devm API leads to certain order of releasing resources.
> So all dependent resources which are not devm-wrapped should be deleted
> with respect to devm-release order. Mutex is one of such objects that
> often is bound to other resources and has no own devm wrapping.
> Since mutex_destroy() actually does nothing in non-debug builds
> frequently calling mutex_destroy() is just ignored which is safe for now
> but wrong formally and can lead to a problem if mutex_destroy() will be
> extended so introduce devm_mutex_init() and devm_mutex_destroy().
>
> Signed-off-by: George Stark <[email protected]>
> ---
> include/linux/mutex.h | 3 +++
> kernel/locking/mutex.c | 37 +++++++++++++++++++++++++++++++++++++
> 2 files changed, 40 insertions(+)
>
> diff --git a/include/linux/mutex.h b/include/linux/mutex.h
> index a33aa9eb9fc3..7f60cd842322 100644
> --- a/include/linux/mutex.h
> +++ b/include/linux/mutex.h
> @@ -119,6 +119,9 @@ do { \
> extern void __mutex_init(struct mutex *lock, const char *name,
> struct lock_class_key *key);
>
> +int devm_mutex_init(struct device *dev, struct mutex *lock);
> +void devm_mutex_destroy(struct device *dev, struct mutex *lock);
> +
> /**
> * mutex_is_locked - is the mutex locked
> * @lock: the mutex to be queried
> diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
> index d973fe6041bf..a73124719dcb 100644
> --- a/kernel/locking/mutex.c
> +++ b/kernel/locking/mutex.c
> @@ -56,6 +56,43 @@ __mutex_init(struct mutex *lock, const char *name, struct lock_class_key *key)
> }
> EXPORT_SYMBOL(__mutex_init);
>
> +static void devm_mutex_release(struct device *dev, void *res)
> +{
> + mutex_destroy(*(struct mutex **)res);
> +}
> +
> +static int devm_mutex_match(struct device *dev, void *res, void *data)
> +{
> + struct mutex **r = res;
> +
> + if (WARN_ON(!r || !*r))
> + return 0;
> +
> + return *r == data;
> +}
> +
> +int devm_mutex_init(struct device *dev, struct mutex *lock)
> +{
> + struct mutex **ptr;
> +
> + ptr = devres_alloc(devm_mutex_release, sizeof(*ptr), GFP_KERNEL);
> + if (!ptr)
> + return -ENOMEM;
> +
> + mutex_init(lock);
> +
> + *ptr = lock;
> + devres_add(dev, ptr);
> + return 0;
> +}
> +EXPORT_SYMBOL(devm_mutex_init);
> +
> +void devm_mutex_destroy(struct device *dev, struct mutex *lock)
> +{
> + devres_release(dev, devm_mutex_release, devm_mutex_match, lock);
> +}
> +EXPORT_SYMBOL(devm_mutex_destroy);
> +
> /*
> * @owner: contains: 'struct task_struct *' to the current lock owner,
> * NULL means not owned. Since task_struct pointers are aligned at
These APIs are specific to devres. I don't believe it is suitable to put
them into the generic mutex.h header file. All devres_* functions are
defined in include/linux/device.h which is probabably not included in
mutex.h. You may consider putting these APIs into device.h instead.
Cheers,
Longman
On Mon, Nov 27, 2023 at 08:55:18PM -0500, Waiman Long wrote:
> On 11/27/23 19:09, George Stark wrote:
...
> These APIs are specific to devres. I don't believe it is suitable to put
> them into the generic mutex.h header file. All devres_* functions are
> defined in include/linux/device.h which is probabably not included in
> mutex.h. You may consider putting these APIs into device.h instead.
+1. We have include/linux/devm-helpers.h.
--
With Best Regards,
Andy Shevchenko
On Tue, Nov 28, 2023 at 03:09:10AM +0300, George Stark wrote:
> Using of devm API leads to certain order of releasing resources.
> So all dependent resources which are not devm-wrapped should be deleted
> with respect to devm-release order. Mutex is one of such objects that
> often is bound to other resources and has no own devm wrapping.
> Since mutex_destroy() actually does nothing in non-debug builds
> frequently calling mutex_destroy() is just ignored which is safe for now
> but wrong formally and can lead to a problem if mutex_destroy() will be
> extended so introduce devm_mutex_init() and devm_mutex_destroy().
>
Aside of the issue raised by Waiman, it is also very undesirable to
introduce EXPORT'ed symbols without a user. Please group this with a
patch that actually makes use of it.
No in-tree users, no export.
Andy, Waiman
Thanks for the review.
I'll move the patch to the devm-helpers.h then.
On 11/28/23 19:35, Andy Shevchenko wrote:
> On Mon, Nov 27, 2023 at 08:55:18PM -0500, Waiman Long wrote:
>> On 11/27/23 19:09, George Stark wrote:
>
> ...
>
>> These APIs are specific to devres. I don't believe it is suitable to put
>> them into the generic mutex.h header file. All devres_* functions are
>> defined in include/linux/device.h which is probabably not included in
>> mutex.h. You may consider putting these APIs into device.h instead.
>
> +1. We have include/linux/devm-helpers.h.
>
--
Best regards
George