Re: [PATCH v3 06/11] v4l: subdev: Make the v4l2-subdev core code enable/disable the privacy LED if present

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 12/16/22 14:56, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> 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@linux.intel.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  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.

> 
>> +
>> +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






> 
>> +
>> +	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 ...

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() ?



> 
>> +	}
>> +
>> +	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,

Hans