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

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

[Ricardo Ribalda]

uvc_gpio_parse() can return -EPROBE_DEFER when the GPIOs it depends on
have not yet been probed.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
Changes in v3:
- Remove duplicated error messages in uvc_probe()
- Link to v2: https://lore.kernel.org/r/20250303-uvc-eprobedefer-v2-0-be7c987cc3ca@chromium.org

Changes in v2:
- Add follow-up patch for using dev_err_probe
- Avoid error_retcode style
- Link to v1: https://lore.kernel.org/r/20250129-uvc-eprobedefer-v1-1-643b2603c0d2@chromium.org

---
Ricardo Ribalda (2):
      media: uvcvideo: Fix deferred probing error
      media: uvcvideo: Use dev_err_probe for devm_gpiod_get_optional

 drivers/media/usb/uvc/uvc_driver.c | 38 ++++++++++++++++++++++++++------------
 1 file changed, 26 insertions(+), 12 deletions(-)
---
base-commit: f4b211714bcc70effa60c34d9fa613d182e3ef1e
change-id: 20250129-uvc-eprobedefer-b5ebb4db63cc

Best regards,
-- 
Ricardo Ribalda <ribalda@chromium.org>

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

[Ricardo Ribalda]

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

-- 
2.49.0.rc0.332.g42c0ae87b1-goog

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

[Laurent Pinchart]

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

>  		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

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

[Ricardo Ribalda]

On Wed, 23 Apr 2025 at 02:06, Laurent Pinchart
<laurent.pinchart@ideasonboard.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@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.

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

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

[Laurent Pinchart]

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

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

[Ricardo Ribalda]

On Wed, 23 Apr 2025 at 07:05, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> 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.

Because it is less risky that way. There are plenty of examples where
the framework functions return code is passed to probe().

The uvc_* functions might or might not work this way. When I do that
assessment for every function I can post a different patch. I thought
that this approach was safer, especially if we are cc-ing stable.

A note in the commit message would have been a nice thing to have I agree :).


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

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

[Hans de Goede]

Hi Ricardo,

On 23-Apr-25 01:18, Ricardo Ribalda wrote:
> On Wed, 23 Apr 2025 at 07:05, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>>
>> 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.
> 
> Because it is less risky that way. There are plenty of examples where
> the framework functions return code is passed to probe().
> 
> The uvc_* functions might or might not work this way. When I do that
> assessment for every function I can post a different patch. I thought
> that this approach was safer, especially if we are cc-ing stable.
> 
> A note in the commit message would have been a nice thing to have I agree :).

I agree with Laurent that just properly propagating the error code of
all functions, without overriding the return value with another -EXXXX code
in some places seems a better and cleaner way to handle this.

In the end the return value of uvc_probe() does not matter that much,
the only difference is that for errors other then -ENODEV the driver-core
will print an extra error message. But we should not fail to probe anyways.

If we get bug reports about this we can revisit, but for simplicity and
consistency reasons I would prefer to just always return the error of
the called function as is.

Regards,

Hans




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

[PATCH v3 2/2] media: uvcvideo: Use dev_err_probe for devm_gpiod_get_optional

[Ricardo Ribalda]

Use the dev_err_probe() helper for devm_gpiod_get_optional(), like we do
with gpiod_to_irq()

That eventually calls device_set_deferred_probe_reason() which can be
helpful for tracking down problems.

Now that all the error paths in uvc_gpio_parse have dev_err_probe, we
can remove the error message in uvc_probe.

Suggested-by: Doug Anderson <dianders@chromium.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_driver.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index e966bdb9239f345fd157588ebdad2b3ebe45168d..d8e51c3db7575bebe7bb700b53b50ae02d355d8e 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1297,8 +1297,13 @@ static int uvc_gpio_parse(struct uvc_device *dev)
 
 	gpio_privacy = devm_gpiod_get_optional(&dev->intf->dev, "privacy",
 					       GPIOD_IN);
-	if (IS_ERR_OR_NULL(gpio_privacy))
-		return PTR_ERR_OR_ZERO(gpio_privacy);
+	if (!gpio_privacy)
+		return 0;
+
+	if (IS_ERR(gpio_privacy))
+		return dev_err_probe(&dev->intf->dev,
+				     PTR_ERR(gpio_privacy),
+				     "Can't get privacy GPIO\n");
 
 	irq = gpiod_to_irq(gpio_privacy);
 	if (irq < 0)
@@ -2240,10 +2245,8 @@ static int uvc_probe(struct usb_interface *intf,
 
 	/* Parse the associated GPIOs. */
 	ret = uvc_gpio_parse(dev);
-	if (ret < 0) {
-		uvc_dbg(dev, PROBE, "Unable to parse UVC GPIOs\n");
+	if (ret < 0)
 		goto error;
-	}
 
 	dev_info(&dev->udev->dev, "Found UVC %u.%02x device %s (%04x:%04x)\n",
 		 dev->uvc_version >> 8, dev->uvc_version & 0xff,

-- 
2.49.0.rc0.332.g42c0ae87b1-goog

Re: [PATCH v3 2/2] media: uvcvideo: Use dev_err_probe for devm_gpiod_get_optional

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Thu, Mar 13, 2025 at 12:20:40PM +0000, Ricardo Ribalda wrote:
> Use the dev_err_probe() helper for devm_gpiod_get_optional(), like we do
> with gpiod_to_irq()
> 
> That eventually calls device_set_deferred_probe_reason() which can be
> helpful for tracking down problems.
> 
> Now that all the error paths in uvc_gpio_parse have dev_err_probe, we
> can remove the error message in uvc_probe.
> 
> Suggested-by: Doug Anderson <dianders@chromium.org>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/usb/uvc/uvc_driver.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index e966bdb9239f345fd157588ebdad2b3ebe45168d..d8e51c3db7575bebe7bb700b53b50ae02d355d8e 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1297,8 +1297,13 @@ static int uvc_gpio_parse(struct uvc_device *dev)
>  
>  	gpio_privacy = devm_gpiod_get_optional(&dev->intf->dev, "privacy",
>  					       GPIOD_IN);
> -	if (IS_ERR_OR_NULL(gpio_privacy))
> -		return PTR_ERR_OR_ZERO(gpio_privacy);
> +	if (!gpio_privacy)
> +		return 0;
> +
> +	if (IS_ERR(gpio_privacy))
> +		return dev_err_probe(&dev->intf->dev,
> +				     PTR_ERR(gpio_privacy),
> +				     "Can't get privacy GPIO\n");
>  
>  	irq = gpiod_to_irq(gpio_privacy);
>  	if (irq < 0)
> @@ -2240,10 +2245,8 @@ static int uvc_probe(struct usb_interface *intf,
>  
>  	/* Parse the associated GPIOs. */
>  	ret = uvc_gpio_parse(dev);
> -	if (ret < 0) {
> -		uvc_dbg(dev, PROBE, "Unable to parse UVC GPIOs\n");
> +	if (ret < 0)
>  		goto error;
> -	}
>  
>  	dev_info(&dev->udev->dev, "Found UVC %u.%02x device %s (%04x:%04x)\n",
>  		 dev->uvc_version >> 8, dev->uvc_version & 0xff,

-- 
Regards,

Laurent Pinchart

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

[Hans de Goede]

Hi Ricardo,

On 13-Mar-25 13:20, Ricardo Ribalda wrote:
> uvc_gpio_parse() can return -EPROBE_DEFER when the GPIOs it depends on
> have not yet been probed.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

Thank you for your continued work on the UVC driver,
I've have merged this series into:

https://gitlab.freedesktop.org/linux-media/users/uvc/-/commits/next/

now.

Regards,

Hans




> ---
> Changes in v3:
> - Remove duplicated error messages in uvc_probe()
> - Link to v2: https://lore.kernel.org/r/20250303-uvc-eprobedefer-v2-0-be7c987cc3ca@chromium.org
> 
> Changes in v2:
> - Add follow-up patch for using dev_err_probe
> - Avoid error_retcode style
> - Link to v1: https://lore.kernel.org/r/20250129-uvc-eprobedefer-v1-1-643b2603c0d2@chromium.org
> 
> ---
> Ricardo Ribalda (2):
>       media: uvcvideo: Fix deferred probing error
>       media: uvcvideo: Use dev_err_probe for devm_gpiod_get_optional
> 
>  drivers/media/usb/uvc/uvc_driver.c | 38 ++++++++++++++++++++++++++------------
>  1 file changed, 26 insertions(+), 12 deletions(-)
> ---
> base-commit: f4b211714bcc70effa60c34d9fa613d182e3ef1e
> change-id: 20250129-uvc-eprobedefer-b5ebb4db63cc
> 
> Best regards,