Received-SPF: pass (google.com: domain of linux-kernel+bounces-20717-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Feedback-ID: i56a14606:Fastmail
User-Agent: Cyrus-JMAP/3.9.0-alpha0-1364-ga51d5fd3b7-fm-20231219.001-ga51d5fd3
Precedence: bulk
MIME-Version: 1.0
Message-Id: <468ce58a-9f4a-4a75-adbf-7beb8fa13580@app.fastmail.com>
In-Reply-To: <4c859da1-9551-4d0b-a19c-f20f1133acac@gmail.com>
References: <20240109090715.982332-1-arnd@kernel.org>
 <4c859da1-9551-4d0b-a19c-f20f1133acac@gmail.com>
Date: Tue, 09 Jan 2024 11:35:25 +0100
From: "Arnd Bergmann" <arnd@arndb.de>
To: "Heiner Kallweit" <hkallweit1@gmail.com>,
 "Arnd Bergmann" <arnd@kernel.org>, "Pavel Machek" <pavel@ucw.cz>,
 "Lee Jones" <lee@kernel.org>
Cc: "Andrew Lunn" <andrew@lunn.ch>, "Hans de Goede" <hdegoede@redhat.com>,
 "Jean-Jacques Hiblot" <jjhiblot@traphandler.com>, linux-leds@vger.kernel.org,
 linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] leds: remove led_init_default_state_get() and
 devm_led_classdev_register_ext() stubs
Content-Type: text/plain

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