Re: [PATCH v6 5/9] leds: st1202: fix spurious pattern sequence start in setup

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The non-blocking `brightness_set` callback is assigned to a sleeping function, and `brightness_set_blocking` improperly discards brightness values.
- [High] Per-LED pattern callbacks inappropriately modify global chip sequencer registers, corrupting state for other channels.
- [High] Device tree node `fwnode` pointer is saved without incrementing its reference count, resulting in a dangling pointer.
--

commit 0028567786c848c91c978540c9eaefe27ea93565
Author: Manuel Fombuena <fombuena@outlook.com>

leds: st1202: fix spurious pattern sequence start in setup

This patch removes a final write to the configuration register in
st1202_setup(). The removal prevents the hardware pattern sequencer from
starting prematurely during device probe before any patterns have been
programmed.

> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index 3203b3768ec52..b8a5712829009 100644
> --- a/drivers/leds/leds-st1202.c
> +++ b/drivers/leds/leds-st1202.c
> @@ -333,11 +333,6 @@ static int st1202_setup(struct st1202_chip *chip)
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = st1202_write_reg(chip, ST1202_CONFIG_REG,
> -				ST1202_CONFIG_REG_PATS | ST1202_CONFIG_REG_PATSR);
> -	if (ret < 0)
> -		return ret;
> -
>  	return 0;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does st1202_dt_init() create a
dangling fwnode pointer?

When retrieving the child nodes via for_each_available_child_of_node_scoped(),
the node reference is dropped automatically at the end of the loop:

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

Because fwnode_handle_get() is not used before assigning to led->fwnode, this
pointer becomes dangling after the loop ends and is later dereferenced in
st1202_probe() when calling devm_led_classdev_register_ext().

Can this lead to a use-after-free, especially if dynamic device trees or
overlays are used?

[Severity: High]
This is a pre-existing issue, but are the blocking and non-blocking brightness
callbacks swapped in st1202_dt_init()?

st1202_dt_init() {
    ...
    led->led_cdev.brightness_set_blocking = st1202_led_set;
    ...
    led->led_cdev.brightness_set = st1202_brightness_set;
    ...
}

The non-blocking brightness_set callback is assigned to
st1202_brightness_set(), which takes a mutex and performs I2C transactions
that can sleep:

st1202_brightness_set() {
    ...
    guard(mutex)(&chip->lock);
    st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num, value);
}

Could this cause a scheduling while atomic bug if triggered from an atomic
context like a software timer?

At the same time, the blocking callback is assigned to st1202_led_set(),
which completely discards intermediate brightness values and only toggles
the channel on or off:

st1202_led_set() {
    ...
    return st1202_channel_set(led->chip, led->led_num, !!value);
}

[Severity: High]
This isn't a bug introduced by this patch, but do the per-LED pattern
callbacks corrupt global sequencer registers for other LEDs?

In st1202_led_pattern_clear(), the function unconditionally zeroes out the
global pattern duration registers:

st1202_led_pattern_clear() {
    ...
    for (int patt = 0; patt < ST1202_MAX_PATTERNS; patt++) {
        ...
        ret = st1202_write_reg(chip, ST1202_PATTERN_DUR + patt, 0);
    }
    ...
}

Similarly, st1202_led_pattern_set() resets the global sequence enable bits
for the whole chip:

st1202_led_pattern_set() {
    ...
    ret = st1202_write_reg(chip, ST1202_CONFIG_REG, (ST1202_CONFIG_REG_PATSR |
                            ST1202_CONFIG_REG_PATS | ST1202_CONFIG_REG_SHFT));
    ...
}

Since these registers are shared across all channels on the chip, will clearing
or setting a pattern on one LED arbitrarily destroy the pattern state of
another LED running simultaneously?

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