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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] leds: pca9532: unrestored led->state on I2C failure breaks
  blink fallback
- [Low] leds: pca9532: dev_warn() log spam on software blink fallback in
  pca9532_set_blink()
--

commit 85607316f9a97fea0c814965c666330e54cd0fdc
Author: Daniil Demakov <t3127430@gmail.com>

leds: pca9532: Propagate I2C errors

This commit checks return values from SMBus read and write calls in the
pca9532 driver and propagates errors through LED, PWM, blink, GPIO, and
device initialization paths.

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

[ ... ]

> @@ -223,15 +262,28 @@ 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) {
> +		dev_warn(&led->client->dev,
> +			 "Blink period too long: %lu ms on, %lu ms off\n",
> +			 delay_on, delay_off);
>  		return -EINVAL;
>  	}
>  
> +	psc = (period * PCA9532_PWM_PERIOD_DIV - 1) / 1000;
> +
>  	led->state = PCA9532_PWM1;
>  	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;
>  
>  	return pca9532_setpwm(data->client, PCA9532_PWM_ID_1);
>  }

[Severity: Medium]
Does propagating an error from pca9532_setpwm() or pca9532_setled() leave
led->state stuck at PCA9532_PWM1 when hardware blink setup fails?

When pca9532_set_blink() returns an error to led_blink_setup(), the LED core
falls back to software blinking via led_set_software_blink().  Because
pca9532_set_brightness() ignores non-zero brightness updates whenever
led->state == PCA9532_PWM1, software blinking can never turn the LED on, and
pca9532_update_hw_blink() will also treat PWM1 as busy for other LEDs on the
chip.

pca9532_set_brightness() and pca9532_gpio_set_value() similarly overwrite
led->state before pca9532_calcpwm(), pca9532_setpwm(), and pca9532_setled()
succeed, leaving software state out of sync with the hardware registers on
I2C errors.

> @@ -244,8 +296,10 @@ static int pca9532_set_blink(struct led_classdev *led_cdev,
>  	struct pca9532_data *data = i2c_get_clientdata(client);
>  	int err;
>  
> -	if (!data->hw_blink)
> +	if (!data->hw_blink) {
> +		dev_warn(&client->dev, "Blinking not supported on this device\n");
>  		return -EINVAL;
> +	}
>  
>  	if (*delay_on == 0 && *delay_off == 0) {
>  		/* led subsystem ask us for a blink rate */
> @@ -254,19 +308,18 @@ static int pca9532_set_blink(struct led_classdev *led_cdev,
>  	}
>  
>  	err = pca9532_update_hw_blink(led, *delay_on, *delay_off);
> -	if (err)
> +	if (err) {
> +		dev_warn(&client->dev, "Failed to set blink: %d\n", err);
>  		return err;
> +	}
>  
> -	pca9532_setled(led);
> -
> -	return 0;
> +	return pca9532_setled(led);
>  }

[Severity: Low]
Should these dev_warn() calls in pca9532_update_hw_blink() and
pca9532_set_blink() be dropped?

In led_blink_setup(), a non-zero return from blink_set() is the normal way to
decline hardware offload and fall back to software blinking.  Warning when
!data->hw_blink, when the period exceeds the hardware limit, or whenever
pca9532_update_hw_blink() returns -EINVAL floods the kernel log on normal
software-blink fallback (and duplicates the warning already printed by
pca9532_setpwm() on I2C failure), reintroducing the log noise removed by
2aad93b6de0d ("leds: pca9532: Remove irrelevant blink configuration error
message").

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