| Message ID | 20200729135636.9220-1-cengiz@kernel.wtf (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | staging: atomisp: move null check to earlier point | expand |
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.
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
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
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
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
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;
`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(-)