[PATCH 02/14] iio: pressure: mprls0025pa: remove redundant mutex

Petre Rodan posted 14 patches 1 month, 3 weeks ago
There is a newer version of this series
[PATCH 02/14] iio: pressure: mprls0025pa: remove redundant mutex
Posted by Petre Rodan 1 month, 3 weeks ago
Remove the redundant mutex since both i2c and spi transfer functions
provide their own locking mechanism.

Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
---
 drivers/iio/pressure/mprls0025pa.c | 6 ------
 drivers/iio/pressure/mprls0025pa.h | 3 ---
 2 files changed, 9 deletions(-)

diff --git a/drivers/iio/pressure/mprls0025pa.c b/drivers/iio/pressure/mprls0025pa.c
index 2336f2760eae..d3e76eed07e6 100644
--- a/drivers/iio/pressure/mprls0025pa.c
+++ b/drivers/iio/pressure/mprls0025pa.c
@@ -188,7 +188,6 @@ static void mpr_reset(struct mpr_data *data)
  * If there is an end of conversion (EOC) interrupt registered the function
  * waits for a maximum of one second for the interrupt.
  *
- * Context: The function can sleep and data->lock should be held when calling it
  * Return:
  * * 0		- OK, the pressure value could be read
  * * -ETIMEDOUT	- Timeout while waiting for the EOC interrupt or busy flag is
@@ -273,7 +272,6 @@ static irqreturn_t mpr_trigger_handler(int irq, void *p)
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct mpr_data *data = iio_priv(indio_dev);
 
-	mutex_lock(&data->lock);
 	ret = mpr_read_pressure(data, &data->chan.pres);
 	if (ret < 0)
 		goto err;
@@ -282,7 +280,6 @@ static irqreturn_t mpr_trigger_handler(int irq, void *p)
 					   iio_get_time_ns(indio_dev));
 
 err:
-	mutex_unlock(&data->lock);
 	iio_trigger_notify_done(indio_dev->trig);
 
 	return IRQ_HANDLED;
@@ -300,9 +297,7 @@ static int mpr_read_raw(struct iio_dev *indio_dev,
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
-		mutex_lock(&data->lock);
 		ret = mpr_read_pressure(data, &pressure);
-		mutex_unlock(&data->lock);
 		if (ret < 0)
 			return ret;
 		*val = pressure;
@@ -342,7 +337,6 @@ int mpr_common_probe(struct device *dev, const struct mpr_ops *ops, int irq)
 	data->ops = ops;
 	data->irq = irq;
 
-	mutex_init(&data->lock);
 	init_completion(&data->completion);
 
 	indio_dev->name = "mprls0025pa";
diff --git a/drivers/iio/pressure/mprls0025pa.h b/drivers/iio/pressure/mprls0025pa.h
index d62a018eaff3..d38ac750e392 100644
--- a/drivers/iio/pressure/mprls0025pa.h
+++ b/drivers/iio/pressure/mprls0025pa.h
@@ -14,7 +14,6 @@
 #include <linux/completion.h>
 #include <linux/delay.h>
 #include <linux/device.h>
-#include <linux/mutex.h>
 #include <linux/stddef.h>
 #include <linux/types.h>
 
@@ -44,7 +43,6 @@ enum mpr_func_id {
  * struct mpr_data
  * @dev: current device structure
  * @ops: functions that implement the sensor reads/writes, bus init
- * @lock: access to device during read
  * @pmin: minimal pressure in pascal
  * @pmax: maximal pressure in pascal
  * @function: transfer function
@@ -66,7 +64,6 @@ enum mpr_func_id {
 struct mpr_data {
 	struct device		*dev;
 	const struct mpr_ops	*ops;
-	struct mutex		lock;
 	u32			pmin;
 	u32			pmax;
 	enum mpr_func_id	function;

-- 
2.51.2
Re: [PATCH 02/14] iio: pressure: mprls0025pa: remove redundant mutex
Posted by Marcelo Schmitt 1 month, 3 weeks ago
On 12/18, Petre Rodan wrote:
> Remove the redundant mutex since both i2c and spi transfer functions
> provide their own locking mechanism.
I don't think that is enough to safely dismiss the mutex lock. There could be
concurrent calls to mpr_read_pressure(). E.g., buffer is enabled and user issues
a single-shot read. To avoid the potential concurrent read (without using the
driver mutex), do iio_device_claim_direct() before calling mpr_read_pressure()
in the IIO_CHAN_INFO_RAW case.
Re: [PATCH 02/14] iio: pressure: mprls0025pa: remove redundant mutex
Posted by Jonathan Cameron 1 month, 2 weeks ago
On Sat, 20 Dec 2025 01:45:01 -0300
Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote:

> On 12/18, Petre Rodan wrote:
> > Remove the redundant mutex since both i2c and spi transfer functions
> > provide their own locking mechanism.  
> I don't think that is enough to safely dismiss the mutex lock. There could be
> concurrent calls to mpr_read_pressure(). E.g., buffer is enabled and user issues
> a single-shot read. To avoid the potential concurrent read (without using the
> driver mutex), do iio_device_claim_direct() before calling mpr_read_pressure()
> in the IIO_CHAN_INFO_RAW case. 
> 
This is a little messier.

Direct claiming is about preventing transitions between
modes (buffered, polled)
It is an implementation quirk that it only allows one claimer of a mode
at a time. (It would be hard to change that now, as we'd likely expose
many corner cases in drivers, but still the meaning is not the same
as a lock).

If the locking is needed to protect bus access sequences that is a driver
specific thing that should be done with a driver specific lock.  So things
like RWM sequences should never rely on claiming direct mode to serialize
things.

Now, it is also possible that all the sequences only make sense if
we are not in buffered mode, in which case we should be claiming direct
mode as suggested by Marcelo. That is typically an 'as well' thing rather
than instead of a local lock. If it is all about preventing concurrency with
a sequence that only happens in buffered mode, then maybe claiming direct
mode to avoid that is enough.

Jonathan