* [PATCH] staging: iio: ad7192: Replace mlock with private driver lock
@ 2017-03-13 10:43 sayli karnik
2017-03-13 11:52 ` Lars-Peter Clausen
0 siblings, 1 reply; 4+ messages in thread
From: sayli karnik @ 2017-03-13 10:43 UTC (permalink / raw)
To: outreachy-kernel
Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
Hartmut Knaack, Peter Meerwald-Stadler, Greg Kroah-Hartman,
linux-iio
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
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] staging: iio: ad7192: Replace mlock with private driver lock
2017-03-13 10:43 [PATCH] staging: iio: ad7192: Replace mlock with private driver lock sayli karnik
@ 2017-03-13 11:52 ` Lars-Peter Clausen
2017-03-13 14:12 ` sayli karnik
0 siblings, 1 reply; 4+ messages in thread
From: Lars-Peter Clausen @ 2017-03-13 11:52 UTC (permalink / raw)
To: sayli karnik, outreachy-kernel
Cc: Michael Hennerich, Jonathan Cameron, Hartmut Knaack,
Peter Meerwald-Stadler, Greg Kroah-Hartman, linux-iio
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;
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] staging: iio: ad7192: Replace mlock with private driver lock
2017-03-13 11:52 ` Lars-Peter Clausen
@ 2017-03-13 14:12 ` sayli karnik
2017-03-13 21:14 ` Jonathan Cameron
0 siblings, 1 reply; 4+ messages in thread
From: sayli karnik @ 2017-03-13 14:12 UTC (permalink / raw)
To: Lars-Peter Clausen
Cc: outreachy-kernel, Michael Hennerich, Jonathan Cameron,
Hartmut Knaack, Peter Meerwald-Stadler, Greg Kroah-Hartman,
linux-iio
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;
>>
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] staging: iio: ad7192: Replace mlock with private driver lock
2017-03-13 14:12 ` sayli karnik
@ 2017-03-13 21:14 ` Jonathan Cameron
0 siblings, 0 replies; 4+ messages in thread
From: Jonathan Cameron @ 2017-03-13 21:14 UTC (permalink / raw)
To: sayli karnik, Lars-Peter Clausen
Cc: outreachy-kernel, Michael Hennerich, Hartmut Knaack,
Peter Meerwald-Stadler, Greg Kroah-Hartman, linux-iio
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;
>>>
>>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-03-13 21:14 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-03-13 10:43 [PATCH] staging: iio: ad7192: Replace mlock with private driver lock sayli karnik
2017-03-13 11:52 ` Lars-Peter Clausen
2017-03-13 14:12 ` sayli karnik
2017-03-13 21:14 ` Jonathan Cameron
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox