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

On Mon, 20 Jul 2026, [email protected] wrote:

> 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.

Please review, explain or fix the Sashiko review(s).

If this is a set, this request includes the other patches too.

> 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

-- 
Lee Jones