Re: [PATCH] iio: hid-sensor-trigger: Fix the race with user space powering up sensors

Jonathan Cameron
2017-08-13T12:12:54Z

On Sat, 12 Aug 2017 09:09:21 -0700
Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> wrote:

> It has been reported for a while that with iio-sensor-proxy service the
> rotation only works after one suspend/resume cycle. This required a wait
> in the systemd unit file to avoid race. I found a Yoga 900 where I could
> reproduce this.
>
> The problem scenerio is:
> - During sensor driver init, enable run time PM and also set a
> auto-suspend for 3 seconds.
> This result in one runtime resume. But there is a check to avoid
> a powerup in this sequence, but rpm is active
> - User space iio-sensor-proxy tries to power up the sensor. Since rpm is
> active it will simply return. But sensors were not actually
> powered up in the prior sequence, so actaully the sensors will not work
> - After 3 seconds the auto suspend kicks
>
> If we add a wait in systemd service file to fire iio-sensor-proxy after
> 3 seconds, then now everything will work as the runtime resume will
> actually powerup the sensor as this is a user request.
>
> To avoid this:
> - Remove the check to match user requested state, this will cause a
> brief powerup, but if the iio-sensor-proxy starts immediately it will
> still work as the sensors are ON.
> - Also move the autosuspend delay to place when user requested turn off
> of sensors, like after user finished raw read or buffer disable
>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> Tested-by: Bastien Nocera <hadess@hadess.net>
>
Applied to the fixes-togreg branch of iio.git and marked for stable.

Thanks,

Jonathan
> ---
> Changes:
> Removed RFT and added Tested-by tag.
>
> 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 16ade0a..0e4b379 100644
> --- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> +++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> @@ -111,8 +111,6 @@ static int _hid_sensor_power_state(struct hid_sensor_common *st, bool state)
> s32 poll_value = 0;
>
> if (state) {
> - if (!atomic_read(&st->user_requested_state))
> - return 0;
> if (sensor_hub_device_open(st->hsdev))
> return -EIO;
>
> @@ -161,6 +159,9 @@ static int _hid_sensor_power_state(struct hid_sensor_common *st, bool state)
> &report_val);
> }
>
> + pr_debug("HID_SENSOR %s set power_state %d report_state %d\n",
> + st->pdev->name, state_val, report_val);
> +
> sensor_hub_get_feature(st->hsdev, st->power_state.report_id,
> st->power_state.index,
> sizeof(state_val), &state_val);
> @@ -182,6 +183,7 @@ int hid_sensor_power_state(struct hid_sensor_common *st, bool state)
> ret = pm_runtime_get_sync(&st->pdev->dev);
> else {
> pm_runtime_mark_last_busy(&st->pdev->dev);
> + pm_runtime_use_autosuspend(&st->pdev->dev);
> ret = pm_runtime_put_autosuspend(&st->pdev->dev);
> }
> if (ret < 0) {
> @@ -285,8 +287,6 @@ int hid_sensor_setup_trigger(struct iio_dev *indio_dev, const char *name,
> /* Default to 3 seconds, but can be changed from sysfs */
> pm_runtime_set_autosuspend_delay(&attrb->pdev->dev,
> 3000);
> - pm_runtime_use_autosuspend(&attrb->pdev->dev);
> -
> return ret;
> error_unreg_trigger:
> iio_trigger_unregister(trig);