On Wed, Nov 20, 2024 at 11:50:15AM +0100, Hans de Goede wrote:
> On 18-Nov-24 5:57 PM, Ricardo Ribalda wrote:
> > On Mon, 18 Nov 2024 at 17:41, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> >> 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@xxxxxxxxxxxxxxx
> >>> Fixes: a763b9fb58be ("media: uvcvideo: Do not return positive errors in uvc_query_ctrl()")
> >>> Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
> >>> ---
> >>> 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.
Rate-limiting won't help much, as I don't expect userspace to trigger
this at high frequency. dev_warn_once() is the simplest option. I'm a
bit concerned that we silently apply the workaround after the first
occurrence, it may lead to difficult to diagnose issues in bug reports.
A flag per control would be nice, but it's probably overkill :-/ Or
maybe it wouldn't be that hard to implement ?
> > 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.
That could be useful, but I don't expect most users would be able to
enable dev_dbg(), so it would be of limited value in bug reports.
> 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(...(
That's an interesting concept :-)
> >> 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,
Laurent Pinchart
