From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 50124C433E9 for ; Thu, 11 Mar 2021 15:58:51 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 055E264FE5 for ; Thu, 11 Mar 2021 15:58:50 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234350AbhCKP6X (ORCPT ); Thu, 11 Mar 2021 10:58:23 -0500 Received: from perceval.ideasonboard.com ([213.167.242.64]:50060 "EHLO perceval.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234061AbhCKP6A (ORCPT ); Thu, 11 Mar 2021 10:58:00 -0500 Received: from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi [62.78.145.57]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 65A71879; Thu, 11 Mar 2021 16:57:58 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1615478278; bh=uh0v20vIuAerIj2S1tG5H54I5B46Ush+Nb3Iwpomybs=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=I0LF6V+jDvhn51RT9PakCHBIk2FP3zx1PsFE4pqMeVqiPKlJZDp/2WPXk1550Q9DD 48HcHgK5a2tUQpKijh0uZ7OARltXcY4gJ8W/IoFNfwiqARowKmBzqg0qQIO8XVlhzV rvmwG37dVm0/Stj5tt15e9V8M5hXOnpIdXu5u4w4= Date: Thu, 11 Mar 2021 17:57:24 +0200 From: Laurent Pinchart To: Ricardo Ribalda Cc: Mauro Carvalho Chehab , Tomasz Figa , Linux Media Mailing List , Linux Kernel Mailing List , Sergey Senozhatsky , Hans Verkuil Subject: Re: [PATCH 03/10] media: uvcvideo: Return -EIO for control errors Message-ID: References: <20210311122040.1264410-1-ribalda@chromium.org> <20210311122040.1264410-5-ribalda@chromium.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Ricardo, Thank you for the patch. On Thu, Mar 11, 2021 at 03:08:22PM +0100, Ricardo Ribalda wrote: > As discussed in the IRC with Hans > > We need to specify in the commit message that this is most likely due > to hw error. > > On Thu, Mar 11, 2021 at 1:20 PM Ricardo Ribalda wrote: > > > > Fixes v4l2-compliance: > > > > Control ioctls (Input 0): > > fail: v4l2-test-controls.cpp(448): s_ctrl returned an error (22) > > test VIDIOC_G/S_CTRL: FAIL > > fail: v4l2-test-controls.cpp(698): s_ext_ctrls returned an error (22) > > test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL As this isn't supposed to happen, how do you reproduce this ? > > Signed-off-by: Ricardo Ribalda > > --- > > drivers/media/usb/uvc/uvc_video.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c > > index f2f565281e63..5442e9be1c55 100644 > > --- a/drivers/media/usb/uvc/uvc_video.c > > +++ b/drivers/media/usb/uvc/uvc_video.c > > @@ -113,7 +113,7 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, > > case 6: /* Invalid control */ > > case 7: /* Invalid Request */ For cases 5-7 I think -EIO is fine, as the driver should really not call this function with an invalid unit, control or request. If it does, it's a bug in the driver (we can check the units and controls the device claims to support, and the requests are defined by the UVC specification), if it doesn't and the device still returns this error, it's a bug on the device side. > > case 8: /* Invalid value within range */ For this case, however, isn't it valid for a device to return an error if the control value isn't valid ? There's one particular code path I'm concerned about, uvc_ioctl_default(UVCIOC_CTRL_QUERY) -> uvc_xu_ctrl_query() -> uvc_query_ctrl(), where it could be useful for userspace to know that the value it sets isn't valid. > > - return -EINVAL; > > + return -EIO; > > default: /* reserved or unknown */ > > break; > > } -- Regards, Laurent Pinchart