Re: [PATCH] staging: iio: ad7192: Replace mlock with private driver lock

[Jonathan Cameron <jic23@kernel.org>]

On 13/03/17 14:12, sayli karnik wrote:
> On Mon, Mar 13, 2017 at 5:22 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>> On 03/13/2017 11:43 AM, sayli karnik wrote:
>>> indio_dev->mlock should be used by the IIO core only for protecting
>>> device operating mode changes. ie. Changes between INDIO_DIRECT_MODE,
>>> INDIO_BUFFER_* modes.
>>> Replace mlock with a lock in the device's global data to protect
>>> hardware state changes.
>>>
>>> Signed-off-by: sayli karnik <karniksayli1995@gmail.com>
>>> ---
>>>  drivers/staging/iio/adc/ad7192.c | 5 +++--
>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/staging/iio/adc/ad7192.c b/drivers/staging/iio/adc/ad7192.c
>>> index 4fc8588..bed48e7 100644
>>> --- a/drivers/staging/iio/adc/ad7192.c
>>> +++ b/drivers/staging/iio/adc/ad7192.c
>>> @@ -162,6 +162,7 @@ struct ad7192_state {
>>>       u32                             scale_avail[8][2];
>>>       u8                              gpocon;
>>>       u8                              devid;
>>> +     struct mutex                    lock;   /* protect sensor state */
>>>
>>>       struct ad_sigma_delta           sd;
>>>  };
>>> @@ -463,10 +464,10 @@ static int ad7192_read_raw(struct iio_dev *indio_dev,
>>>       case IIO_CHAN_INFO_SCALE:
>>>               switch (chan->type) {
>>>               case IIO_VOLTAGE:
>>> -                     mutex_lock(&indio_dev->mlock);
>>> +                     mutex_lock(&st->lock);
>>>                       *val = st->scale_avail[AD7192_CONF_GAIN(st->conf)][0];
>>>                       *val2 = st->scale_avail[AD7192_CONF_GAIN(st->conf)][1];
>>> -                     mutex_unlock(&indio_dev->mlock);
>>> +                     mutex_unlock(&st->lock);
>>
>> Sorry, but this does not make too much sense. If this is the only place
>> where the lock is used there isn't really any mutual exclusion going on
>> since it doesn't prevent any other code from running concurrently.
>>
>> The purpose of taking the lock here is that st->conf is in a consistent
>> state and is not in the process of being changed.
>>
> So you mean indio_dev->mlock is good to go here?
Not as such.  Issue here is the current locking is ineffective.  It's buggy!
So you want to take what Lars has said and think about how to fix it.

Jonathan
> 
>>>                       return IIO_VAL_INT_PLUS_NANO;
>>>               case IIO_TEMP:
>>>                       *val = 0;
>>>
>>