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 <claudiu.beznea@xxxxxxxxxxxxx> 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 <claudiu.beznea@xxxxxxxxxxxxx>
>> ---
>> 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;
>
