All of lore.kernel.org
 help / color / mirror / Atom feed
* [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-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
* 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
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.