On Tue, Jan 9, 2024, at 11:10, Heiner Kallweit wrote: > On 09.01.2024 10:06, Arnd Bergmann wrote: >> From: Arnd Bergmann <arnd@xxxxxxxx> >> >> These two functions have stub implementations that are called when >> NEW_LEDS and/or LEDS_CLASS are disabled, theorerically allowing drivers >> to optionally use the LED subsystem. >> >> However, this has never really worked because a built-in driver is >> unable to link against these functions if the LED class is in a loadable >> module. Heiner ran into this problem with a driver that newly gained >> a LEDS_CLASS dependency and suggested using an IS_REACHABLE() check. >> >> This is the reverse approach, removing the stub entirely to acknowledge >> that it is pointless in its current form, and that not having it avoids >> misleading developers into thinking that they can rely on it. >> >> This survived around 1000 randconfig builds to validate that any callers >> of the interface already have the correct Kconfig dependency already, >> with the exception of the one that Heiner just added. >> >> Cc: Heiner Kallweit <hkallweit1@xxxxxxxxx> >> Link: https://lore.kernel.org/linux-leds/0f6f432b-c650-4bb8-a1b5-fe3372804d52@xxxxxxxxx/T/#u >> Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx> >> --- > > For r8169 we have a Kconfig-based solution now, right. I had a brief look > at other drivers using LED functionality, and already the first one I looked > at seems to suffer from the same problem. input/keyboard/qt2160.c has the > following what should result in the same link error if qt2160 is built-in > and CONFIG_LEDS_CLASS=m. qt2160 has a Kconfig dependency only on I2C. > > #ifdef CONFIG_LEDS_CLASS > static int qt2160_register_leds(struct qt2160_data *qt2160) > { > [...] > error = devm_led_classdev_register(&client->dev, &led->cdev); > [...] > } > #else This is a bug, but I think a different one, with a similar effect. Part of the problem in this driver is that it uses #ifdef instead of "#if IS_ENABLED(CONFIG_LEDS_CLASS)". As a result, it just never uses the LEDS when LEDS_CLASS=m, because that would define CONFIG_LEDS_CLASS_MODULE but not CONFIG_LEDS_CLASS. Changing it to IS_ENABLED() would cause the link failure you describe, but would do it regardless of my change. The same bug seems to be present in other files as well. > 2. If stubs are removed (but also in the current situation, see example), > then it seems some drivers need adding proper build dependencies. I don't see any driver that actually relies on the stub, since that would only work a driver that can never be built-in. If a driver can be built-in (like your r8169 code) and uses the stub, we would have seen it fail to link in randconfig kernels and added a LEDS_CLASS dependency. Arnd
