Re: [PATCH] leds: gpio: Skip unavailable LEDs during shutdown

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The platform data probe path silently skips LEDs when GPIO lookup returns -EPROBE_DEFER, breaking deferred probing.
- [High] Teardown race: gpio_led_shutdown fails to disable software blink timers and active triggers, allowing concurrent LED updates during system shutdown.
- [Low] The driver does not use dev_err_probe() to handle and report probe failures.
- [Low] The driver uses generic names 'priv' and 'gpio_leds_priv' for its private data, violating subsystem naming conventions.
--

--- Patch [1]: [PATCH] leds: gpio: Skip unavailable LEDs during shutdown ---
commit 94da8744ab857bc87f8158e7c51602a324f2f403
Author: Steve Dunnagan <sdunnaga@redhat.com>
Subject: leds: gpio: Skip unavailable LEDs during shutdown

This commit fixes an issue in gpio_led_shutdown() where an error-valued
GPIO descriptor could be passed to gpio_led_set(). It avoids a potential
invalid GPIO operation by ensuring we skip unavailable LEDs during the
shutdown process.

> diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
> index ce4e79939731d..6a2f2386978d7 100644
> --- a/drivers/leds/leds-gpio.c
> +++ b/drivers/leds/leds-gpio.c
> @@ -290,6 +290,9 @@ static void gpio_led_shutdown(struct platform_device *pdev)
>  	for (i = 0; i < priv->num_leds; i++) {

[Severity: Low]
This is a pre-existing issue, but does the use of the generic "priv" variable
name violate subsystem naming conventions?

The LED subsystem guidelines prefer using the device name and "ddata" for
private data structures and variables rather than generic names like "priv" or
"gpio_leds_priv".

>  		struct gpio_led_data *led = &priv->leds[i];
>  
> +		if (IS_ERR(led->gpiod))
> +			continue;
> +
>  		if (!(led->cdev.flags & LED_RETAIN_AT_SHUTDOWN))
>  			gpio_led_set(&led->cdev, LED_OFF);

[Severity: High]
This isn't a bug introduced by this patch, but does gpio_led_shutdown() fail
to disable software blink timers and active triggers?

While the physical GPIO is turned off here, if a heartbeat or timer trigger
fires during the device shutdown phase (while interrupts are still active), it
could queue a work to change the LED brightness. If the underlying GPIO
controller or sleepable bus is already shut down, could this concurrent access
cause a crash or bus lockup?

>  	}

While reviewing this file, I noticed a couple of other pre-existing issues in
gpio_led_probe().

[Severity: High]
This is a pre-existing issue, but does gpio_led_probe() silently swallow
probe deferrals?

drivers/leds/leds-gpio.c:gpio_led_probe() {
    ...
			if (IS_ERR(led_dat->gpiod)) {
				dev_info(&pdev->dev, "Skipping unavailable LED gpio %d (%s)\n",
					 template->gpio, template->name);
				continue;
			}
    ...
}

If the GPIO lookup returns -EPROBE_DEFER (for example, if the controller
probes later), this catch-all check logs a generic message and skips the LED
instead of propagating the error. Will this cause the LED to be permanently
missing since the kernel won't re-attempt probing?

[Severity: Low]
This is another pre-existing issue, but should gpio_led_probe() use
dev_err_probe() to handle failures?

drivers/leds/leds-gpio.c:gpio_led_probe() {
    ...
		priv = gpio_leds_create(pdev);
		if (IS_ERR(priv))
			return PTR_ERR(priv);
    ...
}

Errors originating from gpio_leds_create() are returned directly. Subsystem
guidelines recommend always using dev_err_probe() to report probe failures
and correctly handle -EPROBE_DEFER.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710150841.19669-1-sdunnaga@redhat.com?part=1