Re: [PATCH] leds: lp5860: reject out-of-range channel IDs

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The commit message incorrectly claims that the malformed sub-LED uses the invalid ID for register addressing.
- [Low] The commit message description is not capitalized, violating the LED subsystem conventions.
- [Low] The patch uses `dev_err()` instead of `dev_err_probe()` to report a configuration failure.
--

--- Patch [1]: [PATCH] leds: lp5860: reject out-of-range channel IDs ---
commit 4536ff26f6ab558401ac176c54f90cdb3be700b7
Author: surendra <[email protected]>

leds: lp5860: reject out-of-range channel IDs

This commit fixes an issue where lp5860_iterate_subleds() returns success
when encountering a channel ID that is out of bounds, instead of correctly
returning an error.

> leds: lp5860: reject out-of-range channel IDs

[Severity: Low]
This isn't a bug, but the LED subsystem guidelines require capitalizing the
description after the subsystem prefix. Should this be "Reject out-of-range
channel IDs"?

> The caller therefore treats the malformed
> sub-LED description as valid and uses the invalid ID for register
> addressing.

[Severity: Medium]
Is this description accurate?

Looking at the original code, if channel > LP5860_MAX_LED, it returns `ret`
(which is 0) and aborts the loop before assigning the parsed channel to
led->mc_cdev.subled_info[subled].channel.

Since the subled_info array is zero-filled by allocation, the unassigned
channel would remain 0 rather than containing the invalid out-of-bounds ID.
Could the commit message be updated to better reflect the exact behavior?

> diff --git a/drivers/leds/rgb/leds-lp5860-core.c b/drivers/leds/rgb/leds-lp5860-core.c
> index e21d5f2302be0..75498311b0803 100644
> --- a/drivers/leds/rgb/leds-lp5860-core.c
> +++ b/drivers/leds/rgb/leds-lp5860-core.c
> @@ -114,12 +114,18 @@ 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 < 0) {
>  			dev_err_probe(led->chip->dev, ret,
>  				      "%pfwP: 'reg' property is missing. Skipping.\n", led_node);
>  			fwnode_handle_put(led_node);
>  			return ret;
>  		}
> +		if (channel > LP5860_MAX_LED) {
> +			dev_err(led->chip->dev, "%pfwP: 'reg' property is out of range.\n",
> +				led_node);

[Severity: Low]
Should this use dev_err_probe() instead of dev_err() to maintain consistency
with the surrounding error paths and conform to the LED subsystem guidelines
for probe failures?

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

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