iio: hid-sensor-trigger: Don't touch sensors unless user space requests
diff mbox

Message ID 1507739701-42151-1-git-send-email-srinivas.pandruvada@linux.intel.com
State New
Headers show

Commit Message

Srinivas Pandruvada Oct. 11, 2017, 4:35 p.m. UTC
One of the user complained that on his system Thinkpad Yoga S1, with
commit f1664eaacec3 ("iio: hid-sensor-trigger: Fix the race with user
space powering up sensors") causes the system to resume immediately
on suspend (S3 operation). On this system the sensor hub is on USB
and is a wake up device from S3. So if any sensor sends data on
motion, the system will wake up. This can be a legitimate use case
to wake up device motion, but that needs proper user space support
to set right thresholds.

In fact the above commit didn't cause this regression, but any operation
which cause sensors to wake up would have caused the same issue. So if
user reads the raw sensor data, same issue occurs, with or without this
commit. Only difference is that the above commit by default will trigger
a power up and power down of sensors as part of runtime pm enable
(runtime enable will cause a runtime resume callback followed by
runtime_suspend callback). Previously user has to do some action on
sensors.

On investigation it was observed that the current driver correctly
changing the state of all sensors to power off but then also some sensor
will still send some data. Only option is to never power up any sensor.

Only good option is to:
- Using sysfs interface disable USB as a wakeup device (This will not
need any driver change)

Since some user don't care about sensors. So for those users this change
brings back old functionality. As long as they don't cause any operation
to power up sensors (like raw read or start iio-sensor-proxy service),
the sensors will not be to touched. This is done by delaying run time
enable till user space does some operation with sensors.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=196853
Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---

Since only one user has seen issue, so it may be isolated case.
So I  prefer to go through regular release cycle. If there are more
complains then we can then submit for stable tree.

 drivers/iio/common/hid-sensors/hid-sensor-trigger.c | 12 +++++++++---
 include/linux/hid-sensor-hub.h                      |  1 +
 2 files changed, 10 insertions(+), 3 deletions(-)

Comments

Jonathan Cameron Oct. 14, 2017, 6:31 p.m. UTC | #1
On Wed, 11 Oct 2017 09:35:01 -0700
Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> wrote:

> One of the user complained that on his system Thinkpad Yoga S1, with
> commit f1664eaacec3 ("iio: hid-sensor-trigger: Fix the race with user
> space powering up sensors") causes the system to resume immediately
> on suspend (S3 operation). On this system the sensor hub is on USB
> and is a wake up device from S3. So if any sensor sends data on
> motion, the system will wake up. This can be a legitimate use case
> to wake up device motion, but that needs proper user space support
> to set right thresholds.
> 
> In fact the above commit didn't cause this regression, but any operation
> which cause sensors to wake up would have caused the same issue. So if
> user reads the raw sensor data, same issue occurs, with or without this
> commit. Only difference is that the above commit by default will trigger
> a power up and power down of sensors as part of runtime pm enable
> (runtime enable will cause a runtime resume callback followed by
> runtime_suspend callback). Previously user has to do some action on
> sensors.
> 
> On investigation it was observed that the current driver correctly
> changing the state of all sensors to power off but then also some sensor
> will still send some data. Only option is to never power up any sensor.

Oh goody - broken hardware or at least hardware that behaves rather oddly.

> 
> Only good option is to:
> - Using sysfs interface disable USB as a wakeup device (This will not
> need any driver change)
> 
> Since some user don't care about sensors. So for those users this change
> brings back old functionality. As long as they don't cause any operation
> to power up sensors (like raw read or start iio-sensor-proxy service),
> the sensors will not be to touched. This is done by delaying run time
> enable till user space does some operation with sensors.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=196853
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to play with it.

Thanks,

Jonathan

> ---
> 
> Since only one user has seen issue, so it may be isolated case.
> So I  prefer to go through regular release cycle. If there are more
> complains then we can then submit for stable tree.
> 
>  drivers/iio/common/hid-sensors/hid-sensor-trigger.c | 12 +++++++++---
>  include/linux/hid-sensor-hub.h                      |  1 +
>  2 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> index 0e4b379..2c25ff3 100644
> --- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> +++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> @@ -179,6 +179,10 @@ int hid_sensor_power_state(struct hid_sensor_common *st, bool state)
>  	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)
>  		ret = pm_runtime_get_sync(&st->pdev->dev);
>  	else {
> @@ -221,7 +225,8 @@ static void hid_sensor_set_power_work(struct work_struct *work)
>  	if (attrb->latency_ms > 0)
>  		hid_sensor_set_report_latency(attrb, attrb->latency_ms);
>  
> -	_hid_sensor_power_state(attrb, true);
> +	if (atomic_read(&attrb->user_requested_state))
> +		_hid_sensor_power_state(attrb, true);
>  }
>  
>  static int hid_sensor_data_rdy_trigger_set_state(struct iio_trigger *trig,
> @@ -232,7 +237,9 @@ static int hid_sensor_data_rdy_trigger_set_state(struct iio_trigger *trig,
>  
>  void hid_sensor_remove_trigger(struct hid_sensor_common *attrb)
>  {
> -	pm_runtime_disable(&attrb->pdev->dev);
> +	if (atomic_read(&attrb->runtime_pm_enable))
> +		pm_runtime_disable(&attrb->pdev->dev);
> +
>  	pm_runtime_set_suspended(&attrb->pdev->dev);
>  	pm_runtime_put_noidle(&attrb->pdev->dev);
>  
> @@ -283,7 +290,6 @@ int hid_sensor_setup_trigger(struct iio_dev *indio_dev, const char *name,
>  	INIT_WORK(&attrb->work, hid_sensor_set_power_work);
>  
>  	pm_suspend_ignore_children(&attrb->pdev->dev, true);
> -	pm_runtime_enable(&attrb->pdev->dev);
>  	/* Default to 3 seconds, but can be changed from sysfs */
>  	pm_runtime_set_autosuspend_delay(&attrb->pdev->dev,
>  					 3000);
> diff --git a/include/linux/hid-sensor-hub.h b/include/linux/hid-sensor-hub.h
> index fc7aae6..331dc37 100644
> --- a/include/linux/hid-sensor-hub.h
> +++ b/include/linux/hid-sensor-hub.h
> @@ -231,6 +231,7 @@ struct hid_sensor_common {
>  	unsigned usage_id;
>  	atomic_t data_ready;
>  	atomic_t user_requested_state;
> +	atomic_t runtime_pm_enable;
>  	int poll_interval;
>  	int raw_hystersis;
>  	int latency_ms;

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
index 0e4b379..2c25ff3 100644
--- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
+++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
@@ -179,6 +179,10 @@  int hid_sensor_power_state(struct hid_sensor_common *st, bool state)
 	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)
 		ret = pm_runtime_get_sync(&st->pdev->dev);
 	else {
@@ -221,7 +225,8 @@  static void hid_sensor_set_power_work(struct work_struct *work)
 	if (attrb->latency_ms > 0)
 		hid_sensor_set_report_latency(attrb, attrb->latency_ms);
 
-	_hid_sensor_power_state(attrb, true);
+	if (atomic_read(&attrb->user_requested_state))
+		_hid_sensor_power_state(attrb, true);
 }
 
 static int hid_sensor_data_rdy_trigger_set_state(struct iio_trigger *trig,
@@ -232,7 +237,9 @@  static int hid_sensor_data_rdy_trigger_set_state(struct iio_trigger *trig,
 
 void hid_sensor_remove_trigger(struct hid_sensor_common *attrb)
 {
-	pm_runtime_disable(&attrb->pdev->dev);
+	if (atomic_read(&attrb->runtime_pm_enable))
+		pm_runtime_disable(&attrb->pdev->dev);
+
 	pm_runtime_set_suspended(&attrb->pdev->dev);
 	pm_runtime_put_noidle(&attrb->pdev->dev);
 
@@ -283,7 +290,6 @@  int hid_sensor_setup_trigger(struct iio_dev *indio_dev, const char *name,
 	INIT_WORK(&attrb->work, hid_sensor_set_power_work);
 
 	pm_suspend_ignore_children(&attrb->pdev->dev, true);
-	pm_runtime_enable(&attrb->pdev->dev);
 	/* Default to 3 seconds, but can be changed from sysfs */
 	pm_runtime_set_autosuspend_delay(&attrb->pdev->dev,
 					 3000);
diff --git a/include/linux/hid-sensor-hub.h b/include/linux/hid-sensor-hub.h
index fc7aae6..331dc37 100644
--- a/include/linux/hid-sensor-hub.h
+++ b/include/linux/hid-sensor-hub.h
@@ -231,6 +231,7 @@  struct hid_sensor_common {
 	unsigned usage_id;
 	atomic_t data_ready;
 	atomic_t user_requested_state;
+	atomic_t runtime_pm_enable;
 	int poll_interval;
 	int raw_hystersis;
 	int latency_ms;