[PATCH] iio: light: vcnl4000: Don't power on/off chip in config

[PATCH] iio: light: vcnl4000: Don't power on/off chip in config

[Mårten Lindahl]

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(-)

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;
 }

---
base-commit: 7ba2090ca64ea1aa435744884124387db1fac70f
change-id: 20230907-vcnl4000-pm-fix-b58dc0dffb5c

Best regards,
-- 
Mårten Lindahl <marten.lindahl@axis.com>

Re: [PATCH] iio: light: vcnl4000: Don't power on/off chip in config

[Jonathan Cameron]

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,

Re: [PATCH] iio: light: vcnl4000: Don't power on/off chip in config

[Mårten Lindahl]

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,