On 05/19/2017 12:07 AM, Gustavo A. R. Silva wrote:
>
> Hello everybody,
>
> While looking into Coverity ID 1226903 I ran into the following piece
> of code at drivers/media/pci/cx25821/cx25821-medusa-video.c:393:
>
> 393int medusa_set_videostandard(struct cx25821_dev *dev)
> 394{
> 395 int status = 0;
> 396 u32 value = 0, tmp = 0;
> 397
> 398 if (dev->tvnorm & V4L2_STD_PAL_BG || dev->tvnorm & V4L2_STD_PAL_DK)
> 399 status = medusa_initialize_pal(dev);
> 400 else
> 401 status = medusa_initialize_ntsc(dev);
> 402
> 403 /* Enable DENC_A output */
> 404 value = cx25821_i2c_read(&dev->i2c_bus[0], DENC_A_REG_4, &tmp);
> 405 value = setBitAtPos(value, 4);
> 406 status = cx25821_i2c_write(&dev->i2c_bus[0], DENC_A_REG_4, value);
> 407
> 408 /* Enable DENC_B output */
> 409 value = cx25821_i2c_read(&dev->i2c_bus[0], DENC_B_REG_4, &tmp);
> 410 value = setBitAtPos(value, 4);
> 411 status = cx25821_i2c_write(&dev->i2c_bus[0], DENC_B_REG_4, value);
> 412
> 413 return status;
> 414}
>
> The issue is that the value stored in variable _status_ at lines 399
> and 401 is overwritten by the one stored at line 406 and then at line
> 411, before it can be used.
>
> My question is if the original intention was to ORed the return
> values, something like in the following patch:
>
> index 0a9db05..226d14f 100644
> --- a/drivers/media/pci/cx25821/cx25821-medusa-video.c
> +++ b/drivers/media/pci/cx25821/cx25821-medusa-video.c
> @@ -403,12 +403,12 @@ int medusa_set_videostandard(struct cx25821_dev *dev)
> /* Enable DENC_A output */
> value = cx25821_i2c_read(&dev->i2c_bus[0], DENC_A_REG_4, &tmp);
> value = setBitAtPos(value, 4);
> - status = cx25821_i2c_write(&dev->i2c_bus[0], DENC_A_REG_4, value);
> + status |= cx25821_i2c_write(&dev->i2c_bus[0], DENC_A_REG_4, value);
>
> /* Enable DENC_B output */
> value = cx25821_i2c_read(&dev->i2c_bus[0], DENC_B_REG_4, &tmp);
> value = setBitAtPos(value, 4);
> - status = cx25821_i2c_write(&dev->i2c_bus[0], DENC_B_REG_4, value);
> + status |= cx25821_i2c_write(&dev->i2c_bus[0], DENC_B_REG_4, value);
>
> return status;
> }
This is a crappy driver, they just couldn't be bothered to check the error from
cx25821_i2c_read/write.
Strictly speaking the return value should be checked after every read/write and
returned in case of an error.
Not sure whether it is worth the effort fixing this.
Regards,
Hans
>
> What do you think?
>
> I'd really appreciate any comment on this.
>
> Thank you!
> --
> Gustavo A. R. Silva
>
>
>
>
