linux-kernel - Re: [PATCH v3 1/2] media: uvcvideo: Fix deferred probing error

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: <CANiDSCuO+dHOBtW4yvy1n25QWEs-WdQ9H8Lh2rUtcPbUq3hBkQ@mail.gmail.com>
Date: Wed, 23 Apr 2025 06:50:10 +0800
From: Ricardo Ribalda <ribalda@...omium.org>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Hans de Goede <hdegoede@...hat.com>, Mauro Carvalho Chehab <mchehab@...nel.org>, 
	Mauro Carvalho Chehab <mchehab+huawei@...nel.org>, linux-media@...r.kernel.org, 
	linux-kernel@...r.kernel.org, stable@...r.kernel.org, 
	Douglas Anderson <dianders@...omium.org>
Subject: Re: [PATCH v3 1/2] media: uvcvideo: Fix deferred probing error

On Wed, 23 Apr 2025 at 02:06, Laurent Pinchart
<laurent.pinchart@...asonboard.com> wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
> On Thu, Mar 13, 2025 at 12:20:39PM +0000, Ricardo Ribalda wrote:
> > uvc_gpio_parse() can return -EPROBE_DEFER when the GPIOs it depends on
> > have not yet been probed. This return code should be propagated to the
> > caller of uvc_probe() to ensure that probing is retried when the required
> > GPIOs become available.
> >
> > Currently, this error code is incorrectly converted to -ENODEV,
> > causing some internal cameras to be ignored.
> >
> > This commit fixes this issue by propagating the -EPROBE_DEFER error.
> >
> > Cc: stable@...r.kernel.org
> > Fixes: 2886477ff987 ("media: uvcvideo: Implement UVC_EXT_GPIO_UNIT")
> > Reviewed-by: Douglas Anderson <dianders@...omium.org>
> > Signed-off-by: Ricardo Ribalda <ribalda@...omium.org>
> > ---
> >  drivers/media/usb/uvc/uvc_driver.c | 27 +++++++++++++++++++--------
> >  1 file changed, 19 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index deadbcea5e227c832976fd176c7cdbfd7809c608..e966bdb9239f345fd157588ebdad2b3ebe45168d 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -2231,13 +2231,16 @@ static int uvc_probe(struct usb_interface *intf,
> >  #endif
> >
> >       /* Parse the Video Class control descriptor. */
> > -     if (uvc_parse_control(dev) < 0) {
> > +     ret = uvc_parse_control(dev);
> > +     if (ret < 0) {
> > +             ret = -ENODEV;
>
> Why do you set ret to -ENODEV here...
>
> >               uvc_dbg(dev, PROBE, "Unable to parse UVC descriptors\n");
> >               goto error;
> >       }
> >
> >       /* Parse the associated GPIOs. */
> > -     if (uvc_gpio_parse(dev) < 0) {
> > +     ret = uvc_gpio_parse(dev);
> > +     if (ret < 0) {
> >               uvc_dbg(dev, PROBE, "Unable to parse UVC GPIOs\n");
> >               goto error;
> >       }
> > @@ -2263,24 +2266,32 @@ static int uvc_probe(struct usb_interface *intf,
> >       }
> >
> >       /* Register the V4L2 device. */
> > -     if (v4l2_device_register(&intf->dev, &dev->vdev) < 0)
> > +     ret = v4l2_device_register(&intf->dev, &dev->vdev);
> > +     if (ret < 0)
>
> ... but not here ? The code below is also not very consistant.

For all the "external" functions I was looking into populating their
error code to probe(). Other drivers (check vivid for example) do
exactly this.

There is more value in returning the real cause of the error (ENOMEM,
EINVAL) that the plain ENODEV.

>
> >               goto error;
> >
> >       /* Scan the device for video chains. */
> > -     if (uvc_scan_device(dev) < 0)
> > +     if (uvc_scan_device(dev) < 0) {
> > +             ret = -ENODEV;
> >               goto error;
> > +     }
> >
> >       /* Initialize controls. */
> > -     if (uvc_ctrl_init_device(dev) < 0)
> > +     if (uvc_ctrl_init_device(dev) < 0) {
> > +             ret = -ENODEV;
> >               goto error;
> > +     }
> >
> >       /* Register video device nodes. */
> > -     if (uvc_register_chains(dev) < 0)
> > +     if (uvc_register_chains(dev) < 0) {
> > +             ret = -ENODEV;
> >               goto error;
> > +     }
> >
> >  #ifdef CONFIG_MEDIA_CONTROLLER
> >       /* Register the media device node */
> > -     if (media_device_register(&dev->mdev) < 0)
> > +     ret = media_device_register(&dev->mdev);
> > +     if (ret < 0)
> >               goto error;
> >  #endif
> >       /* Save our data pointer in the interface data. */
> > @@ -2314,7 +2325,7 @@ static int uvc_probe(struct usb_interface *intf,
> >  error:
> >       uvc_unregister_video(dev);
> >       kref_put(&dev->ref, uvc_delete);
> > -     return -ENODEV;
> > +     return ret;
> >  }
> >
> >  static void uvc_disconnect(struct usb_interface *intf)
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda