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

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

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

When CONFIG_ATH9K is built-in but LED support is in a loadable
module, both ath9k drivers fails to link:

x86_64-linux-ld: drivers/net/wireless/ath/ath9k/gpio.o: in function `ath_deinit_leds':
gpio.c:(.text+0x36): undefined reference to `led_classdev_unregister'
x86_64-linux-ld: drivers/net/wireless/ath/ath9k/gpio.o: in function `ath_init_leds':
gpio.c:(.text+0x179): undefined reference to `led_classdev_register_ext'

The problem is that the 'imply' keyword does not enforce any dependency
but is only a weak hint to Kconfig to enable another symbol from a
defconfig file.

Change imply to a 'depends on LEDS_CLASS' that prevents the incorrect
configuration but still allows building the driver without LED support.

The 'select MAC80211_LEDS' is now ensures that the LED support is
actually used if it is present, and the added Kconfig dependency
on MAC80211_LEDS ensures that it cannot be enabled manually when it
has no effect.

Fixes: 197f466e93f5 ("ath9k_htc: Do not select MAC80211_LEDS by default")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/net/wireless/ath/ath9k/Kconfig | 8 ++------
 net/mac80211/Kconfig                   | 2 +-
 2 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/Kconfig b/drivers/net/wireless/ath/ath9k/Kconfig
index a84bb9b6573f..e150d82eddb6 100644
--- a/drivers/net/wireless/ath/ath9k/Kconfig
+++ b/drivers/net/wireless/ath/ath9k/Kconfig
@@ -21,11 +21,9 @@ config ATH9K_BTCOEX_SUPPORT
 config ATH9K
 	tristate "Atheros 802.11n wireless cards support"
 	depends on MAC80211 && HAS_DMA
+	select MAC80211_LEDS if LEDS_CLASS=y || LEDS_CLASS=MAC80211
 	select ATH9K_HW
 	select ATH9K_COMMON
-	imply NEW_LEDS
-	imply LEDS_CLASS
-	imply MAC80211_LEDS
 	help
 	  This module adds support for wireless adapters based on
 	  Atheros IEEE 802.11n AR5008, AR9001 and AR9002 family
@@ -176,11 +174,9 @@ config ATH9K_PCI_NO_EEPROM
 config ATH9K_HTC
 	tristate "Atheros HTC based wireless cards support"
 	depends on USB && MAC80211
+	select MAC80211_LEDS if LEDS_CLASS=y || LEDS_CLASS=MAC80211
 	select ATH9K_HW
 	select ATH9K_COMMON
-	imply NEW_LEDS
-	imply LEDS_CLASS
-	imply MAC80211_LEDS
 	help
 	  Support for Atheros HTC based cards.
 	  Chipsets supported: AR9271
diff --git a/net/mac80211/Kconfig b/net/mac80211/Kconfig
index cd9a9bd242ba..51ec8256b7fa 100644
--- a/net/mac80211/Kconfig
+++ b/net/mac80211/Kconfig
@@ -69,7 +69,7 @@ config MAC80211_MESH
 config MAC80211_LEDS
 	bool "Enable LED triggers"
 	depends on MAC80211
-	depends on LEDS_CLASS
+	depends on LEDS_CLASS=y || LEDS_CLASS=MAC80211
 	select LEDS_TRIGGERS
 	help
 	  This option enables a few LED triggers for different
-- 
2.29.2

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

[Krzysztof Kozlowski]

On Mon, 25 Jan 2021 at 12:36, Arnd Bergmann <arnd@kernel.org> wrote:
>
> From: Arnd Bergmann <arnd@arndb.de>
>
> When CONFIG_ATH9K is built-in but LED support is in a loadable
> module, both ath9k drivers fails to link:
>
> x86_64-linux-ld: drivers/net/wireless/ath/ath9k/gpio.o: in function `ath_deinit_leds':
> gpio.c:(.text+0x36): undefined reference to `led_classdev_unregister'
> x86_64-linux-ld: drivers/net/wireless/ath/ath9k/gpio.o: in function `ath_init_leds':
> gpio.c:(.text+0x179): undefined reference to `led_classdev_register_ext'
>
> The problem is that the 'imply' keyword does not enforce any dependency
> but is only a weak hint to Kconfig to enable another symbol from a
> defconfig file.
>
> Change imply to a 'depends on LEDS_CLASS' that prevents the incorrect
> configuration but still allows building the driver without LED support.
>
> The 'select MAC80211_LEDS' is now ensures that the LED support is
> actually used if it is present, and the added Kconfig dependency
> on MAC80211_LEDS ensures that it cannot be enabled manually when it
> has no effect.

But we do not want to have this dependency (selecting MAC80211_LEDS).
I fixed this problem here:
https://lore.kernel.org/lkml/20201227143034.1134829-1-krzk@kernel.org/
Maybe let's take this approach?

Best regards,
Krzysztof

>
> Fixes: 197f466e93f5 ("ath9k_htc: Do not select MAC80211_LEDS by default")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/net/wireless/ath/ath9k/Kconfig | 8 ++------
>  net/mac80211/Kconfig                   | 2 +-
>  2 files changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath9k/Kconfig b/drivers/net/wireless/ath/ath9k/Kconfig
> index a84bb9b6573f..e150d82eddb6 100644
> --- a/drivers/net/wireless/ath/ath9k/Kconfig
> +++ b/drivers/net/wireless/ath/ath9k/Kconfig
> @@ -21,11 +21,9 @@ config ATH9K_BTCOEX_SUPPORT
>  config ATH9K
>         tristate "Atheros 802.11n wireless cards support"
>         depends on MAC80211 && HAS_DMA
> +       select MAC80211_LEDS if LEDS_CLASS=y || LEDS_CLASS=MAC80211
>         select ATH9K_HW
>         select ATH9K_COMMON
> -       imply NEW_LEDS
> -       imply LEDS_CLASS
> -       imply MAC80211_LEDS
>         help
>           This module adds support for wireless adapters based on
>           Atheros IEEE 802.11n AR5008, AR9001 and AR9002 family
> @@ -176,11 +174,9 @@ config ATH9K_PCI_NO_EEPROM
>  config ATH9K_HTC
>         tristate "Atheros HTC based wireless cards support"
>         depends on USB && MAC80211
> +       select MAC80211_LEDS if LEDS_CLASS=y || LEDS_CLASS=MAC80211
>         select ATH9K_HW
>         select ATH9K_COMMON
> -       imply NEW_LEDS
> -       imply LEDS_CLASS
> -       imply MAC80211_LEDS
>         help
>           Support for Atheros HTC based cards.
>           Chipsets supported: AR9271
> diff --git a/net/mac80211/Kconfig b/net/mac80211/Kconfig
> index cd9a9bd242ba..51ec8256b7fa 100644
> --- a/net/mac80211/Kconfig
> +++ b/net/mac80211/Kconfig
> @@ -69,7 +69,7 @@ config MAC80211_MESH
>  config MAC80211_LEDS
>         bool "Enable LED triggers"
>         depends on MAC80211
> -       depends on LEDS_CLASS
> +       depends on LEDS_CLASS=y || LEDS_CLASS=MAC80211
>         select LEDS_TRIGGERS
>         help
>           This option enables a few LED triggers for different
> --
> 2.29.2
>

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

[Arnd Bergmann]

On Mon, Jan 25, 2021 at 12:40 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On Mon, 25 Jan 2021 at 12:36, Arnd Bergmann <arnd@kernel.org> wrote:
> >
> > From: Arnd Bergmann <arnd@arndb.de>
> >
> > When CONFIG_ATH9K is built-in but LED support is in a loadable
> > module, both ath9k drivers fails to link:
> >
> > x86_64-linux-ld: drivers/net/wireless/ath/ath9k/gpio.o: in function `ath_deinit_leds':
> > gpio.c:(.text+0x36): undefined reference to `led_classdev_unregister'
> > x86_64-linux-ld: drivers/net/wireless/ath/ath9k/gpio.o: in function `ath_init_leds':
> > gpio.c:(.text+0x179): undefined reference to `led_classdev_register_ext'
> >
> > The problem is that the 'imply' keyword does not enforce any dependency
> > but is only a weak hint to Kconfig to enable another symbol from a
> > defconfig file.
> >
> > Change imply to a 'depends on LEDS_CLASS' that prevents the incorrect
> > configuration but still allows building the driver without LED support.
> >
> > The 'select MAC80211_LEDS' is now ensures that the LED support is
> > actually used if it is present, and the added Kconfig dependency
> > on MAC80211_LEDS ensures that it cannot be enabled manually when it
> > has no effect.
>
> But we do not want to have this dependency (selecting MAC80211_LEDS).
> I fixed this problem here:
> https://lore.kernel.org/lkml/20201227143034.1134829-1-krzk@kernel.org/
> Maybe let's take this approach?

Generally speaking, I don't like to have a device driver specific Kconfig
setting 'select' a subsystem', for two reasons:

- you suddenly get asked for tons of new LED specific options when
  enabling seemingly benign options

- Mixing 'depends on' and 'select' leads to bugs with circular
  dependencies that usually require turning some other 'select'
  into 'depends on'.

The problem with LEDS_CLASS in particular is that there is a mix of drivers
using one vs the other roughly 50:50.

      Arnd

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

[Krzysztof Kozlowski]

On Mon, 25 Jan 2021 at 14:09, Arnd Bergmann <arnd@kernel.org> wrote:
>
> On Mon, Jan 25, 2021 at 12:40 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > On Mon, 25 Jan 2021 at 12:36, Arnd Bergmann <arnd@kernel.org> wrote:
> > >
> > > From: Arnd Bergmann <arnd@arndb.de>
> > >
> > > When CONFIG_ATH9K is built-in but LED support is in a loadable
> > > module, both ath9k drivers fails to link:
> > >
> > > x86_64-linux-ld: drivers/net/wireless/ath/ath9k/gpio.o: in function `ath_deinit_leds':
> > > gpio.c:(.text+0x36): undefined reference to `led_classdev_unregister'
> > > x86_64-linux-ld: drivers/net/wireless/ath/ath9k/gpio.o: in function `ath_init_leds':
> > > gpio.c:(.text+0x179): undefined reference to `led_classdev_register_ext'
> > >
> > > The problem is that the 'imply' keyword does not enforce any dependency
> > > but is only a weak hint to Kconfig to enable another symbol from a
> > > defconfig file.
> > >
> > > Change imply to a 'depends on LEDS_CLASS' that prevents the incorrect
> > > configuration but still allows building the driver without LED support.
> > >
> > > The 'select MAC80211_LEDS' is now ensures that the LED support is
> > > actually used if it is present, and the added Kconfig dependency
> > > on MAC80211_LEDS ensures that it cannot be enabled manually when it
> > > has no effect.
> >
> > But we do not want to have this dependency (selecting MAC80211_LEDS).
> > I fixed this problem here:
> > https://lore.kernel.org/lkml/20201227143034.1134829-1-krzk@kernel.org/
> > Maybe let's take this approach?
>
> Generally speaking, I don't like to have a device driver specific Kconfig
> setting 'select' a subsystem', for two reasons:
>
> - you suddenly get asked for tons of new LED specific options when
>   enabling seemingly benign options
>
> - Mixing 'depends on' and 'select' leads to bugs with circular
>   dependencies that usually require turning some other 'select'
>   into 'depends on'.
>
> The problem with LEDS_CLASS in particular is that there is a mix of drivers
> using one vs the other roughly 50:50.

Yes, you are right, I also don't like it. However it was like this
before my commit so I am not introducing a new issue. The point is
that in your choice the MAC80211_LEDS will be selected if LEDS_CLASS
is present, which is exactly what I was trying to fix/remove. My WiFi
dongle does not have a LED and it causes a periodic (every second)
event. However I still have LEDS_CLASS for other LEDS in the system.

Best regards,
Krzysztof

On Mon, 25 Jan 2021 at 14:09, Arnd Bergmann <arnd@kernel.org> wrote:
>
> On Mon, Jan 25, 2021 at 12:40 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > On Mon, 25 Jan 2021 at 12:36, Arnd Bergmann <arnd@kernel.org> wrote:
> > >
> > > From: Arnd Bergmann <arnd@arndb.de>
> > >
> > > When CONFIG_ATH9K is built-in but LED support is in a loadable
> > > module, both ath9k drivers fails to link:
> > >
> > > x86_64-linux-ld: drivers/net/wireless/ath/ath9k/gpio.o: in function `ath_deinit_leds':
> > > gpio.c:(.text+0x36): undefined reference to `led_classdev_unregister'
> > > x86_64-linux-ld: drivers/net/wireless/ath/ath9k/gpio.o: in function `ath_init_leds':
> > > gpio.c:(.text+0x179): undefined reference to `led_classdev_register_ext'
> > >
> > > The problem is that the 'imply' keyword does not enforce any dependency
> > > but is only a weak hint to Kconfig to enable another symbol from a
> > > defconfig file.
> > >
> > > Change imply to a 'depends on LEDS_CLASS' that prevents the incorrect
> > > configuration but still allows building the driver without LED support.
> > >
> > > The 'select MAC80211_LEDS' is now ensures that the LED support is
> > > actually used if it is present, and the added Kconfig dependency
> > > on MAC80211_LEDS ensures that it cannot be enabled manually when it
> > > has no effect.
> >
> > But we do not want to have this dependency (selecting MAC80211_LEDS).
> > I fixed this problem here:
> > https://lore.kernel.org/lkml/20201227143034.1134829-1-krzk@kernel.org/
> > Maybe let's take this approach?
>
> Generally speaking, I don't like to have a device driver specific Kconfig
> setting 'select' a subsystem', for two reasons:
>
> - you suddenly get asked for tons of new LED specific options when
>   enabling seemingly benign options
>
> - Mixing 'depends on' and 'select' leads to bugs with circular
>   dependencies that usually require turning some other 'select'
>   into 'depends on'.
>
> The problem with LEDS_CLASS in particular is that there is a mix of drivers
> using one vs the other roughly 50:50.
>
>       Arnd

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

[Arnd Bergmann]

On Mon, Jan 25, 2021 at 2:27 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On Mon, 25 Jan 2021 at 14:09, Arnd Bergmann <arnd@kernel.org> wrote:
> > On Mon, Jan 25, 2021 at 12:40 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > > On Mon, 25 Jan 2021 at 12:36, Arnd Bergmann <arnd@kernel.org> wrote:
> > > But we do not want to have this dependency (selecting MAC80211_LEDS).
> > > I fixed this problem here:
> > > https://lore.kernel.org/lkml/20201227143034.1134829-1-krzk@kernel.org/
> > > Maybe let's take this approach?
> >
> > Generally speaking, I don't like to have a device driver specific Kconfig
> > setting 'select' a subsystem', for two reasons:
> >
> > - you suddenly get asked for tons of new LED specific options when
> >   enabling seemingly benign options
> >
> > - Mixing 'depends on' and 'select' leads to bugs with circular
> >   dependencies that usually require turning some other 'select'
> >   into 'depends on'.
> >
> > The problem with LEDS_CLASS in particular is that there is a mix of drivers
> > using one vs the other roughly 50:50.
>
> Yes, you are right, I also don't like it. However it was like this
> before my commit so I am not introducing a new issue. The point is
> that in your choice the MAC80211_LEDS will be selected if LEDS_CLASS
> is present, which is exactly what I was trying to fix/remove. My WiFi
> dongle does not have a LED and it causes a periodic (every second)
> event. However I still have LEDS_CLASS for other LEDS in the system.

What is the effect of this lost event every second? If it causes some
runtime warning or other problem, then neither of our fixes would
solve it completely, because someone with a distro kernel would
see the same issue when they have the symbol enabled but no
physical LED in the device.

      Arnd

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

[Krzysztof Kozlowski]

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:
> > On Mon, 25 Jan 2021 at 14:09, Arnd Bergmann <arnd@kernel.org> wrote:
> > > On Mon, Jan 25, 2021 at 12:40 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > > > On Mon, 25 Jan 2021 at 12:36, Arnd Bergmann <arnd@kernel.org> wrote:
> > > > But we do not want to have this dependency (selecting MAC80211_LEDS).
> > > > I fixed this problem here:
> > > > https://lore.kernel.org/lkml/20201227143034.1134829-1-krzk@kernel.org/
> > > > Maybe let's take this approach?
> > >
> > > Generally speaking, I don't like to have a device driver specific Kconfig
> > > setting 'select' a subsystem', for two reasons:
> > >
> > > - you suddenly get asked for tons of new LED specific options when
> > >   enabling seemingly benign options
> > >
> > > - Mixing 'depends on' and 'select' leads to bugs with circular
> > >   dependencies that usually require turning some other 'select'
> > >   into 'depends on'.
> > >
> > > The problem with LEDS_CLASS in particular is that there is a mix of drivers
> > > using one vs the other roughly 50:50.
> >
> > Yes, you are right, I also don't like it. However it was like this
> > before my commit so I am not introducing a new issue. The point is
> > that in your choice the MAC80211_LEDS will be selected if LEDS_CLASS
> > is present, which is exactly what I was trying to fix/remove. My WiFi
> > dongle does not have a LED and it causes a periodic (every second)
> > event. However I still have LEDS_CLASS for other LEDS in the system.
>
> What is the effect of this lost event every second? If it causes some
> runtime warning or other problem, then neither of our fixes would
> solve it completely, because someone with a distro kernel would
> see the same issue when they have the symbol enabled but no
> physical LED in the device.

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.

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.

Best regards,
Krzysztof

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

[Arnd Bergmann]

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. 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.

        Arnd

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

[Kalle Valo]

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

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

[Johannes Berg]

On Wed, 2021-01-27 at 12:35 +0200, Kalle Valo wrote:
> 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?

Sure, that seems fine.

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

johannes

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

[Kalle Valo]

Arnd Bergmann <arnd@kernel.org> wrote:

> From: Arnd Bergmann <arnd@arndb.de>
> 
> When CONFIG_ATH9K is built-in but LED support is in a loadable
> module, both ath9k drivers fails to link:
> 
> x86_64-linux-ld: drivers/net/wireless/ath/ath9k/gpio.o: in function `ath_deinit_leds':
> gpio.c:(.text+0x36): undefined reference to `led_classdev_unregister'
> x86_64-linux-ld: drivers/net/wireless/ath/ath9k/gpio.o: in function `ath_init_leds':
> gpio.c:(.text+0x179): undefined reference to `led_classdev_register_ext'
> 
> The problem is that the 'imply' keyword does not enforce any dependency
> but is only a weak hint to Kconfig to enable another symbol from a
> defconfig file.
> 
> Change imply to a 'depends on LEDS_CLASS' that prevents the incorrect
> configuration but still allows building the driver without LED support.
> 
> The 'select MAC80211_LEDS' is now ensures that the LED support is
> actually used if it is present, and the added Kconfig dependency
> on MAC80211_LEDS ensures that it cannot be enabled manually when it
> has no effect.
> 
> Fixes: 197f466e93f5 ("ath9k_htc: Do not select MAC80211_LEDS by default")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Acked-by: Johannes Berg <johannes@sipsolutions.net>

Patch applied to wireless-drivers.git, thanks.

b64acb28da83 ath9k: fix build error with LEDS_CLASS=m

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20210125113654.2408057-1-arnd@kernel.org/

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