* [PATCH v2 0/2] media: uvcvideo: Support partial control reads and minor changes
@ 2024-10-08 15:00 Ricardo Ribalda
2024-10-08 15:00 ` [PATCH v2 1/2] media: uvcvideo: Support partial control reads Ricardo Ribalda
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Ricardo Ribalda @ 2024-10-08 15:00 UTC (permalink / raw)
To: Laurent Pinchart, Mauro Carvalho Chehab
Cc: linux-media, linux-kernel, Ricardo Ribalda, Sakari Ailus, stable
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 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 | 27 +++++++++++++++++++++++----
1 file changed, 23 insertions(+), 4 deletions(-)
---
base-commit: 9852d85ec9d492ebef56dc5f229416c925758edc
change-id: 20241008-uvc-readless-23f9b8cad0b3
Best regards,
--
Ricardo Ribalda <ribalda@chromium.org>
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH v2 1/2] media: uvcvideo: Support partial control reads 2024-10-08 15:00 [PATCH v2 0/2] media: uvcvideo: Support partial control reads and minor changes Ricardo Ribalda @ 2024-10-08 15:00 ` Ricardo Ribalda 2024-11-18 16:41 ` Hans de Goede 2024-10-08 15:00 ` [PATCH v2 2/2] media: uvcvideo: Add more logging to uvc_query_ctrl() Ricardo Ribalda 2024-10-08 20:08 ` [PATCH v2 0/2] media: uvcvideo: Support partial control reads and minor changes Sakari Ailus 2 siblings, 1 reply; 9+ messages in thread From: Ricardo Ribalda @ 2024-10-08 15:00 UTC (permalink / raw) To: Laurent Pinchart, Mauro Carvalho Chehab Cc: linux-media, linux-kernel, Ricardo Ribalda, Sakari Ailus, stable 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 | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c index cd9c29532fb0..f125b3ba50f2 100644 --- a/drivers/media/usb/uvc/uvc_video.c +++ b/drivers/media/usb/uvc/uvc_video.c @@ -76,14 +76,29 @@ 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; + + /* + * In UVC the data is represented in little-endian by default. + * Some devices return shorter control packages that expected + * for GET_DEF/MAX/MIN if the return value can fit in less + * bytes. + * Zero all the bytes that the device have not written. + */ + memset(data + ret, 0, size - ret); + dev_warn(&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", uvc_query_name(query), cs, unit, ret, size); - return ret < 0 ? ret : -EPIPE; + return ret ? ret : -EPIPE; } /* Reuse data[0] to request the error code. */ -- 2.47.0.rc0.187.ge670bccf7e-goog ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] media: uvcvideo: Support partial control reads 2024-10-08 15:00 ` [PATCH v2 1/2] media: uvcvideo: Support partial control reads Ricardo Ribalda @ 2024-11-18 16:41 ` Hans de Goede 2024-11-18 16:57 ` Ricardo Ribalda 0 siblings, 1 reply; 9+ messages in thread From: Hans de Goede @ 2024-11-18 16:41 UTC (permalink / raw) To: Ricardo Ribalda, Laurent Pinchart, Mauro Carvalho Chehab Cc: linux-media, linux-kernel, Sakari Ailus, stable Hi Ricardo, Thank you for your patch. On 8-Oct-24 5:00 PM, 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 | 19 +++++++++++++++++-- > 1 file changed, 17 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c > index cd9c29532fb0..f125b3ba50f2 100644 > --- a/drivers/media/usb/uvc/uvc_video.c > +++ b/drivers/media/usb/uvc/uvc_video.c > @@ -76,14 +76,29 @@ 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; > + > + /* > + * In UVC the data is represented in little-endian by default. > + * Some devices return shorter control packages that expected > + * for GET_DEF/MAX/MIN if the return value can fit in less > + * bytes. What about GET_CUR/GET_RES ? are those not affected? And if it is not affected should we limit this special handling to GET_DEF/MAX/MIN ? > + * Zero all the bytes that the device have not written. > + */ > + memset(data + ret, 0, size - ret); So your new work around automatically applies to all UVC devices which gives us a short return. I think that is both good and bad at the same time. Good because it avoids the need to add quirks. Bad because what if we get a short return for another reason. You do warn on the short return. So if we get bugs due to hitting the short return for another reason the warning will be i the logs. So all in all think the good outways the bad. So yes this seems like a good solution. > + dev_warn(&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); I do wonder if we need to use dev_warn_ratelimited() or dev_warn_once() here though. If this only impacts GET_DEF/MAX/MIN we will only hit this once per ctrl, after which the cache will be populated. But if GET_CUR is also affected then userspace can trigger this warning. So in that case I think we really should use dev_warn_once() or have a flag per ctrl to track this and only warn once per ctrl if we want to know which ctrls exactly are buggy. What we really do not want is userspace repeatedly calling VIDIOC_G_CTRL / VIDIOC_G_EXT_CTRLS resulting in a message in dmesg every call. > return 0; > + } > > if (ret != -EPIPE) { > dev_err(&dev->udev->dev, > "Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n", > uvc_query_name(query), cs, unit, ret, size); > - return ret < 0 ? ret : -EPIPE; > + return ret ? ret : -EPIPE; It took me a minute to wrap my brain around this and even though I now understand this change I do not like it. There is no need to optimize an error-handling path like this and IMHO the original code is much easier to read: return ret < 0 ? ret : -ESOMETHING; is a well known pattern to check results from functions which return a negative errno, or the amount of bytes read, combined with an earlier success check for ret == amount-expected . By changing this to: return ret ? ret : -EPIPE; You are breaking the pattern recognition people familiar with this kinda code have and IMHO this is not necessary. Also not changing this reduces the patch-size / avoids code-churn which also is a good thing. Please drop this part of the patch. Regards, Hans ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] media: uvcvideo: Support partial control reads 2024-11-18 16:41 ` Hans de Goede @ 2024-11-18 16:57 ` Ricardo Ribalda 2024-11-20 10:50 ` Hans de Goede 0 siblings, 1 reply; 9+ messages in thread From: Ricardo Ribalda @ 2024-11-18 16:57 UTC (permalink / raw) To: Hans de Goede Cc: Laurent Pinchart, Mauro Carvalho Chehab, linux-media, linux-kernel, Sakari Ailus, stable On Mon, 18 Nov 2024 at 17:41, Hans de Goede <hdegoede@redhat.com> wrote: > > Hi Ricardo, > > Thank you for your patch. > > On 8-Oct-24 5:00 PM, 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 | 19 +++++++++++++++++-- > > 1 file changed, 17 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c > > index cd9c29532fb0..f125b3ba50f2 100644 > > --- a/drivers/media/usb/uvc/uvc_video.c > > +++ b/drivers/media/usb/uvc/uvc_video.c > > @@ -76,14 +76,29 @@ 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; > > + > > + /* > > + * In UVC the data is represented in little-endian by default. > > + * Some devices return shorter control packages that expected > > + * for GET_DEF/MAX/MIN if the return value can fit in less > > + * bytes. > > What about GET_CUR/GET_RES ? are those not affected? > > And if it is not affected should we limit this special handling to > GET_DEF/MAX/MIN ? I have only seen it with GET_DEF, but I would not be surprised if it happens for all of them. before: a763b9fb58be ("media: uvcvideo: Do not return positive errors in uvc_query_ctrl()") We were applying the quirk to all the call types, so I'd rather keep the old behaviour. The extra logging will help us find bugs (if any). Let me fix the doc. > > > > + * Zero all the bytes that the device have not written. > > + */ > > + memset(data + ret, 0, size - ret); > > So your new work around automatically applies to all UVC devices which > gives us a short return. I think that is both good and bad at the same > time. Good because it avoids the need to add quirks. Bad because what > if we get a short return for another reason. > > You do warn on the short return. So if we get bugs due to hitting the short > return for another reason the warning will be i the logs. > > So all in all think the good outways the bad. > > So yes this seems like a good solution. > > > + dev_warn(&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); > > I do wonder if we need to use dev_warn_ratelimited() > or dev_warn_once() here though. > > If this only impacts GET_DEF/MAX/MIN we will only hit this > once per ctrl, after which the cache will be populated. > > But if GET_CUR is also affected then userspace can trigger > this warning. So in that case I think we really should use > dev_warn_once() or have a flag per ctrl to track this > and only warn once per ctrl if we want to know which > ctrls exactly are buggy. Let me use dev_warn_once() > > What we really do not want is userspace repeatedly calling > VIDIOC_G_CTRL / VIDIOC_G_EXT_CTRLS resulting in a message > in dmesg every call. > > > return 0; > > + } > > > > if (ret != -EPIPE) { > > dev_err(&dev->udev->dev, > > "Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n", > > uvc_query_name(query), cs, unit, ret, size); > > - return ret < 0 ? ret : -EPIPE; > > + return ret ? ret : -EPIPE; > > It took me a minute to wrap my brain around this and even > though I now understand this change I do not like it. > > There is no need to optimize an error-handling path like this > and IMHO the original code is much easier to read: > > return ret < 0 ? ret : -ESOMETHING; > > is a well known pattern to check results from functions which > return a negative errno, or the amount of bytes read, combined > with an earlier success check for ret == amount-expected . > > By changing this to: > > return ret ? ret : -EPIPE; > > You are breaking the pattern recognition people familiar with > this kinda code have and IMHO this is not necessary. > > Also not changing this reduces the patch-size / avoids code-churn > which also is a good thing. > > Please drop this part of the patch. ack > > Regards, > > Hans > > -- Ricardo Ribalda ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] media: uvcvideo: Support partial control reads 2024-11-18 16:57 ` Ricardo Ribalda @ 2024-11-20 10:50 ` Hans de Goede 0 siblings, 0 replies; 9+ messages in thread From: Hans de Goede @ 2024-11-20 10:50 UTC (permalink / raw) To: Ricardo Ribalda Cc: Laurent Pinchart, Mauro Carvalho Chehab, linux-media, linux-kernel, Sakari Ailus, stable Hi Ricardo, On 18-Nov-24 5:57 PM, Ricardo Ribalda wrote: > On Mon, 18 Nov 2024 at 17:41, Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi Ricardo, >> >> Thank you for your patch. >> >> On 8-Oct-24 5:00 PM, 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 | 19 +++++++++++++++++-- >>> 1 file changed, 17 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c >>> index cd9c29532fb0..f125b3ba50f2 100644 >>> --- a/drivers/media/usb/uvc/uvc_video.c >>> +++ b/drivers/media/usb/uvc/uvc_video.c >>> @@ -76,14 +76,29 @@ 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; >>> + >>> + /* >>> + * In UVC the data is represented in little-endian by default. >>> + * Some devices return shorter control packages that expected >>> + * for GET_DEF/MAX/MIN if the return value can fit in less >>> + * bytes. >> >> What about GET_CUR/GET_RES ? are those not affected? >> >> And if it is not affected should we limit this special handling to >> GET_DEF/MAX/MIN ? > > I have only seen it with GET_DEF, but I would not be surprised if it > happens for all of them. > > before: > a763b9fb58be ("media: uvcvideo: Do not return positive errors in > uvc_query_ctrl()") > We were applying the quirk to all the call types, so I'd rather keep > the old behaviour. > > The extra logging will help us find bugs (if any). > > Let me fix the doc. > >> >> >>> + * Zero all the bytes that the device have not written. >>> + */ >>> + memset(data + ret, 0, size - ret); >> >> So your new work around automatically applies to all UVC devices which >> gives us a short return. I think that is both good and bad at the same >> time. Good because it avoids the need to add quirks. Bad because what >> if we get a short return for another reason. >> >> You do warn on the short return. So if we get bugs due to hitting the short >> return for another reason the warning will be i the logs. >> >> So all in all think the good outways the bad. >> >> So yes this seems like a good solution. >> >>> + dev_warn(&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); >> >> I do wonder if we need to use dev_warn_ratelimited() >> or dev_warn_once() here though. >> >> If this only impacts GET_DEF/MAX/MIN we will only hit this >> once per ctrl, after which the cache will be populated. >> >> But if GET_CUR is also affected then userspace can trigger >> this warning. So in that case I think we really should use >> dev_warn_once() or have a flag per ctrl to track this >> and only warn once per ctrl if we want to know which >> ctrls exactly are buggy. > > Let me use dev_warn_once() Great, thank you. Re-reading this I think what would be best here is to combine dev_warn_once() with a dev_dbg logging the same thing. This way if we want the more fine grained messages for all controls / all of GET_* and not just the first call we can still get them by enabling the debug messages with dyndbg. This combination is used for similar reasons in other places of the kernel. Not sure what Laurent thinks of this though, Laurent ? I wonder if we need some sort of helper for this: dev_warn_once_and_debug(...( Regards, Hans > >> >> What we really do not want is userspace repeatedly calling >> VIDIOC_G_CTRL / VIDIOC_G_EXT_CTRLS resulting in a message >> in dmesg every call. >> >>> return 0; >>> + } >>> >>> if (ret != -EPIPE) { >>> dev_err(&dev->udev->dev, >>> "Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n", >>> uvc_query_name(query), cs, unit, ret, size); >>> - return ret < 0 ? ret : -EPIPE; >>> + return ret ? ret : -EPIPE; >> >> It took me a minute to wrap my brain around this and even >> though I now understand this change I do not like it. >> >> There is no need to optimize an error-handling path like this >> and IMHO the original code is much easier to read: >> >> return ret < 0 ? ret : -ESOMETHING; >> >> is a well known pattern to check results from functions which >> return a negative errno, or the amount of bytes read, combined >> with an earlier success check for ret == amount-expected . >> >> By changing this to: >> >> return ret ? ret : -EPIPE; >> >> You are breaking the pattern recognition people familiar with >> this kinda code have and IMHO this is not necessary. >> >> Also not changing this reduces the patch-size / avoids code-churn >> which also is a good thing. >> >> Please drop this part of the patch. > ack >> >> Regards, >> >> Hans >> >> > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 2/2] media: uvcvideo: Add more logging to uvc_query_ctrl() 2024-10-08 15:00 [PATCH v2 0/2] media: uvcvideo: Support partial control reads and minor changes Ricardo Ribalda 2024-10-08 15:00 ` [PATCH v2 1/2] media: uvcvideo: Support partial control reads Ricardo Ribalda @ 2024-10-08 15:00 ` Ricardo Ribalda 2024-11-18 16:45 ` Hans de Goede 2024-10-08 20:08 ` [PATCH v2 0/2] media: uvcvideo: Support partial control reads and minor changes Sakari Ailus 2 siblings, 1 reply; 9+ messages in thread From: Ricardo Ribalda @ 2024-10-08 15:00 UTC (permalink / raw) To: Laurent Pinchart, Mauro Carvalho Chehab Cc: linux-media, linux-kernel, Ricardo Ribalda, Sakari Ailus 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. Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- drivers/media/usb/uvc/uvc_video.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c index f125b3ba50f2..6efbfa609059 100644 --- a/drivers/media/usb/uvc/uvc_video.c +++ b/drivers/media/usb/uvc/uvc_video.c @@ -111,8 +111,12 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, error = *(u8 *)data; *(u8 *)data = tmp; - if (ret != 1) - return ret < 0 ? ret : -EPIPE; + if (ret != 1) { + dev_err(&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 ? ret : -EPIPE; + } uvc_dbg(dev, CONTROL, "Control error %u\n", error); -- 2.47.0.rc0.187.ge670bccf7e-goog ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] media: uvcvideo: Add more logging to uvc_query_ctrl() 2024-10-08 15:00 ` [PATCH v2 2/2] media: uvcvideo: Add more logging to uvc_query_ctrl() Ricardo Ribalda @ 2024-11-18 16:45 ` Hans de Goede 2024-11-18 17:16 ` Ricardo Ribalda 0 siblings, 1 reply; 9+ messages in thread From: Hans de Goede @ 2024-11-18 16:45 UTC (permalink / raw) To: Ricardo Ribalda, Laurent Pinchart, Mauro Carvalho Chehab Cc: linux-media, linux-kernel, Sakari Ailus Hi Ricardo, Thank you for your patch. On 8-Oct-24 5:00 PM, Ricardo Ribalda wrote: > 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. > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/media/usb/uvc/uvc_video.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c > index f125b3ba50f2..6efbfa609059 100644 > --- a/drivers/media/usb/uvc/uvc_video.c > +++ b/drivers/media/usb/uvc/uvc_video.c > @@ -111,8 +111,12 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, > error = *(u8 *)data; > *(u8 *)data = tmp; > > - if (ret != 1) > - return ret < 0 ? ret : -EPIPE; > + if (ret != 1) { > + dev_err(&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 ? ret : -EPIPE; Adding error logging is always good, but again please drop the change to the return condition and stick with the: return ret < 0 ? ret : -EPIPE; pattern used everywhere. Also in this case the return condition change really should have been in a separate patch since unlike before the success condition did not change, so it really is unrelated to adding the error logging. Regards, Hans ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] media: uvcvideo: Add more logging to uvc_query_ctrl() 2024-11-18 16:45 ` Hans de Goede @ 2024-11-18 17:16 ` Ricardo Ribalda 0 siblings, 0 replies; 9+ messages in thread From: Ricardo Ribalda @ 2024-11-18 17:16 UTC (permalink / raw) To: Hans de Goede Cc: Laurent Pinchart, Mauro Carvalho Chehab, linux-media, linux-kernel, Sakari Ailus Hi Hans On Mon, 18 Nov 2024 at 17:45, Hans de Goede <hdegoede@redhat.com> wrote: > > Hi Ricardo, > > Thank you for your patch. > > On 8-Oct-24 5:00 PM, Ricardo Ribalda wrote: > > 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. > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > --- > > drivers/media/usb/uvc/uvc_video.c | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c > > index f125b3ba50f2..6efbfa609059 100644 > > --- a/drivers/media/usb/uvc/uvc_video.c > > +++ b/drivers/media/usb/uvc/uvc_video.c > > @@ -111,8 +111,12 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, > > error = *(u8 *)data; > > *(u8 *)data = tmp; > > > > - if (ret != 1) > > - return ret < 0 ? ret : -EPIPE; > > + if (ret != 1) { > > + dev_err(&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 ? ret : -EPIPE; > > Adding error logging is always good, but again please drop the change > to the return condition and stick with the: > > return ret < 0 ? ret : -EPIPE; > > pattern used everywhere. > > Also in this case the return condition change really should have > been in a separate patch since unlike before the success condition > did not change, so it really is unrelated to adding the error > logging. It doesn't really change: __uvc_query_ctrl() in this case can return 1, 0 or a retval. But I agree, the change does not provide anything (and I did not mentioned it on the commit msg) let me restore the original return Thanks > > Regards, > > Hans > > > -- Ricardo Ribalda ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 0/2] media: uvcvideo: Support partial control reads and minor changes 2024-10-08 15:00 [PATCH v2 0/2] media: uvcvideo: Support partial control reads and minor changes Ricardo Ribalda 2024-10-08 15:00 ` [PATCH v2 1/2] media: uvcvideo: Support partial control reads Ricardo Ribalda 2024-10-08 15:00 ` [PATCH v2 2/2] media: uvcvideo: Add more logging to uvc_query_ctrl() Ricardo Ribalda @ 2024-10-08 20:08 ` Sakari Ailus 2 siblings, 0 replies; 9+ messages in thread From: Sakari Ailus @ 2024-10-08 20:08 UTC (permalink / raw) To: Ricardo Ribalda Cc: Laurent Pinchart, Mauro Carvalho Chehab, linux-media, linux-kernel, Sakari Ailus, stable On Tue, Oct 08, 2024 at 03:00:06PM +0000, 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> For both: Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com> -- Sakari Ailus ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-11-20 10:50 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-10-08 15:00 [PATCH v2 0/2] media: uvcvideo: Support partial control reads and minor changes Ricardo Ribalda 2024-10-08 15:00 ` [PATCH v2 1/2] media: uvcvideo: Support partial control reads Ricardo Ribalda 2024-11-18 16:41 ` Hans de Goede 2024-11-18 16:57 ` Ricardo Ribalda 2024-11-20 10:50 ` Hans de Goede 2024-10-08 15:00 ` [PATCH v2 2/2] media: uvcvideo: Add more logging to uvc_query_ctrl() Ricardo Ribalda 2024-11-18 16:45 ` Hans de Goede 2024-11-18 17:16 ` Ricardo Ribalda 2024-10-08 20:08 ` [PATCH v2 0/2] media: uvcvideo: Support partial control reads and minor changes Sakari Ailus
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox