Hi,
On 12/16/22 17:44, Laurent Pinchart wrote:
> 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.
Using it for other purposes it not something which I expect
e.g. distros to do OOTB, so normal users won't see the LED used
in another way. But it may be useful for tinkerers who do this
as a local modification, in which case they know the LED
behavior.
With that said I'm fine with just disabling the sysfs interface
once at probe / register time.
Regards,
Hans
>
>> 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.
>
