Re: [PATCH] leds: lp5860: fix init error-path locking

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [Medium] The commit message falsely claims a deadlock existed on the `regmap_update_bits()` error path.
- [Low] The commit message prefix and description violate subsystem capitalization guidelines.

Pre-existing issues:
- [High] The driver destroys its primary mutex in `.remove()` before executing hardware teardown and devres cleanup, causing multiple use-after-destroy locking violations.
- [High] LEDs are unregistered via devres after the chip is manually disabled, risking invalid hardware accesses.
--

--- Patch [1]: [PATCH] leds: lp5860: fix init error-path locking ---
commit 2979577a641c15da3d90ccaf3a97200e7e476dbb
Author: surendra <[email protected]>

leds: lp5860: fix init error-path locking

This commit addresses a locking issue in the initialization error path for
the lp5860 driver. It releases the lock immediately after the regmap update,
avoiding potential lock mismanagement in the subsequent error paths.

> leds: lp5860: fix init error-path locking

[Severity: Low]
According to the LED subsystem naming conventions, should the description
after the subsystem prefix be capitalized? So "fix" would become "Fix".

> lp5860_device_init() jumps to err_disable with the mutex held when
> regmap_update_bits() fails. The error handler calls lp5860_chip_enable(),
> which tries to acquire the same mutex and deadlocks.
>
> Conversely, an lp5860_init_dt() failure reaches the same handler after the
> mutex has been released, causing mutex_unlock() to operate on an unlocked
> mutex.

[Severity: Medium]
Is this description of a deadlock accurate?

Looking at the original code, the err_disable block explicitly executed 
a mutex_unlock() before calling lp5860_chip_enable():

err_disable:
	mutex_unlock(&lp->lock);
	lp5860_chip_enable(lp, LP5860_CHIP_DISABLE);
	return ret;

Since lp5860_chip_enable() uses guard(mutex)(&lp->lock), it safely 
re-locks the just-released mutex. Could the commit message be updated to 
clarify that this fixes a double-unlock bug when lp5860_init_dt() fails, 
rather than a deadlock on the regmap_update_bits() path?

> diff --git a/drivers/leds/rgb/leds-lp5860-core.c b/drivers/leds/rgb/leds-lp5860-core.c
> index fd0e2f6e6e0f4..e21d5f2302be0 100644
> --- a/drivers/leds/rgb/leds-lp5860-core.c
> +++ b/drivers/leds/rgb/leds-lp5860-core.c
> @@ -204,9 +204,9 @@ int lp5860_device_init(struct device *dev)
>  	mutex_lock(&lp->lock);
>  	ret = regmap_update_bits(lp->regmap, LP5860_REG_DEV_INITIAL, LP5860_MODE_MASK,
>  				 LP5860_MODE_1 << LP5860_MODE_SHIFT);
> +	mutex_unlock(&lp->lock);
>  	if (ret)
>  		goto err_disable;
> -	mutex_unlock(&lp->lock);
>  
>  	ret = lp5860_init_dt(lp);
>  	if (ret)
> @@ -215,7 +215,6 @@ int lp5860_device_init(struct device *dev)
>  	return 0;
>  
>  err_disable:
> -	mutex_unlock(&lp->lock);
>  	lp5860_chip_enable(lp, LP5860_CHIP_DISABLE);

[Severity: High]
This is a pre-existing issue, but does destroying the mutex in 
lp5860_remove() before calling hardware teardown lead to a 
use-after-destroy violation?

In the spi driver, we have:

lp5860_remove() {
	...
	mutex_destroy(&lp5860->lock);
	lp5860_device_remove(&spi->dev);
}

But lp5860_device_remove() calls lp5860_chip_enable(), which acquires 
the just-destroyed mutex:

lp5860_chip_enable() {
	guard(mutex)(&lp->lock);
	...
}

Will this cause a kernel crash when lock debugging is enabled?


[Severity: High]
This is also a pre-existing issue, but does unregistering the LEDs via 
devres after manually disabling the chip cause invalid hardware accesses?

The driver uses devm_led_classdev_multicolor_register_ext() to register 
LEDs, which handles unregistration automatically via devres during device 
removal. However, the manual lp5860_device_remove() call happens before 
devres cleanup:

lp5860_remove() {
	...
	lp5860_device_remove(&spi->dev);
}

The automatic devres cleanup will eventually set the brightness to 0, 
invoking lp5860_set_mc_brightness(). Since the chip was already disabled 
manually in lp5860_remove(), could this result in regmap I/O to disabled 
hardware over SPI? Should the hardware teardown be synchronized via a 
managed devres action instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1