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 expectedWhat'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