Re: [PATCH 2/3] media: uvcvideo: Refactor uvc_query_ctrl

[Ricardo Ribalda]

On Tue, 8 Oct 2024 at 21:31, Sakari Ailus <sakari.ailus@xxxxxx> wrote:
>
> Hi Ricardo,
>
> On Tue, Oct 08, 2024 at 09:22:25PM +0800, Ricardo Ribalda wrote:
> > Hi Sakari!
> >
> > On Tue, 8 Oct 2024 at 20:01, Sakari Ailus <sakari.ailus@xxxxxx> wrote:
> > >
> > > Hi Ricardo,
> > >
> > > On Tue, Oct 08, 2024 at 07:06:15AM +0000, Ricardo Ribalda wrote:
> > > > Move the query control error logic to its own function.
> > > > There is no functional change introduced by this patch.
> > > >
> > > > Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
> > > > ---
> > > >  drivers/media/usb/uvc/uvc_video.c | 45 ++++++++++++++++++++++-----------------
> > > >  1 file changed, 26 insertions(+), 19 deletions(-)
> > > >
> > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > > > index 853dfb7b5f7b..a57272a2c9e1 100644
> > > > --- a/drivers/media/usb/uvc/uvc_video.c
> > > > +++ b/drivers/media/usb/uvc/uvc_video.c
> > > > @@ -67,30 +67,12 @@ static const char *uvc_query_name(u8 query)
> > > >       }
> > > >  }
> > > >
> > > > -int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
> > > > -                     u8 intfnum, u8 cs, void *data, u16 size)
> > > > +static int uvc_query_ctrl_error(struct uvc_device *dev, u8 intfnum, void *data)
> > > >  {
> > > >       int ret;
> > > >       u8 error;
> > > >       u8 tmp;
> > > >
> > > > -     ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size,
> > > > -                             UVC_CTRL_CONTROL_TIMEOUT);
> > > > -     if (likely(ret == size))
> > > > -             return 0;
> > > > -
> > > > -     if (ret > 0 && ret < size) {
> > > > -             memset(data + ret, 0, size - ret);
> > > > -             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 ? ret : -EPIPE;
> > > > -     }
> > > > -
> > > >       /* Reuse data[0] to request the error code. */
> > > >       tmp = *(u8 *)data;
> > > >
> > > > @@ -135,6 +117,31 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
> > > >       return -EPIPE;
> > > >  }
> > > >
> > > > +int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
> > > > +                u8 intfnum, u8 cs, void *data, u16 size)
> > > > +{
> > > > +     int ret;
> > > > +
> > > > +     ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size,
> > > > +                            UVC_CTRL_CONTROL_TIMEOUT);
> > > > +     if (likely(ret == size))
> > > > +             return 0;
> > > > +
> > > > +     if (ret == -EPIPE)
> > > > +             return uvc_query_ctrl_error(dev, intfnum, data);
> > > > +
> > > > +     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);
> > >
> > > This message should probably be printed after the check below.
> >
> > If the device is returning less bytes, the hardware is not behaving
> > according to spec and it is good information, specially if you are
> > bringing up a new device.
> > I could make it  a dev_warn() (or even uvc_debug) if ret <size. WDYT?
>
> What I also came to think whether this is worth an explicit quirk flag.
> There could well be devices that have other bugs that would still fall
> under the same check.

Before https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/media/usb/uvc/uvc_video.c?id=a763b9fb58be869e252a7d33acb0a6390b01c801
we were accepting short reads and showing the error.
We are returning to the old behaviour, plus the memset.

A quirk will be difficult to maintain. If we can find a heuristics
that works I would vote for that.


>
> Either way, there should not be a message every single time this workaround
> is applied. Isn't uvc_query_ctrl() also used in some IOCTLs outside probe?

I have only seen this behaviour for GET_DEF, and that value is cached.
Meaning that the warning will only be shown once per control.

>
> >
> >
> > >
> > > I'd actually move the below check before the ret == -EPIPE check as it's a
> > > successful case (and changing the condition to <= would make the ret ==
> > > size check redundant).
> >
> > something like this?
> >
> > if (ret > 0)  {
> >    if (ret != size) {
> >       print_error();
> >       memcpy();
> >    }
> >    return 0;
> > }
>
> Well, if you think it's reasonable to keep the error message (should be a
> warning in that case IMO), then the original code makes sense.
>
> I wonder what Laurent thinks.
>
> >
> > >
> > > > +
> > > > +     if (ret > 0 && ret < size) {
> > > > +             memset(data + ret, 0, size - ret);
> > > > +             return 0;
> > > > +     }
> > > > +
> > > > +     return ret ? ret : -EPIPE;
> > > > +}
> > > > +
> > > >  static const struct usb_device_id elgato_cam_link_4k = {
> > > >       USB_DEVICE(0x0fd9, 0x0066)
> > > >  };
> > > >
> > >
>
> --
> Kind regards,
>
> Sakari Ailus



--
Ricardo Ribalda