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

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

On Wed, Apr 23, 2025 at 06:50:10AM +0800, Ricardo Ribalda wrote:
> On Wed, 23 Apr 2025 at 02:06, Laurent Pinchart wrote:
> > 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@vger.kernel.org
> > > Fixes: 2886477ff987 ("media: uvcvideo: Implement UVC_EXT_GPIO_UNIT")
> > > Reviewed-by: Douglas Anderson <dianders@chromium.org>
> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.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.

Yes, I got that, my question was why you override the return value of
e.g. uvc_parse_control() or uvc_scan_device() with -ENODEV, but not for
e.g. uvc_gpio_parse() or v4l2_device_register(). There's no explanation
in the commit message regarding why they're treated differently.

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