LKML Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] staging: atomisp: move null check to earlier point
@ 2020-07-29 13:56 Cengiz Can
  2020-07-29 15:13 ` Andy Shevchenko
  0 siblings, 1 reply; 15+ messages in thread
From: Cengiz Can @ 2020-07-29 13:56 UTC (permalink / raw)
  To: Mauro Carvalho Chehab, Sakari Ailus, Greg Kroah-Hartman, Andy Shevchenko
  Cc: linux-media, devel, linux-kernel, Cengiz Can

`find_gmin_subdev` function that returns a pointer to `struct
gmin_subdev` can return NULL.

In `gmin_v2p8_ctrl` there's a call to this function but the possibility
of a NULL was not checked before its being dereferenced. ie:

```
/* Acquired here --------v */
struct gmin_subdev *gs = find_gmin_subdev(subdev);
int ret;
int value;

/*  v------Dereferenced here */
if (gs->v2p8_gpio >= 0) {
	pr_info("atomisp_gmin_platform: 2.8v power on GPIO %d\n",
		gs->v2p8_gpio);
	ret = gpio_request(gs->v2p8_gpio, "camera_v2p8");
	if (!ret)
		ret = gpio_direction_output(gs->v2p8_gpio, 0);
	if (ret)
		pr_err("V2P8 GPIO initialization failed\n");
}
```

I have moved the NULL check before deref point.

Caught-by: Coverity Static Analyzer CID 1465536
Signed-off-by: Cengiz Can <cengiz@kernel.wtf>
---
 drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
index 0df46a1af5f0..8e9c5016f299 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
@@ -871,6 +871,11 @@ static int gmin_v2p8_ctrl(struct v4l2_subdev *subdev, int on)
 	int ret;
 	int value;
 
+	if (!gs) {
+		pr_err("Unable to find gmin subdevice\n");
+		return -EINVAL;
+	}
+
 	if (gs->v2p8_gpio >= 0) {
 		pr_info("atomisp_gmin_platform: 2.8v power on GPIO %d\n",
 			gs->v2p8_gpio);
@@ -881,7 +886,7 @@ static int gmin_v2p8_ctrl(struct v4l2_subdev *subdev, int on)
 			pr_err("V2P8 GPIO initialization failed\n");
 	}
 
-	if (!gs || gs->v2p8_on == on)
+	if (gs->v2p8_on == on)
 		return 0;
 	gs->v2p8_on = on;
 
-- 
2.27.0


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] staging: atomisp: move null check to earlier point
  2020-07-29 13:56 [PATCH] staging: atomisp: move null check to earlier point Cengiz Can
@ 2020-07-29 15:13 ` Andy Shevchenko
  2020-07-30  8:45   ` Dan Carpenter
  0 siblings, 1 reply; 15+ messages in thread
From: Andy Shevchenko @ 2020-07-29 15:13 UTC (permalink / raw)
  To: Cengiz Can
  Cc: Mauro Carvalho Chehab, Sakari Ailus, Greg Kroah-Hartman,
	Andy Shevchenko, Linux Media Mailing List,
	open list:STAGING SUBSYSTEM, Linux Kernel Mailing List

On Wed, Jul 29, 2020 at 5:00 PM Cengiz Can <cengiz@kernel.wtf> wrote:
>
> `find_gmin_subdev` function that returns a pointer to `struct
> gmin_subdev` can return NULL.
>
> In `gmin_v2p8_ctrl` there's a call to this function but the possibility
> of a NULL was not checked before its being dereferenced. ie:
>
> ```
> /* Acquired here --------v */
> struct gmin_subdev *gs = find_gmin_subdev(subdev);
> int ret;
> int value;
>
> /*  v------Dereferenced here */
> if (gs->v2p8_gpio >= 0) {
>         pr_info("atomisp_gmin_platform: 2.8v power on GPIO %d\n",
>                 gs->v2p8_gpio);
>         ret = gpio_request(gs->v2p8_gpio, "camera_v2p8");
>         if (!ret)
>                 ret = gpio_direction_output(gs->v2p8_gpio, 0);
>         if (ret)
>                 pr_err("V2P8 GPIO initialization failed\n");
> }
> ```
>
> I have moved the NULL check before deref point.

"Move the NULL check..."
See Submitting Patches documentation how to avoid "This patch", "I", "we", etc.

> diff --git a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
> index 0df46a1af5f0..8e9c5016f299 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
> @@ -871,6 +871,11 @@ static int gmin_v2p8_ctrl(struct v4l2_subdev *subdev, int on)
>         int ret;
>         int value;
>
> +       if (!gs) {
> +               pr_err("Unable to find gmin subdevice\n");

> +               return -EINVAL;

And here is a change of semantics...

> +       }

...

> -       if (!gs || gs->v2p8_on == on)
> +       if (gs->v2p8_on == on)
>                 return 0;

...compared to above.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] staging: atomisp: move null check to earlier point
  2020-07-29 15:13 ` Andy Shevchenko
@ 2020-07-30  8:45   ` Dan Carpenter
  2020-07-30  8:59     ` Cengiz Can
                       ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Dan Carpenter @ 2020-07-30  8:45 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Cengiz Can, open list:STAGING SUBSYSTEM, Andy Shevchenko,
	Greg Kroah-Hartman, Linux Kernel Mailing List, Sakari Ailus,
	Mauro Carvalho Chehab, Linux Media Mailing List

On Wed, Jul 29, 2020 at 06:13:44PM +0300, Andy Shevchenko wrote:
> On Wed, Jul 29, 2020 at 5:00 PM Cengiz Can <cengiz@kernel.wtf> wrote:
> >
> > `find_gmin_subdev` function that returns a pointer to `struct
> > gmin_subdev` can return NULL.
> >
> > In `gmin_v2p8_ctrl` there's a call to this function but the possibility
> > of a NULL was not checked before its being dereferenced. ie:
> >
> > ```
> > /* Acquired here --------v */
> > struct gmin_subdev *gs = find_gmin_subdev(subdev);
> > int ret;
> > int value;
> >
> > /*  v------Dereferenced here */
> > if (gs->v2p8_gpio >= 0) {
> >         pr_info("atomisp_gmin_platform: 2.8v power on GPIO %d\n",
> >                 gs->v2p8_gpio);
> >         ret = gpio_request(gs->v2p8_gpio, "camera_v2p8");
> >         if (!ret)
> >                 ret = gpio_direction_output(gs->v2p8_gpio, 0);
> >         if (ret)
> >                 pr_err("V2P8 GPIO initialization failed\n");
> > }
> > ```
> >
> > I have moved the NULL check before deref point.
> 
> "Move the NULL check..."
> See Submitting Patches documentation how to avoid "This patch", "I", "we", etc.

I always feel like this is a pointless requirement.  We're turning into
bureaucracts.

> 
> > diff --git a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
> > index 0df46a1af5f0..8e9c5016f299 100644
> > --- a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
> > +++ b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
> > @@ -871,6 +871,11 @@ static int gmin_v2p8_ctrl(struct v4l2_subdev *subdev, int on)
> >         int ret;
> >         int value;
> >
> > +       if (!gs) {
> > +               pr_err("Unable to find gmin subdevice\n");
> 
> > +               return -EINVAL;
> 
> And here is a change of semantics...

Yeah.  The change of semantics should be documented in the commit
message, but it's actually correct.  I discussed this with Mauro earlier
but my bug reporting script didn't CC a mailing list and I didn't
catch it.  Mauro suggested:

    53  > Yet, it could make sense to have something like:
    54  > 
    55  >       if (WARN_ON(!gs))
    56  >               return -ENODEV;
    57  > 
    58  > at the beginning of the functions that call find_gmin_subdev().

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] staging: atomisp: move null check to earlier point
  2020-07-30  8:45   ` Dan Carpenter
@ 2020-07-30  8:59     ` Cengiz Can
  2020-07-30 22:17     ` [PATCH v2] " Cengiz Can
  2020-08-06 22:15     ` [PATCH] " Bjorn Helgaas
  2 siblings, 0 replies; 15+ messages in thread
From: Cengiz Can @ 2020-07-30  8:59 UTC (permalink / raw)
  To: Dan Carpenter, Andy Shevchenko
  Cc: open list:STAGING SUBSYSTEM, Andy Shevchenko, Greg Kroah-Hartman,
	Linux Kernel Mailing List, Sakari Ailus, Mauro Carvalho Chehab,
	Linux Media Mailing List



On July 30, 2020 11:48:06 Dan Carpenter <dan.carpenter@oracle.com> wrote:

> On Wed, Jul 29, 2020 at 06:13:44PM +0300, Andy Shevchenko wrote:
>> On Wed, Jul 29, 2020 at 5:00 PM Cengiz Can <cengiz@kernel.wtf> wrote:
>>>
>>> `find_gmin_subdev` function that returns a pointer to `struct
>>> gmin_subdev` can return NULL.
>>>
>>> In `gmin_v2p8_ctrl` there's a call to this function but the possibility
>>> of a NULL was not checked before its being dereferenced. ie:
>>>
>>> ```
>>> /* Acquired here --------v */
>>> struct gmin_subdev *gs = find_gmin_subdev(subdev);
>>> int ret;
>>> int value;
>>>
>>> /*  v------Dereferenced here */
>>> if (gs->v2p8_gpio >= 0) {
>>>  pr_info("atomisp_gmin_platform: 2.8v power on GPIO %d\n",
>>>          gs->v2p8_gpio);
>>>  ret = gpio_request(gs->v2p8_gpio, "camera_v2p8");
>>>  if (!ret)
>>>          ret = gpio_direction_output(gs->v2p8_gpio, 0);
>>>  if (ret)
>>>          pr_err("V2P8 GPIO initialization failed\n");
>>> }
>>> ```
>>>
>>> I have moved the NULL check before deref point.
>>
>> "Move the NULL check..."
>> See Submitting Patches documentation how to avoid "This patch", "I", "we", etc.

Noted. Sorry. I'm not a native English speaker.

>>
>
> I always feel like this is a pointless requirement.  We're turning into
> bureaucracts.
>
>>
>>> diff --git a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c 
>>> b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
>>> index 0df46a1af5f0..8e9c5016f299 100644
>>> --- a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
>>> +++ b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
>>> @@ -871,6 +871,11 @@ static int gmin_v2p8_ctrl(struct v4l2_subdev *subdev, 
>>> int on)
>>>  int ret;
>>>  int value;
>>>
>>> +       if (!gs) {
>>> +               pr_err("Unable to find gmin subdevice\n");
>>
>>> +               return -EINVAL;
>>
>> And here is a change of semantics...
>
> Yeah.  The change of semantics should be documented in the commit
> message, but it's actually correct.  I discussed this with Mauro earlier
> but my bug reporting script didn't CC a mailing list and I didn't
> catch it.  Mauro suggested:
>
>    53  > Yet, it could make sense to have something like:
>    54  >
>    55  >       if (WARN_ON(!gs))
>    56  >               return -ENODEV;
>    57  >
>    58  > at the beginning of the functions that call find_gmin_subdev().

I will be updating v2 according to this.

>
> regards,
> dan carpenter




^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2] staging: atomisp: move null check to earlier point
  2020-07-30  8:45   ` Dan Carpenter
  2020-07-30  8:59     ` Cengiz Can
@ 2020-07-30 22:17     ` Cengiz Can
  2020-07-31  8:38       ` Andy Shevchenko
  2020-08-06 22:15     ` [PATCH] " Bjorn Helgaas
  2 siblings, 1 reply; 15+ messages in thread
From: Cengiz Can @ 2020-07-30 22:17 UTC (permalink / raw)
  To: dan.carpenter, andy.shevchenko
  Cc: andriy.shevchenko, cengiz, devel, gregkh, linux-kernel,
	linux-media, mchehab, sakari.ailus

`find_gmin_subdev` function that returns a pointer to `struct
gmin_subdev` can return NULL.

In `gmin_v2p8_ctrl` there's a call to this function but the possibility
of a NULL was not checked before its being dereferenced. ie:

```
/* Acquired here --------v */
struct gmin_subdev *gs = find_gmin_subdev(subdev);

/*  v------Dereferenced here */
if (gs->v2p8_gpio >= 0) {
    ...
}
```

To avoid the issue, null check has been moved to an earlier point
and return semantics has been changed to reflect this exception.

Please do note that this change introduces a new return value to
`gmin_v2p8_ctrl`.

[NEW] - raise a WARN and return -ENODEV if there are no subdevices.
      - return result of `gpio_request` or `gpio_direction_output`.
      - return 0 if GPIO is ON.
      - return results of `regulator_enable` or `regulator_disable`.
      - according to PMIC type, return result of `axp_regulator_set`
        or `gmin_i2c_write`.
      - return -EINVAL if unknown PMIC type.

Caught-by: Coverity Static Analyzer CID 1465536
Signed-off-by: Cengiz Can <cengiz@kernel.wtf>
---
 drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
index 0df46a1af5f0..1ad0246764a6 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
@@ -871,6 +871,9 @@ static int gmin_v2p8_ctrl(struct v4l2_subdev *subdev, int on)
 	int ret;
 	int value;
 
+	if (WARN_ON(!gs))
+		return -ENODEV;
+
 	if (gs->v2p8_gpio >= 0) {
 		pr_info("atomisp_gmin_platform: 2.8v power on GPIO %d\n",
 			gs->v2p8_gpio);
@@ -881,7 +884,7 @@ static int gmin_v2p8_ctrl(struct v4l2_subdev *subdev, int on)
 			pr_err("V2P8 GPIO initialization failed\n");
 	}
 
-	if (!gs || gs->v2p8_on == on)
+	if (gs->v2p8_on == on)
 		return 0;
 	gs->v2p8_on = on;
 
-- 
2.27.0


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] staging: atomisp: move null check to earlier point
  2020-07-30 22:17     ` [PATCH v2] " Cengiz Can
@ 2020-07-31  8:38       ` Andy Shevchenko
  2020-08-01 21:51         ` [PATCH v3] " Cengiz Can
                           ` (3 more replies)
  0 siblings, 4 replies; 15+ messages in thread
From: Andy Shevchenko @ 2020-07-31  8:38 UTC (permalink / raw)
  To: Cengiz Can
  Cc: dan.carpenter, devel, gregkh, linux-kernel, linux-media, mchehab,
	sakari.ailus

On Fri, Jul 31, 2020 at 01:17:38AM +0300, Cengiz Can wrote:
> `find_gmin_subdev` function that returns a pointer to `struct

func() are referred with name followed by parentheses.

> gmin_subdev` can return NULL.

> In `gmin_v2p8_ctrl` there's a call to this function but the possibility
> of a NULL was not checked before its being dereferenced. ie:

'. ie:' -> ', i.e.:'

> ```

Instead just shift right by two spaces.

> /* Acquired here --------v */
> struct gmin_subdev *gs = find_gmin_subdev(subdev);
> 
> /*  v------Dereferenced here */
> if (gs->v2p8_gpio >= 0) {
>     ...
> }

> ```

Drop this as per above comment.

> To avoid the issue, null check has been moved to an earlier point
> and return semantics has been changed to reflect this exception.

> Please do note that this change introduces a new return value to
> `gmin_v2p8_ctrl`.

This rather should explain better the semantics change, e.g.
"Now the function() refuses to take NULL argument and returns an error. We also
WARN() for sake of the debugging."

> [NEW] - raise a WARN and return -ENODEV if there are no subdevices.
>       - return result of `gpio_request` or `gpio_direction_output`.
>       - return 0 if GPIO is ON.
>       - return results of `regulator_enable` or `regulator_disable`.
>       - according to PMIC type, return result of `axp_regulator_set`
>         or `gmin_i2c_write`.
>       - return -EINVAL if unknown PMIC type.

This has to go after cutter '---' line below.

> Caught-by: Coverity Static Analyzer CID 1465536

Reported-by:

And as discussed previously,
Suggested-by: Mauro ...

> Signed-off-by: Cengiz Can <cengiz@kernel.wtf>

The code looks good enough (WARN() will probably go away when driver gains
better quality).

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v3] staging: atomisp: move null check to earlier point
  2020-07-31  8:38       ` Andy Shevchenko
@ 2020-08-01 21:51         ` Cengiz Can
  2020-08-01 21:55         ` [PATCHi v4] " Cengiz Can
                           ` (2 subsequent siblings)
  3 siblings, 0 replies; 15+ messages in thread
From: Cengiz Can @ 2020-08-01 21:51 UTC (permalink / raw)
  To: andy.shevchenko
  Cc: cengiz, dan.carpenter, devel, gregkh, linux-kernel, linux-media,
	mchehab, sakari.ailus

`find_gmin_subdev()` that returns a pointer to `struct
gmin_subdev` can return NULL.

In `gmin_v2p8_ctrl()` there's a call to this function but the
possibility of a NULL was not checked before its being dereferenced,
i.e.:

  /* Acquired here --------v */
  struct gmin_subdev *gs = find_gmin_subdev(subdev);

  /*  v------Dereferenced here */
  if (gs->v2p8_gpio >= 0) {
      ...
  }

With this change we're null checking `find_gmin_subdev()` result
and return we return an error if that's the case. We also WARN()
for the sake of debugging.

Signed-off-by: Cengiz Can <cengiz@kernel.wtf>
Reported-by: Coverity Static Analyzer CID 1465536
Suggested-by: Mauro Carvalho Chehab <mchehab@kernel.org>
---

Please do note that this change introduces a new return value to
 `gmin_v2p8_ctrl`.
 
 [NEW] - raise a WARN and return -ENODEV if there are no subdevices.
       - return result of `gpio_request` or `gpio_direction_output`.
       - return 0 if GPIO is ON.
       - return results of `regulator_enable` or `regulator_disable`.
       - according to PMIC type, return result of `axp_regulator_set`
         or `gmin_i2c_write`.
       - return -EINVAL if unknown PMIC type.

 drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
index 0df46a1af5f0..1ad0246764a6 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
@@ -871,6 +871,9 @@ static int gmin_v2p8_ctrl(struct v4l2_subdev *subdev, int on)
 	int ret;
 	int value;
 
+	if (WARN_ON(!gs))
+		return -ENODEV;
+
 	if (gs->v2p8_gpio >= 0) {
 		pr_info("atomisp_gmin_platform: 2.8v power on GPIO %d\n",
 			gs->v2p8_gpio);
@@ -881,7 +884,7 @@ static int gmin_v2p8_ctrl(struct v4l2_subdev *subdev, int on)
 			pr_err("V2P8 GPIO initialization failed\n");
 	}
 
-	if (!gs || gs->v2p8_on == on)
+	if (gs->v2p8_on == on)
 		return 0;
 	gs->v2p8_on = on;
 
-- 
2.27.0


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCHi v4] staging: atomisp: move null check to earlier point
  2020-07-31  8:38       ` Andy Shevchenko
  2020-08-01 21:51         ` [PATCH v3] " Cengiz Can
@ 2020-08-01 21:55         ` Cengiz Can
  2020-08-01 21:58         ` [PATCH v5] " Cengiz Can
  2020-08-01 22:01         ` [PATCH v6] " Cengiz Can
  3 siblings, 0 replies; 15+ messages in thread
From: Cengiz Can @ 2020-08-01 21:55 UTC (permalink / raw)
  To: andy.shevchenko
  Cc: cengiz, dan.carpenter, devel, gregkh, linux-kernel, linux-media,
	mchehab, sakari.ailus

`find_gmin_subdev()` that returns a pointer to `struct
gmin_subdev` can return NULL.

In `gmin_v2p8_ctrl()` there's a call to this function but the
possibility of a NULL was not checked before its being dereferenced,
i.e.:

  /* Acquired here --------v */
  struct gmin_subdev *gs = find_gmin_subdev(subdev);

  /*  v------Dereferenced here */
  if (gs->v2p8_gpio >= 0) {
      ...
  }

With this change we're null checking `find_gmin_subdev()` result
and we return an error if that's the case. We also WARN()
for the sake of debugging.

Signed-off-by: Cengiz Can <cengiz@kernel.wtf>
Reported-by: Coverity Static Analyzer CID 1465536
Suggested-by: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Cengiz Can <cengiz@kernel.wtf>
---

 Please do note that this change introduces a new return value to
 `gmin_v2p8_ctrl`.

 [NEW] - raise a WARN and return -ENODEV if there are no subdevices.
       - return result of `gpio_request` or `gpio_direction_output`.
       - return 0 if GPIO is ON.
       - return results of `regulator_enable` or `regulator_disable`.
       - according to PMIC type, return result of `axp_regulator_set`
         or `gmin_i2c_write`.
       - return -EINVAL if unknown PMIC type.
 
 Patch Changelog:
   v4: Fix minor typo in commit message

 drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
index 0df46a1af5f0..1ad0246764a6 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
@@ -871,6 +871,9 @@ static int gmin_v2p8_ctrl(struct v4l2_subdev *subdev, int on)
 	int ret;
 	int value;
 
+	if (WARN_ON(!gs))
+		return -ENODEV;
+
 	if (gs->v2p8_gpio >= 0) {
 		pr_info("atomisp_gmin_platform: 2.8v power on GPIO %d\n",
 			gs->v2p8_gpio);
@@ -881,7 +884,7 @@ static int gmin_v2p8_ctrl(struct v4l2_subdev *subdev, int on)
 			pr_err("V2P8 GPIO initialization failed\n");
 	}
 
-	if (!gs || gs->v2p8_on == on)
+	if (gs->v2p8_on == on)
 		return 0;
 	gs->v2p8_on = on;
 
-- 
2.27.0


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v5] staging: atomisp: move null check to earlier point
  2020-07-31  8:38       ` Andy Shevchenko
  2020-08-01 21:51         ` [PATCH v3] " Cengiz Can
  2020-08-01 21:55         ` [PATCHi v4] " Cengiz Can
@ 2020-08-01 21:58         ` Cengiz Can
  2020-08-01 22:01         ` [PATCH v6] " Cengiz Can
  3 siblings, 0 replies; 15+ messages in thread
From: Cengiz Can @ 2020-08-01 21:58 UTC (permalink / raw)
  To: andy.shevchenko
  Cc: cengiz, dan.carpenter, devel, gregkh, linux-kernel, linux-media,
	mchehab, sakari.ailus

`find_gmin_subdev()` that returns a pointer to `struct
gmin_subdev` can return NULL.

In `gmin_v2p8_ctrl()` there's a call to this function but the
possibility of a NULL was not checked before its being dereferenced,
i.e.:

  /* Acquired here --------v */
  struct gmin_subdev *gs = find_gmin_subdev(subdev);

  /*  v------Dereferenced here */
  if (gs->v2p8_gpio >= 0) {
      ...
  }

With this change we're null checking `find_gmin_subdev()` result
and we return an error if that's the case. We also WARN()
for the sake of debugging.

Signed-off-by: Cengiz Can <cengiz@kernel.wtf>
Reported-by: Coverity Static Analyzer CID 1465536
Suggested-by: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Cengiz Can <cengiz@kernel.wtf>
---

 Please do note that this change introduces a new return value to
 `gmin_v2p8_ctrl()`.

 [NEW] - raise a WARN and return -ENODEV if there are no subdevices.
       - return result of `gpio_request` or `gpio_direction_output`.
       - return 0 if GPIO is ON.
       - return results of `regulator_enable` or `regulator_disable`.
       - according to PMIC type, return result of `axp_regulator_set`
         or `gmin_i2c_write`.
       - return -EINVAL if unknown PMIC type.
 
 Patch Changelog:
   v4: Fix minor typo in commit message
   v5: Remove typo from email subject

 drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
index 0df46a1af5f0..1ad0246764a6 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
@@ -871,6 +871,9 @@ static int gmin_v2p8_ctrl(struct v4l2_subdev *subdev, int on)
 	int ret;
 	int value;
 
+	if (WARN_ON(!gs))
+		return -ENODEV;
+
 	if (gs->v2p8_gpio >= 0) {
 		pr_info("atomisp_gmin_platform: 2.8v power on GPIO %d\n",
 			gs->v2p8_gpio);
@@ -881,7 +884,7 @@ static int gmin_v2p8_ctrl(struct v4l2_subdev *subdev, int on)
 			pr_err("V2P8 GPIO initialization failed\n");
 	}
 
-	if (!gs || gs->v2p8_on == on)
+	if (gs->v2p8_on == on)
 		return 0;
 	gs->v2p8_on = on;
 
-- 
2.27.0


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v6] staging: atomisp: move null check to earlier point
  2020-07-31  8:38       ` Andy Shevchenko
                           ` (2 preceding siblings ...)
  2020-08-01 21:58         ` [PATCH v5] " Cengiz Can
@ 2020-08-01 22:01         ` Cengiz Can
  2020-08-06 18:34           ` Cengiz Can
  3 siblings, 1 reply; 15+ messages in thread
From: Cengiz Can @ 2020-08-01 22:01 UTC (permalink / raw)
  To: andy.shevchenko
  Cc: cengiz, dan.carpenter, devel, gregkh, linux-kernel, linux-media,
	mchehab, sakari.ailus

`find_gmin_subdev()` that returns a pointer to `struct
gmin_subdev` can return NULL.

In `gmin_v2p8_ctrl()` there's a call to this function but the
possibility of a NULL was not checked before its being dereferenced,
i.e.:

  /* Acquired here --------v */
  struct gmin_subdev *gs = find_gmin_subdev(subdev);

  /*  v------Dereferenced here */
  if (gs->v2p8_gpio >= 0) {
      ...
  }

With this change we're null checking `find_gmin_subdev()` result
and we return an error if that's the case. We also WARN()
for the sake of debugging.

Signed-off-by: Cengiz Can <cengiz@kernel.wtf>
Reported-by: Coverity Static Analyzer CID 1465536
Suggested-by: Mauro Carvalho Chehab <mchehab@kernel.org>
---

 Please do note that this change introduces a new return value to
 `gmin_v2p8_ctrl()`.

 [NEW] - raise a WARN and return -ENODEV if there are no subdevices.
       - return result of `gpio_request` or `gpio_direction_output`.
       - return 0 if GPIO is ON.
       - return results of `regulator_enable` or `regulator_disable`.
       - according to PMIC type, return result of `axp_regulator_set`
         or `gmin_i2c_write`.
       - return -EINVAL if unknown PMIC type.

 Patch Changelog:
   v4: Fix minor typo in commit message
   v5: Remove typo from email subject
   v6: Remove duplicate Signed-off-by tag

 drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
index 0df46a1af5f0..1ad0246764a6 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
@@ -871,6 +871,9 @@ static int gmin_v2p8_ctrl(struct v4l2_subdev *subdev, int on)
 	int ret;
 	int value;
 
+	if (WARN_ON(!gs))
+		return -ENODEV;
+
 	if (gs->v2p8_gpio >= 0) {
 		pr_info("atomisp_gmin_platform: 2.8v power on GPIO %d\n",
 			gs->v2p8_gpio);
@@ -881,7 +884,7 @@ static int gmin_v2p8_ctrl(struct v4l2_subdev *subdev, int on)
 			pr_err("V2P8 GPIO initialization failed\n");
 	}
 
-	if (!gs || gs->v2p8_on == on)
+	if (gs->v2p8_on == on)
 		return 0;
 	gs->v2p8_on = on;
 
-- 
2.27.0


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v6] staging: atomisp: move null check to earlier point
  2020-08-01 22:01         ` [PATCH v6] " Cengiz Can
@ 2020-08-06 18:34           ` Cengiz Can
  2020-08-06 18:39             ` Greg KH
  0 siblings, 1 reply; 15+ messages in thread
From: Cengiz Can @ 2020-08-06 18:34 UTC (permalink / raw)
  To: andy.shevchenko
  Cc: dan.carpenter, devel, gregkh, linux-kernel, linux-media, mchehab,
	sakari.ailus, Cengiz Can

Hello Andy,

Can I get some feedback on v6 please?

I hope it suits your standards this time.

Thank you

On August 2, 2020 01:02:22 Cengiz Can <cengiz@kernel.wtf> wrote:

> `find_gmin_subdev()` that returns a pointer to `struct
> gmin_subdev` can return NULL.
>
> In `gmin_v2p8_ctrl()` there's a call to this function but the
> possibility of a NULL was not checked before its being dereferenced,
> i.e.:
>
>  /* Acquired here --------v */
>  struct gmin_subdev *gs = find_gmin_subdev(subdev);
>
>  /*  v------Dereferenced here */
>  if (gs->v2p8_gpio >= 0) {
>      ...
>  }
>
> With this change we're null checking `find_gmin_subdev()` result
> and we return an error if that's the case. We also WARN()
> for the sake of debugging.
>
> Signed-off-by: Cengiz Can <cengiz@kernel.wtf>
> Reported-by: Coverity Static Analyzer CID 1465536
> Suggested-by: Mauro Carvalho Chehab <mchehab@kernel.org>
> ---
>
> Please do note that this change introduces a new return value to
> `gmin_v2p8_ctrl()`.
>
> [NEW] - raise a WARN and return -ENODEV if there are no subdevices.
>       - return result of `gpio_request` or `gpio_direction_output`.
>       - return 0 if GPIO is ON.
>       - return results of `regulator_enable` or `regulator_disable`.
>       - according to PMIC type, return result of `axp_regulator_set`
>         or `gmin_i2c_write`.
>       - return -EINVAL if unknown PMIC type.
>
> Patch Changelog:
>   v4: Fix minor typo in commit message
>   v5: Remove typo from email subject
>   v6: Remove duplicate Signed-off-by tag
>
> drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c 
> b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
> index 0df46a1af5f0..1ad0246764a6 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
> @@ -871,6 +871,9 @@ static int gmin_v2p8_ctrl(struct v4l2_subdev *subdev, 
> int on)
> 	int ret;
> 	int value;
>
> +	if (WARN_ON(!gs))
> +		return -ENODEV;
> +
> 	if (gs->v2p8_gpio >= 0) {
> 		pr_info("atomisp_gmin_platform: 2.8v power on GPIO %d\n",
> 			gs->v2p8_gpio);
> @@ -881,7 +884,7 @@ static int gmin_v2p8_ctrl(struct v4l2_subdev *subdev, 
> int on)
> 			pr_err("V2P8 GPIO initialization failed\n");
> 	}
>
> -	if (!gs || gs->v2p8_on == on)
> +	if (gs->v2p8_on == on)
> 		return 0;
> 	gs->v2p8_on = on;
>
> --
> 2.27.0




^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v6] staging: atomisp: move null check to earlier point
  2020-08-06 18:34           ` Cengiz Can
@ 2020-08-06 18:39             ` Greg KH
  2020-08-06 20:38               ` Cengiz Can
  0 siblings, 1 reply; 15+ messages in thread
From: Greg KH @ 2020-08-06 18:39 UTC (permalink / raw)
  To: Cengiz Can
  Cc: andy.shevchenko, devel, linux-kernel, sakari.ailus, mchehab,
	dan.carpenter, linux-media

On Thu, Aug 06, 2020 at 09:34:22PM +0300, Cengiz Can wrote:
> Hello Andy,
> 
> Can I get some feedback on v6 please?


It's been 4 days, in the middle of a merge window, please give people a
chance to catch up on other things...

and do not top post please.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v6] staging: atomisp: move null check to earlier point
  2020-08-06 18:39             ` Greg KH
@ 2020-08-06 20:38               ` Cengiz Can
  0 siblings, 0 replies; 15+ messages in thread
From: Cengiz Can @ 2020-08-06 20:38 UTC (permalink / raw)
  To: Greg KH
  Cc: andy.shevchenko, devel, linux-kernel, sakari.ailus, mchehab,
	dan.carpenter, linux-media


On August 6, 2020 21:39:21 Greg KH <gregkh@linuxfoundation.org> wrote:

> On Thu, Aug 06, 2020 at 09:34:22PM +0300, Cengiz Can wrote:
>> Hello Andy,
>>
>> Can I get some feedback on v6 please?
>
>
> It's been 4 days, in the middle of a merge window, please give people a
> chance to catch up on other things...

I wasn't aware of that we're currently in a merge window. Sorry for my 
impatience.

>
> and do not top post please.

Sorry. I was tricked by my mobile email client.

>
> thanks,
>
> greg k-h

Thanks again and I wish a smooth merge window to all maintainers.

Cengiz Can



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] staging: atomisp: move null check to earlier point
  2020-07-30  8:45   ` Dan Carpenter
  2020-07-30  8:59     ` Cengiz Can
  2020-07-30 22:17     ` [PATCH v2] " Cengiz Can
@ 2020-08-06 22:15     ` Bjorn Helgaas
  2020-08-07  9:53       ` Dan Carpenter
  2 siblings, 1 reply; 15+ messages in thread
From: Bjorn Helgaas @ 2020-08-06 22:15 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Andy Shevchenko, open list:STAGING SUBSYSTEM,
	Mauro Carvalho Chehab, Greg Kroah-Hartman,
	Linux Kernel Mailing List, Sakari Ailus, Cengiz Can,
	Andy Shevchenko, Linux Media Mailing List

On Thu, Jul 30, 2020 at 11:45:45AM +0300, Dan Carpenter wrote:
> On Wed, Jul 29, 2020 at 06:13:44PM +0300, Andy Shevchenko wrote:
> > On Wed, Jul 29, 2020 at 5:00 PM Cengiz Can <cengiz@kernel.wtf> wrote:
> > >
> > > `find_gmin_subdev` function that returns a pointer to `struct
> > > gmin_subdev` can return NULL.
> > >
> > > In `gmin_v2p8_ctrl` there's a call to this function but the possibility
> > > of a NULL was not checked before its being dereferenced. ie:
> > >
> > > ```
> > > /* Acquired here --------v */
> > > struct gmin_subdev *gs = find_gmin_subdev(subdev);
> > > int ret;
> > > int value;
> > >
> > > /*  v------Dereferenced here */
> > > if (gs->v2p8_gpio >= 0) {
> > >         pr_info("atomisp_gmin_platform: 2.8v power on GPIO %d\n",
> > >                 gs->v2p8_gpio);
> > >         ret = gpio_request(gs->v2p8_gpio, "camera_v2p8");
> > >         if (!ret)
> > >                 ret = gpio_direction_output(gs->v2p8_gpio, 0);
> > >         if (ret)
> > >                 pr_err("V2P8 GPIO initialization failed\n");
> > > }
> > > ```
> > >
> > > I have moved the NULL check before deref point.
> > 
> > "Move the NULL check..."
> > See Submitting Patches documentation how to avoid "This patch", "I", "we", etc.
> 
> I always feel like this is a pointless requirement.  We're turning
> into bureaucrats.

There is a danger of that, and I'm more guilty than most.  But I do
think there's value in consistent style because it allows readers to
focus on the content instead of being distracted by different margins,
grammar ("move vs. moved"), paragraph styles, quoting conventions,
etc.

Ideally we would scan previous commit logs (and the existing code!)
and make new changes fit seamlessly so it looks like everything was
done at the same time by the same person.

But often that doesn't happen.  Sometimes I take the liberty to tweak
things as I apply them to try to avoid trivial rework.

Bjorn

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] staging: atomisp: move null check to earlier point
  2020-08-06 22:15     ` [PATCH] " Bjorn Helgaas
@ 2020-08-07  9:53       ` Dan Carpenter
  0 siblings, 0 replies; 15+ messages in thread
From: Dan Carpenter @ 2020-08-07  9:53 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: open list:STAGING SUBSYSTEM, Andy Shevchenko, Greg Kroah-Hartman,
	Linux Kernel Mailing List, Andy Shevchenko, Sakari Ailus,
	Cengiz Can, Mauro Carvalho Chehab, Linux Media Mailing List

Beyond that, though, I feel like the rules are stupid because I've seen
more than a couple commit messages which were contorted to avoid
imperative.  My own standard for commit messages is that 1) Is the
problem explained, especially what it looks like to user space?  2) Is
it clear what the solution is?  3)  Does the patch itself raise any
questions that I can't figure out and which aren't explained in the
commit message.  And I figure I'm not a domain expert but if I can
understand the commit message probably anyone can.

We've got people who speak English as a second language and then start
imposing pointless rules on top?  It's crazy.  I've had to ask someone
recently to redo a commit message and it seemed very obvious they were
focused on nonsense about imperative and avoiding saying "this patch"
to the extent that I literally could not figure out what they were
saying.  When I read the patch, of course, I could see what they were
doing but from the commit message it was impossible.

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, back to index

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-29 13:56 [PATCH] staging: atomisp: move null check to earlier point Cengiz Can
2020-07-29 15:13 ` Andy Shevchenko
2020-07-30  8:45   ` Dan Carpenter
2020-07-30  8:59     ` Cengiz Can
2020-07-30 22:17     ` [PATCH v2] " Cengiz Can
2020-07-31  8:38       ` Andy Shevchenko
2020-08-01 21:51         ` [PATCH v3] " Cengiz Can
2020-08-01 21:55         ` [PATCHi v4] " Cengiz Can
2020-08-01 21:58         ` [PATCH v5] " Cengiz Can
2020-08-01 22:01         ` [PATCH v6] " Cengiz Can
2020-08-06 18:34           ` Cengiz Can
2020-08-06 18:39             ` Greg KH
2020-08-06 20:38               ` Cengiz Can
2020-08-06 22:15     ` [PATCH] " Bjorn Helgaas
2020-08-07  9:53       ` Dan Carpenter

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git