Re: [PATCH AUTOSEL 6.18-6.12] leds: trigger: gpio: Use GPIOD_FLAGS_BIT_NONEXCLUSIVE

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Using GPIOD_FLAGS_BIT_NONEXCLUSIVE causes premature hardware un-configuration and refcount underflow when the shared GPIO is released, breaking the primary owner.
- [High] The patch fails to achieve its stated goal (supporting SFP shared GPIOs) because the SFP driver does not share IRQs, causing activation to always fail with -EBUSY.
- [Medium] The LED trigger driver blindly overwrites the shared GPIO's consumer name, erasing the primary driver's label.
--

commit 02d3bd93d29878c57dc994ff5d3e680adeef7f2e
Author: Piotr Kubik <piotr@kubik.pl>
Subject: leds: trigger: gpio: Use GPIOD_FLAGS_BIT_NONEXCLUSIVE

This commit adds the GPIOD_FLAGS_BIT_NONEXCLUSIVE flag when requesting
the GPIO in the LED trigger driver. This is intended to allow the trigger
to share a GPIO with another driver, such as an SFP module.

Link: https://patch.msgid.link/20260408115106.379834-1-piotr@kubik.pl

> diff --git a/drivers/leds/trigger/ledtrig-gpio.c b/drivers/leds/trigger/ledtrig-gpio.c
> index 7f6a2352b0acd..3199275fd250b 100644
> --- a/drivers/leds/trigger/ledtrig-gpio.c
> +++ b/drivers/leds/trigger/ledtrig-gpio.c
> @@ -86,7 +86,8 @@ static int gpio_trig_activate(struct led_classdev *led)
>  	 * The generic property "trigger-sources" is followed,
>  	 * and we hope that this is a GPIO.
>  	 */
> -	gpio_data->gpiod = gpiod_get_optional(dev, "trigger-sources", GPIOD_IN);
> +	gpio_data->gpiod = gpiod_get_optional(dev, "trigger-sources",
> +					      GPIOD_IN | GPIOD_FLAGS_BIT_NONEXCLUSIVE);

[Severity: High]
Does acquiring this GPIO non-exclusively expose the hardware state to being
inadvertently destroyed?

Because gpiolib does not implement reference counting for non-exclusive
consumers, any call to gpiod_put() will execute gpiod_free_commit():

drivers/gpio/gpiolib.c:gpiod_free_commit() {
    ...
    clear_bit(GPIOD_FLAG_REQUESTED, &flags);
    ...
}

This unconditionally frees the GPIO hardware state. Since gpio_trig_activate()
calls gpiod_put() on its error path and during deactivation, won't this tear
down the primary owner's (e.g., the SFP driver's) hardware state?


[Severity: High]
Will this IRQ sharing strategy actually work with the SFP driver?

In gpio_trig_activate(), the LED trigger requests the IRQ with IRQF_SHARED:

    ret = request_threaded_irq(gpiod_to_irq(gpio_data->gpiod), NULL,
                    gpio_trig_irq,
                    IRQF_ONESHOT | IRQF_SHARED | IRQF_TRIGGER_RISING
                    | IRQF_TRIGGER_FALLING, "ledtrig-gpio", led);

However, the primary SFP driver requests the same IRQ exclusively:

drivers/net/phy/sfp.c:sfp_probe() {
    ...
    err = devm_request_threaded_irq(sfp->dev, sfp->gpio_irq[i],
                                    NULL, sfp_irq,
                                    IRQF_ONESHOT |
                                    IRQF_TRIGGER_RISING |
                                    IRQF_TRIGGER_FALLING,
                                    sfp_irq_name, sfp);
    ...
}

Since the kernel's IRQ core strictly requires all consumers to use IRQF_SHARED
for sharing to succeed, won't request_threaded_irq() always return -EBUSY? 

If this always fails, it will trigger the error path that calls gpiod_put(),
which leads to the destructive GPIO teardown mentioned above.


[Severity: Medium]
Could this overwrite the original owner's consumer label?

Right after acquiring the GPIO, the trigger driver forcibly sets the
consumer name:

    gpiod_set_consumer_name(gpio_data->gpiod, "led-trigger");

The underlying gpiolib implementation completely overwrites and frees the
old label:

drivers/gpio/gpiolib.c:desc_set_label() {
    ...
    old = rcu_replace_pointer(desc->label, new, 1);
    if (old)
        call_srcu(&desc->gdev->desc_srcu, &old->rh, desc_free_label);
    ...
}

Won't this erase the primary driver's label and corrupt the consumer
tracking for debugging interfaces like debugfs and sysfs? Furthermore, if the
LED trigger fails or deactivates, won't desc_set_label() clear the label
entirely to NULL, permanently masking the primary driver's ownership?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831133314.4125787-347-sashal@kernel.org?part=1