[PATCH v2 0/2] locking/mutex: Mark devm_mutex_init() as __must_check

[PATCH v2 0/2] locking/mutex: Mark devm_mutex_init() as __must_check

[Thomas Weißschuh]

Even if it's not critical, the avoidance of checking the error code
from devm_mutex_init() call today diminishes the point of using devm
variant of it. Tomorrow it may even leak something. Enforce all callers
checking the return value through the compiler.

The series should go through the locking tree.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
Changes in v2:
- Rebase on 6.14-rc1
- Fix up leds-1202 driver
- Link to v1: https://lore.kernel.org/r/20241202-must_check-devm_mutex_init-v1-1-e60eb97b8c72@weissschuh.net

---
Thomas Weißschuh (2):
      leds: st1202: Check for error code from devm_mutex_init() call
      locking/mutex: Mark devm_mutex_init() as __must_check

 drivers/leds/leds-st1202.c |  4 +++-
 include/linux/mutex.h      | 17 ++++++++---------
 2 files changed, 11 insertions(+), 10 deletions(-)
---
base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b
change-id: 20241031-must_check-devm_mutex_init-cac583bda8fe

Best regards,
-- 
Thomas Weißschuh <linux@weissschuh.net>

[PATCH v2 1/2] leds: st1202: Check for error code from devm_mutex_init() call

[Thomas Weißschuh]

Even if it's not critical, the avoidance of checking the error code
from devm_mutex_init() call today diminishes the point of using devm
variant of it. Tomorrow it may even leak something. Add the missed
check.

Fixes: 259230378c65 ("leds: Add LED1202 I2C driver")
Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/leds/leds-st1202.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
index b691c4886993f3e371e857543863c9a724742f39..657c62cb24fa726622369fc965fa7195e73170f0 100644
--- a/drivers/leds/leds-st1202.c
+++ b/drivers/leds/leds-st1202.c
@@ -356,7 +356,9 @@ static int st1202_probe(struct i2c_client *client)
 	if (!chip)
 		return -ENOMEM;
 
-	devm_mutex_init(&client->dev, &chip->lock);
+	ret = devm_mutex_init(&client->dev, &chip->lock);
+	if (ret < 0)
+		return ret;
 	chip->client = client;
 
 	ret = st1202_dt_init(chip);

-- 
2.48.1

Re: (subset) [PATCH v2 1/2] leds: st1202: Check for error code from devm_mutex_init() call

[Lee Jones]

On Tue, 04 Feb 2025 07:52:50 +0100, Thomas Weißschuh wrote:
> Even if it's not critical, the avoidance of checking the error code
> from devm_mutex_init() call today diminishes the point of using devm
> variant of it. Tomorrow it may even leak something. Add the missed
> check.
> 
> 

Applied, thanks!

[1/2] leds: st1202: Check for error code from devm_mutex_init() call
      commit: 8168906bbb3ba678583422de29e6349407a94bb5

--
Lee Jones [李琼斯]

[PATCH v2 2/2] locking/mutex: Mark devm_mutex_init() as __must_check

[Thomas Weißschuh]

Even if it's not critical, the avoidance of checking the error code
from devm_mutex_init() call today diminishes the point of using devm
variant of it. Tomorrow it may even leak something. Enforce all callers
checking the return value through the compiler.

As devm_mutex_init() itself is a macro which can not be annotated,
annotate __devm_mutex_init() instead.
Unfortunately __must_check/warn_unused_result don't propagate through
statement expression. To work around this move the statement expression
into the argument list of the call to __devm_mutex_init() so
devm_mutex_init() directly expands to __devm_mutex_init().

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 include/linux/mutex.h | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index 2bf91b57591b49e4668752e773419ae945f124da..65b28c9e6efc123982d923d1ed171eae471c82c1 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -126,11 +126,11 @@ do {							\
 
 #ifdef CONFIG_DEBUG_MUTEXES
 
-int __devm_mutex_init(struct device *dev, struct mutex *lock);
+int __must_check __devm_mutex_init(struct device *dev, struct mutex *lock);
 
 #else
 
-static inline int __devm_mutex_init(struct device *dev, struct mutex *lock)
+static inline int __must_check __devm_mutex_init(struct device *dev, struct mutex *lock)
 {
 	/*
 	 * When CONFIG_DEBUG_MUTEXES is off mutex_destroy() is just a nop so
@@ -141,13 +141,12 @@ static inline int __devm_mutex_init(struct device *dev, struct mutex *lock)
 
 #endif
 
-#define devm_mutex_init(dev, mutex)			\
-({							\
-	typeof(mutex) mutex_ = (mutex);			\
-							\
-	mutex_init(mutex_);				\
-	__devm_mutex_init(dev, mutex_);			\
-})
+#define devm_mutex_init(dev, mutex) __devm_mutex_init(dev, ({	\
+	typeof(mutex) mutex_ = (mutex);				\
+								\
+	mutex_init(mutex_);					\
+	mutex_;							\
+}))
 
 /*
  * See kernel/locking/mutex.c for detailed documentation of these APIs.

-- 
2.48.1

Re: [PATCH v2 2/2] locking/mutex: Mark devm_mutex_init() as __must_check

[Peter Zijlstra]

On Tue, Feb 04, 2025 at 07:52:51AM +0100, Thomas Weißschuh wrote:
> Even if it's not critical, the avoidance of checking the error code
> from devm_mutex_init() call today diminishes the point of using devm
> variant of it. Tomorrow it may even leak something. Enforce all callers
> checking the return value through the compiler.
> 
> As devm_mutex_init() itself is a macro which can not be annotated,
> annotate __devm_mutex_init() instead.
> Unfortunately __must_check/warn_unused_result don't propagate through
> statement expression. To work around this move the statement expression
> into the argument list of the call to __devm_mutex_init() so
> devm_mutex_init() directly expands to __devm_mutex_init().
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>  include/linux/mutex.h | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/mutex.h b/include/linux/mutex.h
> index 2bf91b57591b49e4668752e773419ae945f124da..65b28c9e6efc123982d923d1ed171eae471c82c1 100644
> --- a/include/linux/mutex.h
> +++ b/include/linux/mutex.h
> @@ -126,11 +126,11 @@ do {							\
>  
>  #ifdef CONFIG_DEBUG_MUTEXES
>  
> -int __devm_mutex_init(struct device *dev, struct mutex *lock);
> +int __must_check __devm_mutex_init(struct device *dev, struct mutex *lock);
>  
>  #else
>  
> -static inline int __devm_mutex_init(struct device *dev, struct mutex *lock)
> +static inline int __must_check __devm_mutex_init(struct device *dev, struct mutex *lock)
>  {
>  	/*
>  	 * When CONFIG_DEBUG_MUTEXES is off mutex_destroy() is just a nop so
> @@ -141,13 +141,12 @@ static inline int __devm_mutex_init(struct device *dev, struct mutex *lock)
>  
>  #endif
>  
> -#define devm_mutex_init(dev, mutex)			\
> -({							\
> -	typeof(mutex) mutex_ = (mutex);			\
> -							\
> -	mutex_init(mutex_);				\
> -	__devm_mutex_init(dev, mutex_);			\
> -})
> +#define devm_mutex_init(dev, mutex) __devm_mutex_init(dev, ({	\
> +	typeof(mutex) mutex_ = (mutex);				\
> +								\
> +	mutex_init(mutex_);					\
> +	mutex_;							\
> +}))

Urgh, that's a bit ugly isn't it.

Now we can either write a helper for that like:

#define mutex_init_ret(mutex)                           \
({                                                      \
	typeof(mutex) mutex_ = (mutex);                 \
	mutex_init(mutex_);                             \
	mutex_;                                         \
})

#define devm_mutex_init(dev, mutex)                     \
        __devm_mutex_init(dev, mutex_init_ret(mutex))


Or we can try and make mutex_init() return the pointer itself. I don't
think that will break anything, but its best to feel that to the robots
to make sure.