Messages in this thread | | | Date | Tue, 20 Jun 2017 16:57:56 -0500 | From | "Gustavo A. R. Silva" <> | Subject | Re: [media-pci-cx25821] question about value overwrite |
| |
Hi Hans,
Quoting Hans Verkuil <hverkuil@xs4all.nl>:
> 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
| |