[PATCH v2 0/2] leds: class: improve led_remove_lookup()

[PATCH v2 0/2] leds: class: improve led_remove_lookup()

[Andy Shevchenko]

This mini-series targets led_remove_lookup() to be more handy for
the optional resources. The first patch makes it no-op for NULL
pointer and the second one makes it idempotent.

Changelog v2:
- added patch 2

v1: 20260327102729.797254-1-andriy.shevchenko@linux.intel.com

Andy Shevchenko (2):
  leds: class: Make led_remove_lookup() NULL-aware
  leds: class: Reinitialise list after dropping from lookup table

 drivers/leds/led-class.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

-- 
2.50.1

[PATCH v2 1/2] leds: class: Make led_remove_lookup() NULL-aware

[Andy Shevchenko]

It is a usual pattern in the kernel to make releasing functions be NULL-aware
so they become a no-op. This helps reducing unneeded checks in the code where
the given resource is optional.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/leds/led-class.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index cad55e2beafa..b53ebe3a0faa 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -421,6 +421,9 @@ EXPORT_SYMBOL_GPL(led_add_lookup);
  */
 void led_remove_lookup(struct led_lookup_data *led_lookup)
 {
+	if (!led_lookup)
+		return;
+
 	mutex_lock(&leds_lookup_lock);
 	list_del(&led_lookup->list);
 	mutex_unlock(&leds_lookup_lock);
-- 
2.50.1

[PATCH v2 2/2] leds: class: Reinitialise list after dropping from lookup table

[Andy Shevchenko]

Currently the lookup table just removes the list entry and leaves
the stale pointers in it. If the lookup is embedded in some data
structure, the pointer to the lookup entry can't be NULL (always
valid), but calling led_remove_lookup() on it twice will lead to
the wrong behaviour. To avoid that the user has to track the state
itself. With this change in place, the user may drop that approach
and use something like

  probe:
	INIT_LIST_HEAD(&lookup.list);
	if (LED lookup is required)
		led_add_lookup(&lookup);

  remove:
	led_remove_lookup(&lookup);

without any additional tracking kept over the device lifetime.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/leds/led-class.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index b53ebe3a0faa..424c07e0ecce 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -425,7 +425,7 @@ void led_remove_lookup(struct led_lookup_data *led_lookup)
 		return;
 
 	mutex_lock(&leds_lookup_lock);
-	list_del(&led_lookup->list);
+	list_del_init(&led_lookup->list);
 	mutex_unlock(&leds_lookup_lock);
 }
 EXPORT_SYMBOL_GPL(led_remove_lookup);
-- 
2.50.1

Re: [PATCH v2 2/2] leds: class: Reinitialise list after dropping from lookup table

[Lee Jones]

On Tue, 31 Mar 2026, Andy Shevchenko wrote:

> Currently the lookup table just removes the list entry and leaves
> the stale pointers in it. If the lookup is embedded in some data
> structure, the pointer to the lookup entry can't be NULL (always
> valid), but calling led_remove_lookup() on it twice will lead to
> the wrong behaviour. To avoid that the user has to track the state
> itself. With this change in place, the user may drop that approach
> and use something like
> 
>   probe:
> 	INIT_LIST_HEAD(&lookup.list);
> 	if (LED lookup is required)
> 		led_add_lookup(&lookup);
> 
>   remove:
> 	led_remove_lookup(&lookup);

How do we feel about a devm_led_add_lookup()?

> without any additional tracking kept over the device lifetime.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/leds/led-class.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index b53ebe3a0faa..424c07e0ecce 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -425,7 +425,7 @@ void led_remove_lookup(struct led_lookup_data *led_lookup)
>  		return;
>  
>  	mutex_lock(&leds_lookup_lock);
> -	list_del(&led_lookup->list);
> +	list_del_init(&led_lookup->list);
>  	mutex_unlock(&leds_lookup_lock);
>  }
>  EXPORT_SYMBOL_GPL(led_remove_lookup);
> -- 
> 2.50.1
> 

-- 
Lee Jones [李琼斯]

Re: [PATCH v2 2/2] leds: class: Reinitialise list after dropping from lookup table

[Andy Shevchenko]

On Thu, Apr 09, 2026 at 04:52:32PM +0100, Lee Jones wrote:
> On Tue, 31 Mar 2026, Andy Shevchenko wrote:
> 
> > Currently the lookup table just removes the list entry and leaves
> > the stale pointers in it. If the lookup is embedded in some data
> > structure, the pointer to the lookup entry can't be NULL (always
> > valid), but calling led_remove_lookup() on it twice will lead to
> > the wrong behaviour. To avoid that the user has to track the state
> > itself. With this change in place, the user may drop that approach
> > and use something like
> > 
> >   probe:
> > 	INIT_LIST_HEAD(&lookup.list);
> > 	if (LED lookup is required)
> > 		led_add_lookup(&lookup);
> > 
> >   remove:
> > 	led_remove_lookup(&lookup);
> 
> How do we feel about a devm_led_add_lookup()?

No strong opinion. If we have that already, great! This patch won't be needed.
If not yet, we may add one, but briefly looking at the kernel I have not found
any other framework doing that.

In any case the user that would need this patch has been already modified the
way that lookup is always there, effectively means we may postpone or even drop
this patch for now.

> > without any additional tracking kept over the device lifetime.

-- 
With Best Regards,
Andy Shevchenko