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.o...
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.o...
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.o...
--- 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,
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",
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",
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
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",
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
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",
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
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",
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.o...
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.o...
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.o...
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,
linux-stable-mirror@lists.linaro.org
-
Hans de Goede -
Laurent Pinchart -
Ricardo Ribalda