* [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 related [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 related [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 related [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 related [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 related [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 related [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, other threads:[~2020-08-07 9:55 UTC | newest] 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
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).