Hi Dan, Colin,
On Sat, Feb 27, 2021 at 01:17:20PM +0300, Dan Carpenter wrote:
> On Fri, Feb 26, 2021 at 11:22:29PM +0000, Colin King wrote:
> > From: Colin Ian King <colin.king@xxxxxxxxxxxxx>
> >
> > The error check on rval from the call to adp1653_get_fault currently
> > returns if rval is non-zero. This appears to be incorrect as the
> > following if statement checks for various bit settings in rval so
> > clearly rval is expected to be non-zero at that point. Coverity
> > flagged the if statement up as deadcode. Fix this so the error
> > return path only occurs when rval is negative.
> >
> > Addresses-Coverity: ("Logically dead code")
> > Fixes: 287980e49ffc ("remove lots of IS_ERR_VALUE abuses")
> > Signed-off-by: Colin Ian King <colin.king@xxxxxxxxxxxxx>
> > ---
> > drivers/media/i2c/adp1653.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c
> > index 522a0b10e415..1a4878385394 100644
> > --- a/drivers/media/i2c/adp1653.c
> > +++ b/drivers/media/i2c/adp1653.c
> > @@ -170,7 +170,7 @@ static int adp1653_set_ctrl(struct v4l2_ctrl *ctrl)
> > int rval;
> >
> > rval = adp1653_get_fault(flash);
> > - if (rval)
> > + if (rval < 0)
> > return rval;
>
> This is good, but all the other callers need to fixed as well:
>
>
> 140 static int adp1653_get_ctrl(struct v4l2_ctrl *ctrl)
> 141 {
> 142 struct adp1653_flash *flash =
> 143 container_of(ctrl->handler, struct adp1653_flash, ctrls);
> 144 int rval;
> 145
> 146 rval = adp1653_get_fault(flash);
> 147 if (rval)
> 148 return rval;
> 149
> 150 ctrl->cur.val = 0;
> 151
> 152 if (flash->fault & ADP1653_REG_FAULT_FLT_SCP)
> ^^^^^^^^^^^^
> flash->fault is the equivalent of "rval" for non-negative returns so
> this condition can never be true. We should never be returning these
> weird firmware ADP1653_REG_FAULT_FLT_SCP fault codes to the v4l2 layers.
I think this could be fixed and cleaned up by always retuning zero on
success, and checking for flash->faults while holding the mutex in
adp1653_init_device. I could fix that, too, just let me know...
--
Regards,
Sakari Ailus
