On Wed, 11 Oct 2017 09:35:01 -0700
Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
