| Message ID | 20210509113354.660190-9-jic23@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | IIO: Runtime PM related cleanups. | expand |
Em Sun, 9 May 2021 12:33:34 +0100 Jonathan Cameron <jic23@kernel.org> escreveu: > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > The call to pm_runtime_put_noidle() in remove() callback is not > balanced by any gets > > Note this doesn't cause any problems beyond reader confusion as the runtime > pm core protects against the reference counter going negative. > > Whilst here, use pm_runtiem_resume_and_get() to simplify code a little. > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> RPM get/put logic LGTM. Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > --- > drivers/iio/common/hid-sensors/hid-sensor-trigger.c | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) > > diff --git a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c > index 5a7b3e253e58..c06537e106e9 100644 > --- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c > +++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c > @@ -163,18 +163,15 @@ int hid_sensor_power_state(struct hid_sensor_common *st, bool state) > > if (state) { > atomic_inc(&st->user_requested_state); > - ret = pm_runtime_get_sync(&st->pdev->dev); > + ret = pm_runtime_resume_and_get(&st->pdev->dev); > } 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); > } > - if (ret < 0) { > - if (state) > - pm_runtime_put_noidle(&st->pdev->dev); > + if (ret < 0) > return ret; > - } > > return 0; > #else > @@ -222,7 +219,6 @@ void hid_sensor_remove_trigger(struct iio_dev *indio_dev, > pm_runtime_disable(&attrb->pdev->dev); > > pm_runtime_set_suspended(&attrb->pdev->dev); > - pm_runtime_put_noidle(&attrb->pdev->dev); > > cancel_work_sync(&attrb->work); > iio_trigger_unregister(attrb->trigger); Thanks, Mauro
On Wed, 12 May 2021 15:44:30 +0200 Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > Em Sun, 9 May 2021 12:33:34 +0100 > Jonathan Cameron <jic23@kernel.org> escreveu: > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > The call to pm_runtime_put_noidle() in remove() callback is not > > balanced by any gets > > > > Note this doesn't cause any problems beyond reader confusion as the runtime > > pm core protects against the reference counter going negative. > > > > Whilst here, use pm_runtiem_resume_and_get() to simplify code a little. > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > > RPM get/put logic LGTM. > > Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> After our side discussion on this one I wanted to take another look. I'm not sure I follow the logic behind the runtime_enable() path and why we don't want to enable runtime_pm until first use. However it's not directly related to this patch and as far as I can tell it's just unsuual rather than broken. As such, applied this one to the togreg branch of iio.git and pushed out as testing. Thanks, Jonathan > > > --- > > drivers/iio/common/hid-sensors/hid-sensor-trigger.c | 8 ++------ > > 1 file changed, 2 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c > > index 5a7b3e253e58..c06537e106e9 100644 > > --- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c > > +++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c > > @@ -163,18 +163,15 @@ int hid_sensor_power_state(struct hid_sensor_common *st, bool state) > > > > if (state) { > > atomic_inc(&st->user_requested_state); > > - ret = pm_runtime_get_sync(&st->pdev->dev); > > + ret = pm_runtime_resume_and_get(&st->pdev->dev); > > } 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); > > } > > - if (ret < 0) { > > - if (state) > > - pm_runtime_put_noidle(&st->pdev->dev); > > + if (ret < 0) > > return ret; > > - } > > > > return 0; > > #else > > @@ -222,7 +219,6 @@ void hid_sensor_remove_trigger(struct iio_dev *indio_dev, > > pm_runtime_disable(&attrb->pdev->dev); > > > > pm_runtime_set_suspended(&attrb->pdev->dev); > > - pm_runtime_put_noidle(&attrb->pdev->dev); > > > > cancel_work_sync(&attrb->work); > > iio_trigger_unregister(attrb->trigger); > > > > Thanks, > Mauro
On Sun, 2021-05-16 at 16:19 +0100, Jonathan Cameron wrote: > On Wed, 12 May 2021 15:44:30 +0200 > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > > > Em Sun, 9 May 2021 12:33:34 +0100 > > Jonathan Cameron <jic23@kernel.org> escreveu: > > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > > > The call to pm_runtime_put_noidle() in remove() callback is not > > > balanced by any gets > > > > > > Note this doesn't cause any problems beyond reader confusion as > > > the runtime > > > pm core protects against the reference counter going negative. > > > > > > Whilst here, use pm_runtiem_resume_and_get() to simplify code a > > > little. > > > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > > > > RPM get/put logic LGTM. > > > > Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > > After our side discussion on this one I wanted to take another look. > I'm not sure I follow the logic behind the runtime_enable() path > and why we don't want to enable runtime_pm until first use. However > it's not directly related to this patch and as far as I can tell > it's just unsuual rather than broken. As such, applied this one to > the > togreg branch of iio.git and pushed out as testing. > This was enabled in init path but caused issue with USB sensor hub in Thinkpasd Yoga S1, where we can never power off sensor and it wakes up device immediately after suspend. commit ad7532cefd11d11a0814a75fb814c205ee3d9d4c Thanks, Srinivas > Thanks, > > Jonathan > > > > > > --- > > > drivers/iio/common/hid-sensors/hid-sensor-trigger.c | 8 ++------ > > > 1 file changed, 2 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c > > > b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c > > > index 5a7b3e253e58..c06537e106e9 100644 > > > --- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c > > > +++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c > > > @@ -163,18 +163,15 @@ int hid_sensor_power_state(struct > > > hid_sensor_common *st, bool state) > > > > > > if (state) { > > > atomic_inc(&st->user_requested_state); > > > - ret = pm_runtime_get_sync(&st->pdev->dev); > > > + ret = pm_runtime_resume_and_get(&st->pdev->dev); > > > } 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); > > > } > > > - if (ret < 0) { > > > - if (state) > > > - pm_runtime_put_noidle(&st->pdev->dev); > > > + if (ret < 0) > > > return ret; > > > - } > > > > > > return 0; > > > #else > > > @@ -222,7 +219,6 @@ void hid_sensor_remove_trigger(struct iio_dev > > > *indio_dev, > > > pm_runtime_disable(&attrb->pdev->dev); > > > > > > pm_runtime_set_suspended(&attrb->pdev->dev); > > > - pm_runtime_put_noidle(&attrb->pdev->dev); > > > > > > cancel_work_sync(&attrb->work); > > > iio_trigger_unregister(attrb->trigger); > > > > > > > > Thanks, > > Mauro >
On Sun, 16 May 2021 19:19:40 -0700 Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> wrote: > On Sun, 2021-05-16 at 16:19 +0100, Jonathan Cameron wrote: > > On Wed, 12 May 2021 15:44:30 +0200 > > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > > > > > Em Sun, 9 May 2021 12:33:34 +0100 > > > Jonathan Cameron <jic23@kernel.org> escreveu: > > > > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > > > > > The call to pm_runtime_put_noidle() in remove() callback is not > > > > balanced by any gets > > > > > > > > Note this doesn't cause any problems beyond reader confusion as > > > > the runtime > > > > pm core protects against the reference counter going negative. > > > > > > > > Whilst here, use pm_runtiem_resume_and_get() to simplify code a > > > > little. > > > > > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > > > > > > RPM get/put logic LGTM. > > > > > > Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > > > > After our side discussion on this one I wanted to take another look. > > I'm not sure I follow the logic behind the runtime_enable() path > > and why we don't want to enable runtime_pm until first use. However > > it's not directly related to this patch and as far as I can tell > > it's just unsuual rather than broken. As such, applied this one to > > the > > togreg branch of iio.git and pushed out as testing. > > > > This was enabled in init path but caused issue with USB sensor hub in > Thinkpasd Yoga S1, where we can never power off sensor and it wakes up > device immediately after suspend. > > commit > ad7532cefd11d11a0814a75fb814c205ee3d9d4c Ah. Thanks for the history. Thanks makes a horrible kind of sense ;) Jonathan > > > Thanks, > Srinivas > > > Thanks, > > > > Jonathan > > > > > > > > > --- > > > > drivers/iio/common/hid-sensors/hid-sensor-trigger.c | 8 ++------ > > > > 1 file changed, 2 insertions(+), 6 deletions(-) > > > > > > > > diff --git a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c > > > > b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c > > > > index 5a7b3e253e58..c06537e106e9 100644 > > > > --- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c > > > > +++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c > > > > @@ -163,18 +163,15 @@ int hid_sensor_power_state(struct > > > > hid_sensor_common *st, bool state) > > > > > > > > if (state) { > > > > atomic_inc(&st->user_requested_state); > > > > - ret = pm_runtime_get_sync(&st->pdev->dev); > > > > + ret = pm_runtime_resume_and_get(&st->pdev->dev); > > > > } 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); > > > > } > > > > - if (ret < 0) { > > > > - if (state) > > > > - pm_runtime_put_noidle(&st->pdev->dev); > > > > + if (ret < 0) > > > > return ret; > > > > - } > > > > > > > > return 0; > > > > #else > > > > @@ -222,7 +219,6 @@ void hid_sensor_remove_trigger(struct iio_dev > > > > *indio_dev, > > > > pm_runtime_disable(&attrb->pdev->dev); > > > > > > > > pm_runtime_set_suspended(&attrb->pdev->dev); > > > > - pm_runtime_put_noidle(&attrb->pdev->dev); > > > > > > > > cancel_work_sync(&attrb->work); > > > > iio_trigger_unregister(attrb->trigger); > > > > > > > > > > > > Thanks, > > > Mauro > > > >
diff --git a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c index 5a7b3e253e58..c06537e106e9 100644 --- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c +++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c @@ -163,18 +163,15 @@ int hid_sensor_power_state(struct hid_sensor_common *st, bool state) if (state) { atomic_inc(&st->user_requested_state); - ret = pm_runtime_get_sync(&st->pdev->dev); + ret = pm_runtime_resume_and_get(&st->pdev->dev); } 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); } - if (ret < 0) { - if (state) - pm_runtime_put_noidle(&st->pdev->dev); + if (ret < 0) return ret; - } return 0; #else @@ -222,7 +219,6 @@ void hid_sensor_remove_trigger(struct iio_dev *indio_dev, pm_runtime_disable(&attrb->pdev->dev); pm_runtime_set_suspended(&attrb->pdev->dev); - pm_runtime_put_noidle(&attrb->pdev->dev); cancel_work_sync(&attrb->work); iio_trigger_unregister(attrb->trigger);