[PATCH] wifi: rtw89: fix LED dependencies

[PATCH] wifi: rtw89: fix LED dependencies

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

The rtw89 driver fails to build when multicolor LED support is in
a loadable module but the rtw89 driver is built-in:

aarch64-linux-ld: drivers/net/wireless/realtek/rtw89/led_mc.o: in function `rtw89_led_mc_brightness_set':
led_mc.c:(.text+0x50): undefined reference to `led_mc_calc_color_components'
aarch64-linux-ld: drivers/net/wireless/realtek/rtw89/led_mc.o: in function `rtw89_led_mc_init':
led_mc.c:(.text+0x434): undefined reference to `led_classdev_multicolor_register_ext'

The problem is a misunderstanding of how Kconfig dependencies
work, as the 'imply' keyword is not sufficient to enable a
a user-visible dependency, and the boolean 'RTW89_LEDS_MC'
symbol cannot determine whether linking against the MC code is
valid.

Address this by using the correct construct to determing whether
linking agains the MAC80211_LEDS and LEDS_CLASS_MULTICOLOR
code is possible, respectively.

Fixes: d910631ff352 ("wifi: rtw89: add LED support to reflect the wireless association status")
Fixes: 721d90c8509a ("wifi: rtw89: add multicolor LED support for RTL8852CU valve board")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/net/wireless/realtek/rtw89/Kconfig | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw89/Kconfig b/drivers/net/wireless/realtek/rtw89/Kconfig
index 7c678dd1f6b3..4000ae344544 100644
--- a/drivers/net/wireless/realtek/rtw89/Kconfig
+++ b/drivers/net/wireless/realtek/rtw89/Kconfig
@@ -208,15 +208,13 @@ config RTW89_DEBUGFS
 config RTW89_LEDS
 	bool
 	depends on RTW89_CORE
-	depends on LEDS_CLASS=y || LEDS_CLASS=MAC80211
-	imply MAC80211_LEDS
+	depends on MAC80211_LEDS=y || MAC80211_LEDS=RTW89_CORE
 	default y
 
 config RTW89_LEDS_MC
 	bool
 	depends on RTW89_LEDS
-	depends on LEDS_CLASS_MULTICOLOR
-	imply LEDS_TRIGGER_TIMER
+	depends on LEDS_CLASS_MULTICOLOR=y || LEDS_CLASS_MULTICOLOR=RTW89_CORE
 	default y
 
 endif
-- 
2.53.0

RE: [PATCH] wifi: rtw89: fix LED dependencies

[Ping-Ke Shih]

Arnd Bergmann <arnd@kernel.org> wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> The rtw89 driver fails to build when multicolor LED support is in
> a loadable module but the rtw89 driver is built-in:
> 
> aarch64-linux-ld: drivers/net/wireless/realtek/rtw89/led_mc.o: in function
> `rtw89_led_mc_brightness_set':
> led_mc.c:(.text+0x50): undefined reference to `led_mc_calc_color_components'
> aarch64-linux-ld: drivers/net/wireless/realtek/rtw89/led_mc.o: in function `rtw89_led_mc_init':
> led_mc.c:(.text+0x434): undefined reference to `led_classdev_multicolor_register_ext'

This was also found by kernel test robot [1], and addressed by patch [2].

[1] https://lore.kernel.org/oe-kbuild-all/202609172315.ZYoKvKBS-lkp@intel.com/
[2] https://patch.msgid.link/20260918021113.25481-1-pkshih@realtek.com

> 
> The problem is a misunderstanding of how Kconfig dependencies
> work, as the 'imply' keyword is not sufficient to enable a
> a user-visible dependency, and the boolean 'RTW89_LEDS_MC'
> symbol cannot determine whether linking against the MC code is
> valid.

People intend to weakly select the dependency. I think keeping
'imply' is harmless.

> 
> Address this by using the correct construct to determing whether
> linking agains the MAC80211_LEDS and LEDS_CLASS_MULTICOLOR
> code is possible, respectively.
> 
> Fixes: d910631ff352 ("wifi: rtw89: add LED support to reflect the wireless association status")
> Fixes: 721d90c8509a ("wifi: rtw89: add multicolor LED support for RTL8852CU valve board")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/net/wireless/realtek/rtw89/Kconfig | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtw89/Kconfig
> b/drivers/net/wireless/realtek/rtw89/Kconfig
> index 7c678dd1f6b3..4000ae344544 100644
> --- a/drivers/net/wireless/realtek/rtw89/Kconfig
> +++ b/drivers/net/wireless/realtek/rtw89/Kconfig
> @@ -208,15 +208,13 @@ config RTW89_DEBUGFS
>  config RTW89_LEDS
>         bool
>         depends on RTW89_CORE
> -       depends on LEDS_CLASS=y || LEDS_CLASS=MAC80211

I remember we fixed to this style years ago by imitating ath10k and iwlwifi,
which they look like that still. 

> -       imply MAC80211_LEDS
> +       depends on MAC80211_LEDS=y || MAC80211_LEDS=RTW89_CORE
>         default y
> 
>  config RTW89_LEDS_MC
>         bool
>         depends on RTW89_LEDS
> -       depends on LEDS_CLASS_MULTICOLOR
> -       imply LEDS_TRIGGER_TIMER
> +       depends on LEDS_CLASS_MULTICOLOR=y || LEDS_CLASS_MULTICOLOR=RTW89_CORE
>         default y
> 
>  endif
> --
> 2.53.0

Re: [PATCH] wifi: rtw89: fix LED dependencies

[Arnd Bergmann]

On Wed, Sep 30, 2026, at 02:28, Ping-Ke Shih wrote:
> Arnd Bergmann <arnd@kernel.org> wrote:
>> The problem is a misunderstanding of how Kconfig dependencies
>> work, as the 'imply' keyword is not sufficient to enable a
>> a user-visible dependency, and the boolean 'RTW89_LEDS_MC'
>> symbol cannot determine whether linking against the MC code is
>> valid.
>
> People intend to weakly select the dependency. I think keeping
> 'imply' is harmless.

Whoever those 'people' are, please tell them to stop using 'imply'.

It's obviously harmless in the sense that it doesn't do enforce
anything, it just make it more error-prone:

 - the first 'imply' only works because MAC80211_LEDS has
   the same dependency as RTW89_LEDS, so it works as a 'select'
   as long as the dependencies don't change. If the dependency
   were to change, the only difference is that imply makes it
   harder to debug because it skips the helpful message from
   kconfig.

- the second 'imply' turns on a random symbol from another
  subsystem, which is discouraged.

- we already have a mix of 'depends on' and 'select'
  for the LED support, which can lead to circular dependencies
  and other problems. Adding a third way can only make it
  worse.

>> Address this by using the correct construct to determing whether
>> linking agains the MAC80211_LEDS and LEDS_CLASS_MULTICOLOR
>> code is possible, respectively.
>> 
>> Fixes: d910631ff352 ("wifi: rtw89: add LED support to reflect the wireless association status")
>> Fixes: 721d90c8509a ("wifi: rtw89: add multicolor LED support for RTL8852CU valve board")
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> ---
>>  drivers/net/wireless/realtek/rtw89/Kconfig | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/net/wireless/realtek/rtw89/Kconfig
>> b/drivers/net/wireless/realtek/rtw89/Kconfig
>> index 7c678dd1f6b3..4000ae344544 100644
>> --- a/drivers/net/wireless/realtek/rtw89/Kconfig
>> +++ b/drivers/net/wireless/realtek/rtw89/Kconfig
>> @@ -208,15 +208,13 @@ config RTW89_DEBUGFS
>>  config RTW89_LEDS
>>         bool
>>         depends on RTW89_CORE
>> -       depends on LEDS_CLASS=y || LEDS_CLASS=MAC80211
>
> I remember we fixed to this style years ago by imitating ath10k and iwlwifi,
> which they look like that still. 

ath10k doesn't use ieee80211_led but seems to just duplicate that code.

I do see that my version also got it wrong, as the 

+	depends on MAC80211_LEDS=y || MAC80211_LEDS=RTW89_CORE

line is nonsense with MAC80211_LEDS being a 'bool' symbol.

The way this was meant to be used is to have

config RTW89_LEDS
      def_bool RTW89_CORE && MAC80211_LEDS

or you can skip the symbol entirely and replace all the
CONFIG_RTW89_LEDS checks in source code and Makefile with
'#ifdef CONFIG_MAC80211_LEDS', see ath5k and ath9k for instance.

     Arnd

RE: [PATCH] wifi: rtw89: fix LED dependencies

[Ping-Ke Shih]

Arnd Bergmann <arnd@arndb.de> wrote:
> On Wed, Sep 30, 2026, at 02:28, Ping-Ke Shih wrote:
> > Arnd Bergmann <arnd@kernel.org> wrote:
> >> The problem is a misunderstanding of how Kconfig dependencies
> >> work, as the 'imply' keyword is not sufficient to enable a
> >> a user-visible dependency, and the boolean 'RTW89_LEDS_MC'
> >> symbol cannot determine whether linking against the MC code is
> >> valid.
> >
> > People intend to weakly select the dependency. I think keeping
> > 'imply' is harmless.
> 
> Whoever those 'people' are, please tell them to stop using 'imply'.
> 
> It's obviously harmless in the sense that it doesn't do enforce
> anything, it just make it more error-prone:
> 
>  - the first 'imply' only works because MAC80211_LEDS has
>    the same dependency as RTW89_LEDS, so it works as a 'select'
>    as long as the dependencies don't change. If the dependency
>    were to change, the only difference is that imply makes it
>    harder to debug because it skips the helpful message from
>    kconfig.
> 
> - the second 'imply' turns on a random symbol from another
>   subsystem, which is discouraged.
> 
> - we already have a mix of 'depends on' and 'select'
>   for the LED support, which can lead to circular dependencies
>   and other problems. Adding a third way can only make it
>   worse.
> 

Agree. I'll remove these two discouraged 'imply'. 

> >> Address this by using the correct construct to determing whether
> >> linking agains the MAC80211_LEDS and LEDS_CLASS_MULTICOLOR
> >> code is possible, respectively.
> >>
> >> Fixes: d910631ff352 ("wifi: rtw89: add LED support to reflect the wireless association status")
> >> Fixes: 721d90c8509a ("wifi: rtw89: add multicolor LED support for RTL8852CU valve board")
> >> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> >> ---
> >>  drivers/net/wireless/realtek/rtw89/Kconfig | 6 ++----
> >>  1 file changed, 2 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/net/wireless/realtek/rtw89/Kconfig
> >> b/drivers/net/wireless/realtek/rtw89/Kconfig
> >> index 7c678dd1f6b3..4000ae344544 100644
> >> --- a/drivers/net/wireless/realtek/rtw89/Kconfig
> >> +++ b/drivers/net/wireless/realtek/rtw89/Kconfig
> >> @@ -208,15 +208,13 @@ config RTW89_DEBUGFS
> >>  config RTW89_LEDS
> >>         bool
> >>         depends on RTW89_CORE
> >> -       depends on LEDS_CLASS=y || LEDS_CLASS=MAC80211
> >
> > I remember we fixed to this style years ago by imitating ath10k and iwlwifi,
> > which they look like that still.
> 
> ath10k doesn't use ieee80211_led but seems to just duplicate that code.
> 
> I do see that my version also got it wrong, as the
> 
> +       depends on MAC80211_LEDS=y || MAC80211_LEDS=RTW89_CORE
> 
> line is nonsense with MAC80211_LEDS being a 'bool' symbol.
> 
> The way this was meant to be used is to have
> 
> config RTW89_LEDS
>       def_bool RTW89_CORE && MAC80211_LEDS
> 

I'd keep first block as

        depends on LEDS_CLASS=y || LEDS_CLASS=MAC80211

mac80211 implements ieee80211_get_assoc_led_name() used by this driver as

  static inline const char *ieee80211_get_assoc_led_name(struct ieee80211_hw *hw)
  {
  #ifdef CONFIG_MAC80211_LEDS
	  return __ieee80211_get_assoc_led_name(hw);
  #else
	  return NULL;
  #endif
  }

That means if CONFIG_MAC80211_LEDS wasn't defined, driver can still use
ieee80211_get_assoc_led_name() as default trigger, but just NULL.
More, use space can adjust LED trigger via sysfs, so having LED support
is still usable if LEDS_CLASS exists.

Ping-Ke

Re: [PATCH] wifi: rtw89: fix LED dependencies

[Arnd Bergmann]

On Fri, Oct 2, 2026, at 02:22, Ping-Ke Shih wrote:
> Arnd Bergmann <arnd@arndb.de> wrote:
>> On Wed, Sep 30, 2026, at 02:28, Ping-Ke Shih wrote:
>> - we already have a mix of 'depends on' and 'select'
>>   for the LED support, which can lead to circular dependencies
>>   and other problems. Adding a third way can only make it
>>   worse.
>> 
>
> Agree. I'll remove these two discouraged 'imply'. 

Ok, thanks!

>> The way this was meant to be used is to have
>> 
>> config RTW89_LEDS
>>       def_bool RTW89_CORE && MAC80211_LEDS
>> 
>
> I'd keep first block as
>
>         depends on LEDS_CLASS=y || LEDS_CLASS=MAC80211
>
> mac80211 implements ieee80211_get_assoc_led_name() used by this driver as
>
>   static inline const char *ieee80211_get_assoc_led_name(struct 
> ieee80211_hw *hw)
>   {
>   #ifdef CONFIG_MAC80211_LEDS
> 	  return __ieee80211_get_assoc_led_name(hw);
>   #else
> 	  return NULL;
>   #endif
>   }
>
> That means if CONFIG_MAC80211_LEDS wasn't defined, driver can still use
> ieee80211_get_assoc_led_name() as default trigger, but just NULL.
> More, use space can adjust LED trigger via sysfs, so having LED support
> is still usable if LEDS_CLASS exists.

Right, this should work and is consistent with how other wireless
drivers do this.

I think it would be nicer to just use MAC80211_LEDS as a simple
dependency as I suggested above. If we do that, it should
be done the same way for all the wireless drivers, but that
is a probably something for another day.

       Arnd