diff mbox series

r8169: fix building with CONFIG_LEDS_CLASS=m

Message ID 20240103102630.3770242-1-arnd@kernel.org (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series r8169: fix building with CONFIG_LEDS_CLASS=m | expand

Checks

Context Check Description
netdev/series_format warning Single patches do not need cover letters; Target tree name not specified in the subject
netdev/tree_selection success Guessed tree name to be net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 8 this patch: 8
netdev/cc_maintainers success CCed 6 of 6 maintainers
netdev/build_clang success Errors and warnings before: 8 this patch: 8
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 7 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Arnd Bergmann Jan. 3, 2024, 10:26 a.m. UTC 
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(+)

Comments

Heiner Kallweit Jan. 3, 2024, 11:33 a.m. UTC | #1 
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
Arnd Bergmann Jan. 3, 2024, 12:45 p.m. UTC | #2 
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);
Heiner Kallweit Jan. 3, 2024, 1:56 p.m. UTC | #3 
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);
diff mbox series

Patch

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