linux-kernel - Re: [PATCH 1/2] leds: remove led_init_default_state_get() and devm_led_classdev_register

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4c859da1-9551-4d0b-a19c-f20f1133acac@gmail.com>
Date: Tue, 9 Jan 2024 11:10:08 +0100
From: Heiner Kallweit <hkallweit1@...il.com>
To: Arnd Bergmann <arnd@...nel.org>, Pavel Machek <pavel@....cz>,
 Lee Jones <lee@...nel.org>
Cc: Andrew Lunn <andrew@...n.ch>, Arnd Bergmann <arnd@...db.de>,
 Hans de Goede <hdegoede@...hat.com>,
 Jean-Jacques Hiblot <jjhiblot@...phandler.com>, linux-leds@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] leds: remove led_init_default_state_get() and
 devm_led_classdev_register_ext() stubs

On 09.01.2024 10:06, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@...db.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@...il.com>
> Link: https://lore.kernel.org/linux-leds/0f6f432b-c650-4bb8-a1b5-fe3372804d52@gmail.com/T/#u
> Signed-off-by: Arnd Bergmann <arnd@...db.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

Personally I don't have a strong opinion pro/con IS_REACHABLE.
I see two options for now:
1. If outcome should be that stubs are a good thing, then the condition
   for the stub should make sure that the stub is chosen when needed.
   This would result in using IS_REACHABLE.
2. If stubs are removed (but also in the current situation, see example),
   then it seems some drivers need adding proper build dependencies.