[PATCH] iio: hid-sensor-trigger: Fix sometimes not powering up the sensor after resume

[PATCH] iio: hid-sensor-trigger: Fix sometimes not powering up the sensor after resume

[Hans de Goede]

hid_sensor_set_power_work() powers the sensors back up after a resume
based on the user_requested_state atomic_t.

But hid_sensor_power_state() treats this as a boolean flag, leading to
the following problematic scenario:

1) Some app starts using the iio-sensor in buffered / triggered mode,
   hid_sensor_data_rdy_trigger_set_state(true) gets called, setting
   user_requested_state to 1.
2) Something directly accesses a _raw value through sysfs, leading
   to a call to hid_sensor_power_state(true) followed by
   hid_sensor_power_state(false) call, this sets user_requested_state
   to 1 followed by setting it to 0.
3) Suspend/resume the machine, hid_sensor_set_power_work() now does
   NOT power the sensor back up because user_requested_state (wrongly)
   is 0. Which stops the app using the sensor in buffered mode from
   receiving any new values.

This commit changes user_requested_state to a counter tracking how many
times hid_sensor_power_state(true) was called instead, fixing this.

Cc: Bastien Nocera <hadess@hadess.net>
Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/iio/common/hid-sensors/hid-sensor-trigger.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
index cfb6588565ba..4905a997a7ec 100644
--- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
+++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
@@ -178,14 +178,14 @@ int hid_sensor_power_state(struct hid_sensor_common *st, bool state)
 #ifdef CONFIG_PM
 	int ret;
 
-	atomic_set(&st->user_requested_state, state);
-
 	if (atomic_add_unless(&st->runtime_pm_enable, 1, 1))
 		pm_runtime_enable(&st->pdev->dev);
 
-	if (state)
+	if (state) {
+		atomic_inc(&st->user_requested_state);
 		ret = pm_runtime_get_sync(&st->pdev->dev);
-	else {
+	} else {
+		atomic_dec(&st->user_requested_state);
 		pm_runtime_mark_last_busy(&st->pdev->dev);
 		pm_runtime_use_autosuspend(&st->pdev->dev);
 		ret = pm_runtime_put_autosuspend(&st->pdev->dev);
-- 
2.17.0

Re: [PATCH] iio: hid-sensor-trigger: Fix sometimes not powering up the sensor after resume

[Jonathan Cameron]

On Sat, 14 Apr 2018 17:09:09 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

> hid_sensor_set_power_work() powers the sensors back up after a resume
> based on the user_requested_state atomic_t.
> 
> But hid_sensor_power_state() treats this as a boolean flag, leading to
> the following problematic scenario:
> 
> 1) Some app starts using the iio-sensor in buffered / triggered mode,
>    hid_sensor_data_rdy_trigger_set_state(true) gets called, setting
>    user_requested_state to 1.
> 2) Something directly accesses a _raw value through sysfs, leading
>    to a call to hid_sensor_power_state(true) followed by
>    hid_sensor_power_state(false) call, this sets user_requested_state
>    to 1 followed by setting it to 0.
> 3) Suspend/resume the machine, hid_sensor_set_power_work() now does
>    NOT power the sensor back up because user_requested_state (wrongly)
>    is 0. Which stops the app using the sensor in buffered mode from
>    receiving any new values.
> 
> This commit changes user_requested_state to a counter tracking how many
> times hid_sensor_power_state(true) was called instead, fixing this.
> 
> Cc: Bastien Nocera <hadess@hadess.net>
> Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Looks sensible to me.

I'll give it a few days at least though for others to comment.

Thanks,

Jonathan

> ---
>  drivers/iio/common/hid-sensors/hid-sensor-trigger.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> index cfb6588565ba..4905a997a7ec 100644
> --- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> +++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> @@ -178,14 +178,14 @@ int hid_sensor_power_state(struct hid_sensor_common *st, bool state)
>  #ifdef CONFIG_PM
>  	int ret;
>  
> -	atomic_set(&st->user_requested_state, state);
> -
>  	if (atomic_add_unless(&st->runtime_pm_enable, 1, 1))
>  		pm_runtime_enable(&st->pdev->dev);
>  
> -	if (state)
> +	if (state) {
> +		atomic_inc(&st->user_requested_state);
>  		ret = pm_runtime_get_sync(&st->pdev->dev);
> -	else {
> +	} else {
> +		atomic_dec(&st->user_requested_state);
>  		pm_runtime_mark_last_busy(&st->pdev->dev);
>  		pm_runtime_use_autosuspend(&st->pdev->dev);
>  		ret = pm_runtime_put_autosuspend(&st->pdev->dev);

Re: [PATCH] iio: hid-sensor-trigger: Fix sometimes not powering up the sensor after resume

[Srinivas Pandruvada]

On Sun, 2018-04-15 at 15:58 +0100, Jonathan Cameron wrote:
> On Sat, 14 Apr 2018 17:09:09 +0200
> Hans de Goede <hdegoede@redhat.com> wrote:
> 
> > hid_sensor_set_power_work() powers the sensors back up after a
> > resume
> > based on the user_requested_state atomic_t.
> > 
> > But hid_sensor_power_state() treats this as a boolean flag, leading
> > to
> > the following problematic scenario:
> > 
> > 1) Some app starts using the iio-sensor in buffered / triggered
> > mode,
> >    hid_sensor_data_rdy_trigger_set_state(true) gets called, setting
> >    user_requested_state to 1.
> > 2) Something directly accesses a _raw value through sysfs, leading
> >    to a call to hid_sensor_power_state(true) followed by
> >    hid_sensor_power_state(false) call, this sets
> > user_requested_state
> >    to 1 followed by setting it to 0.
> > 3) Suspend/resume the machine, hid_sensor_set_power_work() now does
> >    NOT power the sensor back up because user_requested_state
> > (wrongly)
> >    is 0. Which stops the app using the sensor in buffered mode from
> >    receiving any new values.
> > 
> > This commit changes user_requested_state to a counter tracking how
> > many
> > times hid_sensor_power_state(true) was called instead, fixing this.
> > 
> > Cc: Bastien Nocera <hadess@hadess.net>
> > Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

Which App is doing like this?

Thanks,
Srinivas


> 
> Looks sensible to me.
> 
> I'll give it a few days at least though for others to comment.
> 
> Thanks,
> 
> Jonathan
> 
> > ---
> >  drivers/iio/common/hid-sensors/hid-sensor-trigger.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> > b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> > index cfb6588565ba..4905a997a7ec 100644
> > --- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> > +++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> > @@ -178,14 +178,14 @@ int hid_sensor_power_state(struct
> > hid_sensor_common *st, bool state)
> >  #ifdef CONFIG_PM
> >  	int ret;
> >  
> > -	atomic_set(&st->user_requested_state, state);
> > -
> >  	if (atomic_add_unless(&st->runtime_pm_enable, 1, 1))
> >  		pm_runtime_enable(&st->pdev->dev);
> >  
> > -	if (state)
> > +	if (state) {
> > +		atomic_inc(&st->user_requested_state);
> >  		ret = pm_runtime_get_sync(&st->pdev->dev);
> > -	else {
> > +	} else {
> > +		atomic_dec(&st->user_requested_state);
> >  		pm_runtime_mark_last_busy(&st->pdev->dev);
> >  		pm_runtime_use_autosuspend(&st->pdev->dev);
> >  		ret = pm_runtime_put_autosuspend(&st->pdev->dev);
> 
> 

Re: [PATCH] iio: hid-sensor-trigger: Fix sometimes not powering up the sensor after resume

[Hans de Goede]

Hi,

On 16-04-18 00:34, Srinivas Pandruvada wrote:
> On Sun, 2018-04-15 at 15:58 +0100, Jonathan Cameron wrote:
>> On Sat, 14 Apr 2018 17:09:09 +0200
>> Hans de Goede <hdegoede@redhat.com> wrote:
>>
>>> hid_sensor_set_power_work() powers the sensors back up after a
>>> resume
>>> based on the user_requested_state atomic_t.
>>>
>>> But hid_sensor_power_state() treats this as a boolean flag, leading
>>> to
>>> the following problematic scenario:
>>>
>>> 1) Some app starts using the iio-sensor in buffered / triggered
>>> mode,
>>>     hid_sensor_data_rdy_trigger_set_state(true) gets called, setting
>>>     user_requested_state to 1.
>>> 2) Something directly accesses a _raw value through sysfs, leading
>>>     to a call to hid_sensor_power_state(true) followed by
>>>     hid_sensor_power_state(false) call, this sets
>>> user_requested_state
>>>     to 1 followed by setting it to 0.
>>> 3) Suspend/resume the machine, hid_sensor_set_power_work() now does
>>>     NOT power the sensor back up because user_requested_state
>>> (wrongly)
>>>     is 0. Which stops the app using the sensor in buffered mode from
>>>     receiving any new values.
>>>
>>> This commit changes user_requested_state to a counter tracking how
>>> many
>>> times hid_sensor_power_state(true) was called instead, fixing this.
>>>
>>> Cc: Bastien Nocera <hadess@hadess.net>
>>> Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> 
> Which App is doing like this?

No app, just something I noticed while manually testing the
accelerometer while iio-sensor-proxy was also active.

Regards,

Hans



> 
> Thanks,
> Srinivas
> 
> 
>>
>> Looks sensible to me.
>>
>> I'll give it a few days at least though for others to comment.
>>
>> Thanks,
>>
>> Jonathan
>>
>>> ---
>>>   drivers/iio/common/hid-sensors/hid-sensor-trigger.c | 8 ++++----
>>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
>>> b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
>>> index cfb6588565ba..4905a997a7ec 100644
>>> --- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
>>> +++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
>>> @@ -178,14 +178,14 @@ int hid_sensor_power_state(struct
>>> hid_sensor_common *st, bool state)
>>>   #ifdef CONFIG_PM
>>>   	int ret;
>>>   
>>> -	atomic_set(&st->user_requested_state, state);
>>> -
>>>   	if (atomic_add_unless(&st->runtime_pm_enable, 1, 1))
>>>   		pm_runtime_enable(&st->pdev->dev);
>>>   
>>> -	if (state)
>>> +	if (state) {
>>> +		atomic_inc(&st->user_requested_state);
>>>   		ret = pm_runtime_get_sync(&st->pdev->dev);
>>> -	else {
>>> +	} else {
>>> +		atomic_dec(&st->user_requested_state);
>>>   		pm_runtime_mark_last_busy(&st->pdev->dev);
>>>   		pm_runtime_use_autosuspend(&st->pdev->dev);
>>>   		ret = pm_runtime_put_autosuspend(&st->pdev->dev);
>>
>>

Re: [PATCH] iio: hid-sensor-trigger: Fix sometimes not powering up the sensor after resume

[Jonathan Cameron]

On Mon, 16 Apr 2018 07:22:50 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
> 
> On 16-04-18 00:34, Srinivas Pandruvada wrote:
> > On Sun, 2018-04-15 at 15:58 +0100, Jonathan Cameron wrote:  
> >> On Sat, 14 Apr 2018 17:09:09 +0200
> >> Hans de Goede <hdegoede@redhat.com> wrote:
> >>  
> >>> hid_sensor_set_power_work() powers the sensors back up after a
> >>> resume
> >>> based on the user_requested_state atomic_t.
> >>>
> >>> But hid_sensor_power_state() treats this as a boolean flag, leading
> >>> to
> >>> the following problematic scenario:
> >>>
> >>> 1) Some app starts using the iio-sensor in buffered / triggered
> >>> mode,
> >>>     hid_sensor_data_rdy_trigger_set_state(true) gets called, setting
> >>>     user_requested_state to 1.
> >>> 2) Something directly accesses a _raw value through sysfs, leading
> >>>     to a call to hid_sensor_power_state(true) followed by
> >>>     hid_sensor_power_state(false) call, this sets
> >>> user_requested_state
> >>>     to 1 followed by setting it to 0.
> >>> 3) Suspend/resume the machine, hid_sensor_set_power_work() now does
> >>>     NOT power the sensor back up because user_requested_state
> >>> (wrongly)
> >>>     is 0. Which stops the app using the sensor in buffered mode from
> >>>     receiving any new values.
> >>>
> >>> This commit changes user_requested_state to a counter tracking how
> >>> many
> >>> times hid_sensor_power_state(true) was called instead, fixing this.
> >>>
> >>> Cc: Bastien Nocera <hadess@hadess.net>
> >>> Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>  
> > 
> > Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> > 
> > Which App is doing like this?  
> 
> No app, just something I noticed while manually testing the
> accelerometer while iio-sensor-proxy was also active.
> 
Applied to the fixes-togreg branch of iio.git and marked for stable.
Thanks,

Jonathan

> Regards,
> 
> Hans
> 
> 
> 
> > 
> > Thanks,
> > Srinivas
> > 
> >   
> >>
> >> Looks sensible to me.
> >>
> >> I'll give it a few days at least though for others to comment.
> >>
> >> Thanks,
> >>
> >> Jonathan
> >>  
> >>> ---
> >>>   drivers/iio/common/hid-sensors/hid-sensor-trigger.c | 8 ++++----
> >>>   1 file changed, 4 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> >>> b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> >>> index cfb6588565ba..4905a997a7ec 100644
> >>> --- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> >>> +++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> >>> @@ -178,14 +178,14 @@ int hid_sensor_power_state(struct
> >>> hid_sensor_common *st, bool state)
> >>>   #ifdef CONFIG_PM
> >>>   	int ret;
> >>>   
> >>> -	atomic_set(&st->user_requested_state, state);
> >>> -
> >>>   	if (atomic_add_unless(&st->runtime_pm_enable, 1, 1))
> >>>   		pm_runtime_enable(&st->pdev->dev);
> >>>   
> >>> -	if (state)
> >>> +	if (state) {
> >>> +		atomic_inc(&st->user_requested_state);
> >>>   		ret = pm_runtime_get_sync(&st->pdev->dev);
> >>> -	else {
> >>> +	} else {
> >>> +		atomic_dec(&st->user_requested_state);
> >>>   		pm_runtime_mark_last_busy(&st->pdev->dev);
> >>>   		pm_runtime_use_autosuspend(&st->pdev->dev);
> >>>   		ret = pm_runtime_put_autosuspend(&st->pdev->dev);  
> >>
> >>