[PATCH 1/2] staging: iio: adis16240: Replace mlock with driver private lock.

[PATCH 1/2] staging: iio: adis16240: Replace mlock with driver private lock.

[Varsha Rao]

The IIO subsystem is redefining iio_dev->mlock to be used by the IIO
core only for protecting device operating mode changes. ie. Changes
between INDIO_DIRECT_MODE, INDIO_BUFFER_* modes.

In this driver, mlock was being used to protect hardware state changes.
Replace it with a lock in the devices global data. Also declare and
initialize variable st which is pointer to struct adis, to access
txrx_lock.

Signed-off-by: Varsha Rao <rvarsha016@gmail.com>
---
 drivers/staging/iio/accel/adis16240.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/iio/accel/adis16240.c b/drivers/staging/iio/accel/adis16240.c
index 27d7f6a..c182b2d 100644
--- a/drivers/staging/iio/accel/adis16240.c
+++ b/drivers/staging/iio/accel/adis16240.c
@@ -229,11 +229,12 @@ static ssize_t adis16240_read_12bit_signed(struct device *dev,
 {
 	ssize_t ret;
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct adis *st = iio_priv(indio_dev);
 
 	/* Take the iio_dev status lock */
-	mutex_lock(&indio_dev->mlock);
+	mutex_lock(&st->txrx_lock);
 	ret =  adis16240_spi_read_signed(dev, attr, buf, 12);
-	mutex_unlock(&indio_dev->mlock);
+	mutex_unlock(&st->txrx_lock);
 
 	return ret;
 }
@@ -295,31 +296,31 @@ static int adis16240_read_raw(struct iio_dev *indio_dev,
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_CALIBBIAS:
 		bits = 10;
-		mutex_lock(&indio_dev->mlock);
+		mutex_lock(&st->txrx_lock);
 		addr = adis16240_addresses[chan->scan_index][0];
 		ret = adis_read_reg_16(st, addr, &val16);
 		if (ret) {
-			mutex_unlock(&indio_dev->mlock);
+			mutex_unlock(&st->txrx_lock);
 			return ret;
 		}
 		val16 &= (1 << bits) - 1;
 		val16 = (s16)(val16 << (16 - bits)) >> (16 - bits);
 		*val = val16;
-		mutex_unlock(&indio_dev->mlock);
+		mutex_unlock(&st->txrx_lock);
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_PEAK:
 		bits = 10;
-		mutex_lock(&indio_dev->mlock);
+		mutex_lock(&st->txrx_lock);
 		addr = adis16240_addresses[chan->scan_index][1];
 		ret = adis_read_reg_16(st, addr, &val16);
 		if (ret) {
-			mutex_unlock(&indio_dev->mlock);
+			mutex_unlock(&st->txrx_lock);
 			return ret;
 		}
 		val16 &= (1 << bits) - 1;
 		val16 = (s16)(val16 << (16 - bits)) >> (16 - bits);
 		*val = val16;
-		mutex_unlock(&indio_dev->mlock);
+		mutex_unlock(&st->txrx_lock);
 		return IIO_VAL_INT;
 	}
 	return -EINVAL;
-- 
2.9.3

[PATCH 2/2] staging: iio: adis16240: Modify existing comment.

[Varsha Rao]

Modify the existing comment to reflect the changes from mlock to
txrx_lock.

Signed-off-by: Varsha Rao <rvarsha016@gmail.com>
---
 drivers/staging/iio/accel/adis16240.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/iio/accel/adis16240.c b/drivers/staging/iio/accel/adis16240.c
index c182b2d..aa3030c 100644
--- a/drivers/staging/iio/accel/adis16240.c
+++ b/drivers/staging/iio/accel/adis16240.c
@@ -231,7 +231,7 @@ static ssize_t adis16240_read_12bit_signed(struct device *dev,
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct adis *st = iio_priv(indio_dev);
 
-	/* Take the iio_dev status lock */
+	/* Take adis txrx_lock to protect sensor state */
 	mutex_lock(&st->txrx_lock);
 	ret =  adis16240_spi_read_signed(dev, attr, buf, 12);
 	mutex_unlock(&st->txrx_lock);
-- 
2.9.3

Re: [PATCH 2/2] staging: iio: adis16240: Modify existing comment.

[Lars-Peter Clausen]

On 03/13/2017 10:23 AM, Varsha Rao wrote:
> Modify the existing comment to reflect the changes from mlock to
> txrx_lock.
> 
> Signed-off-by: Varsha Rao <rvarsha016@gmail.com>
> ---
>  drivers/staging/iio/accel/adis16240.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/accel/adis16240.c b/drivers/staging/iio/accel/adis16240.c
> index c182b2d..aa3030c 100644
> --- a/drivers/staging/iio/accel/adis16240.c
> +++ b/drivers/staging/iio/accel/adis16240.c
> @@ -231,7 +231,7 @@ static ssize_t adis16240_read_12bit_signed(struct device *dev,
>  	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>  	struct adis *st = iio_priv(indio_dev);
>  
> -	/* Take the iio_dev status lock */
> +	/* Take adis txrx_lock to protect sensor state */

This should be part of the patch that changes the lock.

>  	mutex_lock(&st->txrx_lock);
>  	ret =  adis16240_spi_read_signed(dev, attr, buf, 12);
>  	mutex_unlock(&st->txrx_lock);
> 

Re: [PATCH 1/2] staging: iio: adis16240: Replace mlock with driver private lock.

[Lars-Peter Clausen]

On 03/13/2017 10:23 AM, Varsha Rao wrote:
> The IIO subsystem is redefining iio_dev->mlock to be used by the IIO
> core only for protecting device operating mode changes. ie. Changes
> between INDIO_DIRECT_MODE, INDIO_BUFFER_* modes.
> 
> In this driver, mlock was being used to protect hardware state changes.
> Replace it with a lock in the devices global data. Also declare and
> initialize variable st which is pointer to struct adis, to access
> txrx_lock.
> 
> Signed-off-by: Varsha Rao <rvarsha016@gmail.com>
> ---
>  drivers/staging/iio/accel/adis16240.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/iio/accel/adis16240.c b/drivers/staging/iio/accel/adis16240.c
> index 27d7f6a..c182b2d 100644
> --- a/drivers/staging/iio/accel/adis16240.c
> +++ b/drivers/staging/iio/accel/adis16240.c
> @@ -229,11 +229,12 @@ static ssize_t adis16240_read_12bit_signed(struct device *dev,
>  {
>  	ssize_t ret;
>  	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct adis *st = iio_priv(indio_dev);
>  
>  	/* Take the iio_dev status lock */
> -	mutex_lock(&indio_dev->mlock);
> +	mutex_lock(&st->txrx_lock);

This unfortunately wont work. The adis_read_reg()/adis_write_reg() functions
take the txrx_lock. So this change causes a deadlock, trying to take a lock
that is already locked.

But this lock can probably be removed. It should be safe to run the function
multiple times in parallel.

Same for the other changes in this patch.

>  	ret =  adis16240_spi_read_signed(dev, attr, buf, 12);
> -	mutex_unlock(&indio_dev->mlock);
> +	mutex_unlock(&st->txrx_lock);
>  
>  	return ret;
>  }
> @@ -295,31 +296,31 @@ static int adis16240_read_raw(struct iio_dev *indio_dev,
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_CALIBBIAS:
>  		bits = 10;
> -		mutex_lock(&indio_dev->mlock);
> +		mutex_lock(&st->txrx_lock);
>  		addr = adis16240_addresses[chan->scan_index][0];
>  		ret = adis_read_reg_16(st, addr, &val16);
>  		if (ret) {
> -			mutex_unlock(&indio_dev->mlock);
> +			mutex_unlock(&st->txrx_lock);
>  			return ret;
>  		}
>  		val16 &= (1 << bits) - 1;
>  		val16 = (s16)(val16 << (16 - bits)) >> (16 - bits);
>  		*val = val16;
> -		mutex_unlock(&indio_dev->mlock);
> +		mutex_unlock(&st->txrx_lock);
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_PEAK:
>  		bits = 10;
> -		mutex_lock(&indio_dev->mlock);
> +		mutex_lock(&st->txrx_lock);
>  		addr = adis16240_addresses[chan->scan_index][1];
>  		ret = adis_read_reg_16(st, addr, &val16);
>  		if (ret) {
> -			mutex_unlock(&indio_dev->mlock);
> +			mutex_unlock(&st->txrx_lock);
>  			return ret;
>  		}
>  		val16 &= (1 << bits) - 1;
>  		val16 = (s16)(val16 << (16 - bits)) >> (16 - bits);
>  		*val = val16;
> -		mutex_unlock(&indio_dev->mlock);
> +		mutex_unlock(&st->txrx_lock);
>  		return IIO_VAL_INT;
>  	}
>  	return -EINVAL;
> 

Re: [PATCH 1/2] staging: iio: adis16240: Replace mlock with driver private lock.

[Varsha Rao]

[-- Attachment #1.1: Type: text/plain, Size: 1017 bytes --]



> > +++ b/drivers/staging/iio/accel/adis16240.c 
> > @@ -229,11 +229,12 @@ static ssize_t adis16240_read_12bit_signed(struct 
> device *dev, 
> >  { 
> >          ssize_t ret; 
> >          struct iio_dev *indio_dev = dev_to_iio_dev(dev); 
> > +        struct adis *st = iio_priv(indio_dev); 
> >   
> >          /* Take the iio_dev status lock */ 
> > -        mutex_lock(&indio_dev->mlock); 
> > +        mutex_lock(&st->txrx_lock); 
>
> This unfortunately wont work. The adis_read_reg()/adis_write_reg() 
> functions 
> take the txrx_lock. So this change causes a deadlock, trying to take a 
> lock 
> that is already locked. 
>
>    Yes, it would cause deadlock. Can the lock be placed within the 
   adis_read_reg() function and removing locks from other place?
 

> But this lock can probably be removed. It should be safe to run the 
> function 
> multiple times in parallel. 
>
> Same for the other changes in this patch. 
>
   
   Then should I remove all the locks in this file?
    
   Thanks,
   Varsha

[-- Attachment #1.2: Type: text/html, Size: 1484 bytes --]