linux-kernel - Re: [PATCH v2 0/6] media: uvcvideo: Implement the Privacy GPIO as a subdevice

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20241109150420.359bd50f@foz.lan>
Date: Sat, 9 Nov 2024 15:04:20 +0100
From: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
To: Ricardo Ribalda <ribalda@...omium.org>
Cc: Laurent Pinchart <laurent.pinchart@...asonboard.com>, Mauro Carvalho
 Chehab <mchehab@...nel.org>, Sakari Ailus <sakari.ailus@...ux.intel.com>,
 linux-kernel@...r.kernel.org, linux-media@...r.kernel.org, Yunke Cao
 <yunkec@...omium.org>, Hans Verkuil <hverkuil@...all.nl>, Hans de Goede
 <hdegoede@...hat.com>
Subject: Re: [PATCH v2 0/6] media: uvcvideo: Implement the Privacy GPIO as a
 subdevice

Em Fri, 08 Nov 2024 20:25:44 +0000
Ricardo Ribalda <ribalda@...omium.org> escreveu:

> 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 two 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.
> 
> We tried to fix the power-up issues implementing "granular power
> saving" but it has been more complicated than anticipated....
> 
> Last year, we proposed a patchset to implement the privacy gpio as a
> subdevice https://lore.kernel.org/linux-media/20230111-uvc_privacy_subdev-v1-0-f859ac9a01e3@chromium.org/
> 
> I think it is a pretty clean solution and makes sense to use a
> subdevice for something that is a sub device of the camera :).
> 
> This is an attempt to continue with that approach.
> 
> Tested on gimble:
> gimble-rev3 ~ # v4l2-ctl --all -d /dev/v4l-subdev0

No matter if internally implemented as a subdevice or not,
UVC is not a MC-centric device[1]. 

It means that UVC can be compiled without media controller support,
and that its functionality shall be visible via /dev/video* nodes.

So, whatever internal implementation it is used, it shall not require
config MEDIA_CONTROLLER and the control shall be visible via
/dev/video*.

Moving privacy control out of /dev/video would mean that this will break 
support for it on existing applications, which is a big nack. Now, it would
be acceptable to have this visible via V4L2 and subdev APIs.

[1] https://www.kernel.org/doc/html/latest/userspace-api/media/glossary.html#term-MC-centric

Regards,
Mauro

> Driver Info:
>         Driver version   : 6.6.56
>         Capabilities     : 0x00000000
> Media Driver Info:
>         Driver name      : uvcvideo
>         Model            : HP 5M Camera: HP 5M Camera
>         Serial           : 0001
>         Bus info         : usb-0000:00:14.0-6
>         Media version    : 6.6.56
>         Hardware revision: 0x00009601 (38401)
>         Driver version   : 6.6.56
> Interface Info:
>         ID               : 0x0300001d
>         Type             : V4L Sub-Device
> Entity Info:
>         ID               : 0x00000013 (19)
>         Name             : GPIO
>         Function         : Unknown sub-device (00020006)
> 
> Camera Controls
> 
>                         privacy 0x009a0910 (bool)   : default=0 value=0 flags=read-only, volatile
> 
> gimble-rev3 ~ # media-ctl  -p
> Media controller API version 6.6.56
> 
> Media device information
> ------------------------
> driver          uvcvideo
> model           HP 5M Camera: HP 5M Camera
> serial          0001
> bus info        usb-0000:00:14.0-6
> hw revision     0x9601
> driver version  6.6.56
> 
> Device topology
> - entity 1: HP 5M Camera: HP 5M Camera (1 pad, 1 link)
>             type Node subtype V4L flags 1
>             device node name /dev/video0
>         pad0: Sink
>                 <- "Extension 8":1 [ENABLED,IMMUTABLE]
> 
> - entity 4: HP 5M Camera: HP 5M Camera (0 pad, 0 link)
>             type Node subtype V4L flags 0
>             device node name /dev/video1
> 
> - entity 8: Extension 8 (2 pads, 2 links, 0 routes)
>             type V4L2 subdev subtype Unknown flags 0
>         pad0: Sink
>                 <- "Extension 4":1 [ENABLED,IMMUTABLE]
>         pad1: Source
>                 -> "HP 5M Camera: HP 5M Camera":0 [ENABLED,IMMUTABLE]  
> 
> - entity 11: Extension 4 (2 pads, 2 links, 0 routes)
>              type V4L2 subdev subtype Unknown flags 0
>         pad0: Sink
>                 <- "Processing 2":1 [ENABLED,IMMUTABLE]
>         pad1: Source
>                 -> "Extension 8":0 [ENABLED,IMMUTABLE]  
> 
> - entity 14: Processing 2 (2 pads, 2 links, 0 routes)
>              type V4L2 subdev subtype Unknown flags 0
>         pad0: Sink
>                 <- "Camera 1":0 [ENABLED,IMMUTABLE]
>         pad1: Source
>                 -> "Extension 4":0 [ENABLED,IMMUTABLE]  
> 
> - entity 17: Camera 1 (1 pad, 1 link, 0 routes)
>              type V4L2 subdev subtype Sensor flags 0
>         pad0: Source
>                 -> "Processing 2":0 [ENABLED,IMMUTABLE]  
> 
> - entity 19: GPIO (0 pad, 0 link, 0 routes)
>              type V4L2 subdev subtype Decoder flags 0
>              device node name /dev/v4l-subdev0
> 
> Signed-off-by: Ricardo Ribalda <ribalda@...omium.org>
> ---
> Changes in v2:
> - Rebase on top of https://patchwork.linuxtv.org/project/linux-media/patch/20241106-uvc-crashrmmod-v6-1-fbf9781c6e83@chromium.org/
> - 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 (5):
>       media: uvcvideo: Factor out gpio functions to its own file
>       Revert "media: uvcvideo: Allow entity-defined get_info and get_cur"
>       media: uvcvideo: Create ancillary link for GPIO subdevice
>       media: v4l2-core: Add new MEDIA_ENT_F_GPIO
>       media: uvcvideo: Use MEDIA_ENT_F_GPIO for the GPIO entity
> 
> Yunke Cao (1):
>       media: uvcvideo: Re-implement privacy GPIO as a separate subdevice
> 
>  .../userspace-api/media/mediactl/media-types.rst   |   4 +
>  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                 |  20 ++-
>  drivers/media/usb/uvc/uvc_gpio.c                   | 187 +++++++++++++++++++++
>  drivers/media/usb/uvc/uvc_video.c                  |   4 +
>  drivers/media/usb/uvc/uvcvideo.h                   |  34 ++--
>  drivers/media/v4l2-core/v4l2-async.c               |   3 +-
>  include/uapi/linux/media.h                         |   1 +
>  10 files changed, 252 insertions(+), 167 deletions(-)
> ---
> base-commit: 4353256f5487e0c5c47e8ff764bf4f9e679fb525
> change-id: 20241030-uvc-subdev-89f4467a00b5
> 
> Best regards,



Thanks,
Mauro