Re: [PATCH v3 5/6] leds: aw2013: Prevent writes to unpowered chip

[Lee Jones <lee@kernel.org>]

On Tue, 13 Oct 2026, sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [High] leds: aw2013: Unconditional chip poweroff on I2C error breaks active LEDs
> - [Medium] leds: aw2013: lockless early return skips PWM clearing on shared regulators
> - [Medium] leds: aw2013: state desync if regulator_bulk_disable() fails
> - [Low] leds: aw2013: writes to unpowered chip in aw2013_blink_set()
> --

No more reviews until Sashko is clean or everything has been reasoned about.

[...]

> commit 6dcd66138416065d07df9a5558a059f701861e00
> Author: Paul Sajna <sajattack@postmarketos.org>
> 
>     leds: aw2013: Prevent writes to unpowered chip
> 
>     This patch replaces the chip_in_use check with a chip->enabled boolean in
>     aw2013_brightness_set to prevent writing to the chip when it is unpowered.
>     It also adds aw2013_chip_poweroff() to avoid sending I2C commands when
>     powering on the chip fails.
> 
> >     Because aw2013_chip_enable() is skipped, the driver then unconditionally
> >     calls regmap_write() and regmap_update_bits(), which perform I2C
> >     transactions to the unpowered chip.
> 
> [Severity: Low]
> Does this same issue still occur in aw2013_blink_set()?
> 
> If a prior aw2013_brightness_set() call fails (e.g., regulator enable fails),
> cdev->brightness can remain > 0 while led->chip->enabled is false.
> 
> A subsequent sysfs write to blink delays invokes aw2013_blink_set(), which
> sees brightness > 0, bypasses aw2013_brightness_set(), and unconditionally
> writes to the unpowered chip's registers:
> 
> 	/* Never off - brightness is already set, disable blinking */
> 	if (!*delay_off) {
> 		ret = regmap_update_bits(led->chip->regmap, AW2013_LCFG(num),
> 					 AW2013_LCFG_MD, 0);
> 		goto out;
> 	}
> 
> [ ... ]
> 
> > diff --git a/drivers/leds/leds-aw2013.c b/drivers/leds/leds-aw2013.c
> > index 3b2be960c04d5..6e661afd77755 100644
> > --- a/drivers/leds/leds-aw2013.c
> > +++ b/drivers/leds/leds-aw2013.c
> > @@ -193,15 +193,13 @@ static int aw2013_chip_init(struct aw2013 *chip)
> >  	return ret;
> >  }
> >  
> > -static void aw2013_chip_disable(struct aw2013 *chip)
> > +static void aw2013_chip_poweroff(struct aw2013 *chip)
> >  {
> >  	int ret;
> >  
> >  	if (!chip->enabled)
> >  		return;
> >  
> > -	regmap_write(chip->regmap, AW2013_GCR, 0);
> > -
> >  	ret = regulator_bulk_disable(ARRAY_SIZE(chip->regulators),
> >  				     chip->regulators);
> >  	if (ret) {
> >  		dev_err(&chip->client->dev,
> >  			"Failed to disable regulators: %d\n", ret);
> >  		return;
> >  	}
> >  
> >  	chip->enabled = false;
> >  }
> 
> [Severity: Medium]
> Could this early return cause a hardware state desynchronization if
> regulator_bulk_disable() fails?
> 
> During aw2013_chip_disable(), the driver first writes 0 to AW2013_GCR to
> enter software standby, then calls aw2013_chip_poweroff(). If the regulator
> disable fails here, it returns early leaving chip->enabled = true.
> 
> On subsequent brightness changes, aw2013_chip_enable() will see
> chip->enabled == true and skip aw2013_chip_init(). This skips restoring
> AW2013_GCR to 1, leaving the chip stuck in standby but completely
> unresponsive to brightness changes.
> 
> [ ... ]
> 
> > @@ -253,9 +261,12 @@ static int aw2013_brightness_set(struct led_classdev *cdev,
> >  	struct aw2013_led *led = container_of(cdev, struct aw2013_led, cdev);
> >  	int ret, num;
> >  
> > +	if (!led->chip->enabled && brightness == 0)
> > +		return 0;
> > +
> 
> [Severity: Medium]
> Can this lockless check introduce a TOCTOU race that skips clearing the
> LED's hardware registers?
> 
> If two LEDs are on and both are set to 0 concurrently, the LED core updates
> cdev->brightness = 0 for both before scheduling their asynchronous works.
> 
> The first worker executes aw2013_brightness_set(), acquires the mutex,
> clears its own PWM, and evaluates aw2013_chip_in_use(). Because both
> cdev->brightness values are now 0, aw2013_chip_in_use() returns false,
> causing the first worker to disable the chip (chip->enabled = false) and
> unlock the mutex.
> 
> The second worker then evaluates this new lockless check, which is now
> true, and returns early without ever taking the lock or clearing its PWM
> register over I2C.
> 
> When the regulators are shared with other devices, power is not physically
> cut, and the chip retains its register states in software standby. The next
> time the chip is enabled, will the second LED erroneously turn on because
> its registers were never cleared?
> 
> >  	mutex_lock(&led->chip->mutex);
> >  
> > -	if (aw2013_chip_in_use(led->chip)) {
> > +	if (!led->chip->enabled) {
> >  		ret = aw2013_chip_enable(led->chip);
> >  		if (ret)
> >  			goto error;
> 
> [ ... ]
> 
> > @@ -284,7 +295,12 @@ static int aw2013_brightness_set(struct led_classdev *cdev,
> >  	if (!aw2013_chip_in_use(led->chip))
> >  		aw2013_chip_disable(led->chip);
> >  
> > +	mutex_unlock(&led->chip->mutex);
> > +
> > +	return ret;
> > +
> >  error:
> > +	aw2013_chip_poweroff(led->chip);
> >  	mutex_unlock(&led->chip->mutex);
> >  
> >  	return ret;
> 
> [Severity: High]
> Will this unconditional poweroff disrupt the hardware state of all other
> active LEDs on the same chip?
> 
> If multiple LEDs are ON (so chip->enabled = true) and a transient I2C error
> occurs while changing the brightness of one LED, the code jumps to this
> error label.
> 
> The new aw2013_chip_poweroff() call unconditionally drops power to the
> entire chip. This shuts down all other physically active LEDs without
> updating their cdev->brightness in the LED core.
> 
> Should this error path only roll back aw2013_chip_enable() if it was the
> call that successfully enabled the chip on this specific invocation?
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261012-aw2013-aw20xx-rename-v3-0-cab1fdd050b6@postmarketos.org?part=5

-- 
Lee Jones