Hi Hans,
On Fri, Dec 16, 2022 at 04:45:29PM +0100, Hans de Goede wrote:
> On 12/16/22 14:56, Laurent Pinchart wrote:
> > On Fri, Dec 16, 2022 at 12:30:08PM +0100, Hans de Goede wrote:
> >> Extend the call_s_stream() wrapper to enable/disable sensor privacy LEDs
> >> for sensors with a privacy LED, rather then having to duplicate this code
> >> in all the sensor drivers.
> >>
> >> Suggested-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> >> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> >> ---
> >> drivers/media/v4l2-core/v4l2-subdev.c | 40 +++++++++++++++++++++++++++
> >> include/media/v4l2-subdev.h | 3 ++
> >> 2 files changed, 43 insertions(+)
> >>
> >> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> >> index 4988a25bd8f4..7344f6cd58b7 100644
> >> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> >> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> >> @@ -318,10 +318,44 @@ static int call_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad,
> >> sd->ops->pad->get_mbus_config(sd, pad, config);
> >> }
> >>
> >> +#if IS_REACHABLE(CONFIG_LEDS_CLASS)
> >> +#include <linux/leds.h>
> >
> > Can this be moved to the top of the file ? It doesn't have to be
> > conditionally compiled there.
>
> You mean just the #include right? Ack to that.
Yes, that's what I meant.
> >> +
> >> +static void call_s_stream_update_pled(struct v4l2_subdev *sd, int enable)
> >> +{
> >> + if (!sd->dev)
> >> + return;
> >> +
> >> + /* Try to get privacy-led once, at first s_stream() */
> >> + if (!sd->privacy_led)
> >> + sd->privacy_led = led_get(sd->dev, "privacy-led");
> >
> > I'm not sure I like this much. If the LED provider isn't available
> > (yet), the LED will stay off. That's a privacy concern.
>
> At first I tried to put the led_get() inside v4l2_async_register_subdev_sensor(),
> which could then return an error like -EPROBE_DEFER if the led_get()
> fails (and nicely limits the led_get() to sensors).
>
> The problem which I hit is that v4l2-fwnode.c is build according to
> CONFIG_V4L2_FWNODE where as v4l2-subdev.c is build according to
> CONFIG_VIDEO_DEV
>
> And e.g. CONFIG_VIDEO_DEV could be builtin while CONFIG_V4L2_FWNODE
> could be a module and then having the #if IS_REACHABLE(CONFIG_LEDS_CLASS)
> spread over the 2 could result in different answers in the different
> files ...
>
> One solution here could be to change CONFIG_V4L2_FWNODE and V4L2_ASYNC
> to bools and link the (quite small) objects for these 2 into videodev.ko:
>
> videodev-$(CONFIG_V4L2_FWNODE) += v4l2-fwnode.o
> videodev-$(CONFIG_V4L2_ASYNC) += v4l2-async.o
There's a long overdue simplification of Kconfig symbols in the
subsystem. Another option would be to compile both in a single module,
as they're often used together. I'll let Sakari chime in, I don't have a
strong preference.
> >> +
> >> + if (IS_ERR(sd->privacy_led))
> >> + return;
> >> +
> >> + mutex_lock(&sd->privacy_led->led_access);
> >> +
> >> + if (enable) {
> >> + led_sysfs_disable(sd->privacy_led);
> >> + led_trigger_remove(sd->privacy_led);
> >> + led_set_brightness(sd->privacy_led, sd->privacy_led->max_brightness);
> >> + } else {
> >> + led_set_brightness(sd->privacy_led, 0);
> >> + led_sysfs_enable(sd->privacy_led);
> >
> > I don't think you should reenable control through sysfs here. I don't
> > really see a use case, and you've removed the trigger anyway, so the
> > behaviour would be quite inconsistent.
>
> Hmm, I thought this was a good compromise, this way the LED
> can be used for other purposes when the sensor is off if users
> want to.
>
> Right if users want to use a trigger then they would need
> to re-attach the trigger after using the camera.
>
> But this way at least they can use the LED for other purposes
> which since many users don't use their webcam that often
> might be interesting for some users ...
If the privacy LED starts being used for other purposes, users may get
used to seeing it on at random times, which defeats the point of the
privacy LED in the first place.
> And this is consistent with how flash LEDs are handled.
>
> > Also, I think it would be better if the LED device was marked as "no
> > sysfs" when it is registered.
>
> If we decide to permanently disallow userspace access then
> yes this is an option. Or maybe better (to keep the LED
> drivers generic), do the disable directly after the led_get() ?
I suppose the small race condition wouldn't be a big issue, but if we
decide that the privacy LED should really not be used for user purposes,
then I'd still rather disable userspace access when registering the LED.
> >> + }
> >> +
> >> + mutex_unlock(&sd->privacy_led->led_access);
> >> +}
> >> +#else
> >> +static void call_s_stream_update_pled(struct v4l2_subdev *sd, int enable) {}
> >> +#endif
> >> +
> >> static int call_s_stream(struct v4l2_subdev *sd, int enable)
> >> {
> >> int ret;
> >>
> >> + call_s_stream_update_pled(sd, enable);
> >> +
> >> ret = sd->ops->video->s_stream(sd, enable);
> >>
> >> if (!enable && ret < 0) {
> >
> > You need to turn the LED off when enabling if .s_stream() fails.
>
> Ack.
--
Regards,
Laurent Pinchart
