Re: [PATCH v2 6/7] leds: aw2013: Prevent writes to unpowered chip

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] leds: aw2013: unnecessary power cycle when setting brightness to 0
- [Medium] leds: aw2013: regulator leak on I2C error during turn-off
--

commit f162a4969ac4532fe0ad6e583070bbdf610e7263
Author: Paul Sajna <sajattack@postmarketos.org>

leds: aw2013: Prevent writes to unpowered chip

This commit replaces the chip_in_use check with a chip->enabled boolean
evaluation in aw20xx_brightness_set(). It aims to prevent the driver from
unconditionally calling I2C transactions to an unpowered chip when the LED
core initializes the brightness to 0 during probe, teardown, or
unregistration.

> diff --git a/drivers/leds/leds-aw2013.c b/drivers/leds/leds-aw2013.c
> --- a/drivers/leds/leds-aw2013.c
> +++ b/drivers/leds/leds-aw2013.c
> @@ -257,7 +257,7 @@ static int aw20xx_brightness_set(struct led_classdev *cdev,
>  
>  	mutex_lock(&led->chip->mutex);
>  
> -	if (aw20xx_chip_in_use(led->chip)) {
> +	if (!led->chip->enabled) {
>  		ret = aw20xx_chip_enable(led->chip);
>  		if (ret)
>  			goto error;
>  	}

[Severity: Medium]
Does this introduce an unnecessary power cycle when the brightness is set to 0?

By unconditionally enabling the chip if it is currently disabled, the
driver will incur the performance penalty of powering up regulators and
performing I2C initialization, only to immediately write a 0 and power
down again. This happens during probe, teardown, or when userspace writes
0 to an already off LED.

[ ... ]

>  	num = led->num;
>  
>  	ret = regmap_write(led->chip->regmap, AW20XX_REG_PWM(num), brightness);
>  	if (ret)
>  		goto error;

[Severity: Medium]
Does this leak the regulator enable if the I2C write fails?

If the chip was previously unpowered and was just enabled by the new
check above, an I2C failure here jumps directly to the error label:

aw20xx_brightness_set()
    ...
    error:
        mutex_unlock(&led->chip->mutex);
        return ret;

This bypasses aw20xx_chip_disable(), leaving the regulators permanently
enabled despite the LED logically being off.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261005-aw2013-aw20xx-rename-v2-0-108ecbdf2775@postmarketos.org?part=6