* [media-pci-cx25821] question about value overwrite
@ 2017-05-18 22:07 Gustavo A. R. Silva
2017-05-22 7:45 ` Hans Verkuil
0 siblings, 1 reply; 3+ messages in thread
From: Gustavo A. R. Silva @ 2017-05-18 22:07 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: linux-media, linux-kernel
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;
}
What do you think?
I'd really appreciate any comment on this.
Thank you!
--
Gustavo A. R. Silva
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [media-pci-cx25821] question about value overwrite
2017-05-18 22:07 [media-pci-cx25821] question about value overwrite Gustavo A. R. Silva
@ 2017-05-22 7:45 ` Hans Verkuil
2017-06-20 21:57 ` Gustavo A. R. Silva
0 siblings, 1 reply; 3+ messages in thread
From: Hans Verkuil @ 2017-05-22 7:45 UTC (permalink / raw)
To: Gustavo A. R. Silva, Mauro Carvalho Chehab; +Cc: linux-media, linux-kernel
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
>
>
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [media-pci-cx25821] question about value overwrite
2017-05-22 7:45 ` Hans Verkuil
@ 2017-06-20 21:57 ` Gustavo A. R. Silva
0 siblings, 0 replies; 3+ messages in thread
From: Gustavo A. R. Silva @ 2017-06-20 21:57 UTC (permalink / raw)
To: Hans Verkuil; +Cc: Mauro Carvalho Chehab, linux-media, linux-kernel
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-06-20 21:58 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-18 22:07 [media-pci-cx25821] question about value overwrite Gustavo A. R. Silva
2017-05-22 7:45 ` Hans Verkuil
2017-06-20 21:57 ` Gustavo A. R. Silva
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).