* [PATCH] media: ov7670: check status of ov7670_read
@ 2020-08-28 14:55 trix
2020-08-28 15:13 ` Nick Desaulniers
2020-10-06 12:41 ` Sakari Ailus
0 siblings, 2 replies; 5+ messages in thread
From: trix @ 2020-08-28 14:55 UTC (permalink / raw)
To: corbet, mchehab, natechancellor, ndesaulniers
Cc: linux-media, linux-kernel, Tom Rix
From: Tom Rix <trix@redhat.com>
clang static analysis flags this representative problem
drivers/media/i2c/ov7670.c:1463:9: warning: Assigned
value is garbage or undefined
*value = gain;
^ ~~~~
gain is set by a successful call to ov7670_read()
So check that ov7670_read() is successful.
The remaining static analysis problems are false positives.
There appears to be a limitation with checking the
aggregated returns.
Signed-off-by: Tom Rix <trix@redhat.com>
---
drivers/media/i2c/ov7670.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
index b42b289faaef..001d4b09db72 100644
--- a/drivers/media/i2c/ov7670.c
+++ b/drivers/media/i2c/ov7670.c
@@ -929,6 +929,8 @@ static int ov7670_set_hw(struct v4l2_subdev *sd, int hstart, int hstop,
ret = ov7670_write(sd, REG_HSTART, (hstart >> 3) & 0xff);
ret += ov7670_write(sd, REG_HSTOP, (hstop >> 3) & 0xff);
ret += ov7670_read(sd, REG_HREF, &v);
+ if (ret)
+ return ret;
v = (v & 0xc0) | ((hstop & 0x7) << 3) | (hstart & 0x7);
msleep(10);
ret += ov7670_write(sd, REG_HREF, v);
@@ -938,6 +940,8 @@ static int ov7670_set_hw(struct v4l2_subdev *sd, int hstart, int hstop,
ret += ov7670_write(sd, REG_VSTART, (vstart >> 2) & 0xff);
ret += ov7670_write(sd, REG_VSTOP, (vstop >> 2) & 0xff);
ret += ov7670_read(sd, REG_VREF, &v);
+ if (ret)
+ return ret;
v = (v & 0xf0) | ((vstop & 0x3) << 2) | (vstart & 0x3);
msleep(10);
ret += ov7670_write(sd, REG_VREF, v);
@@ -1460,6 +1464,8 @@ static int ov7670_g_gain(struct v4l2_subdev *sd, __s32 *value)
unsigned char gain;
ret = ov7670_read(sd, REG_GAIN, &gain);
+ if (ret)
+ return ret;
*value = gain;
return ret;
}
@@ -1470,11 +1476,14 @@ static int ov7670_s_gain(struct v4l2_subdev *sd, int value)
unsigned char com8;
ret = ov7670_write(sd, REG_GAIN, value & 0xff);
+ if (ret)
+ return ret;
/* Have to turn off AGC as well */
- if (ret == 0) {
- ret = ov7670_read(sd, REG_COM8, &com8);
- ret = ov7670_write(sd, REG_COM8, com8 & ~COM8_AGC);
- }
+ ret = ov7670_read(sd, REG_COM8, &com8);
+ if (ret)
+ return ret;
+ ret = ov7670_write(sd, REG_COM8, com8 & ~COM8_AGC);
+
return ret;
}
--
2.18.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] media: ov7670: check status of ov7670_read 2020-08-28 14:55 [PATCH] media: ov7670: check status of ov7670_read trix @ 2020-08-28 15:13 ` Nick Desaulniers 2020-08-28 15:36 ` Tom Rix 2020-10-06 12:41 ` Sakari Ailus 1 sibling, 1 reply; 5+ messages in thread From: Nick Desaulniers @ 2020-08-28 15:13 UTC (permalink / raw) To: trix Cc: Jonathan Corbet, Mauro Carvalho Chehab, Nathan Chancellor, linux-media, LKML, Masahiro Yamada, Nathan Huckleberry On Fri, Aug 28, 2020 at 7:55 AM <trix@redhat.com> wrote: > > From: Tom Rix <trix@redhat.com> > > clang static analysis flags this representative problem > > drivers/media/i2c/ov7670.c:1463:9: warning: Assigned > value is garbage or undefined > *value = gain; > ^ ~~~~ > > gain is set by a successful call to ov7670_read() Indeed, it looks like gain is only valid if the return value from ov7670_read() is >= 0. Would it be simpler to just initialize gain to 0 in ov7670_g_gain? Side question; I'm super happy to see someone sending patches for things identified by clang's static analyzer. I'm curious, Tom, how did you run it? Did you use the recently landed in kbuild-next support for the make target `make clang-analyzer`? > > So check that ov7670_read() is successful. > > The remaining static analysis problems are false positives. > There appears to be a limitation with checking the > aggregated returns. > > Signed-off-by: Tom Rix <trix@redhat.com> > --- > drivers/media/i2c/ov7670.c | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c > index b42b289faaef..001d4b09db72 100644 > --- a/drivers/media/i2c/ov7670.c > +++ b/drivers/media/i2c/ov7670.c > @@ -929,6 +929,8 @@ static int ov7670_set_hw(struct v4l2_subdev *sd, int hstart, int hstop, > ret = ov7670_write(sd, REG_HSTART, (hstart >> 3) & 0xff); > ret += ov7670_write(sd, REG_HSTOP, (hstop >> 3) & 0xff); > ret += ov7670_read(sd, REG_HREF, &v); > + if (ret) > + return ret; > v = (v & 0xc0) | ((hstop & 0x7) << 3) | (hstart & 0x7); > msleep(10); > ret += ov7670_write(sd, REG_HREF, v); > @@ -938,6 +940,8 @@ static int ov7670_set_hw(struct v4l2_subdev *sd, int hstart, int hstop, > ret += ov7670_write(sd, REG_VSTART, (vstart >> 2) & 0xff); > ret += ov7670_write(sd, REG_VSTOP, (vstop >> 2) & 0xff); > ret += ov7670_read(sd, REG_VREF, &v); > + if (ret) > + return ret; > v = (v & 0xf0) | ((vstop & 0x3) << 2) | (vstart & 0x3); > msleep(10); > ret += ov7670_write(sd, REG_VREF, v); > @@ -1460,6 +1464,8 @@ static int ov7670_g_gain(struct v4l2_subdev *sd, __s32 *value) > unsigned char gain; > > ret = ov7670_read(sd, REG_GAIN, &gain); > + if (ret) > + return ret; > *value = gain; > return ret; > } > @@ -1470,11 +1476,14 @@ static int ov7670_s_gain(struct v4l2_subdev *sd, int value) > unsigned char com8; > > ret = ov7670_write(sd, REG_GAIN, value & 0xff); > + if (ret) > + return ret; > /* Have to turn off AGC as well */ > - if (ret == 0) { > - ret = ov7670_read(sd, REG_COM8, &com8); > - ret = ov7670_write(sd, REG_COM8, com8 & ~COM8_AGC); > - } > + ret = ov7670_read(sd, REG_COM8, &com8); > + if (ret) > + return ret; > + ret = ov7670_write(sd, REG_COM8, com8 & ~COM8_AGC); > + > return ret; > } > > -- > 2.18.1 > -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] media: ov7670: check status of ov7670_read 2020-08-28 15:13 ` Nick Desaulniers @ 2020-08-28 15:36 ` Tom Rix 0 siblings, 0 replies; 5+ messages in thread From: Tom Rix @ 2020-08-28 15:36 UTC (permalink / raw) To: Nick Desaulniers Cc: Jonathan Corbet, Mauro Carvalho Chehab, Nathan Chancellor, linux-media, LKML, Masahiro Yamada, Nathan Huckleberry On 8/28/20 8:13 AM, Nick Desaulniers wrote: > On Fri, Aug 28, 2020 at 7:55 AM <trix@redhat.com> wrote: >> From: Tom Rix <trix@redhat.com> >> >> clang static analysis flags this representative problem >> >> drivers/media/i2c/ov7670.c:1463:9: warning: Assigned >> value is garbage or undefined >> *value = gain; >> ^ ~~~~ >> >> gain is set by a successful call to ov7670_read() > Indeed, it looks like gain is only valid if the return value from > ov7670_read() is >= 0. Would it be simpler to just initialize gain to > 0 in ov7670_g_gain? It looks like ov7670_read() subroutines both convert > 0 returns to 0. Setting gain, and similar cases, to 0 means it is ok the setting side effect. maybe it would be ok if this was a void return or there was some known poison value. I did not look into that. There are other routines that are more than a simple get() and i wanted to use the same fix for all of them. yeah on clang static analysis making it into kbuild! I have been doing these fixes for a couple of months. with a just-built clang, i run 'scan-build make randomconfig', 'scan-build make', then run scan-view on the html. the randomconfig accounts for the very scattered fixes i have posted. generally i would say kernel is doing pretty well, maybe 20 to 1 false to real problems. Tom > > Side question; I'm super happy to see someone sending patches for > things identified by clang's static analyzer. I'm curious, Tom, how > did you run it? Did you use the recently landed in kbuild-next > support for the make target `make clang-analyzer`? > >> So check that ov7670_read() is successful. >> >> The remaining static analysis problems are false positives. >> There appears to be a limitation with checking the >> aggregated returns. >> >> Signed-off-by: Tom Rix <trix@redhat.com> >> --- >> drivers/media/i2c/ov7670.c | 17 +++++++++++++---- >> 1 file changed, 13 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c >> index b42b289faaef..001d4b09db72 100644 >> --- a/drivers/media/i2c/ov7670.c >> +++ b/drivers/media/i2c/ov7670.c >> @@ -929,6 +929,8 @@ static int ov7670_set_hw(struct v4l2_subdev *sd, int hstart, int hstop, >> ret = ov7670_write(sd, REG_HSTART, (hstart >> 3) & 0xff); >> ret += ov7670_write(sd, REG_HSTOP, (hstop >> 3) & 0xff); >> ret += ov7670_read(sd, REG_HREF, &v); >> + if (ret) >> + return ret; >> v = (v & 0xc0) | ((hstop & 0x7) << 3) | (hstart & 0x7); >> msleep(10); >> ret += ov7670_write(sd, REG_HREF, v); >> @@ -938,6 +940,8 @@ static int ov7670_set_hw(struct v4l2_subdev *sd, int hstart, int hstop, >> ret += ov7670_write(sd, REG_VSTART, (vstart >> 2) & 0xff); >> ret += ov7670_write(sd, REG_VSTOP, (vstop >> 2) & 0xff); >> ret += ov7670_read(sd, REG_VREF, &v); >> + if (ret) >> + return ret; >> v = (v & 0xf0) | ((vstop & 0x3) << 2) | (vstart & 0x3); >> msleep(10); >> ret += ov7670_write(sd, REG_VREF, v); >> @@ -1460,6 +1464,8 @@ static int ov7670_g_gain(struct v4l2_subdev *sd, __s32 *value) >> unsigned char gain; >> >> ret = ov7670_read(sd, REG_GAIN, &gain); >> + if (ret) >> + return ret; >> *value = gain; >> return ret; >> } >> @@ -1470,11 +1476,14 @@ static int ov7670_s_gain(struct v4l2_subdev *sd, int value) >> unsigned char com8; >> >> ret = ov7670_write(sd, REG_GAIN, value & 0xff); >> + if (ret) >> + return ret; >> /* Have to turn off AGC as well */ >> - if (ret == 0) { >> - ret = ov7670_read(sd, REG_COM8, &com8); >> - ret = ov7670_write(sd, REG_COM8, com8 & ~COM8_AGC); >> - } >> + ret = ov7670_read(sd, REG_COM8, &com8); >> + if (ret) >> + return ret; >> + ret = ov7670_write(sd, REG_COM8, com8 & ~COM8_AGC); >> + >> return ret; >> } >> >> -- >> 2.18.1 >> > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] media: ov7670: check status of ov7670_read 2020-08-28 14:55 [PATCH] media: ov7670: check status of ov7670_read trix 2020-08-28 15:13 ` Nick Desaulniers @ 2020-10-06 12:41 ` Sakari Ailus 2020-10-06 14:17 ` Tom Rix 1 sibling, 1 reply; 5+ messages in thread From: Sakari Ailus @ 2020-10-06 12:41 UTC (permalink / raw) To: trix Cc: corbet, mchehab, natechancellor, ndesaulniers, linux-media, linux-kernel Hi Tom, On Fri, Aug 28, 2020 at 07:55:18AM -0700, trix@redhat.com wrote: > From: Tom Rix <trix@redhat.com> > > clang static analysis flags this representative problem > > drivers/media/i2c/ov7670.c:1463:9: warning: Assigned > value is garbage or undefined > *value = gain; > ^ ~~~~ > > gain is set by a successful call to ov7670_read() > > So check that ov7670_read() is successful. > > The remaining static analysis problems are false positives. > There appears to be a limitation with checking the > aggregated returns. > > Signed-off-by: Tom Rix <trix@redhat.com> > --- > drivers/media/i2c/ov7670.c | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c > index b42b289faaef..001d4b09db72 100644 > --- a/drivers/media/i2c/ov7670.c > +++ b/drivers/media/i2c/ov7670.c > @@ -929,6 +929,8 @@ static int ov7670_set_hw(struct v4l2_subdev *sd, int hstart, int hstop, > ret = ov7670_write(sd, REG_HSTART, (hstart >> 3) & 0xff); > ret += ov7670_write(sd, REG_HSTOP, (hstop >> 3) & 0xff); > ret += ov7670_read(sd, REG_HREF, &v); > + if (ret) > + return ret; Thanks for the patch. While the patch fixes a bug, could you also fix adding the return values? These are valid error codes to begin with, but it makes no sense to add them together. > v = (v & 0xc0) | ((hstop & 0x7) << 3) | (hstart & 0x7); > msleep(10); > ret += ov7670_write(sd, REG_HREF, v); > @@ -938,6 +940,8 @@ static int ov7670_set_hw(struct v4l2_subdev *sd, int hstart, int hstop, > ret += ov7670_write(sd, REG_VSTART, (vstart >> 2) & 0xff); > ret += ov7670_write(sd, REG_VSTOP, (vstop >> 2) & 0xff); > ret += ov7670_read(sd, REG_VREF, &v); > + if (ret) > + return ret; > v = (v & 0xf0) | ((vstop & 0x3) << 2) | (vstart & 0x3); > msleep(10); > ret += ov7670_write(sd, REG_VREF, v); > @@ -1460,6 +1464,8 @@ static int ov7670_g_gain(struct v4l2_subdev *sd, __s32 *value) > unsigned char gain; > > ret = ov7670_read(sd, REG_GAIN, &gain); > + if (ret) > + return ret; > *value = gain; > return ret; > } > @@ -1470,11 +1476,14 @@ static int ov7670_s_gain(struct v4l2_subdev *sd, int value) > unsigned char com8; > > ret = ov7670_write(sd, REG_GAIN, value & 0xff); > + if (ret) > + return ret; > /* Have to turn off AGC as well */ > - if (ret == 0) { > - ret = ov7670_read(sd, REG_COM8, &com8); > - ret = ov7670_write(sd, REG_COM8, com8 & ~COM8_AGC); > - } > + ret = ov7670_read(sd, REG_COM8, &com8); > + if (ret) > + return ret; > + ret = ov7670_write(sd, REG_COM8, com8 & ~COM8_AGC); > + > return ret; > } > -- Regards, Sakari Ailus ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] media: ov7670: check status of ov7670_read 2020-10-06 12:41 ` Sakari Ailus @ 2020-10-06 14:17 ` Tom Rix 0 siblings, 0 replies; 5+ messages in thread From: Tom Rix @ 2020-10-06 14:17 UTC (permalink / raw) To: Sakari Ailus Cc: corbet, mchehab, natechancellor, ndesaulniers, linux-media, linux-kernel On 10/6/20 5:41 AM, Sakari Ailus wrote: > Hi Tom, > > On Fri, Aug 28, 2020 at 07:55:18AM -0700, trix@redhat.com wrote: >> From: Tom Rix <trix@redhat.com> >> >> clang static analysis flags this representative problem >> >> drivers/media/i2c/ov7670.c:1463:9: warning: Assigned >> value is garbage or undefined >> *value = gain; >> ^ ~~~~ >> >> gain is set by a successful call to ov7670_read() >> >> So check that ov7670_read() is successful. >> >> The remaining static analysis problems are false positives. >> There appears to be a limitation with checking the >> aggregated returns. >> >> Signed-off-by: Tom Rix <trix@redhat.com> >> --- >> drivers/media/i2c/ov7670.c | 17 +++++++++++++---- >> 1 file changed, 13 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c >> index b42b289faaef..001d4b09db72 100644 >> --- a/drivers/media/i2c/ov7670.c >> +++ b/drivers/media/i2c/ov7670.c >> @@ -929,6 +929,8 @@ static int ov7670_set_hw(struct v4l2_subdev *sd, int hstart, int hstop, >> ret = ov7670_write(sd, REG_HSTART, (hstart >> 3) & 0xff); >> ret += ov7670_write(sd, REG_HSTOP, (hstop >> 3) & 0xff); >> ret += ov7670_read(sd, REG_HREF, &v); >> + if (ret) >> + return ret; > Thanks for the patch. > > While the patch fixes a bug, could you also fix adding the return values? > These are valid error codes to begin with, but it makes no sense to add > them together. Yes. That is a problem, a general problem with this file. I count 10+ I'll see if fixing the general problem also fixes this problem. Tom > >> v = (v & 0xc0) | ((hstop & 0x7) << 3) | (hstart & 0x7); >> msleep(10); >> ret += ov7670_write(sd, REG_HREF, v); >> @@ -938,6 +940,8 @@ static int ov7670_set_hw(struct v4l2_subdev *sd, int hstart, int hstop, >> ret += ov7670_write(sd, REG_VSTART, (vstart >> 2) & 0xff); >> ret += ov7670_write(sd, REG_VSTOP, (vstop >> 2) & 0xff); >> ret += ov7670_read(sd, REG_VREF, &v); >> + if (ret) >> + return ret; >> v = (v & 0xf0) | ((vstop & 0x3) << 2) | (vstart & 0x3); >> msleep(10); >> ret += ov7670_write(sd, REG_VREF, v); >> @@ -1460,6 +1464,8 @@ static int ov7670_g_gain(struct v4l2_subdev *sd, __s32 *value) >> unsigned char gain; >> >> ret = ov7670_read(sd, REG_GAIN, &gain); >> + if (ret) >> + return ret; >> *value = gain; >> return ret; >> } >> @@ -1470,11 +1476,14 @@ static int ov7670_s_gain(struct v4l2_subdev *sd, int value) >> unsigned char com8; >> >> ret = ov7670_write(sd, REG_GAIN, value & 0xff); >> + if (ret) >> + return ret; >> /* Have to turn off AGC as well */ >> - if (ret == 0) { >> - ret = ov7670_read(sd, REG_COM8, &com8); >> - ret = ov7670_write(sd, REG_COM8, com8 & ~COM8_AGC); >> - } >> + ret = ov7670_read(sd, REG_COM8, &com8); >> + if (ret) >> + return ret; >> + ret = ov7670_write(sd, REG_COM8, com8 & ~COM8_AGC); >> + >> return ret; >> } >> ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-10-06 14:17 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-08-28 14:55 [PATCH] media: ov7670: check status of ov7670_read trix 2020-08-28 15:13 ` Nick Desaulniers 2020-08-28 15:36 ` Tom Rix 2020-10-06 12:41 ` Sakari Ailus 2020-10-06 14:17 ` Tom Rix
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).