* [PATCH] r8169: fix building with CONFIG_LEDS_CLASS=m
@ 2024-01-03 10:26 Arnd Bergmann
2024-01-03 11:33 ` Heiner Kallweit
0 siblings, 1 reply; 4+ messages in thread
From: Arnd Bergmann @ 2024-01-03 10:26 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Heiner Kallweit
Cc: Arnd Bergmann, netdev, linux-kernel
From: Arnd Bergmann <arnd@arndb.de>
When r8169 is built-in but the LED support is a loadable module, the
new code to drive the LED now causes a link failure:
ld: drivers/net/ethernet/realtek/r8169_leds.o: in function `rtl8168_init_leds':
r8169_leds.c:(.text+0x36c): undefined reference to `devm_led_classdev_register_ext'
Add a Kconfig dependency to prevent the broken configuration but still
allow having the network code built-in as long as CONFIG_LEDS_TRIGGER_NETDEV
is disabled, regardless of CONFIG_LEDS_CLASS.
Fixes: 18764b883e15 ("r8169: add support for LED's on RTL8168/RTL8101")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/net/ethernet/realtek/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/realtek/Kconfig b/drivers/net/ethernet/realtek/Kconfig
index 93d9df55b361..fd3f18b328de 100644
--- a/drivers/net/ethernet/realtek/Kconfig
+++ b/drivers/net/ethernet/realtek/Kconfig
@@ -98,6 +98,7 @@ config 8139_OLD_RX_RESET
config R8169
tristate "Realtek 8169/8168/8101/8125 ethernet support"
depends on PCI
+ depends on LEDS_CLASS || !LEDS_TRIGGER_NETDEV
select FW_LOADER
select CRC32
select PHYLIB
--
2.39.2
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] r8169: fix building with CONFIG_LEDS_CLASS=m 2024-01-03 10:26 [PATCH] r8169: fix building with CONFIG_LEDS_CLASS=m Arnd Bergmann @ 2024-01-03 11:33 ` Heiner Kallweit 2024-01-03 12:45 ` Arnd Bergmann 0 siblings, 1 reply; 4+ messages in thread From: Heiner Kallweit @ 2024-01-03 11:33 UTC (permalink / raw) To: Arnd Bergmann, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni Cc: Arnd Bergmann, netdev, linux-kernel, linux-leds@vger.kernel.org, Lee Jones On 03.01.2024 11:26, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > When r8169 is built-in but the LED support is a loadable module, the > new code to drive the LED now causes a link failure: > > ld: drivers/net/ethernet/realtek/r8169_leds.o: in function `rtl8168_init_leds': > r8169_leds.c:(.text+0x36c): undefined reference to `devm_led_classdev_register_ext' > > Add a Kconfig dependency to prevent the broken configuration but still > allow having the network code built-in as long as CONFIG_LEDS_TRIGGER_NETDEV > is disabled, regardless of CONFIG_LEDS_CLASS. > The proposed change is more of a workaround IMO. A proper fix (in LED subsystem) has been submitted, but it's not reviewed/applied yet. And I don't think building r8169 should depend on support for an optional feature. This fix would also allow to remove Kconfig dependencies similar to the one proposed here from other drivers. Link to submitted fix: https://lore.kernel.org/linux-leds/0f6f432b-c650-4bb8-a1b5-fe3372804d52@gmail.com/T/#u > Fixes: 18764b883e15 ("r8169: add support for LED's on RTL8168/RTL8101") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/net/ethernet/realtek/Kconfig | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/net/ethernet/realtek/Kconfig b/drivers/net/ethernet/realtek/Kconfig > index 93d9df55b361..fd3f18b328de 100644 > --- a/drivers/net/ethernet/realtek/Kconfig > +++ b/drivers/net/ethernet/realtek/Kconfig > @@ -98,6 +98,7 @@ config 8139_OLD_RX_RESET > config R8169 > tristate "Realtek 8169/8168/8101/8125 ethernet support" > depends on PCI > + depends on LEDS_CLASS || !LEDS_TRIGGER_NETDEV > select FW_LOADER > select CRC32 > select PHYLIB ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] r8169: fix building with CONFIG_LEDS_CLASS=m 2024-01-03 11:33 ` Heiner Kallweit @ 2024-01-03 12:45 ` Arnd Bergmann 2024-01-03 13:56 ` Heiner Kallweit 0 siblings, 1 reply; 4+ messages in thread From: Arnd Bergmann @ 2024-01-03 12:45 UTC (permalink / raw) To: Heiner Kallweit, Arnd Bergmann, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni Cc: Netdev, linux-kernel, linux-leds@vger.kernel.org, Lee Jones On Wed, Jan 3, 2024, at 12:33, Heiner Kallweit wrote: > On 03.01.2024 11:26, Arnd Bergmann wrote: >> From: Arnd Bergmann <arnd@arndb.de> >> >> When r8169 is built-in but the LED support is a loadable module, the >> new code to drive the LED now causes a link failure: >> >> ld: drivers/net/ethernet/realtek/r8169_leds.o: in function `rtl8168_init_leds': >> r8169_leds.c:(.text+0x36c): undefined reference to `devm_led_classdev_register_ext' >> >> Add a Kconfig dependency to prevent the broken configuration but still >> allow having the network code built-in as long as CONFIG_LEDS_TRIGGER_NETDEV >> is disabled, regardless of CONFIG_LEDS_CLASS. >> > The proposed change is more of a workaround IMO. A proper fix (in LED > subsystem) > has been submitted, but it's not reviewed/applied yet. And I don't > think building > r8169 should depend on support for an optional feature. > This fix would also allow to remove Kconfig dependencies similar to the > one > proposed here from other drivers. Link to submitted fix: > > https://lore.kernel.org/linux-leds/0f6f432b-c650-4bb8-a1b5-fe3372804d52@gmail.com/T/#u I think that is a much worse workaround, as this just leads to a feature silently not working even when it is enabled (as loadable module), and hiding other dependency problems where drivers actually require the LED support to do something useful. IS_REACHABLE() is pretty much always a bad idea because of this. If you want the LED support in r8169_leds to be optional, I would suggest adding a separate Kconfig symbol for that, either user visible or hidden, and have the correct Kconfig dependencies for the new symbol, something like the change below (untested). Arnd 8<--- diff --git a/drivers/net/ethernet/realtek/Kconfig b/drivers/net/ethernet/realtek/Kconfig index fd3f18b328de..b54aae30b3c1 100644 --- a/drivers/net/ethernet/realtek/Kconfig +++ b/drivers/net/ethernet/realtek/Kconfig @@ -114,4 +114,9 @@ config R8169 To compile this driver as a module, choose M here: the module will be called r8169. This is recommended. +config R8169_LEDS + def_bool y + depends on LEDS_TRIGGER_NETDEV && R8169 + depends on !(R8169=y && LEDS_CLASS=m) + endif # NET_VENDOR_REALTEK diff --git a/drivers/net/ethernet/realtek/Makefile b/drivers/net/ethernet/realtek/Makefile index adff9ebfbf2c..635491d8826e 100644 --- a/drivers/net/ethernet/realtek/Makefile +++ b/drivers/net/ethernet/realtek/Makefile @@ -6,8 +6,6 @@ obj-$(CONFIG_8139CP) += 8139cp.o obj-$(CONFIG_8139TOO) += 8139too.o obj-$(CONFIG_ATP) += atp.o -r8169-objs += r8169_main.o r8169_firmware.o r8169_phy_config.o -ifdef CONFIG_LEDS_TRIGGER_NETDEV -r8169-objs += r8169_leds.o -endif +r8169-y += r8169_main.o r8169_firmware.o r8169_phy_config.o +r8169-$(CONFIG_R8169_LEDS) += r8169_leds.o obj-$(CONFIG_R8169) += r8169.o diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c index 05ba5f743af2..f3321299b2fa 100644 --- a/drivers/net/ethernet/realtek/r8169_main.c +++ b/drivers/net/ethernet/realtek/r8169_main.c @@ -5356,11 +5356,10 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) if (rc) return rc; -#if IS_REACHABLE(CONFIG_LEDS_CLASS) && IS_ENABLED(CONFIG_LEDS_TRIGGER_NETDEV) - if (tp->mac_version > RTL_GIGA_MAC_VER_06 && + if (IS_ENABLED(CONFIG_R8169_LEDS) && + tp->mac_version > RTL_GIGA_MAC_VER_06 && tp->mac_version < RTL_GIGA_MAC_VER_61) rtl8168_init_leds(dev); -#endif netdev_info(dev, "%s, %pM, XID %03x, IRQ %d\n", rtl_chip_infos[chipset].name, dev->dev_addr, xid, tp->irq); ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] r8169: fix building with CONFIG_LEDS_CLASS=m 2024-01-03 12:45 ` Arnd Bergmann @ 2024-01-03 13:56 ` Heiner Kallweit 0 siblings, 0 replies; 4+ messages in thread From: Heiner Kallweit @ 2024-01-03 13:56 UTC (permalink / raw) To: Arnd Bergmann, Arnd Bergmann, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni Cc: Netdev, linux-kernel, linux-leds@vger.kernel.org, Lee Jones On 03.01.2024 13:45, Arnd Bergmann wrote: > On Wed, Jan 3, 2024, at 12:33, Heiner Kallweit wrote: >> On 03.01.2024 11:26, Arnd Bergmann wrote: >>> From: Arnd Bergmann <arnd@arndb.de> >>> >>> When r8169 is built-in but the LED support is a loadable module, the >>> new code to drive the LED now causes a link failure: >>> >>> ld: drivers/net/ethernet/realtek/r8169_leds.o: in function `rtl8168_init_leds': >>> r8169_leds.c:(.text+0x36c): undefined reference to `devm_led_classdev_register_ext' >>> >>> Add a Kconfig dependency to prevent the broken configuration but still >>> allow having the network code built-in as long as CONFIG_LEDS_TRIGGER_NETDEV >>> is disabled, regardless of CONFIG_LEDS_CLASS. >>> >> The proposed change is more of a workaround IMO. A proper fix (in LED >> subsystem) >> has been submitted, but it's not reviewed/applied yet. And I don't >> think building >> r8169 should depend on support for an optional feature. >> This fix would also allow to remove Kconfig dependencies similar to the >> one >> proposed here from other drivers. Link to submitted fix: >> >> https://lore.kernel.org/linux-leds/0f6f432b-c650-4bb8-a1b5-fe3372804d52@gmail.com/T/#u > > I think that is a much worse workaround, as this just leads to > a feature silently not working even when it is enabled (as loadable > module), and hiding other dependency problems where drivers > actually require the LED support to do something useful. > Whether implicit dependency detection is considered a bad thing, may depend on personal taste. However I see your point. > IS_REACHABLE() is pretty much always a bad idea because of this. > > If you want the LED support in r8169_leds to be optional, I would > suggest adding a separate Kconfig symbol for that, either user > visible or hidden, and have the correct Kconfig dependencies > for the new symbol, something like the change below (untested). > Sounds good, as this would also allow to omit compiling r8169_leds.c if LEDS_CLASS isn't reachable. I'll give it a try. > Arnd > Heiner > 8<--- > diff --git a/drivers/net/ethernet/realtek/Kconfig b/drivers/net/ethernet/realtek/Kconfig > index fd3f18b328de..b54aae30b3c1 100644 > --- a/drivers/net/ethernet/realtek/Kconfig > +++ b/drivers/net/ethernet/realtek/Kconfig > @@ -114,4 +114,9 @@ config R8169 > To compile this driver as a module, choose M here: the module > will be called r8169. This is recommended. > > +config R8169_LEDS > + def_bool y > + depends on LEDS_TRIGGER_NETDEV && R8169 > + depends on !(R8169=y && LEDS_CLASS=m) > + > endif # NET_VENDOR_REALTEK > diff --git a/drivers/net/ethernet/realtek/Makefile b/drivers/net/ethernet/realtek/Makefile > index adff9ebfbf2c..635491d8826e 100644 > --- a/drivers/net/ethernet/realtek/Makefile > +++ b/drivers/net/ethernet/realtek/Makefile > @@ -6,8 +6,6 @@ > obj-$(CONFIG_8139CP) += 8139cp.o > obj-$(CONFIG_8139TOO) += 8139too.o > obj-$(CONFIG_ATP) += atp.o > -r8169-objs += r8169_main.o r8169_firmware.o r8169_phy_config.o > -ifdef CONFIG_LEDS_TRIGGER_NETDEV > -r8169-objs += r8169_leds.o > -endif > +r8169-y += r8169_main.o r8169_firmware.o r8169_phy_config.o > +r8169-$(CONFIG_R8169_LEDS) += r8169_leds.o > obj-$(CONFIG_R8169) += r8169.o > diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c > index 05ba5f743af2..f3321299b2fa 100644 > --- a/drivers/net/ethernet/realtek/r8169_main.c > +++ b/drivers/net/ethernet/realtek/r8169_main.c > @@ -5356,11 +5356,10 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) > if (rc) > return rc; > > -#if IS_REACHABLE(CONFIG_LEDS_CLASS) && IS_ENABLED(CONFIG_LEDS_TRIGGER_NETDEV) > - if (tp->mac_version > RTL_GIGA_MAC_VER_06 && > + if (IS_ENABLED(CONFIG_R8169_LEDS) && > + tp->mac_version > RTL_GIGA_MAC_VER_06 && > tp->mac_version < RTL_GIGA_MAC_VER_61) > rtl8168_init_leds(dev); > -#endif > > netdev_info(dev, "%s, %pM, XID %03x, IRQ %d\n", > rtl_chip_infos[chipset].name, dev->dev_addr, xid, tp->irq); ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-01-03 13:56 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-01-03 10:26 [PATCH] r8169: fix building with CONFIG_LEDS_CLASS=m Arnd Bergmann 2024-01-03 11:33 ` Heiner Kallweit 2024-01-03 12:45 ` Arnd Bergmann 2024-01-03 13:56 ` Heiner Kallweit
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).