[PATCH 0/3] media: uvcvideo: Support partial control reads and minor changes

[PATCH 0/3] media: uvcvideo: Support partial control reads and minor changes

[Ricardo Ribalda]

Some cameras do not return all the bytes requested from a control
if it can fit in less bytes. Eg: returning 0xab instead of 0x00ab.
Support these devices.

Also, now that we are at it, improve uvc_query_ctrl().

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
Ricardo Ribalda (3):
      media: uvcvideo: Support partial control reads
      media: uvcvideo: Refactor uvc_query_ctrl
      media: uvcvideo: Add more logging to uvc_query_ctrl_error()

 drivers/media/usb/uvc/uvc_video.c | 44 +++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 16 deletions(-)
---
base-commit: 9852d85ec9d492ebef56dc5f229416c925758edc
change-id: 20241008-uvc-readless-23f9b8cad0b3

Best regards,
-- 
Ricardo Ribalda <ribalda@chromium.org>

[PATCH 1/3] media: uvcvideo: Support partial control reads

[Ricardo Ribalda]

Some cameras, like the ELMO MX-P3, do not return all the bytes
requested from a control if it can fit in less bytes.
Eg: Returning 0xab instead of 0x00ab.
usb 3-9: Failed to query (GET_DEF) UVC control 3 on unit 2: 1 (exp. 2).

Extend the returned value from the camera and return it.

Cc: stable@vger.kernel.org
Fixes: a763b9fb58be ("media: uvcvideo: Do not return positive errors in uvc_query_ctrl()")
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_video.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index cd9c29532fb0..853dfb7b5f7b 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -79,11 +79,16 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
 	if (likely(ret == size))
 		return 0;
 
+	if (ret > 0 && ret < size) {
+		memset(data + ret, 0, size - ret);
+		return 0;
+	}
+
 	if (ret != -EPIPE) {
 		dev_err(&dev->udev->dev,
 			"Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n",
 			uvc_query_name(query), cs, unit, ret, size);
-		return ret < 0 ? ret : -EPIPE;
+		return ret ? ret : -EPIPE;
 	}
 
 	/* Reuse data[0] to request the error code. */

-- 
2.47.0.rc0.187.ge670bccf7e-goog

Re: [PATCH 1/3] media: uvcvideo: Support partial control reads

[Sakari Ailus]

Hi Ricardo,

On Tue, Oct 08, 2024 at 07:06:14AM +0000, Ricardo Ribalda wrote:
> Some cameras, like the ELMO MX-P3, do not return all the bytes
> requested from a control if it can fit in less bytes.
> Eg: Returning 0xab instead of 0x00ab.
> usb 3-9: Failed to query (GET_DEF) UVC control 3 on unit 2: 1 (exp. 2).
> 
> Extend the returned value from the camera and return it.
> 
> Cc: stable@vger.kernel.org
> Fixes: a763b9fb58be ("media: uvcvideo: Do not return positive errors in uvc_query_ctrl()")

Is this really the patch that introduced the problem?

> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_video.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index cd9c29532fb0..853dfb7b5f7b 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -79,11 +79,16 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
>  	if (likely(ret == size))
>  		return 0;
>  
> +	if (ret > 0 && ret < size) {
> +		memset(data + ret, 0, size - ret);

It'd be nice to have a comment in the code why this is being done
(including it's little endian).

> +		return 0;
> +	}
> +
>  	if (ret != -EPIPE) {
>  		dev_err(&dev->udev->dev,
>  			"Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n",
>  			uvc_query_name(query), cs, unit, ret, size);
> -		return ret < 0 ? ret : -EPIPE;
> +		return ret ? ret : -EPIPE;
>  	}
>  
>  	/* Reuse data[0] to request the error code. */
> 

-- 
Kind regards,

Sakari Ailus

[PATCH 2/3] media: uvcvideo: Refactor uvc_query_ctrl

[Ricardo Ribalda]

Move the query control error logic to its own function.
There is no functional change introduced by this patch.

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

diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index 853dfb7b5f7b..a57272a2c9e1 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -67,30 +67,12 @@ static const char *uvc_query_name(u8 query)
 	}
 }
 
-int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
-			u8 intfnum, u8 cs, void *data, u16 size)
+static int uvc_query_ctrl_error(struct uvc_device *dev, u8 intfnum, void *data)
 {
 	int ret;
 	u8 error;
 	u8 tmp;
 
-	ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size,
-				UVC_CTRL_CONTROL_TIMEOUT);
-	if (likely(ret == size))
-		return 0;
-
-	if (ret > 0 && ret < size) {
-		memset(data + ret, 0, size - ret);
-		return 0;
-	}
-
-	if (ret != -EPIPE) {
-		dev_err(&dev->udev->dev,
-			"Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n",
-			uvc_query_name(query), cs, unit, ret, size);
-		return ret ? ret : -EPIPE;
-	}
-
 	/* Reuse data[0] to request the error code. */
 	tmp = *(u8 *)data;
 
@@ -135,6 +117,31 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
 	return -EPIPE;
 }
 
+int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
+		   u8 intfnum, u8 cs, void *data, u16 size)
+{
+	int ret;
+
+	ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size,
+			       UVC_CTRL_CONTROL_TIMEOUT);
+	if (likely(ret == size))
+		return 0;
+
+	if (ret == -EPIPE)
+		return uvc_query_ctrl_error(dev, intfnum, data);
+
+	dev_err(&dev->udev->dev,
+		"Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n",
+		uvc_query_name(query), cs, unit, ret, size);
+
+	if (ret > 0 && ret < size) {
+		memset(data + ret, 0, size - ret);
+		return 0;
+	}
+
+	return ret ? ret : -EPIPE;
+}
+
 static const struct usb_device_id elgato_cam_link_4k = {
 	USB_DEVICE(0x0fd9, 0x0066)
 };

-- 
2.47.0.rc0.187.ge670bccf7e-goog

Re: [PATCH 2/3] media: uvcvideo: Refactor uvc_query_ctrl

[Sakari Ailus]

Hi Ricardo,

On Tue, Oct 08, 2024 at 07:06:15AM +0000, Ricardo Ribalda wrote:
> Move the query control error logic to its own function.
> There is no functional change introduced by this patch.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_video.c | 45 ++++++++++++++++++++++-----------------
>  1 file changed, 26 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index 853dfb7b5f7b..a57272a2c9e1 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -67,30 +67,12 @@ static const char *uvc_query_name(u8 query)
>  	}
>  }
>  
> -int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
> -			u8 intfnum, u8 cs, void *data, u16 size)
> +static int uvc_query_ctrl_error(struct uvc_device *dev, u8 intfnum, void *data)
>  {
>  	int ret;
>  	u8 error;
>  	u8 tmp;
>  
> -	ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size,
> -				UVC_CTRL_CONTROL_TIMEOUT);
> -	if (likely(ret == size))
> -		return 0;
> -
> -	if (ret > 0 && ret < size) {
> -		memset(data + ret, 0, size - ret);
> -		return 0;
> -	}
> -
> -	if (ret != -EPIPE) {
> -		dev_err(&dev->udev->dev,
> -			"Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n",
> -			uvc_query_name(query), cs, unit, ret, size);
> -		return ret ? ret : -EPIPE;
> -	}
> -
>  	/* Reuse data[0] to request the error code. */
>  	tmp = *(u8 *)data;
>  
> @@ -135,6 +117,31 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
>  	return -EPIPE;
>  }
>  
> +int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
> +		   u8 intfnum, u8 cs, void *data, u16 size)
> +{
> +	int ret;
> +
> +	ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size,
> +			       UVC_CTRL_CONTROL_TIMEOUT);
> +	if (likely(ret == size))
> +		return 0;
> +
> +	if (ret == -EPIPE)
> +		return uvc_query_ctrl_error(dev, intfnum, data);
> +
> +	dev_err(&dev->udev->dev,
> +		"Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n",
> +		uvc_query_name(query), cs, unit, ret, size);

This message should probably be printed after the check below.

I'd actually move the below check before the ret == -EPIPE check as it's a
successful case (and changing the condition to <= would make the ret ==
size check redundant).

> +
> +	if (ret > 0 && ret < size) {
> +		memset(data + ret, 0, size - ret);
> +		return 0;
> +	}
> +
> +	return ret ? ret : -EPIPE;
> +}
> +
>  static const struct usb_device_id elgato_cam_link_4k = {
>  	USB_DEVICE(0x0fd9, 0x0066)
>  };
> 

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 2/3] media: uvcvideo: Refactor uvc_query_ctrl

[Ricardo Ribalda]

Hi Sakari!

On Tue, 8 Oct 2024 at 20:01, Sakari Ailus <sakari.ailus@iki.fi> wrote:
>
> Hi Ricardo,
>
> On Tue, Oct 08, 2024 at 07:06:15AM +0000, Ricardo Ribalda wrote:
> > Move the query control error logic to its own function.
> > There is no functional change introduced by this patch.
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/usb/uvc/uvc_video.c | 45 ++++++++++++++++++++++-----------------
> >  1 file changed, 26 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > index 853dfb7b5f7b..a57272a2c9e1 100644
> > --- a/drivers/media/usb/uvc/uvc_video.c
> > +++ b/drivers/media/usb/uvc/uvc_video.c
> > @@ -67,30 +67,12 @@ static const char *uvc_query_name(u8 query)
> >       }
> >  }
> >
> > -int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
> > -                     u8 intfnum, u8 cs, void *data, u16 size)
> > +static int uvc_query_ctrl_error(struct uvc_device *dev, u8 intfnum, void *data)
> >  {
> >       int ret;
> >       u8 error;
> >       u8 tmp;
> >
> > -     ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size,
> > -                             UVC_CTRL_CONTROL_TIMEOUT);
> > -     if (likely(ret == size))
> > -             return 0;
> > -
> > -     if (ret > 0 && ret < size) {
> > -             memset(data + ret, 0, size - ret);
> > -             return 0;
> > -     }
> > -
> > -     if (ret != -EPIPE) {
> > -             dev_err(&dev->udev->dev,
> > -                     "Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n",
> > -                     uvc_query_name(query), cs, unit, ret, size);
> > -             return ret ? ret : -EPIPE;
> > -     }
> > -
> >       /* Reuse data[0] to request the error code. */
> >       tmp = *(u8 *)data;
> >
> > @@ -135,6 +117,31 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
> >       return -EPIPE;
> >  }
> >
> > +int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
> > +                u8 intfnum, u8 cs, void *data, u16 size)
> > +{
> > +     int ret;
> > +
> > +     ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size,
> > +                            UVC_CTRL_CONTROL_TIMEOUT);
> > +     if (likely(ret == size))
> > +             return 0;
> > +
> > +     if (ret == -EPIPE)
> > +             return uvc_query_ctrl_error(dev, intfnum, data);
> > +
> > +     dev_err(&dev->udev->dev,
> > +             "Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n",
> > +             uvc_query_name(query), cs, unit, ret, size);
>
> This message should probably be printed after the check below.

If the device is returning less bytes, the hardware is not behaving
according to spec and it is good information, specially if you are
bringing up a new device.
I could make it  a dev_warn() (or even uvc_debug) if ret <size. WDYT?


>
> I'd actually move the below check before the ret == -EPIPE check as it's a
> successful case (and changing the condition to <= would make the ret ==
> size check redundant).

something like this?

if (ret > 0)  {
   if (ret != size) {
      print_error();
      memcpy();
   }
   return 0;
}

>
> > +
> > +     if (ret > 0 && ret < size) {
> > +             memset(data + ret, 0, size - ret);
> > +             return 0;
> > +     }
> > +
> > +     return ret ? ret : -EPIPE;
> > +}
> > +
> >  static const struct usb_device_id elgato_cam_link_4k = {
> >       USB_DEVICE(0x0fd9, 0x0066)
> >  };
> >
>
> --
> Kind regards,
>
> Sakari Ailus



-- 
Ricardo Ribalda

Re: [PATCH 2/3] media: uvcvideo: Refactor uvc_query_ctrl

[Sakari Ailus]

Hi Ricardo,

On Tue, Oct 08, 2024 at 09:22:25PM +0800, Ricardo Ribalda wrote:
> Hi Sakari!
> 
> On Tue, 8 Oct 2024 at 20:01, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> >
> > Hi Ricardo,
> >
> > On Tue, Oct 08, 2024 at 07:06:15AM +0000, Ricardo Ribalda wrote:
> > > Move the query control error logic to its own function.
> > > There is no functional change introduced by this patch.
> > >
> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > ---
> > >  drivers/media/usb/uvc/uvc_video.c | 45 ++++++++++++++++++++++-----------------
> > >  1 file changed, 26 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > > index 853dfb7b5f7b..a57272a2c9e1 100644
> > > --- a/drivers/media/usb/uvc/uvc_video.c
> > > +++ b/drivers/media/usb/uvc/uvc_video.c
> > > @@ -67,30 +67,12 @@ static const char *uvc_query_name(u8 query)
> > >       }
> > >  }
> > >
> > > -int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
> > > -                     u8 intfnum, u8 cs, void *data, u16 size)
> > > +static int uvc_query_ctrl_error(struct uvc_device *dev, u8 intfnum, void *data)
> > >  {
> > >       int ret;
> > >       u8 error;
> > >       u8 tmp;
> > >
> > > -     ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size,
> > > -                             UVC_CTRL_CONTROL_TIMEOUT);
> > > -     if (likely(ret == size))
> > > -             return 0;
> > > -
> > > -     if (ret > 0 && ret < size) {
> > > -             memset(data + ret, 0, size - ret);
> > > -             return 0;
> > > -     }
> > > -
> > > -     if (ret != -EPIPE) {
> > > -             dev_err(&dev->udev->dev,
> > > -                     "Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n",
> > > -                     uvc_query_name(query), cs, unit, ret, size);
> > > -             return ret ? ret : -EPIPE;
> > > -     }
> > > -
> > >       /* Reuse data[0] to request the error code. */
> > >       tmp = *(u8 *)data;
> > >
> > > @@ -135,6 +117,31 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
> > >       return -EPIPE;
> > >  }
> > >
> > > +int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
> > > +                u8 intfnum, u8 cs, void *data, u16 size)
> > > +{
> > > +     int ret;
> > > +
> > > +     ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size,
> > > +                            UVC_CTRL_CONTROL_TIMEOUT);
> > > +     if (likely(ret == size))
> > > +             return 0;
> > > +
> > > +     if (ret == -EPIPE)
> > > +             return uvc_query_ctrl_error(dev, intfnum, data);
> > > +
> > > +     dev_err(&dev->udev->dev,
> > > +             "Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n",
> > > +             uvc_query_name(query), cs, unit, ret, size);
> >
> > This message should probably be printed after the check below.
> 
> If the device is returning less bytes, the hardware is not behaving
> according to spec and it is good information, specially if you are
> bringing up a new device.
> I could make it  a dev_warn() (or even uvc_debug) if ret <size. WDYT?

What I also came to think whether this is worth an explicit quirk flag.
There could well be devices that have other bugs that would still fall
under the same check.

Either way, there should not be a message every single time this workaround
is applied. Isn't uvc_query_ctrl() also used in some IOCTLs outside probe?

> 
> 
> >
> > I'd actually move the below check before the ret == -EPIPE check as it's a
> > successful case (and changing the condition to <= would make the ret ==
> > size check redundant).
> 
> something like this?
> 
> if (ret > 0)  {
>    if (ret != size) {
>       print_error();
>       memcpy();
>    }
>    return 0;
> }

Well, if you think it's reasonable to keep the error message (should be a
warning in that case IMO), then the original code makes sense.

I wonder what Laurent thinks.

> 
> >
> > > +
> > > +     if (ret > 0 && ret < size) {
> > > +             memset(data + ret, 0, size - ret);
> > > +             return 0;
> > > +     }
> > > +
> > > +     return ret ? ret : -EPIPE;
> > > +}
> > > +
> > >  static const struct usb_device_id elgato_cam_link_4k = {
> > >       USB_DEVICE(0x0fd9, 0x0066)
> > >  };
> > >
> >

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 2/3] media: uvcvideo: Refactor uvc_query_ctrl

[Ricardo Ribalda]

On Tue, 8 Oct 2024 at 21:31, Sakari Ailus <sakari.ailus@iki.fi> wrote:
>
> Hi Ricardo,
>
> On Tue, Oct 08, 2024 at 09:22:25PM +0800, Ricardo Ribalda wrote:
> > Hi Sakari!
> >
> > On Tue, 8 Oct 2024 at 20:01, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> > >
> > > Hi Ricardo,
> > >
> > > On Tue, Oct 08, 2024 at 07:06:15AM +0000, Ricardo Ribalda wrote:
> > > > Move the query control error logic to its own function.
> > > > There is no functional change introduced by this patch.
> > > >
> > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > ---
> > > >  drivers/media/usb/uvc/uvc_video.c | 45 ++++++++++++++++++++++-----------------
> > > >  1 file changed, 26 insertions(+), 19 deletions(-)
> > > >
> > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > > > index 853dfb7b5f7b..a57272a2c9e1 100644
> > > > --- a/drivers/media/usb/uvc/uvc_video.c
> > > > +++ b/drivers/media/usb/uvc/uvc_video.c
> > > > @@ -67,30 +67,12 @@ static const char *uvc_query_name(u8 query)
> > > >       }
> > > >  }
> > > >
> > > > -int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
> > > > -                     u8 intfnum, u8 cs, void *data, u16 size)
> > > > +static int uvc_query_ctrl_error(struct uvc_device *dev, u8 intfnum, void *data)
> > > >  {
> > > >       int ret;
> > > >       u8 error;
> > > >       u8 tmp;
> > > >
> > > > -     ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size,
> > > > -                             UVC_CTRL_CONTROL_TIMEOUT);
> > > > -     if (likely(ret == size))
> > > > -             return 0;
> > > > -
> > > > -     if (ret > 0 && ret < size) {
> > > > -             memset(data + ret, 0, size - ret);
> > > > -             return 0;
> > > > -     }
> > > > -
> > > > -     if (ret != -EPIPE) {
> > > > -             dev_err(&dev->udev->dev,
> > > > -                     "Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n",
> > > > -                     uvc_query_name(query), cs, unit, ret, size);
> > > > -             return ret ? ret : -EPIPE;
> > > > -     }
> > > > -
> > > >       /* Reuse data[0] to request the error code. */
> > > >       tmp = *(u8 *)data;
> > > >
> > > > @@ -135,6 +117,31 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
> > > >       return -EPIPE;
> > > >  }
> > > >
> > > > +int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
> > > > +                u8 intfnum, u8 cs, void *data, u16 size)
> > > > +{
> > > > +     int ret;
> > > > +
> > > > +     ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size,
> > > > +                            UVC_CTRL_CONTROL_TIMEOUT);
> > > > +     if (likely(ret == size))
> > > > +             return 0;
> > > > +
> > > > +     if (ret == -EPIPE)
> > > > +             return uvc_query_ctrl_error(dev, intfnum, data);
> > > > +
> > > > +     dev_err(&dev->udev->dev,
> > > > +             "Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n",
> > > > +             uvc_query_name(query), cs, unit, ret, size);
> > >
> > > This message should probably be printed after the check below.
> >
> > If the device is returning less bytes, the hardware is not behaving
> > according to spec and it is good information, specially if you are
> > bringing up a new device.
> > I could make it  a dev_warn() (or even uvc_debug) if ret <size. WDYT?
>
> What I also came to think whether this is worth an explicit quirk flag.
> There could well be devices that have other bugs that would still fall
> under the same check.

Before https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/media/usb/uvc/uvc_video.c?id=a763b9fb58be869e252a7d33acb0a6390b01c801
we were accepting short reads and showing the error.
We are returning to the old behaviour, plus the memset.

A quirk will be difficult to maintain. If we can find a heuristics
that works I would vote for that.


>
> Either way, there should not be a message every single time this workaround
> is applied. Isn't uvc_query_ctrl() also used in some IOCTLs outside probe?

I have only seen this behaviour for GET_DEF, and that value is cached.
Meaning that the warning will only be shown once per control.

>
> >
> >
> > >
> > > I'd actually move the below check before the ret == -EPIPE check as it's a
> > > successful case (and changing the condition to <= would make the ret ==
> > > size check redundant).
> >
> > something like this?
> >
> > if (ret > 0)  {
> >    if (ret != size) {
> >       print_error();
> >       memcpy();
> >    }
> >    return 0;
> > }
>
> Well, if you think it's reasonable to keep the error message (should be a
> warning in that case IMO), then the original code makes sense.
>
> I wonder what Laurent thinks.
>
> >
> > >
> > > > +
> > > > +     if (ret > 0 && ret < size) {
> > > > +             memset(data + ret, 0, size - ret);
> > > > +             return 0;
> > > > +     }
> > > > +
> > > > +     return ret ? ret : -EPIPE;
> > > > +}
> > > > +
> > > >  static const struct usb_device_id elgato_cam_link_4k = {
> > > >       USB_DEVICE(0x0fd9, 0x0066)
> > > >  };
> > > >
> > >
>
> --
> Kind regards,
>
> Sakari Ailus



--
Ricardo Ribalda

Re: [PATCH 2/3] media: uvcvideo: Refactor uvc_query_ctrl

[Sakari Ailus]

Hi Ricardo,

On Tue, Oct 08, 2024 at 09:38:58PM +0800, Ricardo Ribalda wrote:
> On Tue, 8 Oct 2024 at 21:31, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> >
> > Hi Ricardo,
> >
> > On Tue, Oct 08, 2024 at 09:22:25PM +0800, Ricardo Ribalda wrote:
> > > Hi Sakari!
> > >
> > > On Tue, 8 Oct 2024 at 20:01, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> > > >
> > > > Hi Ricardo,
> > > >
> > > > On Tue, Oct 08, 2024 at 07:06:15AM +0000, Ricardo Ribalda wrote:
> > > > > Move the query control error logic to its own function.
> > > > > There is no functional change introduced by this patch.
> > > > >
> > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > > ---
> > > > >  drivers/media/usb/uvc/uvc_video.c | 45 ++++++++++++++++++++++-----------------
> > > > >  1 file changed, 26 insertions(+), 19 deletions(-)
> > > > >
> > > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > > > > index 853dfb7b5f7b..a57272a2c9e1 100644
> > > > > --- a/drivers/media/usb/uvc/uvc_video.c
> > > > > +++ b/drivers/media/usb/uvc/uvc_video.c
> > > > > @@ -67,30 +67,12 @@ static const char *uvc_query_name(u8 query)
> > > > >       }
> > > > >  }
> > > > >
> > > > > -int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
> > > > > -                     u8 intfnum, u8 cs, void *data, u16 size)
> > > > > +static int uvc_query_ctrl_error(struct uvc_device *dev, u8 intfnum, void *data)
> > > > >  {
> > > > >       int ret;
> > > > >       u8 error;
> > > > >       u8 tmp;
> > > > >
> > > > > -     ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size,
> > > > > -                             UVC_CTRL_CONTROL_TIMEOUT);
> > > > > -     if (likely(ret == size))
> > > > > -             return 0;
> > > > > -
> > > > > -     if (ret > 0 && ret < size) {
> > > > > -             memset(data + ret, 0, size - ret);
> > > > > -             return 0;
> > > > > -     }
> > > > > -
> > > > > -     if (ret != -EPIPE) {
> > > > > -             dev_err(&dev->udev->dev,
> > > > > -                     "Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n",
> > > > > -                     uvc_query_name(query), cs, unit, ret, size);
> > > > > -             return ret ? ret : -EPIPE;
> > > > > -     }
> > > > > -
> > > > >       /* Reuse data[0] to request the error code. */
> > > > >       tmp = *(u8 *)data;
> > > > >
> > > > > @@ -135,6 +117,31 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
> > > > >       return -EPIPE;
> > > > >  }
> > > > >
> > > > > +int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
> > > > > +                u8 intfnum, u8 cs, void *data, u16 size)
> > > > > +{
> > > > > +     int ret;
> > > > > +
> > > > > +     ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size,
> > > > > +                            UVC_CTRL_CONTROL_TIMEOUT);
> > > > > +     if (likely(ret == size))
> > > > > +             return 0;
> > > > > +
> > > > > +     if (ret == -EPIPE)
> > > > > +             return uvc_query_ctrl_error(dev, intfnum, data);
> > > > > +
> > > > > +     dev_err(&dev->udev->dev,
> > > > > +             "Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n",
> > > > > +             uvc_query_name(query), cs, unit, ret, size);
> > > >
> > > > This message should probably be printed after the check below.
> > >
> > > If the device is returning less bytes, the hardware is not behaving
> > > according to spec and it is good information, specially if you are
> > > bringing up a new device.
> > > I could make it  a dev_warn() (or even uvc_debug) if ret <size. WDYT?
> >
> > What I also came to think whether this is worth an explicit quirk flag.
> > There could well be devices that have other bugs that would still fall
> > under the same check.
> 
> Before https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/media/usb/uvc/uvc_video.c?id=a763b9fb58be869e252a7d33acb0a6390b01c801
> we were accepting short reads and showing the error.
> We are returning to the old behaviour, plus the memset.
> 
> A quirk will be difficult to maintain. If we can find a heuristics
> that works I would vote for that.

Seems reasonable.

> 
> 
> >
> > Either way, there should not be a message every single time this workaround
> > is applied. Isn't uvc_query_ctrl() also used in some IOCTLs outside probe?
> 
> I have only seen this behaviour for GET_DEF, and that value is cached.
> Meaning that the warning will only be shown once per control.

Could you add a comment with this information, also mentioning where this
has been noticed?

Perhaps we should wait for Laurent's comments still.

-- 
Kind regards,

Sakari Ailus

[PATCH 3/3] media: uvcvideo: Add more logging to uvc_query_ctrl_error()

[Ricardo Ribalda]

If we fail to query the ctrl error there is no information on dmesg or
in uvc_dbg, which makes it difficult to debug

Reorder the log position to fix this.

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

diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index a57272a2c9e1..bdc3f545a15e 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -83,11 +83,11 @@ static int uvc_query_ctrl_error(struct uvc_device *dev, u8 intfnum, void *data)
 	error = *(u8 *)data;
 	*(u8 *)data = tmp;
 
+	uvc_dbg(dev, CONTROL, "Control error %u, ret %d\n", error, ret);
+
 	if (ret != 1)
 		return ret < 0 ? ret : -EPIPE;
 
-	uvc_dbg(dev, CONTROL, "Control error %u\n", error);
-
 	switch (error) {
 	case 0:
 		/* Cannot happen - we received a STALL */

-- 
2.47.0.rc0.187.ge670bccf7e-goog