[PATCH v1] leds: Allow to call led_classdev_unregister() unconditionally

[PATCH v1] leds: Allow to call led_classdev_unregister() unconditionally

[Andy Shevchenko]

If in the certain driver the LED is optional, and it's a majority of them,
the call of led_classdev_unregister() still requires some additional checks.

The usual pattern on unregistering is to check for NULL, but we also check
for IS_ERR() in case device_create_with_groups() fails.

The change will reduce a burden in a lot of drivers to repeatedly check
for above conditions.

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

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index d231240c2047..80b62b807ea0 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -287,6 +287,7 @@ int led_classdev_register_ext(struct device *parent,
 		ret = led_add_brightness_hw_changed(led_cdev);
 		if (ret) {
 			device_unregister(led_cdev->dev);
+			led_cdev->dev = NULL;
 			mutex_unlock(&led_cdev->led_access);
 			return ret;
 		}
@@ -332,6 +333,9 @@ EXPORT_SYMBOL_GPL(led_classdev_register_ext);
  */
 void led_classdev_unregister(struct led_classdev *led_cdev)
 {
+	if (IS_ERR_OR_NULL(led_cdev->dev))
+		return;
+
 #ifdef CONFIG_LEDS_TRIGGERS
 	down_write(&led_cdev->trigger_lock);
 	if (led_cdev->trigger)
-- 
2.23.0.rc1

Re: [PATCH v1] leds: Allow to call led_classdev_unregister() unconditionally

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1641 bytes --]

Hi!

> If in the certain driver the LED is optional, and it's a majority of them,
> the call of led_classdev_unregister() still requires some additional checks.
> 
> The usual pattern on unregistering is to check for NULL, but we also check
> for IS_ERR() in case device_create_with_groups() fails.
> 
> The change will reduce a burden in a lot of drivers to repeatedly check
> for above conditions.

I don't see majority of calls being protected.  Doing nothing on NULL
sounds reasonable. I'm less sure about "IS_ERR"...

Best regards,
								Pavel


> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/leds/led-class.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index d231240c2047..80b62b807ea0 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -287,6 +287,7 @@ int led_classdev_register_ext(struct device *parent,
>  		ret = led_add_brightness_hw_changed(led_cdev);
>  		if (ret) {
>  			device_unregister(led_cdev->dev);
> +			led_cdev->dev = NULL;
>  			mutex_unlock(&led_cdev->led_access);
>  			return ret;
>  		}
> @@ -332,6 +333,9 @@ EXPORT_SYMBOL_GPL(led_classdev_register_ext);
>   */
>  void led_classdev_unregister(struct led_classdev *led_cdev)
>  {
> +	if (IS_ERR_OR_NULL(led_cdev->dev))
> +		return;
> +
>  #ifdef CONFIG_LEDS_TRIGGERS
>  	down_write(&led_cdev->trigger_lock);
>  	if (led_cdev->trigger)

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v1] leds: Allow to call led_classdev_unregister() unconditionally

[Andy Shevchenko]

On Fri, Aug 16, 2019 at 01:49:08PM +0200, Pavel Machek wrote:
> Hi!
> 
> > If in the certain driver the LED is optional, and it's a majority of them,
> > the call of led_classdev_unregister() still requires some additional checks.
> > 
> > The usual pattern on unregistering is to check for NULL, but we also check
> > for IS_ERR() in case device_create_with_groups() fails.
> > 
> > The change will reduce a burden in a lot of drivers to repeatedly check
> > for above conditions.
> 
> I don't see majority of calls being protected.  Doing nothing on NULL
> sounds reasonable. I'm less sure about "IS_ERR"...

Majority by drivers, not by absolute number of calls you can grep for.

What I'm referring to is, for example,

     commit 99fef587ff98894426d9bf1f5b7336345052d4b3
     Author: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
     Date:   Mon Dec 3 20:21:41 2018 +0200

         driver core: platform: Respect return code of platform_device_register_full()

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1] leds: Allow to call led_classdev_unregister() unconditionally

[Jacek Anaszewski]

On 8/16/19 1:49 PM, Pavel Machek wrote:
> Hi!
> 
>> If in the certain driver the LED is optional, and it's a majority of them,
>> the call of led_classdev_unregister() still requires some additional checks.
>>
>> The usual pattern on unregistering is to check for NULL, but we also check
>> for IS_ERR() in case device_create_with_groups() fails.
>>
>> The change will reduce a burden in a lot of drivers to repeatedly check
>> for above conditions.
> 
> I don't see majority of calls being protected.  Doing nothing on NULL
> sounds reasonable. I'm less sure about "IS_ERR"...

device_create_groups_vargs() returns ERR_PTR(retval) on error so
led_cdev->dev may be left non-NULL even on error.

We can be correct checking only for NULL if we will add
"led_cdev->dev = NULL" assignment in "if (IS_ERR(led_cdev->dev)" branch
in led_classdev_register_ext(). Nonetheless I'm personally in favor of
Andy's solution.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v1] leds: Allow to call led_classdev_unregister() unconditionally

[Jacek Anaszewski]

On 8/16/19 8:02 PM, Jacek Anaszewski wrote:
> On 8/16/19 1:49 PM, Pavel Machek wrote:
>> Hi!
>>
>>> If in the certain driver the LED is optional, and it's a majority of them,
>>> the call of led_classdev_unregister() still requires some additional checks.
>>>
>>> The usual pattern on unregistering is to check for NULL, but we also check
>>> for IS_ERR() in case device_create_with_groups() fails.
>>>
>>> The change will reduce a burden in a lot of drivers to repeatedly check
>>> for above conditions.
>>
>> I don't see majority of calls being protected.  Doing nothing on NULL
>> sounds reasonable. I'm less sure about "IS_ERR"...
> 
> device_create_groups_vargs() returns ERR_PTR(retval) on error so
> led_cdev->dev may be left non-NULL even on error.
> 
> We can be correct checking only for NULL if we will add
> "led_cdev->dev = NULL" assignment in "if (IS_ERR(led_cdev->dev)" branch
> in led_classdev_register_ext(). Nonetheless I'm personally in favor of
> Andy's solution.
> 

Patch applied, thanks.

-- 
Best regards,
Jacek Anaszewski