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

[sashiko-bot@kernel.org]

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