[PATCH v3] leds: lp5860: Fix LED teardown ordering using devm_add_action_or_reset()

[PATCH v3] 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.

In addition, use scoped_guard(mutex, &lp->lock) to restrict the initial
mode setting lock scope, avoiding holding the mutex across
lp5860_init_dt(), which would cause a self-deadlock when registering
subleds.

Finally, convert mutex_init() to devm_mutex_init() and declare the local
variable ret in lp5860_probe(), allowing lp5860_device_remove() and
lp5860_remove() to be removed completely.

Fixes: f0a66563aa2d ("leds: Add support for TI LP5860 LED driver chip")
Signed-off-by: Surendra Singh Chouhan <kr494167@gmail.com>
---
v3:
 - Use scoped_guard(mutex, &lp->lock) in lp5860_device_init() so the lock is released before calling lp5860_init_dt(), preventing a self-deadlock during subled registration.
 - Declare missing local variable 'ret' in lp5860_probe().
 - Capitalize patch title per LED subsystem guidelines.

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.
 - Use devm_mutex_init() for lock initialization.

 drivers/leds/rgb/leds-lp5860-core.c | 42 +++++++++++++----------------
 drivers/leds/rgb/leds-lp5860-spi.c  | 15 +++--------
 drivers/leds/rgb/leds-lp5860.h      |  1 -
 3 files changed, 22 insertions(+), 36 deletions(-)

diff --git a/drivers/leds/rgb/leds-lp5860-core.c b/drivers/leds/rgb/leds-lp5860-core.c
index fd0e2f6e6e0f..bc2eb7bc76d6 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,25 @@ 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);
-	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;
+	scoped_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)
+			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..5b1a28b63093 100644
--- a/drivers/leds/rgb/leds-lp5860-spi.c
+++ b/drivers/leds/rgb/leds-lp5860-spi.c
@@ -38,6 +38,7 @@ static int lp5860_probe(struct spi_device *spi)
 	struct device *dev = &spi->dev;
 	struct lp5860 *lp5860;
 	unsigned int multi_leds;
+	int ret;
 
 	multi_leds = device_get_child_node_count(dev);
 	if (!multi_leds) {
@@ -61,22 +62,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 +83,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 v3] leds: lp5860: Fix LED teardown ordering using devm_add_action_or_reset()

[sashiko-bot]

> 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.
> 
> In addition, use scoped_guard(mutex, &lp->lock) to restrict the initial
> mode setting lock scope, avoiding holding the mutex across
> lp5860_init_dt(), which would cause a self-deadlock when registering
> subleds.
> 
> Finally, convert mutex_init() to devm_mutex_init() and declare the local
> variable ret in lp5860_probe(), allowing lp5860_device_remove() and
> lp5860_remove() to be removed completely.
> 
> Fixes: f0a66563aa2d ("leds: Add support for TI LP5860 LED driver chip")
> Signed-off-by: Surendra Singh Chouhan <kr494167@gmail.com>

Sashiko has reviewed this patch and found no issues. It looks great!

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