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

["Toke Høiland-Jørgensen" <toke@toke.dk>]

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