2021-03-10 09:04:58

by Dinghao Liu

Subject: [PATCH] media: em28xx: Fix missing check in em28xx_capture_start

There are several em28xx_write_reg() and em28xx_write_reg_bits()
calls that we have caught their return values but lack further
handling. Check and return error on failure just like other calls
in em28xx_capture_start().

Signed-off-by: Dinghao Liu <[email protected]>
---
drivers/media/usb/em28xx/em28xx-core.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
index 584fa400cd7d..2563275fec8e 100644
--- a/drivers/media/usb/em28xx/em28xx-core.c
+++ b/drivers/media/usb/em28xx/em28xx-core.c
@@ -661,6 +661,8 @@ int em28xx_capture_start(struct em28xx *dev, int start)
EM2874_R5F_TS_ENABLE,
start ? EM2874_TS2_CAPTURE_ENABLE : 0x00,
EM2874_TS2_CAPTURE_ENABLE | EM2874_TS2_FILTER_ENABLE | EM2874_TS2_NULL_DISCARD);
+ if (rc < 0)
+ return rc;
} else {
/* FIXME: which is the best order? */
/* video registers are sampled by VREF */
@@ -670,8 +672,11 @@ int em28xx_capture_start(struct em28xx *dev, int start)
return rc;

if (start) {
- if (dev->is_webcam)
+ if (dev->is_webcam) {
rc = em28xx_write_reg(dev, 0x13, 0x0c);
+ if (rc < 0)
+ return rc;
+ }

/* Enable video capture */
rc = em28xx_write_reg(dev, 0x48, 0x00);
@@ -693,6 +698,8 @@ int em28xx_capture_start(struct em28xx *dev, int start)
} else {
/* disable video capture */
rc = em28xx_write_reg(dev, EM28XX_R12_VINENABLE, 0x27);
+ if (rc < 0)
+ return rc;
}
}

--
2.17.1

2021-03-16 09:26:17

by Hans Verkuil

Subject: Re: [PATCH] media: em28xx: Fix missing check in em28xx_capture_start

Hi Dinghao Liu,

Thank you for the patch, but I've decided not to take it. While the
patch looks fine, it is not very useful since the return code of the
em28xx_capture_start() function is never checked. And I am hesitant
to change the behavior here in case it might break something subtle.

Ideally the error code of em28xx_capture_start() should also be
checked, and cascaded all the way up to the higher levels of the code,
but the reality is that if these register writes fail, then you likely
have much bigger problems so I don't think it is worth doing that.

Regards,

Hans

On 10/03/2021 10:00, Dinghao Liu wrote:
> There are several em28xx_write_reg() and em28xx_write_reg_bits()
> calls that we have caught their return values but lack further
> handling. Check and return error on failure just like other calls
> in em28xx_capture_start().
>
> Signed-off-by: Dinghao Liu <[email protected]>
> ---
> drivers/media/usb/em28xx/em28xx-core.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
> index 584fa400cd7d..2563275fec8e 100644
> --- a/drivers/media/usb/em28xx/em28xx-core.c
> +++ b/drivers/media/usb/em28xx/em28xx-core.c
> @@ -661,6 +661,8 @@ int em28xx_capture_start(struct em28xx *dev, int start)
> EM2874_R5F_TS_ENABLE,
> start ? EM2874_TS2_CAPTURE_ENABLE : 0x00,
> EM2874_TS2_CAPTURE_ENABLE | EM2874_TS2_FILTER_ENABLE | EM2874_TS2_NULL_DISCARD);
> + if (rc < 0)
> + return rc;
> } else {
> /* FIXME: which is the best order? */
> /* video registers are sampled by VREF */
> @@ -670,8 +672,11 @@ int em28xx_capture_start(struct em28xx *dev, int start)
> return rc;
>
> if (start) {
> - if (dev->is_webcam)
> + if (dev->is_webcam) {
> rc = em28xx_write_reg(dev, 0x13, 0x0c);
> + if (rc < 0)
> + return rc;
> + }
>
> /* Enable video capture */
> rc = em28xx_write_reg(dev, 0x48, 0x00);
> @@ -693,6 +698,8 @@ int em28xx_capture_start(struct em28xx *dev, int start)
> } else {
> /* disable video capture */
> rc = em28xx_write_reg(dev, EM28XX_R12_VINENABLE, 0x27);
> + if (rc < 0)
> + return rc;
> }
> }
>
>