Re: [PATCH v5 3/4] leds: pca963x: add multicolor LED class support

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] State desynchronization on I2C failure leaves the chip stuck in sleep mode.
--

--- Patch [3]: [PATCH v5 3/4] leds: pca963x: add multicolor LED class support ---
commit 8c03d358e0d33360e4842037176019ecbef4c7b9
Author: Loic Poulain <[email protected]>

leds: pca963x: add multicolor LED class support

Allow grouping of individual PCA963x PWM channels into a single
multicolor LED device by adding support for the LED multicolor class.
A child node with sub-children is treated as a multicolor group,
others are treated as single leds, keeping full backwards compatibility.

> diff --git a/drivers/leds/leds-pca963x.c b/drivers/leds/leds-pca963x.c
> index e3a81c60ee27c..e54a60c7bf282 100644
> --- a/drivers/leds/leds-pca963x.c
> +++ b/drivers/leds/leds-pca963x.c
[ ... ]
> @@ -221,22 +229,52 @@ static int pca963x_led_set(struct led_classdev *led_cdev,
[ ... ]
> +static int pca963x_led_mc_set(struct led_classdev *led_cdev,
> +			      enum led_brightness value)
> +{
> +	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(led_cdev);
> +	struct pca963x_led *led = container_of(mc_cdev, struct pca963x_led, mc_cdev);
> +	unsigned long cached_leds;
> +	int ret;
> +
> +	led_mc_calc_color_components(mc_cdev, value);
> +
> +	guard(mutex)(&led->chip->mutex);
> +
> +	cached_leds = led->chip->leds_on;
> +	for (unsigned int i = 0; i < mc_cdev->num_colors; i++) {
> +		led->led_num = mc_cdev->subled_info[i].channel;
> +		ret = pca963x_brightness(led, mc_cdev->subled_info[i].brightness);
> +		if (ret)
> +			return ret;

[Severity: High]
If an I2C error occurs here on a subsequent iteration of the loop, could the
hardware state become desynchronized from the software state?

Earlier iterations of this loop may have successfully completed and called
pca963x_track_power_state(), which updates the software state in
led->chip->leds_on. However, returning early here skips the
pca963x_sync_power_state() call.

If the early return happens when turning on the LEDs, led->chip->leds_on will
have bits set, but the chip may still be in sleep mode. Future calls to set
brightness would then incorrectly believe the chip is already awake,
potentially leaving the hardware unresponsive until userspace explicitly turns
off every LED on the chip to reset the state.

> +
> +		pca963x_track_power_state(led, mc_cdev->subled_info[i].channel,
> +					  mc_cdev->subled_info[i].brightness);
> +	}
> +
> +	return pca963x_sync_power_state(led, cached_leds);
> +}

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