2020-12-03 07:34:07

by Biwen Li (OSS)

[permalink] [raw]
Subject: [PATCH] gpio: mpc8xxx: resolve coverity warnings

From: Biwen Li <[email protected]>

Resolve coverity warnings as follows,
cond_at_most: Checking gpio >= 28U implies that gpio may be up
to 27 on the false branch.
overrun-call: Overrunning callees array of size 3 by passing
argument gpio (which evaluates to 27)
in call to *mpc8xxx_gc->direction_output

cond_at_least: Checking gpio <= 3U implies that gpio is at least 4 on
the false branch.
overrun-call: Overrunning callee's array of size 3 by passing argument
gpio (which evaluates to 4) in call to *mpc8xxx_gc->direction_output

Signed-off-by: Biwen Li <[email protected]>
---
drivers/gpio/gpio-mpc8xxx.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-mpc8xxx.c b/drivers/gpio/gpio-mpc8xxx.c
index a6c2bbdcaa10..12c9a91d87b7 100644
--- a/drivers/gpio/gpio-mpc8xxx.c
+++ b/drivers/gpio/gpio-mpc8xxx.c
@@ -3,6 +3,7 @@
*
* Copyright (C) 2008 Peter Korsgaard <[email protected]>
* Copyright (C) 2016 Freescale Semiconductor Inc.
+ * Copyright 2020 NXP
*
* This file is licensed under the terms of the GNU General Public License
* version 2. This program is licensed "as is" without any warranty of any
@@ -80,7 +81,7 @@ static int mpc5121_gpio_dir_out(struct gpio_chip *gc,
{
struct mpc8xxx_gpio_chip *mpc8xxx_gc = gpiochip_get_data(gc);
/* GPIO 28..31 are input only on MPC5121 */
- if (gpio >= 28)
+ if (gpio >= 28U)
return -EINVAL;

return mpc8xxx_gc->direction_output(gc, gpio, val);
@@ -91,7 +92,7 @@ static int mpc5125_gpio_dir_out(struct gpio_chip *gc,
{
struct mpc8xxx_gpio_chip *mpc8xxx_gc = gpiochip_get_data(gc);
/* GPIO 0..3 are input only on MPC5125 */
- if (gpio <= 3)
+ if (gpio <= 3U)
return -EINVAL;

return mpc8xxx_gc->direction_output(gc, gpio, val);
--
2.17.1


2020-12-03 08:06:50

by Bartosz Golaszewski

[permalink] [raw]
Subject: Re: [PATCH] gpio: mpc8xxx: resolve coverity warnings

On Thu, Dec 3, 2020 at 8:31 AM Biwen Li <[email protected]> wrote:
>
> From: Biwen Li <[email protected]>
>
> Resolve coverity warnings as follows,
> cond_at_most: Checking gpio >= 28U implies that gpio may be up
> to 27 on the false branch.
> overrun-call: Overrunning callees array of size 3 by passing
> argument gpio (which evaluates to 27)
> in call to *mpc8xxx_gc->direction_output
>
> cond_at_least: Checking gpio <= 3U implies that gpio is at least 4 on
> the false branch.
> overrun-call: Overrunning callee's array of size 3 by passing argument
> gpio (which evaluates to 4) in call to *mpc8xxx_gc->direction_output
>
> Signed-off-by: Biwen Li <[email protected]>
> ---
> drivers/gpio/gpio-mpc8xxx.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpio/gpio-mpc8xxx.c b/drivers/gpio/gpio-mpc8xxx.c
> index a6c2bbdcaa10..12c9a91d87b7 100644
> --- a/drivers/gpio/gpio-mpc8xxx.c
> +++ b/drivers/gpio/gpio-mpc8xxx.c
> @@ -3,6 +3,7 @@
> *
> * Copyright (C) 2008 Peter Korsgaard <[email protected]>
> * Copyright (C) 2016 Freescale Semiconductor Inc.
> + * Copyright 2020 NXP

A copyright notice on a two-line change is a bit too much, don't you think?

> *
> * This file is licensed under the terms of the GNU General Public License
> * version 2. This program is licensed "as is" without any warranty of any
> @@ -80,7 +81,7 @@ static int mpc5121_gpio_dir_out(struct gpio_chip *gc,
> {
> struct mpc8xxx_gpio_chip *mpc8xxx_gc = gpiochip_get_data(gc);
> /* GPIO 28..31 are input only on MPC5121 */
> - if (gpio >= 28)
> + if (gpio >= 28U)
> return -EINVAL;

I don't really understand the commit message but looking at the code
is even more confusing. What are you fixing here actually?

Bartosz

>
> return mpc8xxx_gc->direction_output(gc, gpio, val);
> @@ -91,7 +92,7 @@ static int mpc5125_gpio_dir_out(struct gpio_chip *gc,
> {
> struct mpc8xxx_gpio_chip *mpc8xxx_gc = gpiochip_get_data(gc);
> /* GPIO 0..3 are input only on MPC5125 */
> - if (gpio <= 3)
> + if (gpio <= 3U)
> return -EINVAL;
>
> return mpc8xxx_gc->direction_output(gc, gpio, val);
> --
> 2.17.1
>

2020-12-03 08:10:13

by Biwen Li (OSS)

[permalink] [raw]
Subject: RE: [PATCH] gpio: mpc8xxx: resolve coverity warnings

> On Thu, Dec 3, 2020 at 8:31 AM Biwen Li <[email protected]> wrote:
> >
> > From: Biwen Li <[email protected]>
> >
> > Resolve coverity warnings as follows,
> > cond_at_most: Checking gpio >= 28U implies that gpio may be up
> > to 27 on the false branch.
> > overrun-call: Overrunning callees array of size 3 by passing
> > argument gpio (which evaluates to 27)
> > in call to *mpc8xxx_gc->direction_output
> >
> > cond_at_least: Checking gpio <= 3U implies that gpio is at least 4 on
> > the false branch.
> > overrun-call: Overrunning callee's array of size 3 by passing argument
> > gpio (which evaluates to 4) in call to
> > *mpc8xxx_gc->direction_output
> >
> > Signed-off-by: Biwen Li <[email protected]>
> > ---
> > drivers/gpio/gpio-mpc8xxx.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpio/gpio-mpc8xxx.c b/drivers/gpio/gpio-mpc8xxx.c
> > index a6c2bbdcaa10..12c9a91d87b7 100644
> > --- a/drivers/gpio/gpio-mpc8xxx.c
> > +++ b/drivers/gpio/gpio-mpc8xxx.c
> > @@ -3,6 +3,7 @@
> > *
> > * Copyright (C) 2008 Peter Korsgaard <[email protected]>
> > * Copyright (C) 2016 Freescale Semiconductor Inc.
> > + * Copyright 2020 NXP
>
> A copyright notice on a two-line change is a bit too much, don't you think?
Okay, got it. Will remove it in v2.
>
> > *
> > * This file is licensed under the terms of the GNU General Public License
> > * version 2. This program is licensed "as is" without any warranty
> > of any @@ -80,7 +81,7 @@ static int mpc5121_gpio_dir_out(struct
> > gpio_chip *gc, {
> > struct mpc8xxx_gpio_chip *mpc8xxx_gc = gpiochip_get_data(gc);
> > /* GPIO 28..31 are input only on MPC5121 */
> > - if (gpio >= 28)
> > + if (gpio >= 28U)
> > return -EINVAL;
>
> I don't really understand the commit message but looking at the code is even
> more confusing. What are you fixing here actually?
Try to fix code warning that generated by coverity scan tool(static code analysis tool)
>
> Bartosz
>
> >
> > return mpc8xxx_gc->direction_output(gc, gpio, val); @@ -91,7
> > +92,7 @@ static int mpc5125_gpio_dir_out(struct gpio_chip *gc, {
> > struct mpc8xxx_gpio_chip *mpc8xxx_gc = gpiochip_get_data(gc);
> > /* GPIO 0..3 are input only on MPC5125 */
> > - if (gpio <= 3)
> > + if (gpio <= 3U)
> > return -EINVAL;
> >
> > return mpc8xxx_gc->direction_output(gc, gpio, val);
> > --
> > 2.17.1
> >

2020-12-03 08:24:32

by Bartosz Golaszewski

[permalink] [raw]
Subject: Re: [PATCH] gpio: mpc8xxx: resolve coverity warnings

On Thu, Dec 3, 2020 at 9:07 AM Biwen Li (OSS) <[email protected]> wrote:
>
> > On Thu, Dec 3, 2020 at 8:31 AM Biwen Li <[email protected]> wrote:
> > >
> > > From: Biwen Li <[email protected]>
> > >
> > > Resolve coverity warnings as follows,
> > > cond_at_most: Checking gpio >= 28U implies that gpio may be up
> > > to 27 on the false branch.
> > > overrun-call: Overrunning callees array of size 3 by passing
> > > argument gpio (which evaluates to 27)
> > > in call to *mpc8xxx_gc->direction_output
> > >
> > > cond_at_least: Checking gpio <= 3U implies that gpio is at least 4 on
> > > the false branch.
> > > overrun-call: Overrunning callee's array of size 3 by passing argument
> > > gpio (which evaluates to 4) in call to
> > > *mpc8xxx_gc->direction_output
> > >
> > > Signed-off-by: Biwen Li <[email protected]>
> > > ---
> > > drivers/gpio/gpio-mpc8xxx.c | 5 +++--
> > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpio/gpio-mpc8xxx.c b/drivers/gpio/gpio-mpc8xxx.c
> > > index a6c2bbdcaa10..12c9a91d87b7 100644
> > > --- a/drivers/gpio/gpio-mpc8xxx.c
> > > +++ b/drivers/gpio/gpio-mpc8xxx.c
> > > @@ -3,6 +3,7 @@
> > > *
> > > * Copyright (C) 2008 Peter Korsgaard <[email protected]>
> > > * Copyright (C) 2016 Freescale Semiconductor Inc.
> > > + * Copyright 2020 NXP
> >
> > A copyright notice on a two-line change is a bit too much, don't you think?
> Okay, got it. Will remove it in v2.
> >
> > > *
> > > * This file is licensed under the terms of the GNU General Public License
> > > * version 2. This program is licensed "as is" without any warranty
> > > of any @@ -80,7 +81,7 @@ static int mpc5121_gpio_dir_out(struct
> > > gpio_chip *gc, {
> > > struct mpc8xxx_gpio_chip *mpc8xxx_gc = gpiochip_get_data(gc);
> > > /* GPIO 28..31 are input only on MPC5121 */
> > > - if (gpio >= 28)
> > > + if (gpio >= 28U)
> > > return -EINVAL;
> >
> > I don't really understand the commit message but looking at the code is even
> > more confusing. What are you fixing here actually?
> Try to fix code warning that generated by coverity scan tool(static code analysis tool)

Please explain what benefit there is to using 28U over 28. No tool is
perfect, that's why you should try to understand what it is it's
trying to fix. I don't see any reason this code could fail.

Bartosz

> >
> > Bartosz
> >
> > >
> > > return mpc8xxx_gc->direction_output(gc, gpio, val); @@ -91,7
> > > +92,7 @@ static int mpc5125_gpio_dir_out(struct gpio_chip *gc, {
> > > struct mpc8xxx_gpio_chip *mpc8xxx_gc = gpiochip_get_data(gc);
> > > /* GPIO 0..3 are input only on MPC5125 */
> > > - if (gpio <= 3)
> > > + if (gpio <= 3U)
> > > return -EINVAL;
> > >
> > > return mpc8xxx_gc->direction_output(gc, gpio, val);
> > > --
> > > 2.17.1
> > >

2020-12-03 08:51:27

by Biwen Li (OSS)

[permalink] [raw]
Subject: RE: [PATCH] gpio: mpc8xxx: resolve coverity warnings

>
> On Thu, Dec 3, 2020 at 9:07 AM Biwen Li (OSS) <[email protected]> wrote:
> >
> > > On Thu, Dec 3, 2020 at 8:31 AM Biwen Li <[email protected]> wrote:
> > > >
> > > > From: Biwen Li <[email protected]>
> > > >
> > > > Resolve coverity warnings as follows,
> > > > cond_at_most: Checking gpio >= 28U implies that gpio may be up
> > > > to 27 on the false branch.
> > > > overrun-call: Overrunning callees array of size 3 by passing
> > > > argument gpio (which evaluates to 27)
> > > > in call to *mpc8xxx_gc->direction_output
> > > >
> > > > cond_at_least: Checking gpio <= 3U implies that gpio is at least 4 on
> > > > the false branch.
> > > > overrun-call: Overrunning callee's array of size 3 by passing argument
> > > > gpio (which evaluates to 4) in call to
> > > > *mpc8xxx_gc->direction_output
> > > >
> > > > Signed-off-by: Biwen Li <[email protected]>
> > > > ---
> > > > drivers/gpio/gpio-mpc8xxx.c | 5 +++--
> > > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpio/gpio-mpc8xxx.c
> > > > b/drivers/gpio/gpio-mpc8xxx.c index a6c2bbdcaa10..12c9a91d87b7
> > > > 100644
> > > > --- a/drivers/gpio/gpio-mpc8xxx.c
> > > > +++ b/drivers/gpio/gpio-mpc8xxx.c
> > > > @@ -3,6 +3,7 @@
> > > > *
> > > > * Copyright (C) 2008 Peter Korsgaard <[email protected]>
> > > > * Copyright (C) 2016 Freescale Semiconductor Inc.
> > > > + * Copyright 2020 NXP
> > >
> > > A copyright notice on a two-line change is a bit too much, don't you think?
> > Okay, got it. Will remove it in v2.
> > >
> > > > *
> > > > * This file is licensed under the terms of the GNU General Public License
> > > > * version 2. This program is licensed "as is" without any
> > > > warranty of any @@ -80,7 +81,7 @@ static int
> > > > mpc5121_gpio_dir_out(struct gpio_chip *gc, {
> > > > struct mpc8xxx_gpio_chip *mpc8xxx_gc =
> gpiochip_get_data(gc);
> > > > /* GPIO 28..31 are input only on MPC5121 */
> > > > - if (gpio >= 28)
> > > > + if (gpio >= 28U)
> > > > return -EINVAL;
> > >
> > > I don't really understand the commit message but looking at the code
> > > is even more confusing. What are you fixing here actually?
> > Try to fix code warning that generated by coverity scan tool(static
> > code analysis tool)
>
> Please explain what benefit there is to using 28U over 28. No tool is perfect,
> that's why you should try to understand what it is it's trying to fix. I don't see any
> reason this code could fail.
This code couldn't fail.
The variable gpio is unsigned int type, prefer to append "U" for unsigned typed values, this makes is clearer also when comparing values and variables.
>
> Bartosz
>
> > >
> > > Bartosz
> > >
> > > >
> > > > return mpc8xxx_gc->direction_output(gc, gpio, val); @@
> > > > -91,7
> > > > +92,7 @@ static int mpc5125_gpio_dir_out(struct gpio_chip *gc, {
> > > > struct mpc8xxx_gpio_chip *mpc8xxx_gc =
> gpiochip_get_data(gc);
> > > > /* GPIO 0..3 are input only on MPC5125 */
> > > > - if (gpio <= 3)
> > > > + if (gpio <= 3U)
> > > > return -EINVAL;
> > > >
> > > > return mpc8xxx_gc->direction_output(gc, gpio, val);
> > > > --
> > > > 2.17.1
> > > >

2020-12-03 09:06:23

by Bartosz Golaszewski

[permalink] [raw]
Subject: Re: [PATCH] gpio: mpc8xxx: resolve coverity warnings

On Thu, Dec 3, 2020 at 9:51 AM Biwen Li (OSS) <[email protected]> wrote:
>
> >
> > On Thu, Dec 3, 2020 at 9:07 AM Biwen Li (OSS) <[email protected]> wrote:
> > >
> > > > On Thu, Dec 3, 2020 at 8:31 AM Biwen Li <[email protected]> wrote:
> > > > >
> > > > > From: Biwen Li <[email protected]>
> > > > >
> > > > > Resolve coverity warnings as follows,
> > > > > cond_at_most: Checking gpio >= 28U implies that gpio may be up
> > > > > to 27 on the false branch.
> > > > > overrun-call: Overrunning callees array of size 3 by passing
> > > > > argument gpio (which evaluates to 27)
> > > > > in call to *mpc8xxx_gc->direction_output
> > > > >
> > > > > cond_at_least: Checking gpio <= 3U implies that gpio is at least 4 on
> > > > > the false branch.
> > > > > overrun-call: Overrunning callee's array of size 3 by passing argument
> > > > > gpio (which evaluates to 4) in call to
> > > > > *mpc8xxx_gc->direction_output
> > > > >
> > > > > Signed-off-by: Biwen Li <[email protected]>
> > > > > ---
> > > > > drivers/gpio/gpio-mpc8xxx.c | 5 +++--
> > > > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpio/gpio-mpc8xxx.c
> > > > > b/drivers/gpio/gpio-mpc8xxx.c index a6c2bbdcaa10..12c9a91d87b7
> > > > > 100644
> > > > > --- a/drivers/gpio/gpio-mpc8xxx.c
> > > > > +++ b/drivers/gpio/gpio-mpc8xxx.c
> > > > > @@ -3,6 +3,7 @@
> > > > > *
> > > > > * Copyright (C) 2008 Peter Korsgaard <[email protected]>
> > > > > * Copyright (C) 2016 Freescale Semiconductor Inc.
> > > > > + * Copyright 2020 NXP
> > > >
> > > > A copyright notice on a two-line change is a bit too much, don't you think?
> > > Okay, got it. Will remove it in v2.
> > > >
> > > > > *
> > > > > * This file is licensed under the terms of the GNU General Public License
> > > > > * version 2. This program is licensed "as is" without any
> > > > > warranty of any @@ -80,7 +81,7 @@ static int
> > > > > mpc5121_gpio_dir_out(struct gpio_chip *gc, {
> > > > > struct mpc8xxx_gpio_chip *mpc8xxx_gc =
> > gpiochip_get_data(gc);
> > > > > /* GPIO 28..31 are input only on MPC5121 */
> > > > > - if (gpio >= 28)
> > > > > + if (gpio >= 28U)
> > > > > return -EINVAL;
> > > >
> > > > I don't really understand the commit message but looking at the code
> > > > is even more confusing. What are you fixing here actually?
> > > Try to fix code warning that generated by coverity scan tool(static
> > > code analysis tool)
> >
> > Please explain what benefit there is to using 28U over 28. No tool is perfect,
> > that's why you should try to understand what it is it's trying to fix. I don't see any
> > reason this code could fail.
> This code couldn't fail.
> The variable gpio is unsigned int type, prefer to append "U" for unsigned typed values, this makes is clearer also when comparing values and variables.
> >

This only makes sense if we're using large values that could
potentially overflow a signed int. This is a small constant value so
there's no reason for this churn. NAK

Bartosz

> > Bartosz
> >
> > > >
> > > > Bartosz
> > > >
> > > > >
> > > > > return mpc8xxx_gc->direction_output(gc, gpio, val); @@
> > > > > -91,7
> > > > > +92,7 @@ static int mpc5125_gpio_dir_out(struct gpio_chip *gc, {
> > > > > struct mpc8xxx_gpio_chip *mpc8xxx_gc =
> > gpiochip_get_data(gc);
> > > > > /* GPIO 0..3 are input only on MPC5125 */
> > > > > - if (gpio <= 3)
> > > > > + if (gpio <= 3U)
> > > > > return -EINVAL;
> > > > >
> > > > > return mpc8xxx_gc->direction_output(gc, gpio, val);
> > > > > --
> > > > > 2.17.1
> > > > >