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

"Gustavo A. R. Silva" <garsilva@xxxxxxxxxxxxxx> · Tue, 20 Jun 2017 16:57:56 -0500

Hi Hans,

Quoting Hans Verkuil <hverkuil@xxxxxxxxx>:

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.


Yeah, the same happens in functions medusa_initialize_pal() and  
medusa_initialize_ntsc()

Not sure whether it is worth the effort fixing this.


Thank you for your reply.
--
Gustavo A. R. Silva










