linux-kernel - Re: [PATCH 2/3] media: uvcvideo: Refactor uvc_query

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANiDSCu5ho_CFCgRQVdUzaWtfSi_eNk+N7c-DGxcO+6Ks_Vmvg@mail.gmail.com>
Date: Tue, 8 Oct 2024 21:22:25 +0800
From: Ricardo Ribalda <ribalda@...omium.org>
To: Sakari Ailus <sakari.ailus@....fi>
Cc: Laurent Pinchart <laurent.pinchart@...asonboard.com>, 
	Mauro Carvalho Chehab <mchehab@...nel.org>, linux-media@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] media: uvcvideo: Refactor uvc_query_ctrl

Hi Sakari!

On Tue, 8 Oct 2024 at 20:01, Sakari Ailus <sakari.ailus@....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@...omium.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?


>
> 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;
}

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