Re: [PATCH] ath9k: fix build error with LEDS_CLASS=m

Arnd Bergmann [off-list ref] writes:

On Mon, Jan 25, 2021 at 4:04 PM Krzysztof Kozlowski [off-list ref] wrote:

quoted

On Mon, 25 Jan 2021 at 15:38, Arnd Bergmann [off-list ref] wrote:

quoted

On Mon, Jan 25, 2021 at 2:27 PM Krzysztof Kozlowski [off-list ref] wrote:

I meant that having MAC80211_LEDS selected causes the ath9k driver to
toggle on/off the WiFi LED. Every second, regardless whether it's
doing something or not. In my setup, I have problems with a WiFi
dongle somehow crashing (WiFi disappears, nothing comes from the
dongle... maybe it's Atheros FW, maybe some HW problem) and I found
this LED on/off slightly increases the chances of this dongle-crash.
That was the actual reason behind my commits.

Second reason is that I don't want to send USB commands every second
when the device is idle. It unnecessarily consumes power on my
low-power device.

Ok, I see.

quoted

Of course another solution is to just disable the trigger via sysfs
LED API. It would also work but my patch allows entire code to be
compiled-out (which was conditional in ath9k already).

Therefore the patch I sent allows the ath9k LED option to be fully
choosable. Someone wants every-second-LED-blink, sure, enable
ATH9K_LEDS and you have it. Someone wants to reduce the kernel size,
don't enable ATH9K_LEDS.

Originally, I think this is what CONFIG_MAC80211_LEDS was meant
for, but it seems that this is not actually practical, since this also
gets selected by half of the drivers using it, while the other half have
a dependency on it. Out of the ones that select it, some in turn
select LEDS_CLASS, while some depend on it.

I think this needs a larger-scale cleanup for consistency between
(at least) all the wireless drivers using LEDs.

I agree, this needs cleanup.

Either your patch or mine should get applied in the meantime, and I
don't care much which one in this case, as we still have the remaining
inconsistency.

My problem with Krzysztof's patch[1] is that it adds a new Kconfig
option for ath9k, is that really necessary? Like Arnd said, we should
fix drivers to use CONFIG_MAC80211_LEDS instead of having driver
specific options.

So I would prefer take this Arnd's patch instead and queue it for v5.11.
But as it modifies mac80211 I'll need an ack from Johannes, what do you
think?

[1] https://patchwork.kernel.org/project/linux-wireless/patch/20201227143034.1134829-1-krzk@kernel.org/

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

On Wed, 2021-01-27 at 12:35 +0200, Kalle Valo wrote:

Arnd Bergmann [off-list ref] writes:

quoted

On Mon, Jan 25, 2021 at 4:04 PM Krzysztof Kozlowski [off-list ref] wrote:

quoted

On Mon, 25 Jan 2021 at 15:38, Arnd Bergmann [off-list ref] wrote:

quoted

On Mon, Jan 25, 2021 at 2:27 PM Krzysztof Kozlowski [off-list ref] wrote:

I meant that having MAC80211_LEDS selected causes the ath9k driver to
toggle on/off the WiFi LED. Every second, regardless whether it's
doing something or not. In my setup, I have problems with a WiFi
dongle somehow crashing (WiFi disappears, nothing comes from the
dongle... maybe it's Atheros FW, maybe some HW problem) and I found
this LED on/off slightly increases the chances of this dongle-crash.
That was the actual reason behind my commits.

Second reason is that I don't want to send USB commands every second
when the device is idle. It unnecessarily consumes power on my
low-power device.

Ok, I see.

quoted

Of course another solution is to just disable the trigger via sysfs
LED API. It would also work but my patch allows entire code to be
compiled-out (which was conditional in ath9k already).

Therefore the patch I sent allows the ath9k LED option to be fully
choosable. Someone wants every-second-LED-blink, sure, enable
ATH9K_LEDS and you have it. Someone wants to reduce the kernel size,
don't enable ATH9K_LEDS.

Originally, I think this is what CONFIG_MAC80211_LEDS was meant
for, but it seems that this is not actually practical, since this also
gets selected by half of the drivers using it, while the other half have
a dependency on it. Out of the ones that select it, some in turn
select LEDS_CLASS, while some depend on it.

I think this needs a larger-scale cleanup for consistency between
(at least) all the wireless drivers using LEDs.

I agree, this needs cleanup.

quoted

Either your patch or mine should get applied in the meantime, and I
don't care much which one in this case, as we still have the remaining
inconsistency.

My problem with Krzysztof's patch[1] is that it adds a new Kconfig
option for ath9k, is that really necessary? Like Arnd said, we should
fix drivers to use CONFIG_MAC80211_LEDS instead of having driver
specific options.

So I would prefer take this Arnd's patch instead and queue it for v5.11.
But as it modifies mac80211 I'll need an ack from Johannes, what do you
think?

Sure, that seems fine.

Acked-by: Johannes Berg <johannes@sipsolutions.net>

johannes