Some notebooks have a button to disable the camera (not to be mistaken with the mechanical cover). This is a standard GPIO linked to the camera via the ACPI table.
4 years ago we added support for this button in UVC via the Privacy control. This has three issues: - If the camera has its own privacy control, it will be masked. - We need to power-up the camera to read the privacy control gpio. - Other drivers have not followed this approach and have used evdev.
We tried to fix the power-up issues implementing "granular power saving" but it has been more complicated than anticipated...
This patchset implements the Privacy GPIO as a evdev.
The first patch of this set is already in Laurent's tree... but I include it to get some CI coverage.
Signed-off-by: Ricardo Ribalda ribalda@chromium.org --- Changes in v4: - Remove gpio entity, it is not needed. - Use unit->gpio.irq in free_irq to make smatch happy. - Link to v3: https://lore.kernel.org/r/20241112-uvc-subdev-v3-0-0ea573d41a18@chromium.org
Changes in v3: - CodeStyle (Thanks Sakari) - Re-implement as input device - Make the code depend on UVC_INPUT_EVDEV - Link to v2: https://lore.kernel.org/r/20241108-uvc-subdev-v2-0-85d8a051a3d3@chromium.org
Changes in v2: - Rebase on top of https://patchwork.linuxtv.org/project/linux-media/patch/20241106-uvc-crashrm... - Create uvc_gpio_cleanup and uvc_gpio_deinit - Refactor quirk: do not disable irq - Change define number for MEDIA_ENT_F_GPIO - Link to v1: https://lore.kernel.org/r/20241031-uvc-subdev-v1-0-a68331cedd72@chromium.org
--- Ricardo Ribalda (7): media: uvcvideo: Fix crash during unbind if gpio unit is in use media: uvcvideo: Factor out gpio functions to its own file media: uvcvideo: Re-implement privacy GPIO as an input device Revert "media: uvcvideo: Allow entity-defined get_info and get_cur" media: uvcvideo: Introduce UVC_QUIRK_PRIVACY_DURING_STREAM media: uvcvideo: Make gpio_unit entity-less media: uvcvideo: Remove UVC_EXT_GPIO entity
drivers/media/usb/uvc/Kconfig | 2 +- drivers/media/usb/uvc/Makefile | 3 + drivers/media/usb/uvc/uvc_ctrl.c | 40 ++--------- drivers/media/usb/uvc/uvc_driver.c | 123 ++-------------------------------- drivers/media/usb/uvc/uvc_entity.c | 7 +- drivers/media/usb/uvc/uvc_gpio.c | 134 +++++++++++++++++++++++++++++++++++++ drivers/media/usb/uvc/uvc_status.c | 13 +++- drivers/media/usb/uvc/uvc_video.c | 4 ++ drivers/media/usb/uvc/uvcvideo.h | 43 +++++++----- 9 files changed, 195 insertions(+), 174 deletions(-) --- base-commit: 72ad4ff638047bbbdf3232178fea4bec1f429319 change-id: 20241030-uvc-subdev-89f4467a00b5
Best regards,
We used the wrong device for the device managed functions. We used the usb device, when we should be using the interface device.
If we unbind the driver from the usb interface, the cleanup functions are never called. In our case, the IRQ is never disabled.
If an IRQ is triggered, it will try to access memory sections that are already free, causing an OOPS.
We cannot use the function devm_request_threaded_irq here. The devm_* clean functions may be called after the main structure is released by uvc_delete.
Luckily this bug has small impact, as it is only affected by devices with gpio units and the user has to unbind the device, a disconnect will not trigger this error.
Cc: stable@vger.kernel.org Fixes: 2886477ff987 ("media: uvcvideo: Implement UVC_EXT_GPIO_UNIT") Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Ricardo Ribalda ribalda@chromium.org --- drivers/media/usb/uvc/uvc_driver.c | 28 +++++++++++++++++++++------- drivers/media/usb/uvc/uvcvideo.h | 1 + 2 files changed, 22 insertions(+), 7 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c index faf5386cb061..6d34c910a659 100644 --- a/drivers/media/usb/uvc/uvc_driver.c +++ b/drivers/media/usb/uvc/uvc_driver.c @@ -1295,14 +1295,14 @@ static int uvc_gpio_parse(struct uvc_device *dev) struct gpio_desc *gpio_privacy; int irq;
- gpio_privacy = devm_gpiod_get_optional(&dev->udev->dev, "privacy", + 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);
irq = gpiod_to_irq(gpio_privacy); if (irq < 0) - return dev_err_probe(&dev->udev->dev, irq, + return dev_err_probe(&dev->intf->dev, irq, "No IRQ for privacy GPIO\n");
unit = uvc_alloc_new_entity(dev, UVC_EXT_GPIO_UNIT, @@ -1329,15 +1329,27 @@ static int uvc_gpio_parse(struct uvc_device *dev) static int uvc_gpio_init_irq(struct uvc_device *dev) { struct uvc_entity *unit = dev->gpio_unit; + int ret;
if (!unit || unit->gpio.irq < 0) return 0;
- return devm_request_threaded_irq(&dev->udev->dev, unit->gpio.irq, NULL, - uvc_gpio_irq, - IRQF_ONESHOT | IRQF_TRIGGER_FALLING | - IRQF_TRIGGER_RISING, - "uvc_privacy_gpio", dev); + ret = request_threaded_irq(unit->gpio.irq, NULL, uvc_gpio_irq, + IRQF_ONESHOT | IRQF_TRIGGER_FALLING | + IRQF_TRIGGER_RISING, + "uvc_privacy_gpio", dev); + + unit->gpio.initialized = !ret; + + return ret; +} + +static void uvc_gpio_deinit(struct uvc_device *dev) +{ + if (!dev->gpio_unit || !dev->gpio_unit->gpio.initialized) + return; + + free_irq(dev->gpio_unit->gpio.irq, dev); }
/* ------------------------------------------------------------------------ @@ -1934,6 +1946,8 @@ static void uvc_unregister_video(struct uvc_device *dev) { struct uvc_streaming *stream;
+ uvc_gpio_deinit(dev); + list_for_each_entry(stream, &dev->streams, list) { /* Nothing to do here, continue. */ if (!video_is_registered(&stream->vdev)) diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h index 07f9921d83f2..965a789ed03e 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -234,6 +234,7 @@ struct uvc_entity { u8 *bmControls; struct gpio_desc *gpio_privacy; int irq; + bool initialized; } gpio; };
linux-stable-mirror@lists.linaro.org
-
Ricardo Ribalda