`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 <[email protected]>
---
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
On Wed, Jul 29, 2020 at 5:00 PM Cengiz Can <[email protected]> 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
On Wed, Jul 29, 2020 at 06:13:44PM +0300, Andy Shevchenko wrote:
> On Wed, Jul 29, 2020 at 5:00 PM Cengiz Can <[email protected]> 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
On July 30, 2020 11:48:06 Dan Carpenter <[email protected]> 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 <[email protected]> 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
`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 <[email protected]>
---
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
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 <[email protected]>
The code looks good enough (WARN() will probably go away when driver gains
better quality).
--
With Best Regards,
Andy Shevchenko
`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 <[email protected]>
Reported-by: Coverity Static Analyzer CID 1465536
Suggested-by: Mauro Carvalho Chehab <[email protected]>
---
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
`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 <[email protected]>
Reported-by: Coverity Static Analyzer CID 1465536
Suggested-by: Mauro Carvalho Chehab <[email protected]>
Signed-off-by: Cengiz Can <[email protected]>
---
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
`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 <[email protected]>
Reported-by: Coverity Static Analyzer CID 1465536
Suggested-by: Mauro Carvalho Chehab <[email protected]>
Signed-off-by: Cengiz Can <[email protected]>
---
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
`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 <[email protected]>
Reported-by: Coverity Static Analyzer CID 1465536
Suggested-by: Mauro Carvalho Chehab <[email protected]>
---
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
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 <[email protected]> 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 <[email protected]>
> Reported-by: Coverity Static Analyzer CID 1465536
> Suggested-by: Mauro Carvalho Chehab <[email protected]>
> ---
>
> 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
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
On August 6, 2020 21:39:21 Greg KH <[email protected]> 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
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 <[email protected]> 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
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