| Message ID | 20230907-vcnl4000-pm-fix-v1-1-58a11c1d5a6c@axis.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: light: vcnl4000: Don't power on/off chip in config | expand |
On Thu, 7 Sep 2023 12:53:14 +0200 Mårten Lindahl <marten.lindahl@axis.com> wrote: Hi Mårten, Agree with your reasoning etc (and I guess you've triggered this for real) so just a few patch description etc comments. > After enabling/disabling interrupts on the vcnl4040 chip the als and/or > ps sensor is powered on or off depending on the interrupt enable bits. > This is made as a last step in write_event_config. > > But there is no reason to do this as the runtime PM handles the power > state of the sensors. Interfering with this may impact sensor readings. > I think the following example could be clearer. I haven't checked the naming of states in runtime pm, but a few things seem backwards to me. > Consider the following: > 1. Userspace makes sensor data reading which triggers 2000ms RPM resume > (sensor powered on) timeout It triggers a timeout to do runtime suspend if no access in 2000ms, not resume. > 2. Userspace disables interrupts => powers sensor off > 3. Userspace reads sensor data = 0 because sensor is off and RPM didn't > power on the sensor as resume timeout is still active suspend timeout hasn't yet run out, so the runtime pm subsystem thinks the device is still powered up and doesn't need resuming. > 4. RPM resume timeout passed suspend timeout. > > Powering sensor off in (2) risks a time window of close to 2000ms where > sensor data readings are disabled as in (3). > > Skip setting power state when writing new event config. > > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com> Fixes tag - probably 2 of them. One for the recent change that added the || als_int part and one for wherever the bug originally came from with comments to say why there are two fixes tags. > --- > drivers/iio/light/vcnl4000.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c > index 3a52b09c2823..fdf763a04b0b 100644 > --- a/drivers/iio/light/vcnl4000.c > +++ b/drivers/iio/light/vcnl4000.c > @@ -1513,7 +1513,6 @@ static int vcnl4040_write_event_config(struct iio_dev *indio_dev, > > out: > mutex_unlock(&data->vcnl4000_lock); > - data->chip_spec->set_power_state(data, data->ps_int || data->als_int); This will need manual backporting as this line changed recently and I'm fairly sure the argument is equally valid for the older code. > > return ret; > } > > --- > base-commit: 7ba2090ca64ea1aa435744884124387db1fac70f > change-id: 20230907-vcnl4000-pm-fix-b58dc0dffb5c > > Best regards,
Hi Jonathan! Thanks! I'll rephrase it and add the fixes tags. Kind regards Mårten On 9/10/23 15:32, Jonathan Cameron wrote: > On Thu, 7 Sep 2023 12:53:14 +0200 > Mårten Lindahl <marten.lindahl@axis.com> wrote: > > Hi Mårten, > > Agree with your reasoning etc (and I guess you've triggered this for real) > so just a few patch description etc comments. > >> After enabling/disabling interrupts on the vcnl4040 chip the als and/or >> ps sensor is powered on or off depending on the interrupt enable bits. >> This is made as a last step in write_event_config. >> >> But there is no reason to do this as the runtime PM handles the power >> state of the sensors. Interfering with this may impact sensor readings. >> > I think the following example could be clearer. I haven't checked > the naming of states in runtime pm, but a few things seem backwards to me. > >> Consider the following: >> 1. Userspace makes sensor data reading which triggers 2000ms RPM resume >> (sensor powered on) timeout > It triggers a timeout to do runtime suspend if no access in 2000ms, not resume. > >> 2. Userspace disables interrupts => powers sensor off >> 3. Userspace reads sensor data = 0 because sensor is off and RPM didn't >> power on the sensor as resume timeout is still active > suspend timeout hasn't yet run out, so the runtime pm subsystem thinks the > device is still powered up and doesn't need resuming. > >> 4. RPM resume timeout passed > suspend timeout. > >> Powering sensor off in (2) risks a time window of close to 2000ms where >> sensor data readings are disabled as in (3). >> >> Skip setting power state when writing new event config. >> >> Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com> > Fixes tag - probably 2 of them. One for the recent change that added > the || als_int part and one for wherever the bug originally came from > with comments to say why there are two fixes tags. > >> --- >> drivers/iio/light/vcnl4000.c | 1 - >> 1 file changed, 1 deletion(-) >> >> diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c >> index 3a52b09c2823..fdf763a04b0b 100644 >> --- a/drivers/iio/light/vcnl4000.c >> +++ b/drivers/iio/light/vcnl4000.c >> @@ -1513,7 +1513,6 @@ static int vcnl4040_write_event_config(struct iio_dev *indio_dev, >> >> out: >> mutex_unlock(&data->vcnl4000_lock); >> - data->chip_spec->set_power_state(data, data->ps_int || data->als_int); > This will need manual backporting as this line changed recently and I'm fairly > sure the argument is equally valid for the older code. > >> >> return ret; >> } >> >> --- >> base-commit: 7ba2090ca64ea1aa435744884124387db1fac70f >> change-id: 20230907-vcnl4000-pm-fix-b58dc0dffb5c >> >> Best regards,
diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c index 3a52b09c2823..fdf763a04b0b 100644 --- a/drivers/iio/light/vcnl4000.c +++ b/drivers/iio/light/vcnl4000.c @@ -1513,7 +1513,6 @@ static int vcnl4040_write_event_config(struct iio_dev *indio_dev, out: mutex_unlock(&data->vcnl4000_lock); - data->chip_spec->set_power_state(data, data->ps_int || data->als_int); return ret; }
After enabling/disabling interrupts on the vcnl4040 chip the als and/or ps sensor is powered on or off depending on the interrupt enable bits. This is made as a last step in write_event_config. But there is no reason to do this as the runtime PM handles the power state of the sensors. Interfering with this may impact sensor readings. Consider the following: 1. Userspace makes sensor data reading which triggers 2000ms RPM resume (sensor powered on) timeout 2. Userspace disables interrupts => powers sensor off 3. Userspace reads sensor data = 0 because sensor is off and RPM didn't power on the sensor as resume timeout is still active 4. RPM resume timeout passed Powering sensor off in (2) risks a time window of close to 2000ms where sensor data readings are disabled as in (3). Skip setting power state when writing new event config. Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com> --- drivers/iio/light/vcnl4000.c | 1 - 1 file changed, 1 deletion(-) --- base-commit: 7ba2090ca64ea1aa435744884124387db1fac70f change-id: 20230907-vcnl4000-pm-fix-b58dc0dffb5c Best regards,