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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Low] Commit Message Capitalization Violation

Pre-existing issues:
- [High] Silent truncation of negative error codes from I2C reads leads to hardware register corruption.
- [High] Unprotected I2C writes to shared global hardware blinking registers.
--

--- Patch [3]: [PATCH v6 3/4] leds: pca963x: add multicolor LED class support ---
commit 9035bb6a1e69dbc8b0e467061aaa0b06a70a0f89
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.

> leds: pca963x: add multicolor LED class support

[Severity: Low]
This isn't a bug, but the subsystem prefix description should be capitalized.
Should it be "leds: pca963x: Add multicolor LED class support"?

> diff --git a/drivers/leds/leds-pca963x.c b/drivers/leds/leds-pca963x.c
> index e3a81c60ee27c..f3e4d65e48b4c 100644
> --- a/drivers/leds/leds-pca963x.c
> +++ b/drivers/leds/leds-pca963x.c
> @@ -199,20 +203,24 @@ static void pca963x_blink(struct pca963x_led *led)
>  	led->blinking = true;
>  }

[Severity: High]
This is a pre-existing issue, but are global chip registers modified without
acquiring the chip mutex here?

drivers/leds/leds-pca963x.c:pca963x_blink() {
        ...
        i2c_smbus_write_byte_data(client, chipdef->grppwm, led->gdc);
        i2c_smbus_write_byte_data(client, chipdef->grpfreq, led->gfrq);
        
        if (!(mode2 & PCA963X_MODE2_DMBLNK))
                i2c_smbus_write_byte_data(client, PCA963X_MODE2,
                                          mode2 | PCA963X_MODE2_DMBLNK);
        
        mutex_lock(&led->chip->mutex);
        ...
}

If user-space initiates hardware blinking for two different LEDs concurrently,
could their writes to these global frequency and duty cycle registers
interleave and corrupt the hardware blink state for the entire chip?

[ ... ]

> @@ -221,22 +229,54 @@ static int pca963x_led_set(struct led_classdev *led_cdev,
>  			   enum led_brightness value)
>  {
>  	struct pca963x_led *led;
> +	unsigned long cached_leds;
>  	int ret;
>  
>  	led = container_of(led_cdev, struct pca963x_led, led_cdev);
>  
>  	mutex_lock(&led->chip->mutex);
>  
> +	cached_leds = led->chip->leds_on;
>  	ret = pca963x_brightness(led, value);

[Severity: High]
This is a pre-existing issue, but does assigning the result of
i2c_smbus_read_byte_data() directly to a u8 silently truncate negative error
codes?

If the read fails and returns -EIO, this would truncate the negative error
code into a garbage unsigned 8-bit integer, and we then write it back to the
chip in pca963x_brightness():

drivers/leds/leds-pca963x.c:pca963x_brightness() {
        ...
        ledout = i2c_smbus_read_byte_data(client, ledout_addr);
        ...
        val = (ledout & ~mask) | (PCA963X_LED_ON << shift);
        ret = i2c_smbus_write_byte_data(client, ledout_addr, val);
        ...
}

The same pattern appears in pca963x_blink() with the mode2 variable. Could
this cause hardware register corruption during a transient I2C failure?

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