From: Arnd Bergmann <[email protected]>
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 <[email protected]>
---
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
On 03.01.2024 11:26, Arnd Bergmann wrote:
> From: Arnd Bergmann <[email protected]>
>
> 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/[email protected]/T/#u
> Fixes: 18764b883e15 ("r8169: add support for LED's on RTL8168/RTL8101")
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> 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
On Wed, Jan 3, 2024, at 12:33, Heiner Kallweit wrote:
> On 03.01.2024 11:26, Arnd Bergmann wrote:
>> From: Arnd Bergmann <[email protected]>
>>
>> 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/[email protected]/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);
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 <[email protected]>
>>>
>>> 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/[email protected]/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);