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

[sashiko-bot@kernel.org]

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()
--

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