Re: [PATCH] media: uvcvideo: Fix deferred probing error

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Sun, Feb 23, 2025 at 08:32:24PM +0100, Ricardo Ribalda wrote:
> On Sun, 23 Feb 2025 at 15:36, Laurent Pinchart wrote:
> > 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@vger.kernel.org
> > > Fixes: 2886477ff987 ("media: uvcvideo: Implement UVC_EXT_GPIO_UNIT")
> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.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:

How about the nicer fix for mainline, and a simpler to backport one for
stable ? The main focus should be on mainline, not backports.

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

-- 
Regards,

Laurent Pinchart