Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] media: em28xx: Fix missing check in em28xx_capture_start
To:     Dinghao Liu <dinghao.liu@zju.edu.cn>, kjlu@umn.edu
Cc:     Mauro Carvalho Chehab <mchehab@kernel.org>,
        linux-media@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20210310090032.13477-1-dinghao.liu@zju.edu.cn>
From:   Hans Verkuil <hverkuil@xs4all.nl>
Message-ID: <74451231-31b9-4489-2241-febb05cf5bec@xs4all.nl>
Date:   Tue, 16 Mar 2021 08:55:48 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Firefox/78.0 Thunderbird/78.7.1
MIME-Version: 1.0
In-Reply-To: <20210310090032.13477-1-dinghao.liu@zju.edu.cn>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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 <dinghao.liu@zju.edu.cn>
> ---
>  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;
>  		}
>  	}
>  
>