[PATCH v2] leds: lp5860: fix LED teardown ordering using devm_add_action_or_reset()

[PATCH v2] leds: lp5860: fix LED teardown ordering using devm_add_action_or_reset()

[kr494167]

From: Surendra Singh Chouhan <kr494167@gmail.com>

The driver previously disabled the chip in its remove callback before
devres unregistered the LEDs. LED unregistration turns the LEDs off
through the driver brightness callback, which accessed disabled hardware.

Fix this by registering a devm action via devm_add_action_or_reset()
immediately after enabling the chip in lp5860_device_init(). Due to devm
LIFO (Last-In, First-Out) teardown ordering, devm will automatically
unregister all multicolor LEDs before the devm action disables the chip.

This allows removing lp5860_device_remove() and lp5860_remove() completely.
In addition, convert mutex_init() to devm_mutex_init() to ensure proper
devm-managed mutex lifecycle.

Fixes: f0a66563aa2d ("leds: Add support for TI LP5860 LED driver chip")
Signed-off-by: Surendra Singh Chouhan <kr494167@gmail.com>
---
v2:
 - Use full name "Surendra Singh Chouhan" in From header and Signed-off-by.
 - Convert chip disable sequence to devm_add_action_or_reset() as suggested by Lee Jones, allowing removal of lp5860_device_remove() and lp5860_remove() entirely.
 - Use devm_mutex_init() for lock initialization.

 drivers/leds/rgb/leds-lp5860-core.c | 35 ++++++++++++-----------------
 drivers/leds/rgb/leds-lp5860-spi.c  | 14 +++---------
 drivers/leds/rgb/leds-lp5860.h      |  1 -
 3 files changed, 17 insertions(+), 33 deletions(-)

diff --git a/drivers/leds/rgb/leds-lp5860-core.c b/drivers/leds/rgb/leds-lp5860-core.c
index fd0e2f6e6e0f..bbe299e36ad5 100644
--- a/drivers/leds/rgb/leds-lp5860-core.c
+++ b/drivers/leds/rgb/leds-lp5860-core.c
@@ -188,6 +188,13 @@ static int lp5860_init_dt(struct lp5860 *lp)
 	return 0;
 }
 
+static void lp5860_disable_action(void *data)
+{
+	struct lp5860 *lp = data;
+
+	lp5860_chip_enable(lp, LP5860_CHIP_DISABLE);
+}
+
 int lp5860_device_init(struct device *dev)
 {
 	struct lp5860 *lp = dev_get_drvdata(dev);
@@ -197,38 +204,24 @@ int lp5860_device_init(struct device *dev)
 	if (ret)
 		return ret;
 
+	ret = devm_add_action_or_reset(dev, lp5860_disable_action, lp);
+	if (ret)
+		return ret;
+
 	/*
 	 * Set to 8-bit PWM data without VSYNC.
 	 * Data is sent out for display instantly after received.
 	 */
-	mutex_lock(&lp->lock);
+	guard(mutex)(&lp->lock);
 	ret = regmap_update_bits(lp->regmap, LP5860_REG_DEV_INITIAL, LP5860_MODE_MASK,
 				 LP5860_MODE_1 << LP5860_MODE_SHIFT);
 	if (ret)
-		goto err_disable;
-	mutex_unlock(&lp->lock);
-
-	ret = lp5860_init_dt(lp);
-	if (ret)
-		goto err_disable;
-
-	return 0;
+		return ret;
 
-err_disable:
-	mutex_unlock(&lp->lock);
-	lp5860_chip_enable(lp, LP5860_CHIP_DISABLE);
-	return ret;
+	return lp5860_init_dt(lp);
 }
 EXPORT_SYMBOL_GPL(lp5860_device_init);
 
-void lp5860_device_remove(struct device *dev)
-{
-	struct lp5860 *lp = dev_get_drvdata(dev);
-
-	lp5860_chip_enable(lp, LP5860_CHIP_DISABLE);
-}
-EXPORT_SYMBOL_GPL(lp5860_device_remove);
-
 MODULE_AUTHOR("Steffen Trumtrar <kernel@pengutronix.de>");
 MODULE_DESCRIPTION("TI LP5860 RGB LED core driver");
 MODULE_LICENSE("GPL");
diff --git a/drivers/leds/rgb/leds-lp5860-spi.c b/drivers/leds/rgb/leds-lp5860-spi.c
index 5e0c44854a68..13ad90362d5f 100644
--- a/drivers/leds/rgb/leds-lp5860-spi.c
+++ b/drivers/leds/rgb/leds-lp5860-spi.c
@@ -61,22 +61,15 @@ static int lp5860_probe(struct spi_device *spi)
 				     "Failed to initialise Regmap.\n");
 
 	lp5860->dev = dev;
-	mutex_init(&lp5860->lock);
+	ret = devm_mutex_init(dev, &lp5860->lock);
+	if (ret)
+		return ret;
 
 	spi_set_drvdata(spi, lp5860);
 
 	return lp5860_device_init(dev);
 }
 
-static void lp5860_remove(struct spi_device *spi)
-{
-	struct lp5860 *lp5860 = spi_get_drvdata(spi);
-
-	mutex_destroy(&lp5860->lock);
-
-	lp5860_device_remove(&spi->dev);
-}
-
 static const struct of_device_id lp5860_of_match[] = {
 	{ .compatible = "ti,lp5860" },
 	{}
@@ -89,7 +82,6 @@ static struct spi_driver lp5860_driver = {
 		.of_match_table = lp5860_of_match,
 	},
 	.probe	= lp5860_probe,
-	.remove = lp5860_remove,
 };
 module_spi_driver(lp5860_driver);
 
diff --git a/drivers/leds/rgb/leds-lp5860.h b/drivers/leds/rgb/leds-lp5860.h
index 940be0c6e8da..88e327347130 100644
--- a/drivers/leds/rgb/leds-lp5860.h
+++ b/drivers/leds/rgb/leds-lp5860.h
@@ -263,6 +263,5 @@ struct lp5860 {
 };
 
 int lp5860_device_init(struct device *dev);
-void lp5860_device_remove(struct device *dev);
 
 #endif /* _DRIVERS_LEDS_RGB_LP5860_H */
-- 
2.55.0

Re: [PATCH v2] leds: lp5860: fix LED teardown ordering using devm_add_action_or_reset()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unconditional self-deadlock during probe due to incorrect lock lifetime using `guard(mutex)`.
- [Low] Undeclared variable `ret` used in `lp5860_probe()`.
--

--- Patch [1]: [PATCH v2] leds: lp5860: fix LED teardown ordering using devm_add_action_or_reset() ---
commit fa14917dcf9d1637bf1292a82596892d65e73078
Author: Surendra Singh Chouhan <kr494167@gmail.com>

leds: lp5860: fix LED teardown ordering using devm_add_action_or_reset()

The driver previously disabled the chip in its remove callback before devres
unregistered the LEDs. This patch fixes the teardown ordering by registering a
devm action to disable the chip, ensuring it happens after LED unregistration.
It also converts mutex initialization to use devm_mutex_init().

> diff --git a/drivers/leds/rgb/leds-lp5860-core.c b/drivers/leds/rgb/leds-lp5860-core.c
> index fd0e2f6e6e0f4..bbe299e36ad53 100644
> --- a/drivers/leds/rgb/leds-lp5860-core.c
> +++ b/drivers/leds/rgb/leds-lp5860-core.c
[ ... ]
> @@ -197,38 +204,24 @@ int lp5860_device_init(struct device *dev)
>  	if (ret)
>  		return ret;
>  
> +	ret = devm_add_action_or_reset(dev, lp5860_disable_action, lp);
> +	if (ret)
> +		return ret;
> +
>  	/*
>  	 * Set to 8-bit PWM data without VSYNC.
>  	 * Data is sent out for display instantly after received.
>  	 */
> -	mutex_lock(&lp->lock);
> +	guard(mutex)(&lp->lock);
>  	ret = regmap_update_bits(lp->regmap, LP5860_REG_DEV_INITIAL, LP5860_MODE_MASK,
>  				 LP5860_MODE_1 << LP5860_MODE_SHIFT);
>  	if (ret)
> -		goto err_disable;
> -	mutex_unlock(&lp->lock);
> -
> -	ret = lp5860_init_dt(lp);
> -	if (ret)
> -		goto err_disable;
> -
> -	return 0;
> +		return ret;
>  
> -err_disable:
> -	mutex_unlock(&lp->lock);
> -	lp5860_chip_enable(lp, LP5860_CHIP_DISABLE);
> -	return ret;
> +	return lp5860_init_dt(lp);

[Severity: High]
Does this code introduce a self-deadlock during probe?

By replacing the explicit mutex_lock/unlock with guard(mutex), the lock
lifetime is extended to the end of lp5860_device_init(). This means the
lock is held while calling lp5860_init_dt().

However, lp5860_init_dt() eventually calls lp5860_set_mc_brightness(),
which attempts to acquire the exact same lock:

drivers/leds/rgb/leds-lp5860-core.c:lp5860_set_mc_brightness() {
    ...
    guard(mutex)(&led->chip->lock);
    ...
}

Since Linux mutexes are non-recursive, will this cause an unconditional
deadlock during initialization?

>  }
>  EXPORT_SYMBOL_GPL(lp5860_device_init);
>  
> diff --git a/drivers/leds/rgb/leds-lp5860-spi.c b/drivers/leds/rgb/leds-lp5860-spi.c
> index 5e0c44854a680..13ad90362d5f0 100644
> --- a/drivers/leds/rgb/leds-lp5860-spi.c
> +++ b/drivers/leds/rgb/leds-lp5860-spi.c
[ ... ]
> @@ -61,22 +61,15 @@ static int lp5860_probe(struct spi_device *spi)
>  				     "Failed to initialise Regmap.\n");
>  
>  	lp5860->dev = dev;
> -	mutex_init(&lp5860->lock);
> +	ret = devm_mutex_init(dev, &lp5860->lock);
> +	if (ret)
> +		return ret;

[Severity: Low]
Is the ret variable declared in lp5860_probe()?

It looks like the local variable ret was never declared in this function,
which will cause a build failure.

>  
>  	spi_set_drvdata(spi, lp5860);
>  
>  	return lp5860_device_init(dev);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260806145527.114844-1-kr494167@gmail.com?part=1