Re: [PATCH v7 7/9] leds: st1202: disable channel when brightness is set to zero

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

Pre-existing issues:
- [High] The driver assigns the sleeping `st1202_brightness_set` function to the non-blocking `brightness_set` callback, causing 'scheduling while atomic' panics.
- [High] The `st1202_led_set` function assigned to `brightness_set_blocking` fails to write the actual requested brightness value to the hardware.
- [High] Saving `fwnode_handle` from a child node without taking a reference inside a scoped iterator leads to a use-after-free.
- [Medium] The `brightness_get` callback silently drops I2C hardware errors, resulting in incorrect brightness state reporting.
- [Medium] Global hardware duration registers are corrupted by per-LED pattern changes.
--

commit f5fe39d06324d4108bb5a3ab6d86700cd360c555
Author: Manuel Fombuena <[email protected]>

    leds: st1202: disable channel when brightness is set to zero
    
    When brightness_set() is called with LED_OFF, only the ILED register is
    zeroed; the channel enable bit is left set from probe time. A hardware
    channel enabled with ILED=0 still draws a small residual current, causing
    a dim glow even when the LED is supposed to be off.
    
    Fix this by splitting st1202_channel_set() into a lockless inner function
    __st1202_channel_set() and a locking wrapper, then calling the inner
    function from brightness_set() while it already holds the mutex. The
    channel is now disabled when value is zero and re-enabled when non-zero,
    in the same lock region as the ILED write.

> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index 4909985ade04b..2ecd36aae336a 100644
> --- a/drivers/leds/leds-st1202.c
> +++ b/drivers/leds/leds-st1202.c
> @@ -188,6 +160,40 @@ static int st1202_channel_set(struct st1202_chip *chip, int led_num, bool active

[ ... ]

> +static void st1202_brightness_set(struct led_classdev *led_cdev,
> +				enum led_brightness value)
> +{
> +	struct st1202_led *led = cdev_to_st1202_led(led_cdev);
> +	struct st1202_chip *chip = led->chip;
> +
> +	guard(mutex)(&chip->lock);

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to use a mutex and
i2c operations in st1202_brightness_set()?

The led subsystem requires the brightness_set callback to not sleep, as it can
be invoked from atomic contexts like software blink timers in softirq context.
This function acquires a mutex and performs i2c bus operations which can sleep.

Should this driver implement only brightness_set_blocking instead to avoid
scheduling while atomic panics?

> +
> +	for (int patt = 0; patt < ST1202_MAX_PATTERNS; patt++)
> +		st1202_pwm_pattern_write(chip, led->led_num, patt, ST1202_PATTERN_PWM_FULL);
> +	st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num, value);
> +	__st1202_channel_set(chip, led->led_num, !!value);
> +}
> +
> +static enum led_brightness st1202_brightness_get(struct led_classdev *led_cdev)
> +{
> +	struct st1202_led *led = cdev_to_st1202_led(led_cdev);
> +	struct st1202_chip *chip = led->chip;
> +	u8 value = 0;
> +
> +	guard(mutex)(&chip->lock);
> +
> +	st1202_read_reg(chip, ST1202_ILED_REG0 + led->led_num, &value);

[Severity: Medium]
This is a pre-existing issue, but does this correctly handle i2c read failures?

If st1202_read_reg() fails, the negative error code is dropped and it returns
the uninitialized value (which is 0). This could hide hardware failures and
trick the subsystem into believing the led is powered off.

Should it return the negative error code if the read fails?

> +
> +	return value;
> +}
> +
>  static int st1202_led_set(struct led_classdev *ldev, enum led_brightness value)
>  {
>  	struct st1202_led *led = cdev_to_st1202_led(ldev);
>  
>  	return st1202_channel_set(led->chip, led->led_num, !!value);
>  }

[Severity: High]
This is a pre-existing issue, but does this function actually write the
requested brightness value to the hardware?

It converts the brightness value to a boolean and toggles the channel enable
bit via st1202_channel_set(), but it seems to omit writing the actual
brightness value to the ST1202_ILED_REG0 register.

This means intermediate brightness levels might be ignored, and the led is only
toggled on or off with whatever brightness was previously configured.

> @@ -257,6 +263,10 @@ static int st1202_led_pattern_set(struct led_classdev *ldev,
>  	if (ret != 0)
>  		return ret;
>  
> +	ret = __st1202_channel_set(chip, led->led_num, true);
> +	if (ret != 0)
> +		return ret;
> +

[Severity: Medium]
This is a pre-existing issue, but does setting a pattern on one led corrupt
the timing of concurrent patterns on other leds?

The duration registers are global to the entire chip. If multiple leds
configure patterns, they will overwrite these same global duration registers.

Also, st1202_led_pattern_clear() unconditionally zeroes all global duration
registers, which would break running patterns on all other leds.

>  	ret = st1202_write_reg(chip, ST1202_CONFIG_REG, (ST1202_CONFIG_REG_PATSR |
>  							ST1202_CONFIG_REG_PATS | ST1202_CONFIG_REG_SHFT));

[Severity: High]
This is a pre-existing issue not shown in the diff, but could there be a
use-after-free in st1202_dt_init() when registering the led?

When using for_each_available_child_of_node_scoped(), it automatically calls
of_node_put() on the child device node at the end of each loop iteration.
Inside the loop, the driver saves the pointer via:

    led->fwnode = of_fwnode_handle(child);

but it doesn't increment the reference count. When the driver later registers
the led using led->fwnode, it accesses a node whose reference count has already
been decremented. Could this be an issue if dynamic device tree overlays are
unloaded?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/GV1PR08MB8497C0B898789BB73ACE6EE3C5F52@GV1PR08MB8497.eurprd08.prod.outlook.com?part=7