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

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

[sayli karnik]

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);
 			return IIO_VAL_INT_PLUS_NANO;
 		case IIO_TEMP:
 			*val = 0;
-- 
2.7.4

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

[Lars-Peter Clausen]

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.

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

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

[sayli karnik]

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?

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

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

[Jonathan Cameron]

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