If we fail to query the ctrl error code there is no information on dmesg
or in uvc_dbg. This makes difficult to debug the issue.

Print a proper error message when we cannot retrieve the error code from
the device.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_video.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index f125b3ba50f2..6efbfa609059 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -111,8 +111,12 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
 	error = *(u8 *)data;
 	*(u8 *)data = tmp;
 
-	if (ret != 1)
-		return ret < 0 ? ret : -EPIPE;
+	if (ret != 1) {
+		dev_err(&dev->udev->dev,
+			"Failed to query (%s) UVC error code control %u on unit %u: %d (exp. 1).\n",
+			uvc_query_name(query), cs, unit, ret);
+		return ret ? ret : -EPIPE;
+	}
 
 	uvc_dbg(dev, CONTROL, "Control error %u\n", error);
 

-- 
2.47.0.rc0.187.ge670bccf7e-goog

Hi Ricardo,

Thank you for your patch.

On 8-Oct-24 5:00 PM, Ricardo Ribalda wrote:
> If we fail to query the ctrl error code there is no information on dmesg
> or in uvc_dbg. This makes difficult to debug the issue.
> 
> Print a proper error message when we cannot retrieve the error code from
> the device.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_video.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index f125b3ba50f2..6efbfa609059 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -111,8 +111,12 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
>  	error = *(u8 *)data;
>  	*(u8 *)data = tmp;
>  
> -	if (ret != 1)
> -		return ret < 0 ? ret : -EPIPE;
> +	if (ret != 1) {
> +		dev_err(&dev->udev->dev,
> +			"Failed to query (%s) UVC error code control %u on unit %u: %d (exp. 1).\n",
> +			uvc_query_name(query), cs, unit, ret);
> +		return ret ? ret : -EPIPE;

Adding error logging is always good, but again please drop the change
to the return condition and stick with the:

	return ret < 0 ? ret : -EPIPE;

pattern used everywhere.

Also in this case the return condition change really should have
been in a separate patch since unlike before the success condition
did not change, so it really is unrelated to adding the error
logging.

Regards,

Hans

Hi Hans

On Mon, 18 Nov 2024 at 17:45, Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi Ricardo,
>
> Thank you for your patch.
>
> On 8-Oct-24 5:00 PM, Ricardo Ribalda wrote:
> > If we fail to query the ctrl error code there is no information on dmesg
> > or in uvc_dbg. This makes difficult to debug the issue.
> >
> > Print a proper error message when we cannot retrieve the error code from
> > the device.
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/usb/uvc/uvc_video.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > index f125b3ba50f2..6efbfa609059 100644
> > --- a/drivers/media/usb/uvc/uvc_video.c
> > +++ b/drivers/media/usb/uvc/uvc_video.c
> > @@ -111,8 +111,12 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
> >       error = *(u8 *)data;
> >       *(u8 *)data = tmp;
> >
> > -     if (ret != 1)
> > -             return ret < 0 ? ret : -EPIPE;
> > +     if (ret != 1) {
> > +             dev_err(&dev->udev->dev,
> > +                     "Failed to query (%s) UVC error code control %u on unit %u: %d (exp. 1).\n",
> > +                     uvc_query_name(query), cs, unit, ret);
> > +             return ret ? ret : -EPIPE;
>
> Adding error logging is always good, but again please drop the change
> to the return condition and stick with the:
>
>         return ret < 0 ? ret : -EPIPE;
>
> pattern used everywhere.
>
> Also in this case the return condition change really should have
> been in a separate patch since unlike before the success condition
> did not change, so it really is unrelated to adding the error
> logging.

It doesn't really change: __uvc_query_ctrl() in this case can return
1, 0 or a retval.
But I agree, the change does not provide anything (and I did not
mentioned it on the commit msg) let me restore the original return

Thanks

>
> Regards,
>
> Hans
>
>
>


-- 
Ricardo Ribalda