Re: [PATCH v2] leds: pca9532: Propagate I2C errors

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] leds: pca9532: u8 duty-cycle truncation in
  pca9532_update_hw_blink()
- [Medium] leds: pca9532: unlocked state updates and rollbacks outside
  update_lock
- [Low] leds: pca9532: LED registered before hardware init in
  pca9532_configure()
--

commit 97438085718f3f15d211a12a646ef74e8799f047
Author: Daniil Demakov <t3127430@gmail.com>

leds: pca9532: Propagate I2C errors

This commit checks SMBus read and write return values across LED, PWM,
blink, GPIO, and initialization paths, and restores the previous LED state
when I2C transfers fail.

> diff --git a/drivers/leds/leds-pca9532.c b/drivers/leds/leds-pca9532.c
> index c8085427696ff..0856c06657071 100644
> --- a/drivers/leds/leds-pca9532.c
> +++ b/drivers/leds/leds-pca9532.c

[ ... ]

> @@ -223,17 +273,32 @@ static int pca9532_update_hw_blink(struct pca9532_led *led,
>  		}
>  	}
>  
> -	psc = ((delay_on + delay_off) * PCA9532_PWM_PERIOD_DIV - 1) / 1000;
> -	if (psc > U8_MAX) {
> -		/* Blink period too long to be handled by hardware */
> +	if (delay_on > ULONG_MAX - delay_off)
> +		return -EINVAL;
> +
> +	period = delay_on + delay_off;
> +
> +	if (!period)
> +		return -EINVAL;
> +
> +	if (period > ((U8_MAX + 1) * 1000) /
> +		     PCA9532_PWM_PERIOD_DIV) {
>  		return -EINVAL;
>  	}
>  
> -	led->state = PCA9532_PWM1;
> +	psc = (period * PCA9532_PWM_PERIOD_DIV - 1) / 1000;
> +
>  	data->psc[PCA9532_PWM_ID_1] = psc;
> -	data->pwm[PCA9532_PWM_ID_1] = (delay_on * PCA9532_PWM_DUTY_DIV) / (delay_on + delay_off);
> +	data->pwm[PCA9532_PWM_ID_1] =
> +		(delay_on * PCA9532_PWM_DUTY_DIV) / period;

[Severity: Medium]
Does this truncate the duty cycle to 0 in u8 data->pwm[PCA9532_PWM_ID_1]
when delay_off is 0?

pca9532_set_blink() already replaces (0, 0) with (500, 500) and overflow
is rejected above, so period is never 0 here and the !period check is
unreachable.  When a caller such as delay_off_store() sets delay_off to 0
with a non-zero delay_on, period equals delay_on and the quotient is
PCA9532_PWM_DUTY_DIV (256), which wraps to 0 in u8 and turns the LED off
instead of returning -EINVAL so led_set_software_blink() can handle a
zero delay.  Conversely, when delay_on is 0 and delay_off > 0, this
programs a 0% duty cycle and places the LED into PCA9532_PWM1, causing
subsequent non-zero pca9532_set_brightness() calls to return early at
led->state == PCA9532_PWM1.

Checking if (!delay_on || !delay_off) instead of if (!period) would reject
both cases for software fallback.

> +
> +	ret = pca9532_setpwm(data->client, PCA9532_PWM_ID_1);
> +	if (ret < 0)
> +		return ret;
>  
> -	return pca9532_setpwm(data->client, PCA9532_PWM_ID_1);
> +	led->state = PCA9532_PWM1;
> +
> +	return 0;
>  }
>  
>  static int pca9532_set_blink(struct led_classdev *led_cdev,
> @@ -242,6 +307,7 @@ static int pca9532_set_blink(struct led_classdev *led_cdev,
>  	struct pca9532_led *led = ldev_to_led(led_cdev);
>  	struct i2c_client *client = led->client;
>  	struct pca9532_data *data = i2c_get_clientdata(client);
> +	int old_state = led->state;
>  	int err;
>  
>  	if (!data->hw_blink)
> @@ -257,16 +323,17 @@ static int pca9532_set_blink(struct led_classdev *led_cdev,
>  	if (err)
>  		return err;
>  
> -	pca9532_setled(led);
> +	err = pca9532_setled(led);
> +	if (err < 0)
> +		led->state = old_state;
>  
> -	return 0;
> +	return err;
>  }

[Severity: Medium]
Can updating and rolling back led->state and data->pwm[]/data->psc[]
outside data->update_lock race across concurrent brightness, blink, and
GPIO calls?

data->update_lock is only held inside the leaf helpers pca9532_setpwm()
and pca9532_setled(), while pca9532_set_brightness(),
pca9532_update_hw_blink(), pca9532_set_blink(), and
pca9532_gpio_set_value() read old_state, mutate led->state and shared PWM
parameters, sleep on I2C transfers, and write back old_state without the
lock:

CPU0                                  CPU1
pca9532_set_brightness(led0)
  old_state = led0->state
  led0->state = PCA9532_PWM0
  pca9532_setpwm()
                                      pca9532_gpio_set_value(led0)
                                        old_state = PCA9532_PWM0
                                        led0->state = PCA9532_ON
                                        pca9532_setled(led0)
  pca9532_setled(led0) -> fails
  led0->state = old_state  <-- clobbers CPU1's state

Moving led->state = PCA9532_PWM1 to after the sleeping pca9532_setpwm()
call in pca9532_update_hw_blink() also allows two concurrent
pca9532_set_blink() calls on different LEDs with conflicting delays to
both pass the other->state == PCA9532_PWM1 check and overwrite
data->psc[PCA9532_PWM_ID_1] and data->pwm[PCA9532_PWM_ID_1].  In addition,
if pca9532_setled() fails in pca9532_set_brightness() or
pca9532_set_blink(), restoring only led->state leaves data->pwm[],
data->psc[], and the hardware PWM/PSC registers modified.

[ ... ]

> @@ -435,6 +531,7 @@ static int pca9532_configure(struct i2c_client *client,
>  			led->ldev.brightness_set_blocking =
>  						pca9532_set_brightness;
>  			led->ldev.blink_set = pca9532_set_blink;
> +
>  			err = led_classdev_register(&client->dev, &led->ldev);
>  			if (err < 0) {
>  				dev_err(&client->dev,
> @@ -442,19 +539,35 @@ static int pca9532_configure(struct i2c_client *client,
>  					led->name);
>  				goto exit;
>  			}
> -			pca9532_setled(led);
> +
> +			err = pca9532_setled(led);
> +			if (err < 0) {
> +				dev_err(&client->dev,
> +					"couldn't set LED %s state\n",
> +					led->name);
> +				led_classdev_unregister(&led->ldev);
> +				goto exit;
> +			}

[Severity: Low]
Should pca9532_setled() be called before led_classdev_register(), as
PCA9532_TYPE_N2100_BEEP does below?

led_classdev_register() exposes sysfs attributes and activates
default_trigger before pca9532_setled() writes the initial state, and if
pca9532_setled() fails, led_classdev_unregister() calls
led_set_brightness(LED_OFF) and flushes set_brightness_work, issuing a
second pca9532_setled() transfer on the failing I2C device.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114172431.11075-1-t3127430@gmail.com?part=1