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

[PATCH v4 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 v4:
- Improve comment.
- Keep old likely(ret == size)
- Link to v3: https://lore.kernel.org/r/20241118-uvc-readless-v3-0-d97c1a3084d0@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, 21 insertions(+), 1 deletion(-)
---
base-commit: 9852d85ec9d492ebef56dc5f229416c925758edc
change-id: 20241008-uvc-readless-23f9b8cad0b3

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

[PATCH v4 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()")
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_video.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index cd9c29532fb0..482c4ceceaac 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -79,6 +79,22 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
 	if (likely(ret == size))
 		return 0;
 
+	/*
+	 * In UVC the data is usually represented in little-endian.
+	 * Some devices return shorter USB control packets that expected if the
+	 * returned value can fit in less bytes. Zero all the bytes that the
+	 * device have not written.
+	 * We exclude UVC_GET_INFO from the quirk. UVC_GET_LEN does not need to
+	 * be excluded because its size is always 1.
+	 */
+	if (ret > 0 && query != UVC_GET_INFO) {
+		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,
 			"Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n",

-- 
2.47.0.338.g60cca15819-goog

[PATCH v4 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.

Reviewed-by: Hans de Goede <hdegoede@redhat.com>
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 482c4ceceaac..00d9e3420fb6 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -112,8 +112,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 v4 0/2] media: uvcvideo: Support partial control reads and minor changes

[Hans de Goede]

Hi Ricardo,

On 20-Nov-24 4:26 PM, Ricardo Ribalda wrote:
> 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>

Thank you for your patches, I have pushed both patches to:

https://gitlab.freedesktop.org/linux-media/users/uvc/-/commits/next/

now.

Regards,

Hans



> ---
> Changes in v4:
> - Improve comment.
> - Keep old likely(ret == size)
> - Link to v3: https://lore.kernel.org/r/20241118-uvc-readless-v3-0-d97c1a3084d0@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, 21 insertions(+), 1 deletion(-)
> ---
> base-commit: 9852d85ec9d492ebef56dc5f229416c925758edc
> change-id: 20241008-uvc-readless-23f9b8cad0b3
> 
> Best regards,

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

[Laurent Pinchart]

On Wed, Nov 20, 2024 at 03:26:19PM +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()")
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_video.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index cd9c29532fb0..482c4ceceaac 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -79,6 +79,22 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
>  	if (likely(ret == size))
>  		return 0;
>  
> +	/*
> +	 * In UVC the data is usually represented in little-endian.

I had a comment about this in the previous version, did you ignore it on
purpose because you disagreed, or was it an oversight ?

> +	 * Some devices return shorter USB control packets that expected if the
> +	 * returned value can fit in less bytes. Zero all the bytes that the
> +	 * device have not written.

s/have/has/

And if you meant to start a new paragraph here, a blank line is missing.
Otherwise, no need to break the line before 80 columns.

> +	 * We exclude UVC_GET_INFO from the quirk. UVC_GET_LEN does not need to
> +	 * be excluded because its size is always 1.
> +	 */
> +	if (ret > 0 && query != UVC_GET_INFO) {
> +		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,
>  			"Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n",

-- 
Regards,

Laurent Pinchart

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

[Ricardo Ribalda]

On Tue, 26 Nov 2024 at 19:06, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Wed, Nov 20, 2024 at 03:26:19PM +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()")
> > Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/usb/uvc/uvc_video.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > index cd9c29532fb0..482c4ceceaac 100644
> > --- a/drivers/media/usb/uvc/uvc_video.c
> > +++ b/drivers/media/usb/uvc/uvc_video.c
> > @@ -79,6 +79,22 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
> >       if (likely(ret == size))
> >               return 0;
> >
> > +     /*
> > +      * In UVC the data is usually represented in little-endian.
>
> I had a comment about this in the previous version, did you ignore it on
> purpose because you disagreed, or was it an oversight ?

I rephrased the comment. I added "usually" to make it clear that it
might not be the case for all the data types. Like composed or xu.
I also r/package/packet/

Did I miss another comment?

>
> > +      * Some devices return shorter USB control packets that expected if the
> > +      * returned value can fit in less bytes. Zero all the bytes that the
> > +      * device have not written.
>
> s/have/has/
>
> And if you meant to start a new paragraph here, a blank line is missing.
> Otherwise, no need to break the line before 80 columns.

The patch is already in the uvc tree. How do you want to handle this?

>
> > +      * We exclude UVC_GET_INFO from the quirk. UVC_GET_LEN does not need to
> > +      * be excluded because its size is always 1.
> > +      */
> > +     if (ret > 0 && query != UVC_GET_INFO) {
> > +             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,
> >                       "Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n",
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

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

[Laurent Pinchart]

On Tue, Nov 26, 2024 at 07:12:53PM +0100, Ricardo Ribalda wrote:
> On Tue, 26 Nov 2024 at 19:06, Laurent Pinchart wrote:
> > On Wed, Nov 20, 2024 at 03:26:19PM +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()")
> > > Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > ---
> > >  drivers/media/usb/uvc/uvc_video.c | 16 ++++++++++++++++
> > >  1 file changed, 16 insertions(+)
> > >
> > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > > index cd9c29532fb0..482c4ceceaac 100644
> > > --- a/drivers/media/usb/uvc/uvc_video.c
> > > +++ b/drivers/media/usb/uvc/uvc_video.c
> > > @@ -79,6 +79,22 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
> > >       if (likely(ret == size))
> > >               return 0;
> > >
> > > +     /*
> > > +      * In UVC the data is usually represented in little-endian.
> >
> > I had a comment about this in the previous version, did you ignore it on
> > purpose because you disagreed, or was it an oversight ?
> 
> I rephrased the comment. I added "usually" to make it clear that it
> might not be the case for all the data types. Like composed or xu.

Ah, that's what you meant by "usually". I read it as "usually in
little-endian, but could be big-endian too", which confused me.

Data types that are not integers will not work nicely with the
workaround below. How do you envision that being handled ? Do you
consider that the device will return too few bytes only for integer data
types, or that affected devices don't have controls that use compound
data types ? I don't see what else we could do so I'd be fine with such
a heuristic for this workaround, but it needs to be clearly explained.

> I also r/package/packet/
> 
> Did I miss another comment?
> 
> > > +      * Some devices return shorter USB control packets that expected if the
> > > +      * returned value can fit in less bytes. Zero all the bytes that the
> > > +      * device have not written.
> >
> > s/have/has/
> >
> > And if you meant to start a new paragraph here, a blank line is missing.
> > Otherwise, no need to break the line before 80 columns.
> 
> The patch is already in the uvc tree. How do you want to handle this?

The branch shared between Hans and me can be rebased, it's a staging
area.

> > > +      * We exclude UVC_GET_INFO from the quirk. UVC_GET_LEN does not need to
> > > +      * be excluded because its size is always 1.
> > > +      */
> > > +     if (ret > 0 && query != UVC_GET_INFO) {
> > > +             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,
> > >                       "Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n",

-- 
Regards,

Laurent Pinchart

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

[Ricardo Ribalda]

On Wed, 27 Nov 2024 at 09:34, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Tue, Nov 26, 2024 at 07:12:53PM +0100, Ricardo Ribalda wrote:
> > On Tue, 26 Nov 2024 at 19:06, Laurent Pinchart wrote:
> > > On Wed, Nov 20, 2024 at 03:26:19PM +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()")
> > > > Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > ---
> > > >  drivers/media/usb/uvc/uvc_video.c | 16 ++++++++++++++++
> > > >  1 file changed, 16 insertions(+)
> > > >
> > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > > > index cd9c29532fb0..482c4ceceaac 100644
> > > > --- a/drivers/media/usb/uvc/uvc_video.c
> > > > +++ b/drivers/media/usb/uvc/uvc_video.c
> > > > @@ -79,6 +79,22 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
> > > >       if (likely(ret == size))
> > > >               return 0;
> > > >
> > > > +     /*
> > > > +      * In UVC the data is usually represented in little-endian.
> > >
> > > I had a comment about this in the previous version, did you ignore it on
> > > purpose because you disagreed, or was it an oversight ?
> >
> > I rephrased the comment. I added "usually" to make it clear that it
> > might not be the case for all the data types. Like composed or xu.
>
> Ah, that's what you meant by "usually". I read it as "usually in
> little-endian, but could be big-endian too", which confused me.
>
> Data types that are not integers will not work nicely with the
> workaround below. How do you envision that being handled ? Do you
> consider that the device will return too few bytes only for integer data
> types, or that affected devices don't have controls that use compound
> data types ? I don't see what else we could do so I'd be fine with such
> a heuristic for this workaround, but it needs to be clearly explained.

Non integer datatypes might work if the last part of the data is
expected to be zero.
I do not think that we can find a heuristic that can work for all the cases.

For years we have ignored partial reads and it has never been an
issue. I vote for not adding any heuristics, the logging should help
identify future issues (if there is any).

>
> > I also r/package/packet/
> >
> > Did I miss another comment?
> >
> > > > +      * Some devices return shorter USB control packets that expected if the
> > > > +      * returned value can fit in less bytes. Zero all the bytes that the
> > > > +      * device have not written.
> > >
> > > s/have/has/
> > >
> > > And if you meant to start a new paragraph here, a blank line is missing.
> > > Otherwise, no need to break the line before 80 columns.
> >
> > The patch is already in the uvc tree. How do you want to handle this?
>
> The branch shared between Hans and me can be rebased, it's a staging
> area.

I will send a new version, fixing the typo. and the missing new line.
I will also remove the sentence
`* In UVC the data is usually represented in little-endian.`
It is confusing.


>
> > > > +      * We exclude UVC_GET_INFO from the quirk. UVC_GET_LEN does not need to
> > > > +      * be excluded because its size is always 1.
> > > > +      */
> > > > +     if (ret > 0 && query != UVC_GET_INFO) {
> > > > +             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,
> > > >                       "Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n",
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

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

[Laurent Pinchart]

On Wed, Nov 27, 2024 at 09:58:21AM +0100, Ricardo Ribalda wrote:
> On Wed, 27 Nov 2024 at 09:34, Laurent Pinchart wrote:
> > On Tue, Nov 26, 2024 at 07:12:53PM +0100, Ricardo Ribalda wrote:
> > > On Tue, 26 Nov 2024 at 19:06, Laurent Pinchart wrote:
> > > > On Wed, Nov 20, 2024 at 03:26:19PM +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()")
> > > > > Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > > ---
> > > > >  drivers/media/usb/uvc/uvc_video.c | 16 ++++++++++++++++
> > > > >  1 file changed, 16 insertions(+)
> > > > >
> > > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > > > > index cd9c29532fb0..482c4ceceaac 100644
> > > > > --- a/drivers/media/usb/uvc/uvc_video.c
> > > > > +++ b/drivers/media/usb/uvc/uvc_video.c
> > > > > @@ -79,6 +79,22 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
> > > > >       if (likely(ret == size))
> > > > >               return 0;
> > > > >
> > > > > +     /*
> > > > > +      * In UVC the data is usually represented in little-endian.
> > > >
> > > > I had a comment about this in the previous version, did you ignore it on
> > > > purpose because you disagreed, or was it an oversight ?
> > >
> > > I rephrased the comment. I added "usually" to make it clear that it
> > > might not be the case for all the data types. Like composed or xu.
> >
> > Ah, that's what you meant by "usually". I read it as "usually in
> > little-endian, but could be big-endian too", which confused me.
> >
> > Data types that are not integers will not work nicely with the
> > workaround below. How do you envision that being handled ? Do you
> > consider that the device will return too few bytes only for integer data
> > types, or that affected devices don't have controls that use compound
> > data types ? I don't see what else we could do so I'd be fine with such
> > a heuristic for this workaround, but it needs to be clearly explained.
> 
> Non integer datatypes might work if the last part of the data is
> expected to be zero.
> I do not think that we can find a heuristic that can work for all the cases.
> 
> For years we have ignored partial reads and it has never been an
> issue. I vote for not adding any heuristics, the logging should help
> identify future issues (if there is any).

What you're doing below is already a heuristic :-) I don't think the
code needs to be changed, but I'd like this comment to explain why we
consider that the heuristic in this patch is fine, to help the person
(possibly you or me) who will read this code in a year and wonder what's
going on.

> > > I also r/package/packet/
> > >
> > > Did I miss another comment?
> > >
> > > > > +      * Some devices return shorter USB control packets that expected if the
> > > > > +      * returned value can fit in less bytes. Zero all the bytes that the
> > > > > +      * device have not written.
> > > >
> > > > s/have/has/
> > > >
> > > > And if you meant to start a new paragraph here, a blank line is missing.
> > > > Otherwise, no need to break the line before 80 columns.
> > >
> > > The patch is already in the uvc tree. How do you want to handle this?
> >
> > The branch shared between Hans and me can be rebased, it's a staging
> > area.
> 
> I will send a new version, fixing the typo. and the missing new line.
> I will also remove the sentence
> `* In UVC the data is usually represented in little-endian.`
> It is confusing.
> 
> > > > > +      * We exclude UVC_GET_INFO from the quirk. UVC_GET_LEN does not need to
> > > > > +      * be excluded because its size is always 1.
> > > > > +      */
> > > > > +     if (ret > 0 && query != UVC_GET_INFO) {
> > > > > +             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,
> > > > >                       "Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n",

-- 
Regards,

Laurent Pinchart

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

[Ricardo Ribalda]

On Wed, 27 Nov 2024 at 10:14, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Wed, Nov 27, 2024 at 09:58:21AM +0100, Ricardo Ribalda wrote:
> > On Wed, 27 Nov 2024 at 09:34, Laurent Pinchart wrote:
> > > On Tue, Nov 26, 2024 at 07:12:53PM +0100, Ricardo Ribalda wrote:
> > > > On Tue, 26 Nov 2024 at 19:06, Laurent Pinchart wrote:
> > > > > On Wed, Nov 20, 2024 at 03:26:19PM +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()")
> > > > > > Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > > > ---
> > > > > >  drivers/media/usb/uvc/uvc_video.c | 16 ++++++++++++++++
> > > > > >  1 file changed, 16 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > > > > > index cd9c29532fb0..482c4ceceaac 100644
> > > > > > --- a/drivers/media/usb/uvc/uvc_video.c
> > > > > > +++ b/drivers/media/usb/uvc/uvc_video.c
> > > > > > @@ -79,6 +79,22 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
> > > > > >       if (likely(ret == size))
> > > > > >               return 0;
> > > > > >
> > > > > > +     /*
> > > > > > +      * In UVC the data is usually represented in little-endian.
> > > > >
> > > > > I had a comment about this in the previous version, did you ignore it on
> > > > > purpose because you disagreed, or was it an oversight ?
> > > >
> > > > I rephrased the comment. I added "usually" to make it clear that it
> > > > might not be the case for all the data types. Like composed or xu.
> > >
> > > Ah, that's what you meant by "usually". I read it as "usually in
> > > little-endian, but could be big-endian too", which confused me.
> > >
> > > Data types that are not integers will not work nicely with the
> > > workaround below. How do you envision that being handled ? Do you
> > > consider that the device will return too few bytes only for integer data
> > > types, or that affected devices don't have controls that use compound
> > > data types ? I don't see what else we could do so I'd be fine with such
> > > a heuristic for this workaround, but it needs to be clearly explained.
> >
> > Non integer datatypes might work if the last part of the data is
> > expected to be zero.
> > I do not think that we can find a heuristic that can work for all the cases.
> >
> > For years we have ignored partial reads and it has never been an
> > issue. I vote for not adding any heuristics, the logging should help
> > identify future issues (if there is any).
>
> What you're doing below is already a heuristic :-) I don't think the
> code needs to be changed, but I'd like this comment to explain why we
> consider that the heuristic in this patch is fine, to help the person
> (possibly you or me) who will read this code in a year and wonder what's
> going on.

What about:

* Some devices return shorter USB control packets than expected if the
* returned value can fit in less bytes. Zero all the bytes that the
* device has not written.
*
* This quirk is applied to all datatypes, even to non little-endian integers
* or composite values. We exclude UVC_GET_INFO from the quirk.
* UVC_GET_LEN does not need to be excluded because its size is
* always 1.

>
> > > > I also r/package/packet/
> > > >
> > > > Did I miss another comment?
> > > >
> > > > > > +      * Some devices return shorter USB control packets that expected if the
> > > > > > +      * returned value can fit in less bytes. Zero all the bytes that the
> > > > > > +      * device have not written.
> > > > >
> > > > > s/have/has/
> > > > >
> > > > > And if you meant to start a new paragraph here, a blank line is missing.
> > > > > Otherwise, no need to break the line before 80 columns.
> > > >
> > > > The patch is already in the uvc tree. How do you want to handle this?
> > >
> > > The branch shared between Hans and me can be rebased, it's a staging
> > > area.
> >
> > I will send a new version, fixing the typo. and the missing new line.
> > I will also remove the sentence
> > `* In UVC the data is usually represented in little-endian.`
> > It is confusing.
> >
> > > > > > +      * We exclude UVC_GET_INFO from the quirk. UVC_GET_LEN does not need to
> > > > > > +      * be excluded because its size is always 1.
> > > > > > +      */
> > > > > > +     if (ret > 0 && query != UVC_GET_INFO) {
> > > > > > +             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,
> > > > > >                       "Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n",
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

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

[Laurent Pinchart]

On Wed, Nov 27, 2024 at 10:35:54AM +0100, Ricardo Ribalda wrote:
> On Wed, 27 Nov 2024 at 10:14, Laurent Pinchart wrote:
> > On Wed, Nov 27, 2024 at 09:58:21AM +0100, Ricardo Ribalda wrote:
> > > On Wed, 27 Nov 2024 at 09:34, Laurent Pinchart wrote:
> > > > On Tue, Nov 26, 2024 at 07:12:53PM +0100, Ricardo Ribalda wrote:
> > > > > On Tue, 26 Nov 2024 at 19:06, Laurent Pinchart wrote:
> > > > > > On Wed, Nov 20, 2024 at 03:26:19PM +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()")
> > > > > > > Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > > > > ---
> > > > > > >  drivers/media/usb/uvc/uvc_video.c | 16 ++++++++++++++++
> > > > > > >  1 file changed, 16 insertions(+)
> > > > > > >
> > > > > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > > > > > > index cd9c29532fb0..482c4ceceaac 100644
> > > > > > > --- a/drivers/media/usb/uvc/uvc_video.c
> > > > > > > +++ b/drivers/media/usb/uvc/uvc_video.c
> > > > > > > @@ -79,6 +79,22 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
> > > > > > >       if (likely(ret == size))
> > > > > > >               return 0;
> > > > > > >
> > > > > > > +     /*
> > > > > > > +      * In UVC the data is usually represented in little-endian.
> > > > > >
> > > > > > I had a comment about this in the previous version, did you ignore it on
> > > > > > purpose because you disagreed, or was it an oversight ?
> > > > >
> > > > > I rephrased the comment. I added "usually" to make it clear that it
> > > > > might not be the case for all the data types. Like composed or xu.
> > > >
> > > > Ah, that's what you meant by "usually". I read it as "usually in
> > > > little-endian, but could be big-endian too", which confused me.
> > > >
> > > > Data types that are not integers will not work nicely with the
> > > > workaround below. How do you envision that being handled ? Do you
> > > > consider that the device will return too few bytes only for integer data
> > > > types, or that affected devices don't have controls that use compound
> > > > data types ? I don't see what else we could do so I'd be fine with such
> > > > a heuristic for this workaround, but it needs to be clearly explained.
> > >
> > > Non integer datatypes might work if the last part of the data is
> > > expected to be zero.
> > > I do not think that we can find a heuristic that can work for all the cases.
> > >
> > > For years we have ignored partial reads and it has never been an
> > > issue. I vote for not adding any heuristics, the logging should help
> > > identify future issues (if there is any).
> >
> > What you're doing below is already a heuristic :-) I don't think the
> > code needs to be changed, but I'd like this comment to explain why we
> > consider that the heuristic in this patch is fine, to help the person
> > (possibly you or me) who will read this code in a year and wonder what's
> > going on.
> 
> What about:
> 
> * Some devices return shorter USB control packets than expected if the
> * returned value can fit in less bytes. Zero all the bytes that the
> * device has not written.
> *
> * This quirk is applied to all datatypes, even to non little-endian integers
> * or composite values. We exclude UVC_GET_INFO from the quirk.
> * UVC_GET_LEN does not need to be excluded because its size is
> * always 1.

For the second paragraph you could write

 * This quirk is applied to all controls, regardless of their data type. Most
 * controls are little-endian integers, in which case the missing bytes become 0
 * MSBs. For other data types, a different heuristic could be implemented if a
 * device is found needing it.
 *
 * We exclude UVC_GET_INFO from the quirk. UVC_GET_LEN does not need to be
 * excluded because its size is always 1.

> > > > > I also r/package/packet/
> > > > >
> > > > > Did I miss another comment?
> > > > >
> > > > > > > +      * Some devices return shorter USB control packets that expected if the
> > > > > > > +      * returned value can fit in less bytes. Zero all the bytes that the
> > > > > > > +      * device have not written.
> > > > > >
> > > > > > s/have/has/
> > > > > >
> > > > > > And if you meant to start a new paragraph here, a blank line is missing.
> > > > > > Otherwise, no need to break the line before 80 columns.
> > > > >
> > > > > The patch is already in the uvc tree. How do you want to handle this?
> > > >
> > > > The branch shared between Hans and me can be rebased, it's a staging
> > > > area.
> > >
> > > I will send a new version, fixing the typo. and the missing new line.
> > > I will also remove the sentence
> > > `* In UVC the data is usually represented in little-endian.`
> > > It is confusing.
> > >
> > > > > > > +      * We exclude UVC_GET_INFO from the quirk. UVC_GET_LEN does not need to
> > > > > > > +      * be excluded because its size is always 1.
> > > > > > > +      */
> > > > > > > +     if (ret > 0 && query != UVC_GET_INFO) {
> > > > > > > +             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,
> > > > > > >                       "Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n",

-- 
Regards,

Laurent Pinchart