Re: [PATCH v2 19/26] media: ov9650: fix bogus warnings
On Thu, Nov 02, 2017 at 10:06:06AM +, Nicholas Mc Guire wrote: > On Wed, Nov 01, 2017 at 05:05:56PM -0400, Mauro Carvalho Chehab wrote: > > The smatch logic gets confused with the syntax used to check if the > > ov9650x_read() reads succedded: > > drivers/media/i2c/ov9650.c:895 __g_volatile_ctrl() error: uninitialized > > symbol 'reg2'. > > drivers/media/i2c/ov9650.c:895 __g_volatile_ctrl() error: uninitialized > > symbol 'reg1'. > > > > There's nothing wrong with the original logic, except that > > it is a little more harder to review. > > Maybe I do not understand the original logic correctly but > ov965x_read(...) is passing on the return value from > i2c_transfer() -> __i2c_transfer() and thus if one of those > calls would have been a negativ error status it would have simply > executed the next call to ov965x_read() and if that > call would have suceeded it would eventually reach the > line " exposure = ((reg2 & 0x3f) << 10) | (reg1 << 2) |..." > with the potential to operate on uninitialized registers reg0/1/2 > the current code sems only to handle error conditions in the last > call to ov965x_read() correctly. sorry - sent that out too fast - the logic is equivalent the negative returns are treated correctly because only the success case is being returned explicidly as 0 in ov965x_read() return statement by checking for the expecte return of 1 from i2c_transfer() wrapping it to 0. sorry for the noise. thx! hofrat > > I think this is actually not an equivalent transform but a bug-fix > of case V4L2_CID_EXPOSURE_AUTO: (aside from being a code inconsistency) > So should this not carry a > Fixes: 84a15ded76ec ("[media] V4L: Add driver for OV9650/52 image sensors") > tag ? > > thx! > hofrat > > > > > So, let's stick with the syntax that won't cause read > > issues. > > > > Signed-off-by: Mauro Carvalho Chehab> > Reviewed-by: Nicholas Mc Guire > > > --- > > drivers/media/i2c/ov9650.c | 10 ++ > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/media/i2c/ov9650.c b/drivers/media/i2c/ov9650.c > > index 69433e1e2533..e519f278d5f9 100644 > > --- a/drivers/media/i2c/ov9650.c > > +++ b/drivers/media/i2c/ov9650.c > > @@ -886,10 +886,12 @@ static int __g_volatile_ctrl(struct ov965x *ov965x, > > struct v4l2_ctrl *ctrl) > > if (ctrl->val == V4L2_EXPOSURE_MANUAL) > > return 0; > > ret = ov965x_read(client, REG_COM1, ); > > - if (!ret) > > - ret = ov965x_read(client, REG_AECH, ); > > - if (!ret) > > - ret = ov965x_read(client, REG_AECHM, ); > > + if (ret < 0) > > + return ret; > > + ret = ov965x_read(client, REG_AECH, ); > > + if (ret < 0) > > + return ret; > > + ret = ov965x_read(client, REG_AECHM, ); > > if (ret < 0) > > return ret; > > exposure = ((reg2 & 0x3f) << 10) | (reg1 << 2) | > > -- > > 2.13.6 > >
Re: [PATCH v2 19/26] media: ov9650: fix bogus warnings
On Wed, Nov 01, 2017 at 05:05:56PM -0400, Mauro Carvalho Chehab wrote: > The smatch logic gets confused with the syntax used to check if the > ov9650x_read() reads succedded: > drivers/media/i2c/ov9650.c:895 __g_volatile_ctrl() error: uninitialized > symbol 'reg2'. > drivers/media/i2c/ov9650.c:895 __g_volatile_ctrl() error: uninitialized > symbol 'reg1'. > > There's nothing wrong with the original logic, except that > it is a little more harder to review. Maybe I do not understand the original logic correctly but ov965x_read(...) is passing on the return value from i2c_transfer() -> __i2c_transfer() and thus if one of those calls would have been a negativ error status it would have simply executed the next call to ov965x_read() and if that call would have suceeded it would eventually reach the line " exposure = ((reg2 & 0x3f) << 10) | (reg1 << 2) |..." with the potential to operate on uninitialized registers reg0/1/2 the current code sems only to handle error conditions in the last call to ov965x_read() correctly. I think this is actually not an equivalent transform but a bug-fix of case V4L2_CID_EXPOSURE_AUTO: (aside from being a code inconsistency) So should this not carry a Fixes: 84a15ded76ec ("[media] V4L: Add driver for OV9650/52 image sensors") tag ? thx! hofrat > > So, let's stick with the syntax that won't cause read > issues. > > Signed-off-by: Mauro Carvalho ChehabReviewed-by: Nicholas Mc Guire > --- > drivers/media/i2c/ov9650.c | 10 ++ > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/i2c/ov9650.c b/drivers/media/i2c/ov9650.c > index 69433e1e2533..e519f278d5f9 100644 > --- a/drivers/media/i2c/ov9650.c > +++ b/drivers/media/i2c/ov9650.c > @@ -886,10 +886,12 @@ static int __g_volatile_ctrl(struct ov965x *ov965x, > struct v4l2_ctrl *ctrl) > if (ctrl->val == V4L2_EXPOSURE_MANUAL) > return 0; > ret = ov965x_read(client, REG_COM1, ); > - if (!ret) > - ret = ov965x_read(client, REG_AECH, ); > - if (!ret) > - ret = ov965x_read(client, REG_AECHM, ); > + if (ret < 0) > + return ret; > + ret = ov965x_read(client, REG_AECH, ); > + if (ret < 0) > + return ret; > + ret = ov965x_read(client, REG_AECHM, ); > if (ret < 0) > return ret; > exposure = ((reg2 & 0x3f) << 10) | (reg1 << 2) | > -- > 2.13.6 >
[PATCH v2 19/26] media: ov9650: fix bogus warnings
The smatch logic gets confused with the syntax used to check if the ov9650x_read() reads succedded: drivers/media/i2c/ov9650.c:895 __g_volatile_ctrl() error: uninitialized symbol 'reg2'. drivers/media/i2c/ov9650.c:895 __g_volatile_ctrl() error: uninitialized symbol 'reg1'. There's nothing wrong with the original logic, except that it is a little more harder to review. So, let's stick with the syntax that won't cause read issues. Signed-off-by: Mauro Carvalho Chehab--- drivers/media/i2c/ov9650.c | 10 ++ 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/drivers/media/i2c/ov9650.c b/drivers/media/i2c/ov9650.c index 69433e1e2533..e519f278d5f9 100644 --- a/drivers/media/i2c/ov9650.c +++ b/drivers/media/i2c/ov9650.c @@ -886,10 +886,12 @@ static int __g_volatile_ctrl(struct ov965x *ov965x, struct v4l2_ctrl *ctrl) if (ctrl->val == V4L2_EXPOSURE_MANUAL) return 0; ret = ov965x_read(client, REG_COM1, ); - if (!ret) - ret = ov965x_read(client, REG_AECH, ); - if (!ret) - ret = ov965x_read(client, REG_AECHM, ); + if (ret < 0) + return ret; + ret = ov965x_read(client, REG_AECH, ); + if (ret < 0) + return ret; + ret = ov965x_read(client, REG_AECHM, ); if (ret < 0) return ret; exposure = ((reg2 & 0x3f) << 10) | (reg1 << 2) | -- 2.13.6