linux-kernel - Re: [PATCH] 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] [day] [month] [year] [list]
Message-ID: <CANiDSCupq4A=ctR=Kkp7VxB+gvw=Z8MdDZHDShVMMAzms0VUAg@mail.gmail.com>
Date: Sun, 23 Feb 2025 20:32:24 +0100
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
Subject: Re: [PATCH] media: uvcvideo: Fix deferred probing error

On Sun, 23 Feb 2025 at 15:36, Laurent Pinchart
<laurent.pinchart@...asonboard.com> wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
> On Wed, Jan 29, 2025 at 12:39:46PM +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")
> > Signed-off-by: Ricardo Ribalda <ribalda@...omium.org>
> > ---
> >  drivers/media/usb/uvc/uvc_driver.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index a10d4f4d9f95..73a7f23b616c 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -2253,9 +2253,10 @@ static int uvc_probe(struct usb_interface *intf,
> >       }
> >
> >       /* 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;
> > +             goto error_retcode;
> >       }
> >
> >       dev_info(&dev->udev->dev, "Found UVC %u.%02x device %s (%04x:%04x)\n",
> > @@ -2328,9 +2329,11 @@ static int uvc_probe(struct usb_interface *intf,
> >       return 0;
> >
> >  error:
> > +     ret = -ENODEV;
> > +error_retcode:
>
> This isn't very nice. Could we instead also propagate error codes from
> other locations in the uvc_probe() function ? If you want to minimize
> changes, you can initialize ret to -ENODEV, and turn the (ret < 0) check
> for uvc_gpio_parse() to a (ret) check.

Not very nice, but easy to backport to stables. What about a follow-up
change like this:

index c93abe2367aa..8c67feca1688 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -2261,7 +2261,7 @@ static int uvc_probe(struct usb_interface *intf,
        ret = uvc_gpio_parse(dev);
        if (ret < 0) {
                uvc_dbg(dev, PROBE, "Unable to parse UVC GPIOs\n");
-               goto error_retcode;
+               goto error;
        }

        dev_info(&dev->udev->dev, "Found UVC %u.%02x device %s (%04x:%04x)\n",
@@ -2285,24 +2285,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)
                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. */
@@ -2334,8 +2342,6 @@ static int uvc_probe(struct usb_interface *intf,
        return 0;

 error:
-       ret = -ENODEV;
-error_retcode:
        uvc_unregister_video(dev);
        kref_put(&dev->ref, uvc_delete);
        return ret;


>
> >       uvc_unregister_video(dev);
> >       kref_put(&dev->ref, uvc_delete);
> > -     return -ENODEV;
> > +     return ret;
> >  }
> >
> >  static void uvc_disconnect(struct usb_interface *intf)
> >
> > ---
> > base-commit: c4b7779abc6633677e6edb79e2809f4f61fde157
> > change-id: 20250129-uvc-eprobedefer-b5ebb4db63cc
>
> --
> Regards,
>
> Laurent Pinchart

Let me know what do you think so I can send a v2 with the change
proposed by Doug.

Regards!

-- 
Ricardo Ribalda