| Message ID | 20240410100301.658824-1-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] media: v4l: Don't turn on privacy LED if streamon fails | expand |
Hi Sakari, Thank you for fixing this. On 4/10/24 12:03 PM, Sakari Ailus wrote: > Turn on the privacy LED only if streamon succeeds. This can be done after > enabling streaming on the sensor. > > Fixes: b6e10ff6c23d ("media: v4l2-core: Make the v4l2-core code enable/disable the privacy LED if present") > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/v4l2-core/v4l2-subdev.c | 19 ++++++++++--------- > 1 file changed, 10 insertions(+), 9 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index 4c6198c48dd6..acb7c3003ab6 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -412,15 +412,6 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable) > if (WARN_ON(!!sd->enabled_streams == !!enable)) > return 0; > > -#if IS_REACHABLE(CONFIG_LEDS_CLASS) > - if (!IS_ERR_OR_NULL(sd->privacy_led)) { > - if (enable) > - led_set_brightness(sd->privacy_led, > - sd->privacy_led->max_brightness); > - else > - led_set_brightness(sd->privacy_led, 0); > - } > -#endif > ret = sd->ops->video->s_stream(sd, enable); > > if (!enable && ret < 0) { > @@ -431,6 +422,16 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable) > if (!ret) > sd->enabled_streams = enable ? BIT(0) : 0; > > +#if IS_REACHABLE(CONFIG_LEDS_CLASS) > + if (!IS_ERR_OR_NULL(sd->privacy_led) && !(enable && ret < 0)) { There already is a: if (!enable && ret < 0) ret = 0; block above to ignore stream-off errors, so you can just test for: if (!ret && !IS_ERR_OR_NULL(sd->privacy_led)) { ... } And then you can move the whole block to inside the existing: if (!ret) sd->enabled_streams = enable ? BIT(0) : 0; block, so you get something like this: if (!ret) { sd->enabled_streams = enable ? BIT(0) : 0; #if IS_REACHABLE(CONFIG_LEDS_CLASS) if (!IS_ERR_OR_NULL(sd->privacy_led)) { if (enable) led_set_brightness(sd->privacy_led, sd->privacy_led->max_brightness); else led_set_brightness(sd->privacy_led, 0); } #endif } Which then cleans up nicely by Tomi's patch introducing the privacy LED helper functions, to something like this: if (!ret { if (enable) { sd->enabled_streams = BIT(0); v4l2_subdev_enable_privacy_led(sd); } else { sd->enabled_streams = 0; v4l2_subdev_disable_privacy_led(sd); } } Regards, Hans
Hi Hans, Thanks for the review. On Wed, Apr 10, 2024 at 12:12:33PM +0200, Hans de Goede wrote: > Hi Sakari, > > Thank you for fixing this. > > On 4/10/24 12:03 PM, Sakari Ailus wrote: > > Turn on the privacy LED only if streamon succeeds. This can be done after > > enabling streaming on the sensor. > > > > Fixes: b6e10ff6c23d ("media: v4l2-core: Make the v4l2-core code enable/disable the privacy LED if present") > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > drivers/media/v4l2-core/v4l2-subdev.c | 19 ++++++++++--------- > > 1 file changed, 10 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > > index 4c6198c48dd6..acb7c3003ab6 100644 > > --- a/drivers/media/v4l2-core/v4l2-subdev.c > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > > @@ -412,15 +412,6 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable) > > if (WARN_ON(!!sd->enabled_streams == !!enable)) > > return 0; > > > > -#if IS_REACHABLE(CONFIG_LEDS_CLASS) > > - if (!IS_ERR_OR_NULL(sd->privacy_led)) { > > - if (enable) > > - led_set_brightness(sd->privacy_led, > > - sd->privacy_led->max_brightness); > > - else > > - led_set_brightness(sd->privacy_led, 0); > > - } > > -#endif > > ret = sd->ops->video->s_stream(sd, enable); > > > > if (!enable && ret < 0) { > > @@ -431,6 +422,16 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable) > > if (!ret) > > sd->enabled_streams = enable ? BIT(0) : 0; > > > > +#if IS_REACHABLE(CONFIG_LEDS_CLASS) > > + if (!IS_ERR_OR_NULL(sd->privacy_led) && !(enable && ret < 0)) { > > There already is a: > > if (!enable && ret < 0) > ret = 0; > > block above to ignore stream-off errors, so you can just test for: > > if (!ret && !IS_ERR_OR_NULL(sd->privacy_led)) { > ... > } > > And then you can move the whole block to inside the existing: > > if (!ret) > sd->enabled_streams = enable ? BIT(0) : 0; > > block, so you get something like this: > > if (!ret) { > sd->enabled_streams = enable ? BIT(0) : 0; > #if IS_REACHABLE(CONFIG_LEDS_CLASS) > if (!IS_ERR_OR_NULL(sd->privacy_led)) { > if (enable) > led_set_brightness(sd->privacy_led, > sd->privacy_led->max_brightness); > else > led_set_brightness(sd->privacy_led, 0); > } > #endif > } Seems reasonable, I'll send v2. > > Which then cleans up nicely by Tomi's patch > introducing the privacy LED helper functions, > to something like this: > > if (!ret { > if (enable) { > sd->enabled_streams = BIT(0); > v4l2_subdev_enable_privacy_led(sd); > } else { > sd->enabled_streams = 0; > v4l2_subdev_disable_privacy_led(sd); > } > } >
On Wed, Apr 10, 2024 at 11:31:48AM +0000, Sakari Ailus wrote: > Hi Hans, > > Thanks for the review. > > On Wed, Apr 10, 2024 at 12:12:33PM +0200, Hans de Goede wrote: > > Hi Sakari, > > > > Thank you for fixing this. > > > > On 4/10/24 12:03 PM, Sakari Ailus wrote: > > > Turn on the privacy LED only if streamon succeeds. This can be done after > > > enabling streaming on the sensor. > > > > > > Fixes: b6e10ff6c23d ("media: v4l2-core: Make the v4l2-core code enable/disable the privacy LED if present") > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > --- > > > drivers/media/v4l2-core/v4l2-subdev.c | 19 ++++++++++--------- > > > 1 file changed, 10 insertions(+), 9 deletions(-) > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > > > index 4c6198c48dd6..acb7c3003ab6 100644 > > > --- a/drivers/media/v4l2-core/v4l2-subdev.c > > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > > > @@ -412,15 +412,6 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable) > > > if (WARN_ON(!!sd->enabled_streams == !!enable)) > > > return 0; > > > > > > -#if IS_REACHABLE(CONFIG_LEDS_CLASS) > > > - if (!IS_ERR_OR_NULL(sd->privacy_led)) { > > > - if (enable) > > > - led_set_brightness(sd->privacy_led, > > > - sd->privacy_led->max_brightness); > > > - else > > > - led_set_brightness(sd->privacy_led, 0); > > > - } > > > -#endif > > > ret = sd->ops->video->s_stream(sd, enable); > > > > > > if (!enable && ret < 0) { > > > @@ -431,6 +422,16 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable) > > > if (!ret) > > > sd->enabled_streams = enable ? BIT(0) : 0; > > > > > > +#if IS_REACHABLE(CONFIG_LEDS_CLASS) > > > + if (!IS_ERR_OR_NULL(sd->privacy_led) && !(enable && ret < 0)) { > > > > There already is a: > > > > if (!enable && ret < 0) > > ret = 0; > > > > block above to ignore stream-off errors, so you can just test for: > > > > if (!ret && !IS_ERR_OR_NULL(sd->privacy_led)) { > > ... > > } > > > > And then you can move the whole block to inside the existing: > > > > if (!ret) > > sd->enabled_streams = enable ? BIT(0) : 0; > > > > block, so you get something like this: > > > > if (!ret) { > > sd->enabled_streams = enable ? BIT(0) : 0; > > #if IS_REACHABLE(CONFIG_LEDS_CLASS) > > if (!IS_ERR_OR_NULL(sd->privacy_led)) { > > if (enable) > > led_set_brightness(sd->privacy_led, > > sd->privacy_led->max_brightness); > > else > > led_set_brightness(sd->privacy_led, 0); > > } > > #endif > > } > > Seems reasonable, I'll send v2. I was a bit too fast to hit y. ret will be non-zero if streamon fails (enable non-zero), in which case the LED needs to be turned off. So you can't have !ret condition for all LED control. > > > > > Which then cleans up nicely by Tomi's patch > > introducing the privacy LED helper functions, > > to something like this: > > > > if (!ret { > > if (enable) { > > sd->enabled_streams = BIT(0); > > v4l2_subdev_enable_privacy_led(sd); > > } else { > > sd->enabled_streams = 0; > > v4l2_subdev_disable_privacy_led(sd); > > } > > } > > >
Hi, On 4/10/24 1:35 PM, Sakari Ailus wrote: > On Wed, Apr 10, 2024 at 11:31:48AM +0000, Sakari Ailus wrote: >> Hi Hans, >> >> Thanks for the review. >> >> On Wed, Apr 10, 2024 at 12:12:33PM +0200, Hans de Goede wrote: >>> Hi Sakari, >>> >>> Thank you for fixing this. >>> >>> On 4/10/24 12:03 PM, Sakari Ailus wrote: >>>> Turn on the privacy LED only if streamon succeeds. This can be done after >>>> enabling streaming on the sensor. >>>> >>>> Fixes: b6e10ff6c23d ("media: v4l2-core: Make the v4l2-core code enable/disable the privacy LED if present") >>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> >>>> --- >>>> drivers/media/v4l2-core/v4l2-subdev.c | 19 ++++++++++--------- >>>> 1 file changed, 10 insertions(+), 9 deletions(-) >>>> >>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c >>>> index 4c6198c48dd6..acb7c3003ab6 100644 >>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c >>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c >>>> @@ -412,15 +412,6 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable) >>>> if (WARN_ON(!!sd->enabled_streams == !!enable)) >>>> return 0; >>>> >>>> -#if IS_REACHABLE(CONFIG_LEDS_CLASS) >>>> - if (!IS_ERR_OR_NULL(sd->privacy_led)) { >>>> - if (enable) >>>> - led_set_brightness(sd->privacy_led, >>>> - sd->privacy_led->max_brightness); >>>> - else >>>> - led_set_brightness(sd->privacy_led, 0); >>>> - } >>>> -#endif >>>> ret = sd->ops->video->s_stream(sd, enable); >>>> >>>> if (!enable && ret < 0) { >>>> @@ -431,6 +422,16 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable) >>>> if (!ret) >>>> sd->enabled_streams = enable ? BIT(0) : 0; >>>> >>>> +#if IS_REACHABLE(CONFIG_LEDS_CLASS) >>>> + if (!IS_ERR_OR_NULL(sd->privacy_led) && !(enable && ret < 0)) { >>> >>> There already is a: >>> >>> if (!enable && ret < 0) >>> ret = 0; >>> >>> block above to ignore stream-off errors, so you can just test for: >>> >>> if (!ret && !IS_ERR_OR_NULL(sd->privacy_led)) { >>> ... >>> } >>> >>> And then you can move the whole block to inside the existing: >>> >>> if (!ret) >>> sd->enabled_streams = enable ? BIT(0) : 0; >>> >>> block, so you get something like this: >>> >>> if (!ret) { >>> sd->enabled_streams = enable ? BIT(0) : 0; >>> #if IS_REACHABLE(CONFIG_LEDS_CLASS) >>> if (!IS_ERR_OR_NULL(sd->privacy_led)) { >>> if (enable) >>> led_set_brightness(sd->privacy_led, >>> sd->privacy_led->max_brightness); >>> else >>> led_set_brightness(sd->privacy_led, 0); >>> } >>> #endif >>> } >> >> Seems reasonable, I'll send v2. > > I was a bit too fast to hit y. ret will be non-zero if streamon fails > (enable non-zero), in which case the LED needs to be turned off. So you > can't have !ret condition for all LED control. But if stream-on fails then AFAICT the LED was never turned on in the first place. stream on / off calls must always be balanced, right ? IOW stream-on is not supposed to get called if the device is already streaming and if the device is not already streaming then the LED will already be off. Regards, Hans > >> >>> >>> Which then cleans up nicely by Tomi's patch >>> introducing the privacy LED helper functions, >>> to something like this: >>> >>> if (!ret { >>> if (enable) { >>> sd->enabled_streams = BIT(0); >>> v4l2_subdev_enable_privacy_led(sd); >>> } else { >>> sd->enabled_streams = 0; >>> v4l2_subdev_disable_privacy_led(sd); >>> } >>> } >>> >> >
On Wed, Apr 10, 2024 at 01:37:13PM +0200, Hans de Goede wrote: > Hi, > > On 4/10/24 1:35 PM, Sakari Ailus wrote: > > On Wed, Apr 10, 2024 at 11:31:48AM +0000, Sakari Ailus wrote: > >> Hi Hans, > >> > >> Thanks for the review. > >> > >> On Wed, Apr 10, 2024 at 12:12:33PM +0200, Hans de Goede wrote: > >>> Hi Sakari, > >>> > >>> Thank you for fixing this. > >>> > >>> On 4/10/24 12:03 PM, Sakari Ailus wrote: > >>>> Turn on the privacy LED only if streamon succeeds. This can be done after > >>>> enabling streaming on the sensor. > >>>> > >>>> Fixes: b6e10ff6c23d ("media: v4l2-core: Make the v4l2-core code enable/disable the privacy LED if present") > >>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > >>>> --- > >>>> drivers/media/v4l2-core/v4l2-subdev.c | 19 ++++++++++--------- > >>>> 1 file changed, 10 insertions(+), 9 deletions(-) > >>>> > >>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > >>>> index 4c6198c48dd6..acb7c3003ab6 100644 > >>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c > >>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c > >>>> @@ -412,15 +412,6 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable) > >>>> if (WARN_ON(!!sd->enabled_streams == !!enable)) > >>>> return 0; > >>>> > >>>> -#if IS_REACHABLE(CONFIG_LEDS_CLASS) > >>>> - if (!IS_ERR_OR_NULL(sd->privacy_led)) { > >>>> - if (enable) > >>>> - led_set_brightness(sd->privacy_led, > >>>> - sd->privacy_led->max_brightness); > >>>> - else > >>>> - led_set_brightness(sd->privacy_led, 0); > >>>> - } > >>>> -#endif > >>>> ret = sd->ops->video->s_stream(sd, enable); > >>>> > >>>> if (!enable && ret < 0) { > >>>> @@ -431,6 +422,16 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable) > >>>> if (!ret) > >>>> sd->enabled_streams = enable ? BIT(0) : 0; > >>>> > >>>> +#if IS_REACHABLE(CONFIG_LEDS_CLASS) > >>>> + if (!IS_ERR_OR_NULL(sd->privacy_led) && !(enable && ret < 0)) { > >>> > >>> There already is a: > >>> > >>> if (!enable && ret < 0) > >>> ret = 0; > >>> > >>> block above to ignore stream-off errors, so you can just test for: > >>> > >>> if (!ret && !IS_ERR_OR_NULL(sd->privacy_led)) { > >>> ... > >>> } > >>> > >>> And then you can move the whole block to inside the existing: > >>> > >>> if (!ret) > >>> sd->enabled_streams = enable ? BIT(0) : 0; > >>> > >>> block, so you get something like this: > >>> > >>> if (!ret) { > >>> sd->enabled_streams = enable ? BIT(0) : 0; > >>> #if IS_REACHABLE(CONFIG_LEDS_CLASS) > >>> if (!IS_ERR_OR_NULL(sd->privacy_led)) { > >>> if (enable) > >>> led_set_brightness(sd->privacy_led, > >>> sd->privacy_led->max_brightness); > >>> else > >>> led_set_brightness(sd->privacy_led, 0); > >>> } > >>> #endif > >>> } > >> > >> Seems reasonable, I'll send v2. > > > > I was a bit too fast to hit y. ret will be non-zero if streamon fails > > (enable non-zero), in which case the LED needs to be turned off. So you > > can't have !ret condition for all LED control. > > But if stream-on fails then AFAICT the LED was never turned on in the first > place. stream on / off calls must always be balanced, right ? > > IOW stream-on is not supposed to get called if the device is already > streaming and if the device is not already streaming then the LED will > already be off. Ah, right. I missed that part. I'll send v2 after all. :-)
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c index 4c6198c48dd6..acb7c3003ab6 100644 --- a/drivers/media/v4l2-core/v4l2-subdev.c +++ b/drivers/media/v4l2-core/v4l2-subdev.c @@ -412,15 +412,6 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable) if (WARN_ON(!!sd->enabled_streams == !!enable)) return 0; -#if IS_REACHABLE(CONFIG_LEDS_CLASS) - if (!IS_ERR_OR_NULL(sd->privacy_led)) { - if (enable) - led_set_brightness(sd->privacy_led, - sd->privacy_led->max_brightness); - else - led_set_brightness(sd->privacy_led, 0); - } -#endif ret = sd->ops->video->s_stream(sd, enable); if (!enable && ret < 0) { @@ -431,6 +422,16 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable) if (!ret) sd->enabled_streams = enable ? BIT(0) : 0; +#if IS_REACHABLE(CONFIG_LEDS_CLASS) + if (!IS_ERR_OR_NULL(sd->privacy_led) && !(enable && ret < 0)) { + if (enable) + led_set_brightness(sd->privacy_led, + sd->privacy_led->max_brightness); + else + led_set_brightness(sd->privacy_led, 0); + } +#endif + return ret; }
Turn on the privacy LED only if streamon succeeds. This can be done after enabling streaming on the sensor. Fixes: b6e10ff6c23d ("media: v4l2-core: Make the v4l2-core code enable/disable the privacy LED if present") Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/media/v4l2-core/v4l2-subdev.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)