diff mbox

gpio: fix ifnullfree.cocci warnings (fwd)

Message ID alpine.DEB.2.20.1803300829580.2870@hadrien (mailing list archive)
State New, archived
Headers show

Commit Message

Julia Lawall March 30, 2018, 6:32 a.m. UTC 
From: Fengguang Wu <fengguang.wu@intel.com>

NULL check before kfree is not needed.

Generated by: scripts/coccinelle/free/ifnullfree.cocci

Fixes: ad85ae608fe0 ("gpio: Remove VLA from gpiolib")
CC: Laura Abbott <labbott@redhat.com>
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
Signed-off-by: Julia Lawall <julia.lawall@lip6.fr>

---

url:
https://github.com/0day-ci/linux/commits/Laura-Abbott/gpio-Remove-VLA-from-gpiolib/20180330-065639
base:
https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git
for-next

 gpiolib.c |    9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

Comments

Lukas Wunner April 2, 2018, 8:31 a.m. UTC | #1 
On Fri, Mar 30, 2018 at 08:32:56AM +0200, Julia Lawall wrote:
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2774,8 +2773,7 @@ int gpiod_get_array_value_complex(bool r
>  			trace_gpio_value(desc_to_gpio(desc), 1, value);
>  		}
> 
> -		if (slowpath)
> -			kfree(slowpath);
> +		kfree(slowpath);
>  	}
>  	return 0;
>  }
> @@ -3020,8 +3018,7 @@ int gpiod_set_array_value_complex(bool r
>  		if (count != 0)
>  			gpio_chip_set_multiple(chip, mask, bits);
> 
> -		if (slowpath)
> -			kfree(slowpath);
> +		kfree(slowpath);
>  	}
>  	return 0;
>  }

The problem I see here is that kfree may not be in L1 cache,
and in that case checking for non-NULL locally in this function
should actually be cheaper.

Note that kfree() need only be called in the slowpath, which
is the *unlikely* case.  Letting the branch predictor assume
that kfree() is not called is the right thing to do here.

The function is a hot path, on the Revolution Pi open source PLCs
we're calling it every 250 usec to poll digital inputs and update
digital outputs.

Would "if (unlikely(slowpath))" be sufficient to make coccinelle
happy?  That's what I'd suggest then.

Otherwise "if (unlikely(chip->ngpio > FASTPATH_NGPIO))" could be used,
though that might be minimally slower due to the pointer chasing.


> @@ -2758,8 +2758,7 @@ int gpiod_get_array_value_complex(bool r
> 
>  		ret = gpio_chip_get_multiple(chip, mask, bits);
>  		if (ret) {
> -			if (slowpath)
> -				kfree(slowpath);
> +			kfree(slowpath);
>  			return ret;
>  		}
> 

This particular change on the other hand is fine because the kfree()
is occurring in an error path, which we'll normally not enter anyway.

Thanks,

Lukas
Julia Lawall April 2, 2018, 9:04 a.m. UTC | #2 
On Mon, 2 Apr 2018, Lukas Wunner wrote:

> On Fri, Mar 30, 2018 at 08:32:56AM +0200, Julia Lawall wrote:
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -2774,8 +2773,7 @@ int gpiod_get_array_value_complex(bool r
> >  			trace_gpio_value(desc_to_gpio(desc), 1, value);
> >  		}
> >
> > -		if (slowpath)
> > -			kfree(slowpath);
> > +		kfree(slowpath);
> >  	}
> >  	return 0;
> >  }
> > @@ -3020,8 +3018,7 @@ int gpiod_set_array_value_complex(bool r
> >  		if (count != 0)
> >  			gpio_chip_set_multiple(chip, mask, bits);
> >
> > -		if (slowpath)
> > -			kfree(slowpath);
> > +		kfree(slowpath);
> >  	}
> >  	return 0;
> >  }
>
> The problem I see here is that kfree may not be in L1 cache,
> and in that case checking for non-NULL locally in this function
> should actually be cheaper.
>
> Note that kfree() need only be called in the slowpath, which
> is the *unlikely* case.  Letting the branch predictor assume
> that kfree() is not called is the right thing to do here.
>
> The function is a hot path, on the Revolution Pi open source PLCs
> we're calling it every 250 usec to poll digital inputs and update
> digital outputs.
>
> Would "if (unlikely(slowpath))" be sufficient to make coccinelle
> happy?  That's what I'd suggest then.

I think that indeed it would not complain any more in this case.  But at
least from kbuild, you shouldn't get another report on this, even if you
don't change it.  Of course, someone else running the script could find
the issue again.  If it would seem appropriate to you, adding a comment
would also at least capture the knowledge that the change is not wanted.

julia

>
> Otherwise "if (unlikely(chip->ngpio > FASTPATH_NGPIO))" could be used,
> though that might be minimally slower due to the pointer chasing.
>
>
> > @@ -2758,8 +2758,7 @@ int gpiod_get_array_value_complex(bool r
> >
> >  		ret = gpio_chip_get_multiple(chip, mask, bits);
> >  		if (ret) {
> > -			if (slowpath)
> > -				kfree(slowpath);
> > +			kfree(slowpath);
> >  			return ret;
> >  		}
> >
>
> This particular change on the other hand is fine because the kfree()
> is occurring in an error path, which we'll normally not enter anyway.
>
> Thanks,
>
> Lukas
>
diff mbox

Patch

--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2758,8 +2758,7 @@  int gpiod_get_array_value_complex(bool r

 		ret = gpio_chip_get_multiple(chip, mask, bits);
 		if (ret) {
-			if (slowpath)
-				kfree(slowpath);
+			kfree(slowpath);
 			return ret;
 		}

@@ -2774,8 +2773,7 @@  int gpiod_get_array_value_complex(bool r
 			trace_gpio_value(desc_to_gpio(desc), 1, value);
 		}

-		if (slowpath)
-			kfree(slowpath);
+		kfree(slowpath);
 	}
 	return 0;
 }
@@ -3020,8 +3018,7 @@  int gpiod_set_array_value_complex(bool r
 		if (count != 0)
 			gpio_chip_set_multiple(chip, mask, bits);

-		if (slowpath)
-			kfree(slowpath);
+		kfree(slowpath);
 	}
 	return 0;
 }