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

[PATCH v3 0/2] 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() logging.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
Changes in v3:
- Improve documentation.
- Do not change return sequence.
- Use dev_ratelimit and dev_warn_once
- Link to v2: https://lore.kernel.org/r/20241008-uvc-readless-v2-0-04d9d51aee56@chromium.org

Changes in v2:
- Rewrite error handling (Thanks Sakari)
- Discard 2/3. It is not needed after rewriting the error handling.
- Link to v1: https://lore.kernel.org/r/20241008-uvc-readless-v1-0-042ac4581f44@chromium.org

---
Ricardo Ribalda (2):
      media: uvcvideo: Support partial control reads
      media: uvcvideo: Add more logging to uvc_query_ctrl()

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

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

[PATCH v3 1/2] 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 | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index cd9c29532fb0..e165850397a0 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -76,8 +76,22 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
 
 	ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size,
 				UVC_CTRL_CONTROL_TIMEOUT);
-	if (likely(ret == size))
+	if (ret > 0) {
+		if (size == ret)
+			return 0;
+
+		/*
+		 * In UVC the data is represented in little-endian by default.
+		 * Some devices return shorter control packages that expected
+		 * if the return value can fit in less bytes.
+		 * Zero all the bytes that the device have not written.
+		 */
+		memset(data + ret, 0, size - ret);
+		dev_warn_once(&dev->udev->dev,
+			      "UVC non compliance: %s control %u on unit %u returned %d bytes when we expected %u.\n",
+			      uvc_query_name(query), cs, unit, ret, size);
 		return 0;
+	}
 
 	if (ret != -EPIPE) {
 		dev_err(&dev->udev->dev,

-- 
2.47.0.338.g60cca15819-goog

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

[Hans de Goede]

Hi,

On 18-Nov-24 6:16 PM, 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()")
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans



> ---
>  drivers/media/usb/uvc/uvc_video.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index cd9c29532fb0..e165850397a0 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -76,8 +76,22 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
>  
>  	ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size,
>  				UVC_CTRL_CONTROL_TIMEOUT);
> -	if (likely(ret == size))
> +	if (ret > 0) {
> +		if (size == ret)
> +			return 0;
> +
> +		/*
> +		 * In UVC the data is represented in little-endian by default.
> +		 * Some devices return shorter control packages that expected
> +		 * if the return value can fit in less bytes.
> +		 * Zero all the bytes that the device have not written.
> +		 */
> +		memset(data + ret, 0, size - ret);
> +		dev_warn_once(&dev->udev->dev,
> +			      "UVC non compliance: %s control %u on unit %u returned %d bytes when we expected %u.\n",
> +			      uvc_query_name(query), cs, unit, ret, size);
>  		return 0;
> +	}
>  
>  	if (ret != -EPIPE) {
>  		dev_err(&dev->udev->dev,
> 

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

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Mon, Nov 18, 2024 at 05:16:51PM +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()")
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_video.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index cd9c29532fb0..e165850397a0 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -76,8 +76,22 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
>  
>  	ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size,
>  				UVC_CTRL_CONTROL_TIMEOUT);
> -	if (likely(ret == size))
> +	if (ret > 0) {
> +		if (size == ret)
> +			return 0;

Why is this within the ret > 0 block ? I would write

	if (likely(ret == size))
		return 0;

	if (ret > 0) {

> +
> +		/*
> +		 * In UVC the data is represented in little-endian by default.

By default, or always ?

> +		 * Some devices return shorter control packages that expected

What's a "control package" ?

I think you meants "than expected", not "that expected".

> +		 * if the return value can fit in less bytes.
> +		 * Zero all the bytes that the device have not written.
> +		 */

Do we want to apply this workaround to GET_INFO and GET_LEN, or can we
restrict it to GET_CUR, GET_MIN, GET_MAX and GET_RES ?

> +		memset(data + ret, 0, size - ret);
> +		dev_warn_once(&dev->udev->dev,
> +			      "UVC non compliance: %s control %u on unit %u returned %d bytes when we expected %u.\n",
> +			      uvc_query_name(query), cs, unit, ret, size);
>  		return 0;
> +	}
>  
>  	if (ret != -EPIPE) {
>  		dev_err(&dev->udev->dev,
> 

-- 
Regards,

Laurent Pinchart

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

[Ricardo Ribalda]

Hi Laurent

On Wed, 20 Nov 2024 at 15:05, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
> On Mon, Nov 18, 2024 at 05:16:51PM +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()")
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/usb/uvc/uvc_video.c | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > index cd9c29532fb0..e165850397a0 100644
> > --- a/drivers/media/usb/uvc/uvc_video.c
> > +++ b/drivers/media/usb/uvc/uvc_video.c
> > @@ -76,8 +76,22 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
> >
> >       ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size,
> >                               UVC_CTRL_CONTROL_TIMEOUT);
> > -     if (likely(ret == size))
> > +     if (ret > 0) {
> > +             if (size == ret)
> > +                     return 0;
>
> Why is this within the ret > 0 block ? I would write
>
>         if (likely(ret == size))
>                 return 0;
>
>         if (ret > 0) {
>
> > +
> > +             /*
> > +              * In UVC the data is represented in little-endian by default.
>
> By default, or always ?
>
> > +              * Some devices return shorter control packages that expected
>
> What's a "control package" ?

usb control transfers.
>
> I think you meants "than expected", not "that expected".
>
> > +              * if the return value can fit in less bytes.
> > +              * Zero all the bytes that the device have not written.
> > +              */
>
> Do we want to apply this workaround to GET_INFO and GET_LEN, or can we
> restrict it to GET_CUR, GET_MIN, GET_MAX and GET_RES ?

I believe that the original behaviour before
a763b9fb58be ("media: uvcvideo: Do not return positive errors in
uvc_query_ctrl()")
was used for all types. I think the safest thing to do is to go back
to the old behaviour.

Let me know what you prefer.


>
> > +             memset(data + ret, 0, size - ret);
> > +             dev_warn_once(&dev->udev->dev,
> > +                           "UVC non compliance: %s control %u on unit %u returned %d bytes when we expected %u.\n",
> > +                           uvc_query_name(query), cs, unit, ret, size);
> >               return 0;
> > +     }
> >
> >       if (ret != -EPIPE) {
> >               dev_err(&dev->udev->dev,
> >
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

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

[Laurent Pinchart]

On Wed, Nov 20, 2024 at 03:43:22PM +0100, Ricardo Ribalda wrote:
> On Wed, 20 Nov 2024 at 15:05, Laurent Pinchart wrote:
> > On Mon, Nov 18, 2024 at 05:16:51PM +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()")
> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > ---
> > >  drivers/media/usb/uvc/uvc_video.c | 16 +++++++++++++++-
> > >  1 file changed, 15 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > > index cd9c29532fb0..e165850397a0 100644
> > > --- a/drivers/media/usb/uvc/uvc_video.c
> > > +++ b/drivers/media/usb/uvc/uvc_video.c
> > > @@ -76,8 +76,22 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
> > >
> > >       ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size,
> > >                               UVC_CTRL_CONTROL_TIMEOUT);
> > > -     if (likely(ret == size))
> > > +     if (ret > 0) {
> > > +             if (size == ret)
> > > +                     return 0;
> >
> > Why is this within the ret > 0 block ? I would write
> >
> >         if (likely(ret == size))
> >                 return 0;
> >
> >         if (ret > 0) {
> >
> > > +
> > > +             /*
> > > +              * In UVC the data is represented in little-endian by default.
> >
> > By default, or always ?
> >
> > > +              * Some devices return shorter control packages that expected
> >
> > What's a "control package" ?
> 
> usb control transfers.

Ah, did you mean "packet" instead of "package" ?

> > I think you meants "than expected", not "that expected".
> >
> > > +              * if the return value can fit in less bytes.
> > > +              * Zero all the bytes that the device have not written.
> > > +              */
> >
> > Do we want to apply this workaround to GET_INFO and GET_LEN, or can we
> > restrict it to GET_CUR, GET_MIN, GET_MAX and GET_RES ?
> 
> I believe that the original behaviour before
> a763b9fb58be ("media: uvcvideo: Do not return positive errors in
> uvc_query_ctrl()")
> was used for all types. I think the safest thing to do is to go back
> to the old behaviour.

I don't see how reverting that commit would help, or how that's related
to the question at hand.

I understand the device you're dealing with shortens transfers for
integer values when they would contain leading 0x00 bytes. The
workaround should be OK when retrieving the control value or its limits
and resolution. I wonder if it would be dangerous for GET_INFO, which
retrieves a bitmask. Does the device you've tested this with skip the
MSB for GET_INFO as well ?

Conceptually GET_LEN could be similarly excluded from the workaround,
but it will never take this code path as it's a 1 byte value.

> Let me know what you prefer.
> 
> > > +             memset(data + ret, 0, size - ret);
> > > +             dev_warn_once(&dev->udev->dev,
> > > +                           "UVC non compliance: %s control %u on unit %u returned %d bytes when we expected %u.\n",
> > > +                           uvc_query_name(query), cs, unit, ret, size);
> > >               return 0;
> > > +     }
> > >
> > >       if (ret != -EPIPE) {
> > >               dev_err(&dev->udev->dev,

-- 
Regards,

Laurent Pinchart

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

[Ricardo Ribalda]

On Wed, 20 Nov 2024 at 15:53, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Wed, Nov 20, 2024 at 03:43:22PM +0100, Ricardo Ribalda wrote:
> > On Wed, 20 Nov 2024 at 15:05, Laurent Pinchart wrote:
> > > On Mon, Nov 18, 2024 at 05:16:51PM +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()")
> > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > ---
> > > >  drivers/media/usb/uvc/uvc_video.c | 16 +++++++++++++++-
> > > >  1 file changed, 15 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > > > index cd9c29532fb0..e165850397a0 100644
> > > > --- a/drivers/media/usb/uvc/uvc_video.c
> > > > +++ b/drivers/media/usb/uvc/uvc_video.c
> > > > @@ -76,8 +76,22 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
> > > >
> > > >       ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size,
> > > >                               UVC_CTRL_CONTROL_TIMEOUT);
> > > > -     if (likely(ret == size))
> > > > +     if (ret > 0) {
> > > > +             if (size == ret)
> > > > +                     return 0;
> > >
> > > Why is this within the ret > 0 block ? I would write
> > >
> > >         if (likely(ret == size))
> > >                 return 0;
> > >
> > >         if (ret > 0) {
> > >
> > > > +
> > > > +             /*
> > > > +              * In UVC the data is represented in little-endian by default.
> > >
> > > By default, or always ?
> > >
> > > > +              * Some devices return shorter control packages that expected
> > >
> > > What's a "control package" ?
> >
> > usb control transfers.
>
> Ah, did you mean "packet" instead of "package" ?
>
> > > I think you meants "than expected", not "that expected".
> > >
> > > > +              * if the return value can fit in less bytes.
> > > > +              * Zero all the bytes that the device have not written.
> > > > +              */
> > >
> > > Do we want to apply this workaround to GET_INFO and GET_LEN, or can we
> > > restrict it to GET_CUR, GET_MIN, GET_MAX and GET_RES ?
> >
> > I believe that the original behaviour before
> > a763b9fb58be ("media: uvcvideo: Do not return positive errors in
> > uvc_query_ctrl()")
> > was used for all types. I think the safest thing to do is to go back
> > to the old behaviour.
>
> I don't see how reverting that commit would help, or how that's related
> to the question at hand.
>
> I understand the device you're dealing with shortens transfers for
> integer values when they would contain leading 0x00 bytes. The
> workaround should be OK when retrieving the control value or its limits
> and resolution. I wonder if it would be dangerous for GET_INFO, which
> retrieves a bitmask. Does the device you've tested this with skip the
> MSB for GET_INFO as well ?

I have not seen it mangling GET_INFO. Let's exclude GET_INFO it from
the quirk then.



>
> Conceptually GET_LEN could be similarly excluded from the workaround,
> but it will never take this code path as it's a 1 byte value.
>
> > Let me know what you prefer.
> >
> > > > +             memset(data + ret, 0, size - ret);
> > > > +             dev_warn_once(&dev->udev->dev,
> > > > +                           "UVC non compliance: %s control %u on unit %u returned %d bytes when we expected %u.\n",
> > > > +                           uvc_query_name(query), cs, unit, ret, size);
> > > >               return 0;
> > > > +     }
> > > >
> > > >       if (ret != -EPIPE) {
> > > >               dev_err(&dev->udev->dev,
>
> --
> Regards,
>
> Laurent Pinchart



--
Ricardo Ribalda

[PATCH v3 2/2] media: uvcvideo: Add more logging to uvc_query_ctrl()

[Ricardo Ribalda]

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 | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index e165850397a0..0195394e10b5 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -110,8 +110,12 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
 	error = *(u8 *)data;
 	*(u8 *)data = tmp;
 
-	if (ret != 1)
+	if (ret != 1) {
+		dev_err_ratelimited(&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 < 0 ? ret : -EPIPE;
+	}
 
 	uvc_dbg(dev, CONTROL, "Control error %u\n", error);
 

-- 
2.47.0.338.g60cca15819-goog

Re: [PATCH v3 2/2] media: uvcvideo: Add more logging to uvc_query_ctrl()

[Hans de Goede]

Hi,

On 18-Nov-24 6:16 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>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans



> ---
>  drivers/media/usb/uvc/uvc_video.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index e165850397a0..0195394e10b5 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -110,8 +110,12 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
>  	error = *(u8 *)data;
>  	*(u8 *)data = tmp;
>  
> -	if (ret != 1)
> +	if (ret != 1) {
> +		dev_err_ratelimited(&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 < 0 ? ret : -EPIPE;
> +	}
>  
>  	uvc_dbg(dev, CONTROL, "Control error %u\n", error);
>  
>