[PATCH ath-next] wifi: ath9k: release the LED GPIO after ath9k_stop()

[PATCH ath-next] wifi: ath9k: release the LED GPIO after ath9k_stop()

[Rosen Penev]

ath9k_deinit_device() frees the LED pin in ath_deinit_leds() before it
unregisters the hw. ath9k_stop() then switches the LED pin back to an
input with ath9k_hw_gpio_request_in(). For a SoC GPIO outside of the
WMAC, ath9k_hw_gpio_request() takes a new descriptor through
gpiod_get_index() at that point, and nothing releases it again. The
next probe fails with "request GPIO12 failed:-16" and every LED update
hits the WARN_ON() in ath9k_hw_set_gpio(). That request also runs
under sc->sc_pcu_lock, while gpiod_get_index() and
gpiod_set_consumer_name() may sleep.

Free the LED pin after ieee80211_unregister_hw() instead. This also
frees the pin when the LED class device could not be registered,
which leaked it before.

Seen on a TP-Link Archer C7 v2 (QCA9558, LED on SoC GPIO 12) when
reloading ath9k.

Fixes: e34f2ff40e03 ("ath9k: bring back direction setting in ath9k_{start_stop}")
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/net/wireless/ath/ath9k/gpio.c | 2 --
 drivers/net/wireless/ath/ath9k/init.c | 5 +++++
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/gpio.c b/drivers/net/wireless/ath/ath9k/gpio.c
index 2dbc7efdd637..d525d8ef9369 100644
--- a/drivers/net/wireless/ath/ath9k/gpio.c
+++ b/drivers/net/wireless/ath/ath9k/gpio.c
@@ -67,8 +67,6 @@ void ath_deinit_leds(struct ath_softc *sc)
 
 	ath_led_brightness(&sc->led_cdev, LED_OFF);
 	led_classdev_unregister(&sc->led_cdev);
-
-	ath9k_hw_gpio_free(sc->sc_ah, sc->sc_ah->led_pin);
 }
 
 void ath_init_leds(struct ath_softc *sc)
diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c
index a7a11781d83d..f6aa2ec1683c 100644
--- a/drivers/net/wireless/ath/ath9k/init.c
+++ b/drivers/net/wireless/ath/ath9k/init.c
@@ -1128,6 +1128,11 @@ void ath9k_deinit_device(struct ath_softc *sc)
 	ath9k_deinit_debug(sc);
 	ath9k_deinit_wow(hw);
 	ieee80211_unregister_hw(hw);
+
+	/* release the LED pin only after ath9k_stop() has configured it */
+	if (sc->sc_ah->led_pin >= 0)
+		ath9k_hw_gpio_free(sc->sc_ah, sc->sc_ah->led_pin);
+
 	ath_rx_cleanup(sc);
 	ath9k_deinit_softc(sc);
 }
-- 
2.56.0

Re: [PATCH ath-next] wifi: ath9k: release the LED GPIO after ath9k_stop()

[Toke Høiland-Jørgensen]

Rosen Penev <rosenp@gmail.com> writes:

> ath9k_deinit_device() frees the LED pin in ath_deinit_leds() before it
> unregisters the hw. ath9k_stop() then switches the LED pin back to an
> input with ath9k_hw_gpio_request_in(). For a SoC GPIO outside of the
> WMAC, ath9k_hw_gpio_request() takes a new descriptor through
> gpiod_get_index() at that point, and nothing releases it again. The
> next probe fails with "request GPIO12 failed:-16" and every LED update
> hits the WARN_ON() in ath9k_hw_set_gpio(). That request also runs
> under sc->sc_pcu_lock, while gpiod_get_index() and
> gpiod_set_consumer_name() may sleep.
>
> Free the LED pin after ieee80211_unregister_hw() instead. This also
> frees the pin when the LED class device could not be registered,
> which leaked it before.

Hmm, instead of reworking the deinit code, how about we just drop the
ath9k_hw_gpio_request_in() call from ath9k_stop()? It's an LED pin, why
would it ever need to be reset? The commit that introduced that call
says it was "for consistency", which seems entirely bogus in this case?

-Toke

Re: [PATCH ath-next] wifi: ath9k: release the LED GPIO after ath9k_stop()

[Rosen Penev]

On Fri, Oct 23, 2026 at 8:30 AM Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>
> Rosen Penev <rosenp@gmail.com> writes:
>
> > ath9k_deinit_device() frees the LED pin in ath_deinit_leds() before it
> > unregisters the hw. ath9k_stop() then switches the LED pin back to an
> > input with ath9k_hw_gpio_request_in(). For a SoC GPIO outside of the
> > WMAC, ath9k_hw_gpio_request() takes a new descriptor through
> > gpiod_get_index() at that point, and nothing releases it again. The
> > next probe fails with "request GPIO12 failed:-16" and every LED update
> > hits the WARN_ON() in ath9k_hw_set_gpio(). That request also runs
> > under sc->sc_pcu_lock, while gpiod_get_index() and
> > gpiod_set_consumer_name() may sleep.
> >
> > Free the LED pin after ieee80211_unregister_hw() instead. This also
> > frees the pin when the LED class device could not be registered,
> > which leaked it before.
>
> Hmm, instead of reworking the deinit code, how about we just drop the
> ath9k_hw_gpio_request_in() call from ath9k_stop()? It's an LED pin, why
> would it ever need to be reset? The commit that introduced that call
> says it was "for consistency", which seems entirely bogus in this case?
Why would it be requested and then not? I imagine on rebind it would
error out in such a case.
>
> -Toke

Re: [PATCH ath-next] wifi: ath9k: release the LED GPIO after ath9k_stop()

[Toke Høiland-Jørgensen]

Rosen Penev <rosenp@gmail.com> writes:

> On Fri, Oct 23, 2026 at 8:30 AM Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>>
>> Rosen Penev <rosenp@gmail.com> writes:
>>
>> > ath9k_deinit_device() frees the LED pin in ath_deinit_leds() before it
>> > unregisters the hw. ath9k_stop() then switches the LED pin back to an
>> > input with ath9k_hw_gpio_request_in(). For a SoC GPIO outside of the
>> > WMAC, ath9k_hw_gpio_request() takes a new descriptor through
>> > gpiod_get_index() at that point, and nothing releases it again. The
>> > next probe fails with "request GPIO12 failed:-16" and every LED update
>> > hits the WARN_ON() in ath9k_hw_set_gpio(). That request also runs
>> > under sc->sc_pcu_lock, while gpiod_get_index() and
>> > gpiod_set_consumer_name() may sleep.
>> >
>> > Free the LED pin after ieee80211_unregister_hw() instead. This also
>> > frees the pin when the LED class device could not be registered,
>> > which leaked it before.
>>
>> Hmm, instead of reworking the deinit code, how about we just drop the
>> ath9k_hw_gpio_request_in() call from ath9k_stop()? It's an LED pin, why
>> would it ever need to be reset? The commit that introduced that call
>> says it was "for consistency", which seems entirely bogus in this case?
> Why would it be requested and then not? I imagine on rebind it would
> error out in such a case.

No, I don't think so?

The current code does ath9k_hw_gpio_request_out() on init, and
ath9k_hw_gpio_free() on deinit. That's nicely paired, and only breaks
because ath9k_stop() for some inexplicable reason does another
ath9k_hw_gpio_request_in(). Which there's no reason to do; the pin is
never used as an input, and doing so would not make any sense anyway
when there's an LED attached to it...

In fact, the code moving you're doing in your patch ends up doing
exactly the same thing as just removing that call. Right now the
sequence is:

ath9k_hw_gpio_free() /* frees the 'out' pin and sets ah->gpiods[gpio] = NULL */
ath9k_hw_gpio_request_in() /* succeeds and re-allocates ah->gpiods[gpio] */

but with your change it becomes:

ath9k_hw_gpio_request_in() /* exits as no-op because ah->gpiods[gpio] is non-NULL */
ath9k_hw_gpio_free() /* frees the 'out' pin and sets ah->gpiods[gpio] = NULL */

so we'll end up with exactly the same thing happening as if we just got
rid of ath9k_hw_gpio_request_in(), but the LED init/deinit code will be
decoupled in a weird way.

-Toke