Re: [RFT][PATCH] iio: hid-sensor-trigger: Fix the race with user space powering up sensors
On Sat, 2017-08-12 at 14:58 +0200, Bastien Nocera wrote: > On Sat, 2017-08-12 at 13:16 +0100, Jonathan Cameron wrote: > > > > On Fri, 11 Aug 2017 16:04:30 +0200 > > Bastien Nocerawrote: > > > > > > > > Woot! > > > [...] > > > > > > > > Signed-off-by: Srinivas Pandruvada > > > nt > > > > el.com> > > > Tested-by: Bastien Nocera > > > > > > I'm still chasing a couple of bugs in the user-space side of > > > things > > > caused by the removal of the timeout. > > > > > > Thanks! > > Is it worth me sitting on this for a week or so to see if it deals > > with > > all the reported issues around this? > > > > Or are you happy that the test set you have is sufficient to verify > > it? > I'd rather have it merged ASAP. The fact that it didn't need to sleep > for 3 seconds allowed me to find a couple of problems in GNOME's use > of > this feature, with the sensor showing up before the desktop has > started. Those will likely be taken care of next week as well. > > I've tested it on a couple of machines, and it's working as expected. > > > > > Definitely good to put this one to bed finally! > And no one's happier than me in this one. I can concentrate on bugs I > wrote myself now ;) I will resubmit patch by adding Testedby and removing [RFT]. Thanks, Srinivas
Re: [RFT][PATCH] iio: hid-sensor-trigger: Fix the race with user space powering up sensors
On Sat, 2017-08-12 at 14:58 +0200, Bastien Nocera wrote: > On Sat, 2017-08-12 at 13:16 +0100, Jonathan Cameron wrote: > > > > On Fri, 11 Aug 2017 16:04:30 +0200 > > Bastien Nocera wrote: > > > > > > > > Woot! > > > [...] > > > > > > > > Signed-off-by: Srinivas Pandruvada > > > nt > > > > el.com> > > > Tested-by: Bastien Nocera > > > > > > I'm still chasing a couple of bugs in the user-space side of > > > things > > > caused by the removal of the timeout. > > > > > > Thanks! > > Is it worth me sitting on this for a week or so to see if it deals > > with > > all the reported issues around this? > > > > Or are you happy that the test set you have is sufficient to verify > > it? > I'd rather have it merged ASAP. The fact that it didn't need to sleep > for 3 seconds allowed me to find a couple of problems in GNOME's use > of > this feature, with the sensor showing up before the desktop has > started. Those will likely be taken care of next week as well. > > I've tested it on a couple of machines, and it's working as expected. > > > > > Definitely good to put this one to bed finally! > And no one's happier than me in this one. I can concentrate on bugs I > wrote myself now ;) I will resubmit patch by adding Testedby and removing [RFT]. Thanks, Srinivas
Re: [RFT][PATCH] iio: hid-sensor-trigger: Fix the race with user space powering up sensors
On Sat, 2017-08-12 at 13:16 +0100, Jonathan Cameron wrote: > On Fri, 11 Aug 2017 16:04:30 +0200 > Bastien Nocerawrote: > > > Woot! > > > > On Thu, 2017-08-10 at 16:24 -0700, Srinivas Pandruvada 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 > > el.com> > > > > Tested-by: Bastien Nocera > > > > I'm still chasing a couple of bugs in the user-space side of things > > caused by the removal of the timeout. > > > > Thanks! > > Is it worth me sitting on this for a week or so to see if it deals > with > all the reported issues around this? > > Or are you happy that the test set you have is sufficient to verify > it? I'd rather have it merged ASAP. The fact that it didn't need to sleep for 3 seconds allowed me to find a couple of problems in GNOME's use of this feature, with the sensor showing up before the desktop has started. Those will likely be taken care of next week as well. I've tested it on a couple of machines, and it's working as expected. > Definitely good to put this one to bed finally! And no one's happier than me in this one. I can concentrate on bugs I wrote myself now ;)
Re: [RFT][PATCH] iio: hid-sensor-trigger: Fix the race with user space powering up sensors
On Sat, 2017-08-12 at 13:16 +0100, Jonathan Cameron wrote: > On Fri, 11 Aug 2017 16:04:30 +0200 > Bastien Nocera wrote: > > > Woot! > > > > On Thu, 2017-08-10 at 16:24 -0700, Srinivas Pandruvada 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 > > el.com> > > > > Tested-by: Bastien Nocera > > > > I'm still chasing a couple of bugs in the user-space side of things > > caused by the removal of the timeout. > > > > Thanks! > > Is it worth me sitting on this for a week or so to see if it deals > with > all the reported issues around this? > > Or are you happy that the test set you have is sufficient to verify > it? I'd rather have it merged ASAP. The fact that it didn't need to sleep for 3 seconds allowed me to find a couple of problems in GNOME's use of this feature, with the sensor showing up before the desktop has started. Those will likely be taken care of next week as well. I've tested it on a couple of machines, and it's working as expected. > Definitely good to put this one to bed finally! And no one's happier than me in this one. I can concentrate on bugs I wrote myself now ;)
Re: [RFT][PATCH] iio: hid-sensor-trigger: Fix the race with user space powering up sensors
On Fri, 11 Aug 2017 16:04:30 +0200 Bastien Nocerawrote: > Woot! > > On Thu, 2017-08-10 at 16:24 -0700, Srinivas Pandruvada 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 > > Tested-by: Bastien Nocera > > I'm still chasing a couple of bugs in the user-space side of things > caused by the removal of the timeout. > > Thanks! Is it worth me sitting on this for a week or so to see if it deals with all the reported issues around this? Or are you happy that the test set you have is sufficient to verify it? Definitely good to put this one to bed finally! Jonathan
Re: [RFT][PATCH] iio: hid-sensor-trigger: Fix the race with user space powering up sensors
On Fri, 11 Aug 2017 16:04:30 +0200 Bastien Nocera wrote: > Woot! > > On Thu, 2017-08-10 at 16:24 -0700, Srinivas Pandruvada 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 > > Tested-by: Bastien Nocera > > I'm still chasing a couple of bugs in the user-space side of things > caused by the removal of the timeout. > > Thanks! Is it worth me sitting on this for a week or so to see if it deals with all the reported issues around this? Or are you happy that the test set you have is sufficient to verify it? Definitely good to put this one to bed finally! Jonathan
Re: [RFT][PATCH] iio: hid-sensor-trigger: Fix the race with user space powering up sensors
Woot! On Thu, 2017-08-10 at 16:24 -0700, Srinivas Pandruvada 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 PandruvadaTested-by: Bastien Nocera I'm still chasing a couple of bugs in the user-space side of things caused by the removal of the timeout. Thanks!
Re: [RFT][PATCH] iio: hid-sensor-trigger: Fix the race with user space powering up sensors
Woot! On Thu, 2017-08-10 at 16:24 -0700, Srinivas Pandruvada 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 Tested-by: Bastien Nocera I'm still chasing a couple of bugs in the user-space side of things caused by the removal of the timeout. Thanks!
[RFT][PATCH] iio: hid-sensor-trigger: Fix the race with user space powering up sensors
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--- 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(>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) _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), _val); @@ -182,6 +183,7 @@ int hid_sensor_power_state(struct hid_sensor_common *st, bool state) ret = pm_runtime_get_sync(>pdev->dev); else { pm_runtime_mark_last_busy(>pdev->dev); + pm_runtime_use_autosuspend(>pdev->dev); ret = pm_runtime_put_autosuspend(>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(>pdev->dev, 3000); - pm_runtime_use_autosuspend(>pdev->dev); - return ret; error_unreg_trigger: iio_trigger_unregister(trig); -- 2.5.5
[RFT][PATCH] iio: hid-sensor-trigger: Fix the race with user space powering up sensors
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 --- 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(>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) _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), _val); @@ -182,6 +183,7 @@ int hid_sensor_power_state(struct hid_sensor_common *st, bool state) ret = pm_runtime_get_sync(>pdev->dev); else { pm_runtime_mark_last_busy(>pdev->dev); + pm_runtime_use_autosuspend(>pdev->dev); ret = pm_runtime_put_autosuspend(>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(>pdev->dev, 3000); - pm_runtime_use_autosuspend(>pdev->dev); - return ret; error_unreg_trigger: iio_trigger_unregister(trig); -- 2.5.5