* [PATCH] staging: iio: accel: replace mlock with driver private lock
@ 2017-03-13 23:58 Aishwarya Pant
2017-03-14 17:11 ` Lars-Peter Clausen
0 siblings, 1 reply; 3+ messages in thread
From: Aishwarya Pant @ 2017-03-13 23:58 UTC (permalink / raw)
To: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
Hartmut Knaack, Peter Meerwald-Stadler, Greg Kroah-Hartman
Cc: outreachy-kernel
IIO subsystem is redefining iio_dev mlock to be used by IIO core only
for protecting device operating mode changes.
In the driver adis164201, a struct adis16201_state has been introduced
to store driver state information. Wherever iio_dev->mlock was used to
protect hardware state changes, it has been replaced by driver private
lock.
Signed-off-by: Aishwarya Pant <aishpant@gmail.com>
---
drivers/staging/iio/accel/adis16201.c | 34 ++++++++++++++++++++--------------
1 file changed, 20 insertions(+), 14 deletions(-)
diff --git a/drivers/staging/iio/accel/adis16201.c b/drivers/staging/iio/accel/adis16201.c
index d6c8658..6d84eae 100644
--- a/drivers/staging/iio/accel/adis16201.c
+++ b/drivers/staging/iio/accel/adis16201.c
@@ -150,6 +150,11 @@
#define ADIS16201_ERROR_ACTIVE BIT(14)
+struct adis16201_state {
+ struct adis adis; /* adis device */
+ struct mutex lock; /* protect state changes */
+};
+
enum adis16201_scan {
ADIS16201_SCAN_ACC_X,
ADIS16201_SCAN_ACC_Y,
@@ -172,7 +177,7 @@ static int adis16201_read_raw(struct iio_dev *indio_dev,
int *val, int *val2,
long mask)
{
- struct adis *st = iio_priv(indio_dev);
+ struct adis16201_state *st = iio_priv(indio_dev);
int ret;
int bits;
u8 addr;
@@ -223,17 +228,17 @@ static int adis16201_read_raw(struct iio_dev *indio_dev,
default:
return -EINVAL;
}
- mutex_lock(&indio_dev->mlock);
+ mutex_lock(&st->lock);
addr = adis16201_addresses[chan->scan_index];
- ret = adis_read_reg_16(st, addr, &val16);
+ ret = adis_read_reg_16(&st->adis, addr, &val16);
if (ret) {
- mutex_unlock(&indio_dev->mlock);
+ mutex_unlock(&st->lock);
return ret;
}
val16 &= (1 << bits) - 1;
val16 = (s16)(val16 << (16 - bits)) >> (16 - bits);
*val = val16;
- mutex_unlock(&indio_dev->mlock);
+ mutex_unlock(&st->lock);
return IIO_VAL_INT;
}
return -EINVAL;
@@ -245,7 +250,7 @@ static int adis16201_write_raw(struct iio_dev *indio_dev,
int val2,
long mask)
{
- struct adis *st = iio_priv(indio_dev);
+ struct adis16201_state *st = iio_priv(indio_dev);
int bits;
s16 val16;
u8 addr;
@@ -264,7 +269,7 @@ static int adis16201_write_raw(struct iio_dev *indio_dev,
}
val16 = val & ((1 << bits) - 1);
addr = adis16201_addresses[chan->scan_index];
- return adis_write_reg_16(st, addr, val16);
+ return adis_write_reg_16(&st->adis, addr, val16);
}
return -EINVAL;
}
@@ -318,7 +323,7 @@ static const struct adis_data adis16201_data = {
static int adis16201_probe(struct spi_device *spi)
{
int ret;
- struct adis *st;
+ struct adis16201_state *st;
struct iio_dev *indio_dev;
/* setup the industrialio driver allocated elements */
@@ -327,6 +332,7 @@ static int adis16201_probe(struct spi_device *spi)
return -ENOMEM;
st = iio_priv(indio_dev);
+ mutex_init(&st->lock);
/* this is only used for removal purposes */
spi_set_drvdata(spi, indio_dev);
@@ -338,15 +344,15 @@ static int adis16201_probe(struct spi_device *spi)
indio_dev->num_channels = ARRAY_SIZE(adis16201_channels);
indio_dev->modes = INDIO_DIRECT_MODE;
- ret = adis_init(st, indio_dev, spi, &adis16201_data);
+ ret = adis_init(&st->adis, indio_dev, spi, &adis16201_data);
if (ret)
return ret;
- ret = adis_setup_buffer_and_trigger(st, indio_dev, NULL);
+ ret = adis_setup_buffer_and_trigger(&st->adis, indio_dev, NULL);
if (ret)
return ret;
/* Get the device into a sane initial state */
- ret = adis_initial_startup(st);
+ ret = adis_initial_startup(&st->adis);
if (ret)
goto error_cleanup_buffer_trigger;
@@ -356,17 +362,17 @@ static int adis16201_probe(struct spi_device *spi)
return 0;
error_cleanup_buffer_trigger:
- adis_cleanup_buffer_and_trigger(st, indio_dev);
+ adis_cleanup_buffer_and_trigger(&st->adis, indio_dev);
return ret;
}
static int adis16201_remove(struct spi_device *spi)
{
struct iio_dev *indio_dev = spi_get_drvdata(spi);
- struct adis *st = iio_priv(indio_dev);
+ struct adis16201_state *st = iio_priv(indio_dev);
iio_device_unregister(indio_dev);
- adis_cleanup_buffer_and_trigger(st, indio_dev);
+ adis_cleanup_buffer_and_trigger(&st->adis, indio_dev);
return 0;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] staging: iio: accel: replace mlock with driver private lock
2017-03-13 23:58 [PATCH] staging: iio: accel: replace mlock with driver private lock Aishwarya Pant
@ 2017-03-14 17:11 ` Lars-Peter Clausen
2017-03-14 1:05 ` Aishwarya Pant
0 siblings, 1 reply; 3+ messages in thread
From: Lars-Peter Clausen @ 2017-03-14 17:11 UTC (permalink / raw)
To: Aishwarya Pant, Michael Hennerich, Jonathan Cameron,
Hartmut Knaack, Peter Meerwald-Stadler, Greg Kroah-Hartman
Cc: outreachy-kernel
On 03/14/2017 12:58 AM, Aishwarya Pant wrote:
> IIO subsystem is redefining iio_dev mlock to be used by IIO core only
> for protecting device operating mode changes.
>
> In the driver adis164201, a struct adis16201_state has been introduced
> to store driver state information. Wherever iio_dev->mlock was used to
> protect hardware state changes, it has been replaced by driver private
> lock.
>
> Signed-off-by: Aishwarya Pant <aishpant@gmail.com>
> ---
> drivers/staging/iio/accel/adis16201.c | 34 ++++++++++++++++++++--------------
> 1 file changed, 20 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/staging/iio/accel/adis16201.c b/drivers/staging/iio/accel/adis16201.c
> index d6c8658..6d84eae 100644
> --- a/drivers/staging/iio/accel/adis16201.c
> +++ b/drivers/staging/iio/accel/adis16201.c
> @@ -150,6 +150,11 @@
>
> #define ADIS16201_ERROR_ACTIVE BIT(14)
>
> +struct adis16201_state {
> + struct adis adis; /* adis device */
> + struct mutex lock; /* protect state changes */
> +};
> +
> enum adis16201_scan {
> ADIS16201_SCAN_ACC_X,
> ADIS16201_SCAN_ACC_Y,
> @@ -172,7 +177,7 @@ static int adis16201_read_raw(struct iio_dev *indio_dev,
> int *val, int *val2,
> long mask)
> {
> - struct adis *st = iio_priv(indio_dev);
> + struct adis16201_state *st = iio_priv(indio_dev);
> int ret;
> int bits;
> u8 addr;
> @@ -223,17 +228,17 @@ static int adis16201_read_raw(struct iio_dev *indio_dev,
> default:
> return -EINVAL;
> }
> - mutex_lock(&indio_dev->mlock);
> + mutex_lock(&st->lock);
I think this lock could simply be removed completely. adis_read_reg_16() is
already atomic and there is nothing else to protect here.
> addr = adis16201_addresses[chan->scan_index];
> - ret = adis_read_reg_16(st, addr, &val16);
> + ret = adis_read_reg_16(&st->adis, addr, &val16);
> if (ret) {
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(&st->lock);
> return ret;
> }
> val16 &= (1 << bits) - 1;
> val16 = (s16)(val16 << (16 - bits)) >> (16 - bits);
> *val = val16;
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(&st->lock);
> return IIO_VAL_INT;
> }
> return -EINVAL;
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] staging: iio: accel: replace mlock with driver private lock
2017-03-14 17:11 ` Lars-Peter Clausen
@ 2017-03-14 1:05 ` Aishwarya Pant
0 siblings, 0 replies; 3+ messages in thread
From: Aishwarya Pant @ 2017-03-14 1:05 UTC (permalink / raw)
To: Lars-Peter Clausen
Cc: Michael Hennerich, Jonathan Cameron, Hartmut Knaack,
Peter Meerwald-Stadler, Greg Kroah-Hartman, outreachy-kernel
On Tue, Mar 14, 2017 at 06:11:51PM +0100, Lars-Peter Clausen wrote:
> On 03/14/2017 12:58 AM, Aishwarya Pant wrote:
> > IIO subsystem is redefining iio_dev mlock to be used by IIO core only
> > for protecting device operating mode changes.
> >
> > In the driver adis164201, a struct adis16201_state has been introduced
> > to store driver state information. Wherever iio_dev->mlock was used to
> > protect hardware state changes, it has been replaced by driver private
> > lock.
> >
> > Signed-off-by: Aishwarya Pant <aishpant@gmail.com>
> > ---
> > drivers/staging/iio/accel/adis16201.c | 34 ++++++++++++++++++++--------------
> > 1 file changed, 20 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/staging/iio/accel/adis16201.c b/drivers/staging/iio/accel/adis16201.c
> > index d6c8658..6d84eae 100644
> > --- a/drivers/staging/iio/accel/adis16201.c
> > +++ b/drivers/staging/iio/accel/adis16201.c
> > @@ -150,6 +150,11 @@
> >
> > #define ADIS16201_ERROR_ACTIVE BIT(14)
> >
> > +struct adis16201_state {
> > + struct adis adis; /* adis device */
> > + struct mutex lock; /* protect state changes */
> > +};
> > +
> > enum adis16201_scan {
> > ADIS16201_SCAN_ACC_X,
> > ADIS16201_SCAN_ACC_Y,
> > @@ -172,7 +177,7 @@ static int adis16201_read_raw(struct iio_dev *indio_dev,
> > int *val, int *val2,
> > long mask)
> > {
> > - struct adis *st = iio_priv(indio_dev);
> > + struct adis16201_state *st = iio_priv(indio_dev);
> > int ret;
> > int bits;
> > u8 addr;
> > @@ -223,17 +228,17 @@ static int adis16201_read_raw(struct iio_dev *indio_dev,
> > default:
> > return -EINVAL;
> > }
> > - mutex_lock(&indio_dev->mlock);
> > + mutex_lock(&st->lock);
>
> I think this lock could simply be removed completely. adis_read_reg_16() is
> already atomic and there is nothing else to protect here.
I believed the lock was put in here to prevent race condition in this
line:
addr = adis16201_addresses[chan->scan_index];
Is that not the case here?
>
> > addr = adis16201_addresses[chan->scan_index];
> > - ret = adis_read_reg_16(st, addr, &val16);
> > + ret = adis_read_reg_16(&st->adis, addr, &val16);
> > if (ret) {
> > - mutex_unlock(&indio_dev->mlock);
> > + mutex_unlock(&st->lock);
> > return ret;
> > }
> > val16 &= (1 << bits) - 1;
> > val16 = (s16)(val16 << (16 - bits)) >> (16 - bits);
> > *val = val16;
> > - mutex_unlock(&indio_dev->mlock);
> > + mutex_unlock(&st->lock);
> > return IIO_VAL_INT;
> > }
> > return -EINVAL;
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-03-14 18:44 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-13 23:58 [PATCH] staging: iio: accel: replace mlock with driver private lock Aishwarya Pant
2017-03-14 17:11 ` Lars-Peter Clausen
2017-03-14 1:05 ` Aishwarya Pant
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.