Re: [media-pci-cx25821] question about value overwrite

[Hans Verkuil]

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
> 
> 
> 
>