[PATCH] leds: trigger: oneshot: One function call less in pattern_init() after error detection

[PATCH] leds: trigger: oneshot: One function call less in pattern_init() after error detection

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 26 Dec 2023 22:02:08 +0100

The kfree() function was called in one case by
the pattern_init() function during error handling
even if the passed variable contained a null pointer.
This issue was detected by using the Coccinelle software.

Thus use another label.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/leds/trigger/ledtrig-oneshot.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/trigger/ledtrig-oneshot.c b/drivers/leds/trigger/ledtrig-oneshot.c
index bee3bd452abf..31061ec0afe6 100644
--- a/drivers/leds/trigger/ledtrig-oneshot.c
+++ b/drivers/leds/trigger/ledtrig-oneshot.c
@@ -134,7 +134,7 @@ static void pattern_init(struct led_classdev *led_cdev)

 	pattern = led_get_default_pattern(led_cdev, &size);
 	if (!pattern)
-		goto out_default;
+		goto out_settings;

 	if (size != 2) {
 		dev_warn(led_cdev->dev,
@@ -151,6 +151,7 @@ static void pattern_init(struct led_classdev *led_cdev)

 out_default:
 	kfree(pattern);
+out_settings:
 	led_cdev->blink_delay_on = DEFAULT_DELAY;
 	led_cdev->blink_delay_off = DEFAULT_DELAY;
 }
--
2.43.0

Re: [PATCH] leds: trigger: oneshot: One function call less in pattern_init() after error detection

[Lee Jones]

On Tue, 26 Dec 2023, Markus Elfring wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 26 Dec 2023 22:02:08 +0100
> 
> The kfree() function was called in one case by
> the pattern_init() function during error handling
> even if the passed variable contained a null pointer.

It's totally valid to call kfree() on a NULL pointer:

  * If @object is NULL, no operation is performed.

Why do we care all that much?

> This issue was detected by using the Coccinelle software.
> 
> Thus use another label.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/leds/trigger/ledtrig-oneshot.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/leds/trigger/ledtrig-oneshot.c b/drivers/leds/trigger/ledtrig-oneshot.c
> index bee3bd452abf..31061ec0afe6 100644
> --- a/drivers/leds/trigger/ledtrig-oneshot.c
> +++ b/drivers/leds/trigger/ledtrig-oneshot.c
> @@ -134,7 +134,7 @@ static void pattern_init(struct led_classdev *led_cdev)
> 
>  	pattern = led_get_default_pattern(led_cdev, &size);
>  	if (!pattern)
> -		goto out_default;
> +		goto out_settings;
> 
>  	if (size != 2) {
>  		dev_warn(led_cdev->dev,
> @@ -151,6 +151,7 @@ static void pattern_init(struct led_classdev *led_cdev)
> 
>  out_default:
>  	kfree(pattern);
> +out_settings:
>  	led_cdev->blink_delay_on = DEFAULT_DELAY;
>  	led_cdev->blink_delay_off = DEFAULT_DELAY;
>  }
> --
> 2.43.0
> 

-- 
Lee Jones [李琼斯]

Re: leds: trigger: oneshot: One function call less in pattern_init() after error detection

[Markus Elfring]

>> The kfree() function was called in one case by
>> the pattern_init() function during error handling
>> even if the passed variable contained a null pointer.
>
> It's totally valid to call kfree() on a NULL pointer:
>
>   * If @object is NULL, no operation is performed.
>
> Why do we care all that much?

Would you dare to categorise such special function calls as redundant?

Should they be skipped in more cases?

See also:
https://wiki.sei.cmu.edu/confluence/display/c/MEM12-C.+Consider+using+a+goto+chain+when+leaving+a+function+on+error+when+using+and+releasing+resources


Regards,
Markus

Re: leds: trigger: oneshot: One function call less in pattern_init() after error detection

[Lee Jones]

On Thu, 11 Jan 2024, Markus Elfring wrote:

> >> The kfree() function was called in one case by
> >> the pattern_init() function during error handling
> >> even if the passed variable contained a null pointer.
> >
> > It's totally valid to call kfree() on a NULL pointer:
> >
> >   * If @object is NULL, no operation is performed.
> >
> > Why do we care all that much?
> 
> Would you dare to categorise such special function calls as redundant?
> 
> Should they be skipped in more cases?
> 
> See also:
> https://wiki.sei.cmu.edu/confluence/display/c/MEM12-C.+Consider+using+a+goto+chain+when+leaving+a+function+on+error+when+using+and+releasing+resources

I have no idea what you're trying to say.

The premise of your patch is based on the fact that we shouldn't call
kfree() with a NULL pointer.  When in actual fact kfree() is more than
capable of handling cases where the passed object is NULL, and goes as
far as to document as such.  Meaning that unless I'm convinced
otherwise, patches like this remain in the category of churn.

-- 
Lee Jones [李琼斯]