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

From: Kalle Valo <kvalo@codeaurora.org>
To: Arnd Bergmann <arnd@kernel.org>
Cc: Krzysztof Kozlowski <krzk@kernel.org>,
	QCA ath9k Development <ath9k-devel@qca.qualcomm.com>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Johannes Berg <johannes@sipsolutions.net>,
	Arnd Bergmann <arnd@arndb.de>,
	Masahiro Yamada <masahiroy@kernel.org>,
	Flavio Suligoi <f.suligoi@asem.it>,
	linux-wireless <linux-wireless@vger.kernel.org>,
	Networking <netdev@vger.kernel.org>,
	"linux-kernel\@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] ath9k: fix build error with LEDS_CLASS=m
Date: Wed, 27 Jan 2021 12:35:37 +0200	[thread overview]
Message-ID: <87bldaacqu.fsf@codeaurora.org> (raw)
In-Reply-To: <CAK8P3a3ygYTEwjLbFuArdfNF1-yydVjtS2NZDAURKjOJGAxkAQ@mail.gmail.com> (Arnd Bergmann's message of "Mon, 25 Jan 2021 16:22:18 +0100")

Arnd Bergmann <arnd@kernel.org> writes:

> On Mon, Jan 25, 2021 at 4:04 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>> On Mon, 25 Jan 2021 at 15:38, Arnd Bergmann <arnd@kernel.org> wrote:
>> > On Mon, Jan 25, 2021 at 2:27 PM Krzysztof Kozlowski <krzk@kernel.org> 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.
>
>> 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