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

[Ping-Ke Shih]

Arnd Bergmann <arnd@xxxxxxxx> wrote:
> On Wed, Sep 30, 2026, at 02:28, Ping-Ke Shih wrote:
> > Arnd Bergmann <arnd@xxxxxxxxxx> 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@xxxxxxxx>
> >> ---
> >>  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