| Message ID | 20240109090715.982332-1-arnd@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] leds: remove led_init_default_state_get() and devm_led_classdev_register_ext() stubs | expand |
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
Hey Arnd, Many thanks for the patch. Arnd Bergmann <arnd@kernel.org> writes: > From: Arnd Bergmann <arnd@arndb.de> > > Along the same lines as making devm_led_classdev_register() declared > extern unconditional, do the same thing for the two sub-classes > that have similar stubs. > > The users of these interfaces go to great lengths to allow building > with both the generic leds API and the extended version, but realistically > there is not much use in this, so just simplify it to always rely > on it and remove the confusing fallback logic. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/leds/Kconfig | 4 ++-- > drivers/leds/flash/Kconfig | 4 ++-- > drivers/staging/greybus/Kconfig | 2 +- > drivers/staging/greybus/light.c | 21 -------------------- > include/linux/led-class-flash.h | 24 ----------------------- > include/linux/led-class-multicolor.h | 29 ---------------------------- > 6 files changed, 5 insertions(+), 79 deletions(-) > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > index d721b254e1e4..9613a45a35bd 100644 > --- a/drivers/leds/Kconfig > +++ b/drivers/leds/Kconfig > @@ -395,7 +395,7 @@ config LEDS_LP3952 > config LEDS_LP50XX > tristate "LED Support for TI LP5036/30/24/18/12/09 LED driver chip" > depends on LEDS_CLASS && REGMAP_I2C > - depends on LEDS_CLASS_MULTICOLOR || !LEDS_CLASS_MULTICOLOR > + depends on LEDS_CLASS_MULTICOLOR > help > If you say yes here you get support for the Texas Instruments > LP5036, LP5030, LP5024, LP5018, LP5012 and LP5009 LED driver. > @@ -406,7 +406,7 @@ config LEDS_LP50XX > config LEDS_LP55XX_COMMON > tristate "Common Driver for TI/National LP5521/5523/55231/5562/8501" > depends on LEDS_CLASS > - depends on LEDS_CLASS_MULTICOLOR || !LEDS_CLASS_MULTICOLOR > + depends on LEDS_CLASS_MULTICOLOR > depends on OF > depends on I2C > select FW_LOADER > diff --git a/drivers/leds/flash/Kconfig b/drivers/leds/flash/Kconfig > index 4e08dbc05709..b95f90cd5749 100644 > --- a/drivers/leds/flash/Kconfig > +++ b/drivers/leds/flash/Kconfig > @@ -51,8 +51,8 @@ config LEDS_MAX77693 > config LEDS_MT6360 > tristate "LED Support for Mediatek MT6360 PMIC" > depends on LEDS_CLASS && OF > - depends on LEDS_CLASS_FLASH || !LEDS_CLASS_FLASH > - depends on LEDS_CLASS_MULTICOLOR || !LEDS_CLASS_MULTICOLOR > + depends on LEDS_CLASS_FLASH > + depends on LEDS_CLASS_MULTICOLOR > depends on V4L2_FLASH_LED_CLASS || !V4L2_FLASH_LED_CLASS > depends on MFD_MT6360 > help > diff --git a/drivers/staging/greybus/Kconfig b/drivers/staging/greybus/Kconfig > index 927cfa4bc989..1e745a8d439c 100644 > --- a/drivers/staging/greybus/Kconfig > +++ b/drivers/staging/greybus/Kconfig > @@ -64,7 +64,7 @@ config GREYBUS_HID > > config GREYBUS_LIGHT > tristate "Greybus LED Class driver" > - depends on LEDS_CLASS > + depends on LEDS_CLASS_FLASH Agree with the change for the greybus driver, one note, since you are cleaning up this, maybe also remove the other #if down when setting the flash specific operations, right? #if IS_REACHABLE(CONFIG_LEDS_CLASS_FLASH) /* Flash specific operations */ static int gb_lights_flash_intensity_set(struct led_classdev_flash *fcdev, u32 brightness) { struct gb_channel *channel = container_of(fcdev, struct gb_channel, fled); int ret; . . . . #else static int gb_lights_channel_flash_config(struct gb_channel *channel) { struct gb_connection *connection = get_conn_from_channel(channel); dev_err(&connection->bundle->dev, "no support for flash devices\n"); return 0; } . . . Thanks again. Cheers, Rui > help > Select this option if you have a device that follows the > Greybus LED Class specification. > diff --git a/drivers/staging/greybus/light.c b/drivers/staging/greybus/light.c > index 87d36948c610..d62f97249aca 100644 > --- a/drivers/staging/greybus/light.c > +++ b/drivers/staging/greybus/light.c > @@ -29,13 +29,9 @@ struct gb_channel { > struct attribute_group *attr_group; > const struct attribute_group **attr_groups; > struct led_classdev *led; > -#if IS_REACHABLE(CONFIG_LEDS_CLASS_FLASH) > struct led_classdev_flash fled; > struct led_flash_setting intensity_uA; > struct led_flash_setting timeout_us; > -#else > - struct led_classdev cled; > -#endif > struct gb_light *light; > bool is_registered; > bool releasing; > @@ -84,7 +80,6 @@ static bool is_channel_flash(struct gb_channel *channel) > | GB_CHANNEL_MODE_INDICATOR)); > } > > -#if IS_REACHABLE(CONFIG_LEDS_CLASS_FLASH) > static struct gb_channel *get_channel_from_cdev(struct led_classdev *cdev) > { > struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(cdev); > @@ -153,22 +148,6 @@ static int __gb_lights_flash_brightness_set(struct gb_channel *channel) > > return __gb_lights_flash_intensity_set(channel, intensity); > } > -#else > -static struct gb_channel *get_channel_from_cdev(struct led_classdev *cdev) > -{ > - return container_of(cdev, struct gb_channel, cled); > -} > - > -static struct led_classdev *get_channel_cdev(struct gb_channel *channel) > -{ > - return &channel->cled; > -} > - > -static int __gb_lights_flash_brightness_set(struct gb_channel *channel) > -{ > - return 0; > -} > -#endif > > static int gb_lights_color_set(struct gb_channel *channel, u32 color); > static int gb_lights_fade_set(struct gb_channel *channel); > diff --git a/include/linux/led-class-flash.h b/include/linux/led-class-flash.h > index 612b4cab3819..36df927ec4b7 100644 > --- a/include/linux/led-class-flash.h > +++ b/include/linux/led-class-flash.h > @@ -85,7 +85,6 @@ static inline struct led_classdev_flash *lcdev_to_flcdev( > return container_of(lcdev, struct led_classdev_flash, led_cdev); > } > > -#if IS_ENABLED(CONFIG_LEDS_CLASS_FLASH) > /** > * led_classdev_flash_register_ext - register a new object of LED class with > * init data and with support for flash LEDs > @@ -116,29 +115,6 @@ int devm_led_classdev_flash_register_ext(struct device *parent, > void devm_led_classdev_flash_unregister(struct device *parent, > struct led_classdev_flash *fled_cdev); > > -#else > - > -static inline int led_classdev_flash_register_ext(struct device *parent, > - struct led_classdev_flash *fled_cdev, > - struct led_init_data *init_data) > -{ > - return 0; > -} > - > -static inline void led_classdev_flash_unregister(struct led_classdev_flash *fled_cdev) {}; > -static inline int devm_led_classdev_flash_register_ext(struct device *parent, > - struct led_classdev_flash *fled_cdev, > - struct led_init_data *init_data) > -{ > - return 0; > -} > - > -static inline void devm_led_classdev_flash_unregister(struct device *parent, > - struct led_classdev_flash *fled_cdev) > -{}; > - > -#endif /* IS_ENABLED(CONFIG_LEDS_CLASS_FLASH) */ > - > static inline int led_classdev_flash_register(struct device *parent, > struct led_classdev_flash *fled_cdev) > { > diff --git a/include/linux/led-class-multicolor.h b/include/linux/led-class-multicolor.h > index 210d57bcd767..db9f34c6736e 100644 > --- a/include/linux/led-class-multicolor.h > +++ b/include/linux/led-class-multicolor.h > @@ -30,7 +30,6 @@ static inline struct led_classdev_mc *lcdev_to_mccdev( > return container_of(led_cdev, struct led_classdev_mc, led_cdev); > } > > -#if IS_ENABLED(CONFIG_LEDS_CLASS_MULTICOLOR) > /** > * led_classdev_multicolor_register_ext - register a new object of led_classdev > * class with support for multicolor LEDs > @@ -64,34 +63,6 @@ int devm_led_classdev_multicolor_register_ext(struct device *parent, > > void devm_led_classdev_multicolor_unregister(struct device *parent, > struct led_classdev_mc *mcled_cdev); > -#else > - > -static inline int led_classdev_multicolor_register_ext(struct device *parent, > - struct led_classdev_mc *mcled_cdev, > - struct led_init_data *init_data) > -{ > - return 0; > -} > - > -static inline void led_classdev_multicolor_unregister(struct led_classdev_mc *mcled_cdev) {}; > -static inline int led_mc_calc_color_components(struct led_classdev_mc *mcled_cdev, > - enum led_brightness brightness) > -{ > - return 0; > -} > - > -static inline int devm_led_classdev_multicolor_register_ext(struct device *parent, > - struct led_classdev_mc *mcled_cdev, > - struct led_init_data *init_data) > -{ > - return 0; > -} > - > -static inline void devm_led_classdev_multicolor_unregister(struct device *parent, > - struct led_classdev_mc *mcled_cdev) > -{}; > - > -#endif /* IS_ENABLED(CONFIG_LEDS_CLASS_MULTICOLOR) */ > > static inline int led_classdev_multicolor_register(struct device *parent, > struct led_classdev_mc *mcled_cdev) > -- > 2.39.2
On Tue, Jan 09, 2024 at 10:06:40AM +0100, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > Along the same lines as making devm_led_classdev_register() declared > extern unconditional, do the same thing for the two sub-classes > that have similar stubs. > > The users of these interfaces go to great lengths to allow building > with both the generic leds API and the extended version, but realistically > there is not much use in this, so just simplify it to always rely > on it and remove the confusing fallback logic. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/leds/Kconfig | 4 ++-- > drivers/leds/flash/Kconfig | 4 ++-- > drivers/staging/greybus/Kconfig | 2 +- > drivers/staging/greybus/light.c | 21 -------------------- > include/linux/led-class-flash.h | 24 ----------------------- > include/linux/led-class-multicolor.h | 29 ---------------------------- > 6 files changed, 5 insertions(+), 79 deletions(-) Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
On Tue, 09 Jan 2024 10:06:39 +0100, Arnd Bergmann wrote: > 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. > > [...] Applied, thanks! [1/2] leds: remove led_init_default_state_get() and devm_led_classdev_register_ext() stubs commit: c0b64609dada48907b04b48873ba052efa8f121d [2/2] leds: make flash and multicolor dependencies unconditional commit: 54602f38551e89b520611ffb9df05232d1bf73f8 -- Lee Jones [李琼斯]
diff --git a/include/linux/leds.h b/include/linux/leds.h index 4754b02d3a2c..7598d472903a 100644 --- a/include/linux/leds.h +++ b/include/linux/leds.h @@ -82,15 +82,7 @@ struct led_init_data { bool devname_mandatory; }; -#if IS_ENABLED(CONFIG_NEW_LEDS) enum led_default_state led_init_default_state_get(struct fwnode_handle *fwnode); -#else -static inline enum led_default_state -led_init_default_state_get(struct fwnode_handle *fwnode) -{ - return LEDS_DEFSTATE_OFF; -} -#endif struct led_hw_trigger_type { int dummy; @@ -279,20 +271,9 @@ static inline int led_classdev_register(struct device *parent, return led_classdev_register_ext(parent, led_cdev, NULL); } -#if IS_ENABLED(CONFIG_LEDS_CLASS) int devm_led_classdev_register_ext(struct device *parent, struct led_classdev *led_cdev, struct led_init_data *init_data); -#else -static inline int -devm_led_classdev_register_ext(struct device *parent, - struct led_classdev *led_cdev, - struct led_init_data *init_data) -{ - return 0; -} -#endif - static inline int devm_led_classdev_register(struct device *parent, struct led_classdev *led_cdev) {