[02/16] iio: adc: at91-sama5d2_adc: lock around oversampling and sample freq

Commit Message

Claudiu Beznea June 9, 2022, 8:31 a.m. UTC

.read_raw()/.write_raw() could be called asynchronously from user space
or other in kernel drivers. Without locking on st->lock these could be
called asynchronously while there is a conversion in progress. Read will
be harmless but changing registers while conversion is in progress may
lead to inconsistent results. Thus, to avoid this lock st->lock.

Fixes: 27e177190891 ("iio:adc:at91_adc8xx: introduce new atmel adc driver")
Fixes: 6794e23fa3fe ("iio: adc: at91-sama5d2_adc: add support for oversampling resolution")
Signed-off-by: Claudiu Beznea <[e-mail]>
---
 drivers/iio/adc/at91-sama5d2_adc.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

Comments

Jonathan Cameron June 11, 2022, 5:30 p.m. UTC | #1

On Thu, 9 Jun 2022 11:31:59 +0300
Claudiu Beznea <[e-mail]> wrote:

> .read_raw()/.write_raw() could be called asynchronously from user space
> or other in kernel drivers. Without locking on st->lock these could be
> called asynchronously while there is a conversion in progress. Read will
> be harmless but changing registers while conversion is in progress may
> lead to inconsistent results. Thus, to avoid this lock st->lock.

The patch makes sense, but I'm not convinced all of the changes below
involve any changes to registers. E.g. at91_adc_adjust_val_osr()
is using the cached value of something in a register, but not the
register itself, so please update the description to mention cached state.

Other comments inline.
> 
> Fixes: 27e177190891 ("iio:adc:at91_adc8xx: introduce new atmel adc driver")
> Fixes: 6794e23fa3fe ("iio: adc: at91-sama5d2_adc: add support for oversampling resolution")
> Signed-off-by: Claudiu Beznea <[e-mail]>
> ---
>  drivers/iio/adc/at91-sama5d2_adc.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index 32b6f157b803..a672a520cdc0 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -1542,10 +1542,11 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
>  		ret = at91_adc_read_position(st, chan->channel,
>  					     &tmp_val);
>  		*val = tmp_val;
> +		ret = at91_adc_adjust_val_osr(st, val);
>  		mutex_unlock(&st->lock);
>  		iio_device_release_direct_mode(indio_dev);
>  
> -		return at91_adc_adjust_val_osr(st, val);
> +		return ret;
>  	}
>  	if (chan->type == IIO_PRESSURE) {
>  		ret = iio_device_claim_direct_mode(indio_dev);
> @@ -1556,10 +1557,11 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
>  		ret = at91_adc_read_pressure(st, chan->channel,
>  					     &tmp_val);
>  		*val = tmp_val;
> +		ret = at91_adc_adjust_val_osr(st, val);
>  		mutex_unlock(&st->lock);
>  		iio_device_release_direct_mode(indio_dev);
>  
> -		return at91_adc_adjust_val_osr(st, val);
> +		return ret;
>  	}
>  
>  	/* in this case we have a voltage channel */
> @@ -1620,11 +1622,15 @@ static int at91_adc_read_raw(struct iio_dev *indio_dev,
>  		return IIO_VAL_FRACTIONAL_LOG2;
>  
>  	case IIO_CHAN_INFO_SAMP_FREQ:
> +		mutex_lock(&st->lock);
>  		*val = at91_adc_get_sample_freq(st);

So this is a straight read of a cached value.  The only thing you 'might'
arguably be protecting against is read/write tearing due to it in theory
being possible to write part of the value whilst reading.  I don't
see that being a concern for st->current_sample_rate

> +		mutex_unlock(&st->lock);
>  		return IIO_VAL_INT;
>  
>  	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		mutex_lock(&st->lock);
>  		*val = st->oversampling_ratio;
Likewise, what are you protecting against racing with this that can't
just occur before or after the lock?

> +		mutex_unlock(&st->lock);
>  		return IIO_VAL_INT;
>  
>  	default:
> @@ -1644,18 +1650,23 @@ static int at91_adc_write_raw(struct iio_dev *indio_dev,
>  		    (val != AT91_OSR_16SAMPLES))
>  			return -EINVAL;
>  		/* if no change, optimize out */
> +		mutex_lock(&st->lock);
>  		if (val == st->oversampling_ratio)
> -			return 0;
It should be race free to check this outside the lock.

Definitely valid to lock around the cached value write and the config
write though.

> +			goto unlock;
If you did want to have locking as now then flip the logic

		if (val != st->oversampling_ratio) {
			st->oversampling_ratio = val;
			at91_adc_config_emr(st);
		}
		mutex_unlock()
..

Goto always have a cost in readability so if you can avoid them with
a simple flip of logic like this it is usually a good idea.
(exception is error code which should always be out of line as
that is more common so what we expect to see).

>  		st->oversampling_ratio = val;
>  		/* update ratio */
>  		at91_adc_config_emr(st);
> +unlock:
> +		mutex_unlock(&st->lock);
>  		return 0;
>  	case IIO_CHAN_INFO_SAMP_FREQ:
>  		if (val < st->soc_info.min_sample_rate ||
>  		    val > st->soc_info.max_sample_rate)
>  			return -EINVAL;
>  
> +		mutex_lock(&st->lock);
>  		at91_adc_setup_samp_freq(indio_dev, val);
> +		mutex_unlock(&st->lock);
>  		return 0;
>  	default:
>  		return -EINVAL;

Claudiu Beznea June 14, 2022, 8:19 a.m. UTC | #2

On 11.06.2022 20:30, Jonathan Cameron wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Thu, 9 Jun 2022 11:31:59 +0300
> Claudiu Beznea <[e-mail]> wrote:
> 
>> .read_raw()/.write_raw() could be called asynchronously from user space
>> or other in kernel drivers. Without locking on st->lock these could be
>> called asynchronously while there is a conversion in progress. Read will
>> be harmless but changing registers while conversion is in progress may
>> lead to inconsistent results. Thus, to avoid this lock st->lock.
> 
> The patch makes sense, but I'm not convinced all of the changes below
> involve any changes to registers. E.g. at91_adc_adjust_val_osr()
> is using the cached value of something in a register, but not the
> register itself, so please update the description to mention cached state.
> 
> Other comments inline.
>>
>> Fixes: 27e177190891 ("iio:adc:at91_adc8xx: introduce new atmel adc driver")
>> Fixes: 6794e23fa3fe ("iio: adc: at91-sama5d2_adc: add support for oversampling resolution")
>> Signed-off-by: Claudiu Beznea <[e-mail]>
>> ---
>>  drivers/iio/adc/at91-sama5d2_adc.c | 17 ++++++++++++++---
>>  1 file changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
>> index 32b6f157b803..a672a520cdc0 100644
>> --- a/drivers/iio/adc/at91-sama5d2_adc.c
>> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
>> @@ -1542,10 +1542,11 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
>>               ret = at91_adc_read_position(st, chan->channel,
>>                                            &tmp_val);
>>               *val = tmp_val;
>> +             ret = at91_adc_adjust_val_osr(st, val);
>>               mutex_unlock(&st->lock);
>>               iio_device_release_direct_mode(indio_dev);
>>
>> -             return at91_adc_adjust_val_osr(st, val);
>> +             return ret;
>>       }
>>       if (chan->type == IIO_PRESSURE) {
>>               ret = iio_device_claim_direct_mode(indio_dev);
>> @@ -1556,10 +1557,11 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
>>               ret = at91_adc_read_pressure(st, chan->channel,
>>                                            &tmp_val);
>>               *val = tmp_val;
>> +             ret = at91_adc_adjust_val_osr(st, val);
>>               mutex_unlock(&st->lock);
>>               iio_device_release_direct_mode(indio_dev);
>>
>> -             return at91_adc_adjust_val_osr(st, val);
>> +             return ret;
>>       }
>>
>>       /* in this case we have a voltage channel */
>> @@ -1620,11 +1622,15 @@ static int at91_adc_read_raw(struct iio_dev *indio_dev,
>>               return IIO_VAL_FRACTIONAL_LOG2;
>>
>>       case IIO_CHAN_INFO_SAMP_FREQ:
>> +             mutex_lock(&st->lock);
>>               *val = at91_adc_get_sample_freq(st);
> 
> So this is a straight read of a cached value.  The only thing you 'might'
> arguably be protecting against is read/write tearing due to it in theory
> being possible to write part of the value whilst reading.  

Yes, for these kind of scenarios I kept the lock around cached values, too.

> I don't
> see that being a concern for st->current_sample_rate

I am not fully aware of all the user space tools that are retrieving this
and how this is used and thus I kept the lock also around the cached values
to protect the user space tools being polluted with wrong values, if any.

> 
>> +             mutex_unlock(&st->lock);
>>               return IIO_VAL_INT;
>>
>>       case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>> +             mutex_lock(&st->lock);
>>               *val = st->oversampling_ratio;
> Likewise, what are you protecting against racing with this that can't
> just occur before or after the lock?

Same as above.

> 
>> +             mutex_unlock(&st->lock);
>>               return IIO_VAL_INT;
>>
>>       default:
>> @@ -1644,18 +1650,23 @@ static int at91_adc_write_raw(struct iio_dev *indio_dev,
>>                   (val != AT91_OSR_16SAMPLES))
>>                       return -EINVAL;
>>               /* if no change, optimize out */
>> +             mutex_lock(&st->lock);
>>               if (val == st->oversampling_ratio)
>> -                     return 0;
> It should be race free to check this outside the lock.
> 
> Definitely valid to lock around the cached value write and the config
> write though.
> 
>> +                     goto unlock;
> If you did want to have locking as now then flip the logic
> 
>                 if (val != st->oversampling_ratio) {
>                         st->oversampling_ratio = val;
>                         at91_adc_config_emr(st);
>                 }
>                 mutex_unlock()
> ..
> 

OK, thanks!

> Goto always have a cost in readability so if you can avoid them with
> a simple flip of logic like this it is usually a good idea.
> (exception is error code which should always be out of line as
> that is more common so what we expect to see).
> 
>>               st->oversampling_ratio = val;
>>               /* update ratio */
>>               at91_adc_config_emr(st);
>> +unlock:
>> +             mutex_unlock(&st->lock);
>>               return 0;
>>       case IIO_CHAN_INFO_SAMP_FREQ:
>>               if (val < st->soc_info.min_sample_rate ||
>>                   val > st->soc_info.max_sample_rate)
>>                       return -EINVAL;
>>
>> +             mutex_lock(&st->lock);
>>               at91_adc_setup_samp_freq(indio_dev, val);
>> +             mutex_unlock(&st->lock);
>>               return 0;
>>       default:
>>               return -EINVAL;
>

Patch

diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index 32b6f157b803..a672a520cdc0 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -1542,10 +1542,11 @@  static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
 		ret = at91_adc_read_position(st, chan->channel,
 					     &tmp_val);
 		*val = tmp_val;
+		ret = at91_adc_adjust_val_osr(st, val);
 		mutex_unlock(&st->lock);
 		iio_device_release_direct_mode(indio_dev);
 
-		return at91_adc_adjust_val_osr(st, val);
+		return ret;
 	}
 	if (chan->type == IIO_PRESSURE) {
 		ret = iio_device_claim_direct_mode(indio_dev);
@@ -1556,10 +1557,11 @@  static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
 		ret = at91_adc_read_pressure(st, chan->channel,
 					     &tmp_val);
 		*val = tmp_val;
+		ret = at91_adc_adjust_val_osr(st, val);
 		mutex_unlock(&st->lock);
 		iio_device_release_direct_mode(indio_dev);
 
-		return at91_adc_adjust_val_osr(st, val);
+		return ret;
 	}
 
 	/* in this case we have a voltage channel */
@@ -1620,11 +1622,15 @@  static int at91_adc_read_raw(struct iio_dev *indio_dev,
 		return IIO_VAL_FRACTIONAL_LOG2;
 
 	case IIO_CHAN_INFO_SAMP_FREQ:
+		mutex_lock(&st->lock);
 		*val = at91_adc_get_sample_freq(st);
+		mutex_unlock(&st->lock);
 		return IIO_VAL_INT;
 
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		mutex_lock(&st->lock);
 		*val = st->oversampling_ratio;
+		mutex_unlock(&st->lock);
 		return IIO_VAL_INT;
 
 	default:
@@ -1644,18 +1650,23 @@  static int at91_adc_write_raw(struct iio_dev *indio_dev,
 		    (val != AT91_OSR_16SAMPLES))
 			return -EINVAL;
 		/* if no change, optimize out */
+		mutex_lock(&st->lock);
 		if (val == st->oversampling_ratio)
-			return 0;
+			goto unlock;
 		st->oversampling_ratio = val;
 		/* update ratio */
 		at91_adc_config_emr(st);
+unlock:
+		mutex_unlock(&st->lock);
 		return 0;
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		if (val < st->soc_info.min_sample_rate ||
 		    val > st->soc_info.max_sample_rate)
 			return -EINVAL;
 
+		mutex_lock(&st->lock);
 		at91_adc_setup_samp_freq(indio_dev, val);
+		mutex_unlock(&st->lock);
 		return 0;
 	default:
 		return -EINVAL;