Re: [PATCH v6 1/1] leds: st1202: Add hardware-accelerated blink support

On Thu, 06 Aug 2026, [email protected] wrote:

> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> 
> New issues:
> - [High] Hardware blinking destructively overwrites the PWM state of all other active LEDs, forcing them permanently off.
> - [Medium] Hardware blink forces maximum brightness, ignoring user-configured brightness.

Please review, explain or fix the Sashiko review(s).

If this is a set, this request includes the other patches too.

> Pre-existing issues:
> - [High] The driver improperly assigns a sleeping function to the led_cdev->brightness_set callback, causing a 'sleeping in atomic' bug.
> - [High] A dangling fwnode pointer is stored in `led->fwnode` due to a dropped reference count, leading to a UAF upon dereference.
> --
> 
> --- Patch [1]: [PATCH v6 1/1] leds: st1202: Add hardware-accelerated blink support ---
> commit d3cba330dd18edf23569d762b5d09a2ae91bdf35
> Author: Manuel Fombuena <[email protected]>
> 
> leds: st1202: Add hardware-accelerated blink support
> 
> This commit implements blink_set() to enable hardware-accelerated blinking
> via the timer trigger on the ST1202 LED driver. The pattern engine is used
> to produce a two-step sequence with delays clamped to hardware limits.
> 
> > diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> > index 168df5ecf27b7..ea019d03ad907 100644
> > --- a/drivers/leds/leds-st1202.c
> > +++ b/drivers/leds/leds-st1202.c
> [ ... ]
> > @@ -275,6 +276,89 @@ static int st1202_led_pattern_set(struct led_classdev *ldev,
> [ ... ]
> > +	/* Zero out PWM for all other active channels to prevent them from blinking */
> > +	for (int i = 0; i < ST1202_MAX_LEDS; i++) {
> > +		if (!chip->leds[i].is_active || i == led->led_num)
> > +			continue;
> > +		ret = st1202_pwm_pattern_write(chip, i, 0, LED_OFF);
> 
> [Severity: High]
> Does this loop permanently overwrite the PWM configuration for other active
> LEDs?
> 
> If another LED was previously configured to be statically on, it looks like
> this forces it to be permanently dark. When the hardware blink is stopped,
> the original PWM configurations for these LEDs are never restored, which
> would leave them completely desynchronized from their software
> led_cdev->brightness state.
> 
> > +		if (ret)
> > +			return ret;
> > +		ret = st1202_pwm_pattern_write(chip, i, 1, LED_OFF);
> > +		if (ret)
> > +			return ret;
> > +	}
> [ ... ]
> > +	for (int pattern = 2; pattern < ST1202_MAX_PATTERNS; pattern++) {
> > +		ret = st1202_write_reg(chip, ST1202_PATTERN_DUR + pattern, 0);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	ret = st1202_write_reg(chip, ST1202_PATTERN_REP, U8_MAX);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num, U8_MAX);
> 
> [Severity: Medium]
> Does this force the LED to maximum brightness during a blink, ignoring the
> currently configured led_cdev->brightness or led_cdev->blink_brightness?
> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = __st1202_channel_set(chip, led->led_num, true);
> [ ... ]
> > @@ -301,6 +385,7 @@ static int st1202_dt_init(struct st1202_chip *chip)
> >  		led->led_cdev.pattern_set = st1202_led_pattern_set;
> >  		led->led_cdev.pattern_clear = st1202_led_pattern_clear;
> >  		led->led_cdev.default_trigger = "pattern";
> > +		led->led_cdev.blink_set = st1202_blink_set;
> >  		led->led_cdev.brightness_set = st1202_brightness_set;
> 
> [Severity: High]
> This is a pre-existing issue, but does assigning st1202_brightness_set() to
> the non-blocking brightness_set callback cause a "sleeping in atomic" panic?
> 
> The LED subsystem requires that brightness_set not sleep, as it is called
> from atomic contexts like software timers. Looking at
> st1202_brightness_set(), it takes a mutex and performs I2C operations which
> can sleep:
> 
> st1202_brightness_set()
>     guard(mutex)(&chip->lock);
>     ...
>     st1202_write_reg() -> i2c_smbus_write_byte_data()
> 
> Should this be assigned to brightness_set_blocking instead?
> 
> >  		led->led_cdev.brightness_get = st1202_brightness_get;
> >  	}
> 
> [Severity: High]
> This is a pre-existing issue, but I noticed a potential use-after-free
> relating to how led->fwnode is stored earlier in st1202_dt_init().
> 
> The driver iterates over child nodes using
> for_each_available_child_of_node_scoped(), which automatically decrements
> the child node's reference count at the end of each iteration.
> 
> Inside this loop, the fwnode is stored without incrementing its refcount:
> 
>     led->fwnode = of_fwnode_handle(child);
> 
> Since fwnode_handle_get() is not called, does led->fwnode become a dangling
> pointer when the loop finishes? This pointer is later passed to the LED
> core during registration in st1202_probe(), which could cause a UAF if the
> memory is reclaimed.
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/GV1PR08MB8497362791595D5C0197701DC5D22@GV1PR08MB8497.eurprd08.prod.outlook.com?part=1

-- 
Lee Jones