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: 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
Best regards,
Hi,
On Wed, Jan 29, 2025 at 4:40 AM Ricardo Ribalda ribalda@chromium.org 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;
FWIW, since you're specifically considering the -EPROBE_DEFER case, it's probably worthwhile to make sure that dev_err_probe() is called. That eventually calls device_set_deferred_probe_reason() which can be helpful for tracking down problems.
It looks like uvc_gpio_parse() already calls this if gpiod_to_irq() returns an error code probably you also want to make sure that dev_err_probe() also gets called in the case where devm_gpiod_get_optional() returns -EPROBE_DEFER. In that case, potentially one could also get rid of the uvc_dbg() above.
In any case, IMO even without those changes your patch is still worth landing. ...and maybe my suggestion should be in a separate follow-on patch anyway. Thus:
Reviewed-by: Douglas Anderson dianders@chromium.org
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@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.
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
On Sun, 23 Feb 2025 at 15:36, Laurent Pinchart laurent.pinchart@ideasonboard.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@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:
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!
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!
linux-stable-mirror@lists.linaro.org
-
Doug Anderson -
Laurent Pinchart -
Ricardo Ribalda