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