Re: [PATCH 1/2] leds: remove led_init_default_state_get() and devm_led_classdev_register_ext() stubs

["Arnd Bergmann" <arnd@arndb.de>]

On Tue, Jan 9, 2024, at 11:10, Heiner Kallweit wrote:
> On 09.01.2024 10:06, Arnd Bergmann wrote:
>> From: Arnd Bergmann <arnd@arndb.de>
>> 
>> 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@gmail.com>
>> Link: https://lore.kernel.org/linux-leds/0f6f432b-c650-4bb8-a1b5-fe3372804d52@gmail.com/T/#u
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> ---
>
> 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