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: <20241126172205.GK5461@pendragon.ideasonboard.com>
Date: Tue, 26 Nov 2024 19:22:05 +0200
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Hans de Goede <hdegoede@...hat.com>
Cc: Ricardo Ribalda <ribalda@...omium.org>,
	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>
Subject: Re: [PATCH v2 0/6] media: uvcvideo: Implement the Privacy GPIO as a
 subdevice

On Mon, Nov 25, 2024 at 02:29:51PM +0100, Hans de Goede wrote:
> On 25-Nov-24 1:49 PM, Laurent Pinchart wrote:
> > On Mon, Nov 25, 2024 at 01:25:41PM +0100, Hans de Goede wrote:
> 
> <snip>
> 
> >> I see 2 ways of doing that:
> >>
> >> 1. Use pm_runtime_set_autosuspend_delay() with a delay of say 1 second
> >> and then on set_ctrl do a pm_runtime_get_sync() +
> >> pm_runtime_put_autosuspend() giving the camera 1 second to finish
> >> applying the async ctrl (which might not be enough for e.g homing) +
> >> also avoid doing suspend + resume all the time if multiple ctrls are send
> >>
> >> 2. Instead of immediately powering on the camera on /dev/video# open
> >> track per fh if the camera has been powered on and then on the first
> >> set-ctrl, or the first other IOCTL like try/set-fmt which requires
> >> the camera to be powered on power it up and then keep it on until
> >> the fh is closed, since apps hitting these paths are likely to do
> >> more stuff which requires the camera to be powered on.
> > 
> > A mode of operation where a userspace action causes a state change and
> > the only way to change back to the previous state is to close the device
> > often leads to problems. I'd rather not do this unless we have to
> > completely rule out all other options.
> 
> But we already have that today. We already do the usb_autopm_get_interface()
> as soon as /dev/video# gets opened and the only way to undo it is to close
> /dev/video#.

Yes, but close() is the counterpart of open(). Breaking the symmetry is
what bothers me, it's not nice from an application point of view. It
wouldn't force instance have solved the issue of keeping the device
powered when used through libcamera (or anything else that keeps the
device node open after using the camera, or just after querying some of
its capabilities through TRY_FMT).

> What I'm suggesting is to no longer do the usb_autopm_get_interface()
> on all opens, but only on some.
> 
> Where "some" are the ones where we come to the conclusion that we actually
> need to power-up the USB-bus / interface because we want to talk to
> the device.
> 
> IOW delay the usb_autopm_get_interface() until the first action which
> actually requires it.
> 
> This should be a very minimal change from the pov of USB interactions
> with the actual device, so a small change of regressions while at
> least not powering on the device during udev discovery.
> 
> I guess one could argue that the cases where this is a win are so
> small that this is not worth it.

Is there a significant enough gain through this approach, and are the
other approaches impractical ? If so we can consider this.

-- 
Regards,

Laurent Pinchart