Skip to contentSkip to conversation

staging: iio: adis16240: Replace mlock with driver private lock.

PATCH In review Varsha Rao 2 participants 5 messages Mar 13, 2017 1 message not in the archive

Subsystems (4)
Staging IIO Analog Devices IIO drivers Staging - industrial io
lore
Share & tools
lore

Varsha Rao

author
#1 lore raw

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

In reply to a message not in the archive

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 <[email protected]>
---
 drivers/staging/iio/accel/adis16240.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)
drivers/staging/iio/accel/adis16240.c +9 -8
index 27d7f6a..c182b2d 100644
@@ -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;
Signature · 1 line
2.9.3
author
#4 lore raw

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

In reply to a message not in the archive

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

Signed-off-by Varsha Rao <[email protected]>
---
 drivers/staging/iio/accel/adis16240.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/iio/accel/adis16240.c +1 -1
index c182b2d..aa3030c 100644
@@ -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);
Signature · 1 line
2.9.3
#5 lore raw

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

Quoted 13 lines · Varsha Rao

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

Signed-off-by Varsha Rao <[email protected]>
---
 drivers/staging/iio/accel/adis16240.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/iio/accel/adis16240.c +0 -0
index c182b2d..aa3030c 100644

drivers/staging/iio/accel/adis16240.c static ssize_t adis16240_read_12bit_signed(struct device *dev, · line 234

Quoted 5 lines
drivers/staging/iio/accel/adis16240.c +1 -1
@@ -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.

Quoted 4 lines
drivers/staging/iio/accel/adis16240.c +0 -0
 	mutex_lock(&st->txrx_lock);
 	ret =  adis16240_spi_read_signed(dev, attr, buf, 12);
 	mutex_unlock(&st->txrx_lock);
#2 lore raw
In reply to Varsha Rao
Quoted 19 lines · 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 <[email protected]>
---
 drivers/staging/iio/accel/adis16240.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)
drivers/staging/iio/accel/adis16240.c +0 -0
index 27d7f6a..c182b2d 100644

drivers/staging/iio/accel/adis16240.c static ssize_t adis16240_read_12bit_signed(struct device *dev, · line 235

Quoted 8 lines
drivers/staging/iio/accel/adis16240.c +2 -1
@@ -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.

Quoted 45 lines
drivers/staging/iio/accel/adis16240.c +7 -7
 	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;
author
#3 lore raw
Quoted 14 lines

Review static ssize_t adis16240_read_12bit_signed(struct

Quoted 4 lines

+++ b/drivers/staging/iio/accel/adis16240.c

(quoted diff) +0 -0
@@ -229,11 +229,12 @@ static ssize_t adis16240_read_12bit_signed(struct

device *dev,

drivers/staging/iio/accel/adis16240.c static ssize_t adis16240_read_12bit_signed(struct device *dev, · line 235

Quoted 8 lines
drivers/staging/iio/accel/adis16240.c +2 -1
 { 
         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);
Quoted 7 lines

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?

Quoted 6 lines

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