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

[Sakari Ailus <sakari.ailus@iki.fi>]

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@iki.fi> 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@chromium.org>
> > > ---
> > >  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.

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'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