Re: [PATCH v2] leds: lp5860: Return an error for an out-of-range 'reg' property

[Lee Jones <lee@kernel.org>]

Please consider / reply to these.

/* Sashiko Automation: Reviewed (3 Findings) */

On Thu, 18 Jun 2026, Mert Seftali wrote:

> lp5860_iterate_subleds() checks the result of reading the "reg" property
> and the channel range in a single condition:
> 
> 	if (ret < 0 || channel > LP5860_MAX_LED)
> 
> When fwnode_property_read_u32() succeeds but the channel is out of range,
> ret is 0, so the error path passes 0 to dev_err_probe() and returns 0 -
> an out-of-range "reg" is silently accepted instead of rejected. The
> shared "'reg' property is missing" message is also inaccurate when the
> property is present but out of range.
> 
> Split the two cases: report a read failure with if (ret), and reject an
> out-of-range channel with -EINVAL. Each case now has its own accurate
> error message.
> 
> Fixes: 3daf2c4ef82b ("leds: Add support for TI LP5860 LED driver chip")
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <error27@gmail.com>
> Closes: https://lore.kernel.org/r/202605210624.3gcr3prk-lkp@intel.com/
> Signed-off-by: Mert Seftali <mertsftl@gmail.com>
> ---
> Changes in v2 (per Lee Jones review):
> - Split the combined read/range test into separate checks: use if(ret)
>   for the read failure, and reject an out-of-range channel with -EINVAL.
> - Give each case its own accurate message instead of the shared
>   "missing" one, and drop the nested "if (ret >= 0)".
> 
>  drivers/leds/rgb/leds-lp5860-core.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/leds/rgb/leds-lp5860-core.c b/drivers/leds/rgb/leds-lp5860-core.c
> index fd0e2f6e6e0f..bc59be87b08f 100644
> --- a/drivers/leds/rgb/leds-lp5860-core.c
> +++ b/drivers/leds/rgb/leds-lp5860-core.c
> @@ -114,13 +114,21 @@ static int lp5860_iterate_subleds(struct lp5860_led *led, struct led_init_data *
>  		}
>  
>  		ret = fwnode_property_read_u32(led_node, "reg", &channel);
> -		if (ret < 0 || channel > LP5860_MAX_LED) {
> +		if (ret) {
>  			dev_err_probe(led->chip->dev, ret,
> -				      "%pfwP: 'reg' property is missing. Skipping.\n", led_node);
> +				      "%pfwP: Cannot read 'reg' property. Skipping.\n", led_node);

This isn't a bug introduced by this patch, but while reviewing the driver
I noticed a double unlock in lp5860_device_init():

drivers/leds/rgb/leds-lp5860-core.c:lp5860_device_init() {
        ...
        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;

err_disable:
        mutex_unlock(&lp->lock);
        ...
}



[Severity: High]
If lp5860_init_dt() fails, doesn't the jump to err_disable result in
calling mutex_unlock() on an already unlocked mutex?


This is also a pre-existing issue, but I noticed a potential
use-after-destroy involving the same mutex in the removal path:

drivers/leds/rgb/leds-lp5860-spi.c:lp5860_remove() {
        ...
        mutex_destroy(&lp5860->lock);

        lp5860_device_remove(&spi->dev);
}

Calling lp5860_device_remove() ends up taking the lock again:

drivers/leds/rgb/leds-lp5860-core.c:lp5860_device_remove() {
        ...
        lp5860_chip_enable(lp, LP5860_CHIP_DISABLE);
}

drivers/leds/rgb/leds-lp5860-core.c:lp5860_chip_enable() {
        guard(mutex)(&lp->lock);
        return regmap_write(lp->regmap, LP5860_REG_CHIP_EN, enable);
}



[Severity: High]
Does this sequence result in lp5860_chip_enable() locking a mutex that
was just destroyed by lp5860_remove()?

>  			fwnode_handle_put(led_node);
>  			return ret;
>  		}
>  
> +		if (channel > LP5860_MAX_LED) {
> +			dev_err_probe(led->chip->dev, -EINVAL,
> +				      "%pfwP: 'reg' %u is out of range. Skipping.\n",
> +				      led_node, channel);
> +			fwnode_handle_put(led_node);
> +			return -EINVAL;
> +		}
> +
>  		led->mc_cdev.subled_info[subled].color_index = color_index;
>  		led->mc_cdev.subled_info[subled].channel = channel;
>  		ret = lp5860_led_init(led, init_data->fwnode, channel);
> -- 
> 2.54.0
> 
> 

-- 
Lee Jones