lkml.org 
[lkml]   [2017]   [Jun]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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




\
 
 \ /
  Last update: 2017-06-20 23:58    [W:0.071 / U:0.596 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site