| Message ID | 20210529164420.11454-1-rdunlap@infradead.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [v2] wireless: carl9170: fix LEDS build errors & warnings | expand |
On 29/05/2021 18:44, Randy Dunlap wrote: > kernel test robot reports over 200 build errors and warnings > that are due to this Kconfig problem when CARL9170=m, > MAC80211=y, and LEDS_CLASS=m. > > WARNING: unmet direct dependencies detected for MAC80211_LEDS > Depends on [n]: NET [=y] && WIRELESS [=y] && MAC80211 [=y] && (LEDS_CLASS [=m]=y || LEDS_CLASS [=m]=MAC80211 [=y]) > Selected by [m]: > - CARL9170_LEDS [=y] && NETDEVICES [=y] && WLAN [=y] && WLAN_VENDOR_ATH [=y] && CARL9170 [=m] > > CARL9170_LEDS selects MAC80211_LEDS even though its kconfig > dependencies are not met. This happens because 'select' does not follow > any Kconfig dependency chains. > > Fix this by making CARL9170_LEDS always set/enabled if certain > conditions are met: LEDS_CLASS=y or LEDS_CLASS=CARL9170, just as > this is done for Mediatek MT76. > Hmm, that commit was really a long time ago. In fact it was part of the initial series for that driver (from September 2010) 1d7e1e6b1b8ed ("carl9170: Makefile, Kconfig files and MAINTAINERS") From what I can tell, the "bool / tristate" of LED_CLASS flipped from tristate (2006) to bool (2010) and back to tristate (2012). And since MAC80211_LEDS is involved it seems that the latest change from Arnd: b64acb28da83 ("ath9k: fix build error with LEDS_CLASS=m") So, the "select" stuff was present from the start, but it hasn't been kept up to date I guess. > Fixes: 1d7e1e6b1b8ed ("carl9170: Makefile, Kconfig files and MAINTAINERS") > Signed-off-by: Randy Dunlap <rdunlap@infradead.org> > Reported-by: kernel test robot <lkp@intel.com> > Cc: Kalle Valo <kvalo@codeaurora.org> > Cc: Christian Lamparter <chunkeey@googlemail.com> > Cc: linux-wireless@vger.kernel.org > Cc: Arnd Bergmann <arnd@arndb.de> > Suggested-by: Arnd Bergmann <arnd@arndb.de> > --- > v2: modify as suggesed by Arnd > > drivers/net/wireless/ath/carl9170/Kconfig | 12 +++--------- > 1 file changed, 3 insertions(+), 9 deletions(-) > > --- linux-next-20210528.orig/drivers/net/wireless/ath/carl9170/Kconfig > +++ linux-next-20210528/drivers/net/wireless/ath/carl9170/Kconfig > @@ -15,16 +15,10 @@ config CARL9170 > If you choose to build a module, it'll be called carl9170. > > config CARL9170_LEDS > - bool "SoftLED Support" > - depends on CARL9170 > - select MAC80211_LEDS > - select LEDS_CLASS > - select NEW_LEDS > + bool > default y > - help > - This option is necessary, if you want your device' LEDs to blink > - > - Say Y, unless you need the LEDs for firmware debugging. > + depends on CARL9170 > + depends on LEDS_CLASS=y || LEDS_CLASS=CARL9170 What happens if CARL9170=M|Y, LED_CLASS=M|Y but MAC80211_LEDS=N? (with =N, I mean of course: # CONFIG_MAC80211_LEDS is not set. ) From what I can tell, in this case, the driver will create the LEDs devices just fine. But since the triggers aren't available (because they are provided by MAC80211_LEDS) these LEDs will be still called "tx" and "assoc" but are completely "brainless"/"off". (carl9170 sets sane default triggers for the LEDS in carl9170_led_register) --- err = carl9170_led_register_led(ar, 0, "tx", ieee80211_get_tx_led_name(ar->hw)); if (err) goto fail; if (ar->features & CARL9170_ONE_LED) return 0; err = carl9170_led_register_led(ar, 1, "assoc", ieee80211_get_assoc_led_name(ar->hw)); --- I would have liked to keep that extra dependency to MAC80211_LEDS. Would this work with depends/imply? Or will this break in a different way? Cheers, Christian
Hi Christian, On 5/29/21 10:42 AM, Christian Lamparter wrote: > On 29/05/2021 18:44, Randy Dunlap wrote: >> kernel test robot reports over 200 build errors and warnings >> that are due to this Kconfig problem when CARL9170=m, >> MAC80211=y, and LEDS_CLASS=m. >> >> WARNING: unmet direct dependencies detected for MAC80211_LEDS >> Depends on [n]: NET [=y] && WIRELESS [=y] && MAC80211 [=y] && (LEDS_CLASS [=m]=y || LEDS_CLASS [=m]=MAC80211 [=y]) >> Selected by [m]: >> - CARL9170_LEDS [=y] && NETDEVICES [=y] && WLAN [=y] && WLAN_VENDOR_ATH [=y] && CARL9170 [=m] >> >> CARL9170_LEDS selects MAC80211_LEDS even though its kconfig >> dependencies are not met. This happens because 'select' does not follow >> any Kconfig dependency chains. >> >> Fix this by making CARL9170_LEDS always set/enabled if certain >> conditions are met: LEDS_CLASS=y or LEDS_CLASS=CARL9170, just as >> this is done for Mediatek MT76. >> > > Hmm, that commit was really a long time ago. In fact it was part of > the initial series for that driver (from September 2010) > 1d7e1e6b1b8ed ("carl9170: Makefile, Kconfig files and MAINTAINERS") > > From what I can tell, the "bool / tristate" of LED_CLASS flipped > from tristate (2006) to bool (2010) and back to tristate (2012). > > And since MAC80211_LEDS is involved it seems that the latest change > from Arnd: > b64acb28da83 ("ath9k: fix build error with LEDS_CLASS=m") > > So, the "select" stuff was present from the start, but it > hasn't been kept up to date I guess. > >> Fixes: 1d7e1e6b1b8ed ("carl9170: Makefile, Kconfig files and MAINTAINERS") >> Signed-off-by: Randy Dunlap <rdunlap@infradead.org> >> Reported-by: kernel test robot <lkp@intel.com> >> Cc: Kalle Valo <kvalo@codeaurora.org> >> Cc: Christian Lamparter <chunkeey@googlemail.com> >> Cc: linux-wireless@vger.kernel.org >> Cc: Arnd Bergmann <arnd@arndb.de> >> Suggested-by: Arnd Bergmann <arnd@arndb.de> >> --- >> v2: modify as suggesed by Arnd >> >> drivers/net/wireless/ath/carl9170/Kconfig | 12 +++--------- >> 1 file changed, 3 insertions(+), 9 deletions(-) >> >> --- linux-next-20210528.orig/drivers/net/wireless/ath/carl9170/Kconfig >> +++ linux-next-20210528/drivers/net/wireless/ath/carl9170/Kconfig >> @@ -15,16 +15,10 @@ config CARL9170 >> If you choose to build a module, it'll be called carl9170. >> config CARL9170_LEDS >> - bool "SoftLED Support" >> - depends on CARL9170 >> - select MAC80211_LEDS >> - select LEDS_CLASS >> - select NEW_LEDS >> + bool >> default y >> - help >> - This option is necessary, if you want your device' LEDs to blink >> - >> - Say Y, unless you need the LEDs for firmware debugging. >> + depends on CARL9170 >> + depends on LEDS_CLASS=y || LEDS_CLASS=CARL9170 > > What happens if CARL9170=M|Y, LED_CLASS=M|Y but MAC80211_LEDS=N? > (with =N, I mean of course: # CONFIG_MAC80211_LEDS is not set. ) > From what I can tell, in this case, the driver will create the LEDs > devices just fine. But since the triggers aren't available (because > they are provided by MAC80211_LEDS) these LEDs will be still called > "tx" and "assoc" but are completely "brainless"/"off". > > (carl9170 sets sane default triggers for the LEDS in carl9170_led_register) > --- > err = carl9170_led_register_led(ar, 0, "tx", > ieee80211_get_tx_led_name(ar->hw)); > if (err) > goto fail; > > if (ar->features & CARL9170_ONE_LED) > return 0; > > err = carl9170_led_register_led(ar, 1, "assoc", > ieee80211_get_assoc_led_name(ar->hw)); > --- > > I would have liked to keep that extra dependency to MAC80211_LEDS. > Would this work with depends/imply? Or will this break in a different way? I like that idea. I've give it a whirl of testing... thanks.
--- linux-next-20210528.orig/drivers/net/wireless/ath/carl9170/Kconfig +++ linux-next-20210528/drivers/net/wireless/ath/carl9170/Kconfig @@ -15,16 +15,10 @@ config CARL9170 If you choose to build a module, it'll be called carl9170. config CARL9170_LEDS - bool "SoftLED Support" - depends on CARL9170 - select MAC80211_LEDS - select LEDS_CLASS - select NEW_LEDS + bool default y - help - This option is necessary, if you want your device' LEDs to blink - - Say Y, unless you need the LEDs for firmware debugging. + depends on CARL9170 + depends on LEDS_CLASS=y || LEDS_CLASS=CARL9170 config CARL9170_DEBUGFS bool "DebugFS Support"
kernel test robot reports over 200 build errors and warnings that are due to this Kconfig problem when CARL9170=m, MAC80211=y, and LEDS_CLASS=m. WARNING: unmet direct dependencies detected for MAC80211_LEDS Depends on [n]: NET [=y] && WIRELESS [=y] && MAC80211 [=y] && (LEDS_CLASS [=m]=y || LEDS_CLASS [=m]=MAC80211 [=y]) Selected by [m]: - CARL9170_LEDS [=y] && NETDEVICES [=y] && WLAN [=y] && WLAN_VENDOR_ATH [=y] && CARL9170 [=m] CARL9170_LEDS selects MAC80211_LEDS even though its kconfig dependencies are not met. This happens because 'select' does not follow any Kconfig dependency chains. Fix this by making CARL9170_LEDS always set/enabled if certain conditions are met: LEDS_CLASS=y or LEDS_CLASS=CARL9170, just as this is done for Mediatek MT76. Fixes: 1d7e1e6b1b8ed ("carl9170: Makefile, Kconfig files and MAINTAINERS") Signed-off-by: Randy Dunlap <rdunlap@infradead.org> Reported-by: kernel test robot <lkp@intel.com> Cc: Kalle Valo <kvalo@codeaurora.org> Cc: Christian Lamparter <chunkeey@googlemail.com> Cc: linux-wireless@vger.kernel.org Cc: Arnd Bergmann <arnd@arndb.de> Suggested-by: Arnd Bergmann <arnd@arndb.de> --- v2: modify as suggesed by Arnd drivers/net/wireless/ath/carl9170/Kconfig | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-)