Re: [PATCH v2 1/2] media: uvcvideo: Support partial control reads

[Laurent Pinchart]

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