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

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

Pre-existing issues:
- [High] The driver incorrectly registers a sleeping function for the non-blocking `brightness_set` callback, leading to 'scheduling while atomic' panics.
- [High] `st1202_led_set` (mapped to `brightness_set_blocking`) fails to apply the requested brightness level.
- [High] The driver saves `fwnode` pointers from a scoped device tree iterator without incrementing their reference counts.
- [High] The `pattern_set` implementation fails to clear unused pattern slots when a pattern sequence shorter than 8 steps is provided.
--

commit 8a281666b5378bc7bda3d6eddc8d9626baa87b6e
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.

> 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);
> +
> +	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);
> +}

[Severity: High]
This isn't a bug introduced by this patch, but this function sleeps by
acquiring a mutex and performing I2C operations.

Since this function is assigned to led_cdev.brightness_set (which is expected
to be non-blocking and can be called from atomic contexts like software blink
timers), will this lead to a scheduling while atomic panic? Should this be
assigned to brightness_set_blocking instead?

[ ... ]
>  static int st1202_led_set(struct led_classdev *ldev, enum led_brightness value)
>  {
>  	struct st1202_led *led = cdev_to_st1202_led(ldev);

[Severity: High]
This is a pre-existing issue, but st1202_led_set() discards the requested
brightness level.

Based on the surrounding code, this function only toggles the channel boolean:

    return st1202_channel_set(led->chip, led->led_num, !!value);

Since this function is mapped to the brightness_set_blocking callback,
wouldn't this mean sysfs writes fail to apply the actual requested brightness
to the hardware register, effectively breaking brightness control?

> @@ -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;
> +
>  	ret = st1202_write_reg(chip, ST1202_CONFIG_REG, (ST1202_CONFIG_REG_PATSR |
>  							ST1202_CONFIG_REG_PATS | ST1202_CONFIG_REG_SHFT));
>  	if (ret != 0)
>  		return ret;

[Severity: High]
This is a pre-existing issue, but does st1202_led_pattern_set() fail to clear
unused pattern slots?

If a pattern sequence shorter than 8 steps (ST1202_MAX_PATTERNS) is provided,
the loop writes up to len but doesn't explicitly write a 0 duration to the
remaining pattern slots. If a longer sequence was previously set, will the
hardware continue executing leftover uninitialized steps using stale data and
cause erratic flashing?

[Severity: High]
Additionally, there is a pre-existing use-after-free issue in
st1202_dt_init() which isn't shown in this diff.

When saving fwnode pointers from a scoped device tree iterator:

    for_each_available_child_of_node_scoped(dev_of_node(dev), child) {
        ...
        led->fwnode = of_fwnode_handle(child);
    }

The scoped iterator automatically drops the reference to the child node when
advancing or exiting. Since fwnode_handle_get() is not called, are we storing
dangling pointers that could be freed before being passed to
devm_led_classdev_register_ext() later in the probe?

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