[PATCH v6 0/3] locking/mutex: Mark devm_mutex_init() as __must_check

[PATCH v6 0/3] locking/mutex: Mark devm_mutex_init() as __must_check

[Thomas Weißschuh]

devm_mutex_init() can fail. Make sure everybody checks the return value.
All patches should go through the mutex tree together.

It would be great if this could go in through a single tree at once.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
Changes in v6:
- Rebase on v6.16-rc1
- Pick up review tag from Bartosz
- Fix up spi-nxp-fspi
- Fix up leds-lp8860
- Link to v5: https://lore.kernel.org/r/20250505-must_check-devm_mutex_init-v5-1-92fa4b793c6e@weissschuh.net

Changes in v5:
- Pick up review tag from Andy
- Link to v4: https://lore.kernel.org/r/20250407-must_check-devm_mutex_init-v4-1-587bacc9f6b3@weissschuh.net

Changes in v4:
- Drop already applied leds-1202 driver patch
- Rebase on v6.15-rc1
- Link to v3: https://lore.kernel.org/r/20250208-must_check-devm_mutex_init-v3-0-245e417dcc9e@weissschuh.net

Changes in v3:
- Introduce and use helper macro __mutex_init_ret()
- Link to v2: https://lore.kernel.org/r/20250204-must_check-devm_mutex_init-v2-0-7b6271c4b7e6@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 (3):
      spi: spi-nxp-fspi: check return value of devm_mutex_init()
      leds: lp8860: Check return value of devm_mutex_init()
      locking/mutex: Mark devm_mutex_init() as __must_check

 drivers/leds/leds-lp8860.c |  4 +++-
 drivers/spi/spi-nxp-fspi.c |  4 +++-
 include/linux/mutex.h      | 11 +++++++----
 3 files changed, 13 insertions(+), 6 deletions(-)
---
base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
change-id: 20241031-must_check-devm_mutex_init-cac583bda8fe

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

[PATCH v6 1/3] spi: spi-nxp-fspi: check return value of devm_mutex_init()

[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: 48900813abd2 ("spi: spi-nxp-fspi: remove the goto in probe")
Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/spi/spi-nxp-fspi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-nxp-fspi.c b/drivers/spi/spi-nxp-fspi.c
index e63c77e418231cd0698ffb73eeeebfbe63cc3065..f3d5765054132cd18b7257439ece971f58e9ceb2 100644
--- a/drivers/spi/spi-nxp-fspi.c
+++ b/drivers/spi/spi-nxp-fspi.c
@@ -1273,7 +1273,9 @@ static int nxp_fspi_probe(struct platform_device *pdev)
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to request irq\n");
 
-	devm_mutex_init(dev, &f->lock);
+	ret = devm_mutex_init(dev, &f->lock);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to initialize lock\n");
 
 	ctlr->bus_num = -1;
 	ctlr->num_chipselect = NXP_FSPI_MAX_CHIPSELECT;

-- 
2.49.0

Re: [PATCH v6 1/3] spi: spi-nxp-fspi: check return value of devm_mutex_init()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 395 bytes --]

On Mon, Jun 09, 2025 at 10:38:37PM +0200, 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.

I don't understand the comment about leaking here?  We might end up with
an unitialised mutex but how would we leak anything?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v6 1/3] spi: spi-nxp-fspi: check return value of devm_mutex_init()

[Andy Shevchenko]

On Mon, Jun 09, 2025 at 09:59:46PM +0100, Mark Brown wrote:
> On Mon, Jun 09, 2025 at 10:38:37PM +0200, 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.
> 
> I don't understand the comment about leaking here?  We might end up with
> an unitialised mutex but how would we leak anything?

In case if the mutex_init() allocates something that needs to be freed
(in the future).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 1/3] spi: spi-nxp-fspi: check return value of devm_mutex_init()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 464 bytes --]

On Tue, Jun 10, 2025 at 12:57:37PM +0300, Andy Shevchenko wrote:
> On Mon, Jun 09, 2025 at 09:59:46PM +0100, Mark Brown wrote:

> > I don't understand the comment about leaking here?  We might end up with
> > an unitialised mutex but how would we leak anything?

> In case if the mutex_init() allocates something that needs to be freed
> (in the future).

I don't see how checking the return value impacts that?  The management
via devm is still there either way.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v6 1/3] spi: spi-nxp-fspi: check return value of devm_mutex_init()

[Andy Shevchenko]

On Tue, Jun 10, 2025 at 12:46:12PM +0100, Mark Brown wrote:
> On Tue, Jun 10, 2025 at 12:57:37PM +0300, Andy Shevchenko wrote:
> > On Mon, Jun 09, 2025 at 09:59:46PM +0100, Mark Brown wrote:
> 
> > > I don't understand the comment about leaking here?  We might end up with
> > > an unitialised mutex but how would we leak anything?
> 
> > In case if the mutex_init() allocates something that needs to be freed
> > (in the future).
> 
> I don't see how checking the return value impacts that?  The management
> via devm is still there either way.

I see now what you mean. Yes, this is more likely applicable to non-devm case.
Thomas, can you adjust the commit message(s), please, for v7?

-- 
With Best Regards,
Andy Shevchenko

[PATCH v6 2/3] leds: lp8860: Check return value of devm_mutex_init()

[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: 87a59548af95 ("leds: lp8860: Use new mutex guards to cleanup function exits")
Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/leds/leds-lp8860.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/leds-lp8860.c b/drivers/leds/leds-lp8860.c
index 52b97c9f2a03567aa12d4f63a951593a5e7017d5..0962c00c215a11f555a7878a3b65824b5219a1fa 100644
--- a/drivers/leds/leds-lp8860.c
+++ b/drivers/leds/leds-lp8860.c
@@ -307,7 +307,9 @@ static int lp8860_probe(struct i2c_client *client)
 	led->client = client;
 	led->led_dev.brightness_set_blocking = lp8860_brightness_set;
 
-	devm_mutex_init(&client->dev, &led->lock);
+	ret = devm_mutex_init(&client->dev, &led->lock);
+	if (ret)
+		return dev_err_probe(&client->dev, ret, "Failed to initialize lock\n");
 
 	led->regmap = devm_regmap_init_i2c(client, &lp8860_regmap_config);
 	if (IS_ERR(led->regmap)) {

-- 
2.49.0

Re: [PATCH v6 2/3] leds: lp8860: Check return value of devm_mutex_init()

[Andrew Davis]

On 6/9/25 3:38 PM, 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.
> 
> Fixes: 87a59548af95 ("leds: lp8860: Use new mutex guards to cleanup function exits")
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>   drivers/leds/leds-lp8860.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/leds/leds-lp8860.c b/drivers/leds/leds-lp8860.c
> index 52b97c9f2a03567aa12d4f63a951593a5e7017d5..0962c00c215a11f555a7878a3b65824b5219a1fa 100644
> --- a/drivers/leds/leds-lp8860.c
> +++ b/drivers/leds/leds-lp8860.c
> @@ -307,7 +307,9 @@ static int lp8860_probe(struct i2c_client *client)
>   	led->client = client;
>   	led->led_dev.brightness_set_blocking = lp8860_brightness_set;
>   
> -	devm_mutex_init(&client->dev, &led->lock);
> +	ret = devm_mutex_init(&client->dev, &led->lock);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret, "Failed to initialize lock\n");

I don't think the lock initialization can actually fail, if anything ever breaks
it will be the devm allocation, which is a ENOMEM situation, so probably not worth
printing anything. Either way is fine for __must_check sake so,

Acked-by: Andrew Davis <afd@ti.com>

>   
>   	led->regmap = devm_regmap_init_i2c(client, &lp8860_regmap_config);
>   	if (IS_ERR(led->regmap)) {
> 

[PATCH v6 3/3] 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 the 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, it can not be annotated
directly. Annotate __devm_mutex_init() instead.
Unfortunately __must_check/warn_unused_result don't propagate through
statement expression. So move the statement expression into the argument
list of the call to __devm_mutex_init() through a helper macro.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 include/linux/mutex.h | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index a039fa8c17807c700d3b61193feac0418cad1243..00afd341d293ddfcc0a427b576efdce044955e38 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,14 +141,17 @@ static inline int __devm_mutex_init(struct device *dev, struct mutex *lock)
 
 #endif
 
-#define devm_mutex_init(dev, mutex)			\
+#define __mutex_init_ret(mutex)				\
 ({							\
 	typeof(mutex) mutex_ = (mutex);			\
 							\
 	mutex_init(mutex_);				\
-	__devm_mutex_init(dev, mutex_);			\
+	mutex_;						\
 })
 
+#define devm_mutex_init(dev, mutex) \
+	__devm_mutex_init(dev, __mutex_init_ret(mutex))
+
 /*
  * See kernel/locking/mutex.c for detailed documentation of these APIs.
  * Also see Documentation/locking/mutex-design.rst.

-- 
2.49.0