| Message ID | 20241120-uvc-readless-v4-1-4672dbef3d46@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | media: uvcvideo: Support partial control reads and minor changes | expand |
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 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, 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",
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",
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(+)