Skip to contentSkip to conversation

staging: iio: accel: adis16201: remove iio_dev mlock

PATCH In review Aishwarya Pant 3 participants 5 messages Mar 18, 2017 – Mar 20, 2017

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

Aishwarya Pant

author
#0 lore raw

PATCH v3 1/1 staging: iio: accel: adis16201: remove iio_dev mlock

In the driver adis16201 read raw does not require an iio_dev->mlock for reads. It can run concurrently as adis_read_reg_16() is protected by a transaction lock.

Signed-off-by Aishwarya Pant <[email protected]>

---
Changes in v3:
-- Fix change log format
-- Send patch to linuc-iio
Changes in v2:
-- Remove read lock

 drivers/staging/iio/accel/adis16201.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)
drivers/staging/iio/accel/adis16201.c +1 -5
index d6c8658..fd2baf2 100644
@@ -223,17 +223,13 @@ static int adis16201_read_raw(struct iio_dev *indio_dev,
 		default:
 			return -EINVAL;
 		}
-		mutex_lock(&indio_dev->mlock);
 		addr = adis16201_addresses[chan->scan_index];
 		ret = adis_read_reg_16(st, addr, &val16);
-		if (ret) {
-			mutex_unlock(&indio_dev->mlock);
+		if (ret)
 			return ret;
-		}
 		val16 &= (1 << bits) - 1;
 		val16 = (s16)(val16 << (16 - bits)) >> (16 - bits);
 		*val = val16;
-		mutex_unlock(&indio_dev->mlock);
 		return IIO_VAL_INT;
 	}
 	return -EINVAL;
Signature · 1 line
2.7.4
#1 lore raw
Quoted 3 lines · Aishwarya Pant

In the driver adis16201 read raw does not require an iio_dev->mlock for reads. It can run concurrently as adis_read_reg_16() is protected by a transaction lock.

Hi Aishwarya,
Perhaps the Subject & logs could be made clearer.
The subject line may suggest you removed mlock form iio_dev stuct. You removed the locking on raw reads of IIO_CHAN_INFO_CALIBBIAS. alisons

Quoted 45 lines
Signed-off-by Aishwarya Pant <[email protected]>

---
Changes in v3:
-- Fix change log format
-- Send patch to linuc-iio
Changes in v2:
-- Remove read lock

 drivers/staging/iio/accel/adis16201.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)
drivers/staging/iio/accel/adis16201.c +1 -5
index d6c8658..fd2baf2 100644
@@ -223,17 +223,13 @@ static int adis16201_read_raw(struct iio_dev *indio_dev,
 		default:
 			return -EINVAL;
 		}
-		mutex_lock(&indio_dev->mlock);
 		addr = adis16201_addresses[chan->scan_index];
 		ret = adis_read_reg_16(st, addr, &val16);
-		if (ret) {
-			mutex_unlock(&indio_dev->mlock);
+		if (ret)
 			return ret;
-		}
 		val16 &= (1 << bits) - 1;
 		val16 = (s16)(val16 << (16 - bits)) >> (16 - bits);
 		*val = val16;
-		mutex_unlock(&indio_dev->mlock);
 		return IIO_VAL_INT;
 	}
 	return -EINVAL;
Signature · 8 lines
2.7.4

-- 
You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20170317214356.GA8369%40aishwarya.
For more options, visit https://groups.google.com/d/optout.
maintainer
#2 lore raw
In reply to Aishwarya Pant
Quoted 5 lines · Aishwarya Pant

In the driver adis16201 read raw does not require an iio_dev->mlock for reads. It can run concurrently as adis_read_reg_16() is protected by a transaction lock.

Signed-off-by Aishwarya Pant <[email protected]>

Great, one suggestion for a follow up patch below.

Applied to the togreg branch of iio.git and pushed out as testing for the autobuilders to play with it.

Thanks,

Jonathan

Quoted 16 lines

---
Changes in v3:
-- Fix change log format
-- Send patch to linuc-iio
Changes in v2:
-- Remove read lock

 drivers/staging/iio/accel/adis16201.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)
drivers/staging/iio/accel/adis16201.c +0 -0
index d6c8658..fd2baf2 100644

drivers/staging/iio/accel/adis16201.c static int adis16201_read_raw(struct iio_dev *indio_dev, · line 227

Quoted 6 lines
drivers/staging/iio/accel/adis16201.c +0 -1
@@ -223,17 +223,13 @@ static int adis16201_read_raw(struct iio_dev *indio_dev,
 		default:
 			return -EINVAL;
 		}
-		mutex_lock(&indio_dev->mlock);
 		addr = adis16201_addresses[chan->scan_index];
 		ret = adis_read_reg_16(st, addr, &val16);

Not a lot of point in having the local variable addr given it's only used here. Perhaps you could follow up with a patch getting rid of it?

Quoted 13 lines
drivers/staging/iio/accel/adis16201.c +1 -4
-		if (ret) {
-			mutex_unlock(&indio_dev->mlock);
+		if (ret)
 			return ret;
-		}
 		val16 &= (1 << bits) - 1;
 		val16 = (s16)(val16 << (16 - bits)) >> (16 - bits);
 		*val = val16;
-		mutex_unlock(&indio_dev->mlock);
 		return IIO_VAL_INT;
 	}
 	return -EINVAL;
author
#3 lore raw
Quoted 25 lines · Jonathan Cameron
Quoted 5 lines · Aishwarya Pant

In the driver adis16201 read raw does not require an iio_dev->mlock for reads. It can run concurrently as adis_read_reg_16() is protected by a transaction lock.

Signed-off-by Aishwarya Pant <[email protected]>

Great, one suggestion for a follow up patch below.

Applied to the togreg branch of iio.git and pushed out as testing for the autobuilders to play with it.

Thanks,

Jonathan

Quoted 11 lines

---
Changes in v3:
-- Fix change log format
-- Send patch to linuc-iio
Changes in v2:
-- Remove read lock

 drivers/staging/iio/accel/adis16201.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)
Quoted 14 lines

drivers/staging/iio/accel/adis16201.c static int adis16201_read_raw(struct iio_dev *indio_dev, · line 227

Quoted 11 lines
drivers/staging/iio/accel/adis16201.c +0 -1
index d6c8658..fd2baf2 100644
@@ -223,17 +223,13 @@ static int adis16201_read_raw(struct iio_dev *indio_dev,
 		default:
 			return -EINVAL;
 		}
-		mutex_lock(&indio_dev->mlock);
 		addr = adis16201_addresses[chan->scan_index];
 		ret = adis_read_reg_16(st, addr, &val16);

Not a lot of point in having the local variable addr given it's only used here. Perhaps you could follow up with a patch getting rid of it?

Sure. I will look at clean-up opportunities here.

But I'm unsure of where I should be rebasing from? greg's staging tree or the togreg branch of the iio tree.
https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/
According to the link, togreg was last updated 2 weeks ago.

thanks
Aishwarya

Quoted 14 lines
Quoted 13 lines
drivers/staging/iio/accel/adis16201.c +1 -4
-		if (ret) {
-			mutex_unlock(&indio_dev->mlock);
+		if (ret)
 			return ret;
-		}
 		val16 &= (1 << bits) - 1;
 		val16 = (s16)(val16 << (16 - bits)) >> (16 - bits);
 		*val = val16;
-		mutex_unlock(&indio_dev->mlock);
 		return IIO_VAL_INT;
 	}
 	return -EINVAL;
maintainer
#4 lore raw
Quoted 43 lines · Aishwarya Pant
Quoted 25 lines · Jonathan Cameron
Quoted 5 lines · Aishwarya Pant

In the driver adis16201 read raw does not require an iio_dev->mlock for reads. It can run concurrently as adis_read_reg_16() is protected by a transaction lock.

Signed-off-by Aishwarya Pant <[email protected]>

Great, one suggestion for a follow up patch below.

Applied to the togreg branch of iio.git and pushed out as testing for the autobuilders to play with it.

Thanks,

Jonathan

Quoted 11 lines

---
Changes in v3:
-- Fix change log format
-- Send patch to linuc-iio
Changes in v2:
-- Remove read lock

 drivers/staging/iio/accel/adis16201.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)
Quoted 14 lines

drivers/staging/iio/accel/adis16201.c static int adis16201_read_raw(struct iio_dev *indio_dev, · line 227

Quoted 11 lines
drivers/staging/iio/accel/adis16201.c +0 -1
index d6c8658..fd2baf2 100644
@@ -223,17 +223,13 @@ static int adis16201_read_raw(struct iio_dev *indio_dev,
 		default:
 			return -EINVAL;
 		}
-		mutex_lock(&indio_dev->mlock);
 		addr = adis16201_addresses[chan->scan_index];
 		ret = adis_read_reg_16(st, addr, &val16);

Not a lot of point in having the local variable addr given it's only used here. Perhaps you could follow up with a patch getting rid of it?

Sure. I will look at clean-up opportunities here.

Quoted 8 lines

But I'm unsure of where I should be rebasing from? greg's staging tree or the togreg branch of the iio tree.
https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/
According to the link, togreg was last updated 2 weeks ago.

thanks
Aishwarya

Hohum. That's me forgetting to push it out before applying new patches.

The togreg branch is non rebasing, so I try to get build feedback from the 0-day service (amazing system that builds 100s of configurations for every tree that changes at least a few times a day).

After that comes in good I push out (in theory). On busy days I tend to forget. So whilst in theory I'd advise against it, the testing branch is moderately safe to send patches against.

It's all rather high churn in some of these drivers at the moment for some reason ;) Alison did warn me before posting this task!

Gah, apparently I got really distracted yesterday and didn't push out at all. Should now be up as testing. If all good should get it out as togreg tomorrow.

Jonathan

Quoted 20 lines
Quoted 15 lines
Quoted 4 lines
drivers/staging/iio/accel/adis16201.c +0 -2
-		if (ret) {
-			mutex_unlock(&indio_dev->mlock);

drivers/staging/iio/accel/adis16201.c static int adis16201_read_raw(struct iio_dev *indio_dev, · line 235

Quoted 10 lines
drivers/staging/iio/accel/adis16201.c +1 -2
+		if (ret)
 			return ret;
-		}
 		val16 &= (1 << bits) - 1;
 		val16 = (s16)(val16 << (16 - bits)) >> (16 - bits);
 		*val = val16;
-		mutex_unlock(&indio_dev->mlock);
 		return IIO_VAL_INT;
 	}
 	return -EINVAL;

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html