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

On Thu, 06 Aug 2026, [email protected] wrote:

> 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()`.

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

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

> --
> 
> --- Patch [1]: [PATCH v2] leds: lp5860: fix LED teardown ordering using devm_add_action_or_reset() ---
> commit fa14917dcf9d1637bf1292a82596892d65e73078
> Author: Surendra Singh Chouhan <[email protected]>
> 
> 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/[email protected]?part=1
> 

-- 
Lee Jones