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

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

[Mert Seftali]

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);
 			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

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

[Lee Jones]

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

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

[Mert Seftali]

On Thu, 2 Jul 2026, Lee Jones wrote:
> This isn't a bug introduced by this patch, but while reviewing the driver
> I noticed a double unlock in lp5860_device_init():
[...]
> If lp5860_init_dt() fails, doesn't the jump to err_disable result in
> calling mutex_unlock() on an already unlocked mutex?

Confirmed, i'll move the unlock out of err_disable so it can't fire twice.

> This is also a pre-existing issue, but I noticed a potential
> use-after-destroy involving the same mutex in the removal path:
[...]
> Does this sequence result in lp5860_chip_enable() locking a mutex that
> was just destroyed by lp5860_remove()?

i'll swap the order so device_remove() runs before
mutex_destroy().

I'll send both as a small two-patch series crediting you as
Reported-by or let me know if you'd rather the Sashiko findings be credited
to the tool instead.

thanks for flagging these!
Mert

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

[Mert Seftali]

On Thu, 2 Jul 2026, Lee Jones wrote:
> This is also a pre-existing issue, but I noticed a potential
> use-after-destroy involving the same mutex in the removal path:
> [...]
> Does this sequence result in lp5860_chip_enable() locking a mutex that
> was just destroyed by lp5860_remove()?

Hi Lee,
following up on the use-after-destroy: i looked into it properly and the
swap i mentioned isn't enough on its own.

Even with device_remove() before mutex_destroy(), the mutex is still
destroyed inside .remove(). The LED classdevs are devm-registered, so
they get torn down afterwards and led_classdev_unregister() sets
brightness to LED_OFF which runs the blocking setter under
guard(mutex)(&chip->lock). So that teardown locks the mutex after
.remove() has already destroyed it.

I reproduced the ordering under DEBUG_MUTEXES with a small devres + mutex
test that mirrors that teardown (i don't have the part): with the swap,
device_remove() comes out fine but the mirrored teardown still trips
DEBUG_LOCKS_WARN_ON(lock->magic != lock), and deferring the destroy
clears it.

So i'd switch the lock to devm_mutex_init() and drop the manual
mutex_destroy() from .remove(). devres then destroys it in LIFO order
after the LED classdevs are unregistered, so nothing locks it once it is
gone. That covers the case you flagged and this teardown path too.

Thanks,
Mert