Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1644933 > unrolled thread

[media-pci-cx25821] question about value overwrite

Back to article view | Back to linux.kernel

Contents

  [media-pci-cx25821] question about value overwrite "Gustavo A. R. Silva" <garsilva@embeddedor.com> - 2017-05-19 00:10 +0200
    Re: [media-pci-cx25821] question about value overwrite Hans Verkuil <hverkuil@xs4all.nl> - 2017-05-22 09:50 +0200

#1644933 — [media-pci-cx25821] question about value overwrite

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

[toc] | [next] | [standalone]

#1646577

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web