[RFC PATCH 0/8] IIO: Use the new cleanup.h magic

[RFC PATCH 0/8] IIO: Use the new cleanup.h magic

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

NB: I've only tested the example driver (dummy) changes - others are
build tested only so I'd like to see at least some testing on the
actual hardware.

Recently support was added to the kernel for automated cleanup based on
scope.  If the relevant variable goes out of scope, a destructor
function is called.

In most cases we are dealing with simple locking in which the guard() and
scoped_guard() magic can be used.

The one common more complex case is
iio_device_claim_direct_mode() / iio_device_release_direct_mode() which
takes a mutex only if we are not in buffered mode.
Typically this is used to avoid interfering with a devices configuration
when we are also streaming data into kernel buffers.  It's semantics
are that it will fail if we are in buffered mode (allow us to return
-EBUSY to userspace to indicate it should come back later) and otherwise
take the iio_dev->mlock and hold it until manually released.

Having looked at the various similar cases in Peter's patch set that
introduced this cleanup magic, I came up with the following:
(patch 1)
+DEFINE_CLASS(iio_claim_direct, struct iio_dev *,
+	     iio_device_release_direct_mode(_T),
+	     ({
+			struct iio_dev *dev;
+			int d = iio_device_claim_direct_mode(_T);
+
+			if (d < 0)
+				dev = ERR_PTR(d);
+			else
+				dev = _T;
+			dev;
+	     }),
+	     struct iio_dev *_T);
+

This returns a copy of the struct iio_dev pointer passed in if the lock
was taken, if not it returns ERR_PTR(-EBUSY).
iio_device_release_direct_mode() now safely handles ERR_PTR() and so
cleanup only unlocks the mutex if this succeeded.

It is used as

+ *	CLASS(iio_claim_direct, claimed_dev)(indio_dev);
+ *	if (IS_ERR(claimed_dev))
+ *		return PTR_ERR(claimed_dev);

when claimed_dev goes out of scope, iio_device_release_direct_mode() is
called.  I'm looking for review on whether this is correct / the simplest
approach.

This series is meant to introduce how this is used including adding it
to the dummy / example driver. I've converted a random set of drivers
over and if people are happy, many more would benefit from this treatment.
It's an RFC mostly to indicate to people that there may be dragons!

Jonathan Cameron (8):
  iio: locking: introduce __cleanup() based direct mode claiming
    infrastructure
  iio: dummy: Add use of new automated cleanup of locks and direct mode
    claiming.
  iio: accel: adxl367: Use automated cleanup for locks and iio direct
    mode.
  iio: imu: bmi323: Use cleanup handling for
    iio_device_claim_direct_mode()
  iio: adc: max1363: Use automatic cleanup for locks and iio mode
    claiming.
  iio: proximity: sx9360: Use automated cleanup for locks and IIO mode
    claiming.
  iio: proximity: sx9324: Use automated cleanup for locks and IIO mode
    claiming.
  iio: proximity: sx9310: Use automated cleanup for locks and IIO mode
    claiming.

 drivers/iio/accel/adxl367.c          | 214 ++++++++++-----------------
 drivers/iio/adc/max1363.c            |  63 ++++----
 drivers/iio/dummy/iio_simple_dummy.c | 145 +++++++++---------
 drivers/iio/imu/bmi323/bmi323_core.c |  61 ++++----
 drivers/iio/industrialio-core.c      |   4 +
 drivers/iio/proximity/sx9310.c       | 120 ++++++---------
 drivers/iio/proximity/sx9324.c       | 113 ++++++--------
 drivers/iio/proximity/sx9360.c       | 117 ++++++---------
 include/linux/iio/iio.h              |  25 ++++
 9 files changed, 368 insertions(+), 494 deletions(-)

-- 
2.42.0

[RFC PATCH 1/8] iio: locking: introduce __cleanup() based direct mode claiming infrastructure

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Allows use of:

	CLASS(iio_claim_direct, claimed_dev)(indio_dev);
	if (IS_ERR(claimed_dev))
		return PTR_ERR(claimed_dev);

	st = iio_priv(claimed_dev);

to automatically call iio_device_release_direct_mode() based on scope.
Typically seen in combination with local device specific locks which
are already have automated cleanup options via guard(mutex)(&st->lock)
and scoped_guard().  Using both together allows most error handling to
be automated.

Note that whilst this pattern results in a struct iio_dev *claimed_dev
that can be used, it is not necessary to do so as long as that pointer
has been checked for errors as in the example.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/industrialio-core.c |  4 ++++
 include/linux/iio/iio.h         | 25 +++++++++++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index c77745b594bd..93bfad105eb5 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -2065,6 +2065,10 @@ EXPORT_SYMBOL_GPL(iio_device_claim_direct_mode);
  */
 void iio_device_release_direct_mode(struct iio_dev *indio_dev)
 {
+	/* Auto cleanup can result in this being called with an ERR_PTR */
+	if (IS_ERR(indio_dev))
+		return;
+
 	mutex_unlock(&to_iio_dev_opaque(indio_dev)->mlock);
 }
 EXPORT_SYMBOL_GPL(iio_device_release_direct_mode);
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index d0ce3b71106a..11c42170fda1 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -9,6 +9,7 @@
 
 #include <linux/device.h>
 #include <linux/cdev.h>
+#include <linux/cleanup.h>
 #include <linux/slab.h>
 #include <linux/iio/types.h>
 /* IIO TODO LIST */
@@ -644,6 +645,30 @@ int __devm_iio_device_register(struct device *dev, struct iio_dev *indio_dev,
 int iio_push_event(struct iio_dev *indio_dev, u64 ev_code, s64 timestamp);
 int iio_device_claim_direct_mode(struct iio_dev *indio_dev);
 void iio_device_release_direct_mode(struct iio_dev *indio_dev);
+/*
+ * Auto cleanup version of iio_device_claim_direct_mode,
+ *
+ *	CLASS(iio_claim_direct, claimed_dev)(indio_dev);
+ *	if (IS_ERR(claimed_dev))
+ *		return PTR_ERR(claimed_dev);
+ *
+ *	st = iio_priv(claimed_dev);
+ *	....
+ */
+DEFINE_CLASS(iio_claim_direct, struct iio_dev *,
+	     iio_device_release_direct_mode(_T),
+	     ({
+			struct iio_dev *dev;
+			int d = iio_device_claim_direct_mode(_T);
+
+			if (d < 0)
+				dev = ERR_PTR(d);
+			else
+				dev = _T;
+			dev;
+	     }),
+	     struct iio_dev *_T);
+
 int iio_device_claim_buffer_mode(struct iio_dev *indio_dev);
 void iio_device_release_buffer_mode(struct iio_dev *indio_dev);
 
-- 
2.42.0

[RFC PATCH 2/8] iio: dummy: Add use of new automated cleanup of locks and direct mode claiming.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

As this driver is faking buffered access there isn't strictly any need
to prevent direct access whilst it is running. However, we do want this
to look like real devices where such restrictions are commonly needed.
Adding such protections also allow testing the automated cleanup via

CLASS(iio_claim_direct, claimed_dev)(indio_dev);
if (IS_ERR(claimed_dev))
	return PTR_ERR(claimed_dev);

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/dummy/iio_simple_dummy.c | 145 ++++++++++++++-------------
 1 file changed, 74 insertions(+), 71 deletions(-)

diff --git a/drivers/iio/dummy/iio_simple_dummy.c b/drivers/iio/dummy/iio_simple_dummy.c
index c24f609c2ade..e412f6a84fc3 100644
--- a/drivers/iio/dummy/iio_simple_dummy.c
+++ b/drivers/iio/dummy/iio_simple_dummy.c
@@ -282,66 +282,73 @@ static int iio_dummy_read_raw(struct iio_dev *indio_dev,
 			      int *val2,
 			      long mask)
 {
+	/*
+	 * Whilst it can be elegant to use the claimed device for this, it's not necessary
+	 * where we have a mixture of paths accessing under that protection, to prevent
+	 * access that might disrupt the buffered flow, and those that only care about
+	 * protection of the device specific state.
+	 */
 	struct iio_dummy_state *st = iio_priv(indio_dev);
-	int ret = -EINVAL;
 
-	mutex_lock(&st->lock);
 	switch (mask) {
-	case IIO_CHAN_INFO_RAW: /* magic value - channel value read */
+	case IIO_CHAN_INFO_RAW: { /* magic value - channel value read */
+		CLASS(iio_claim_direct, claimed_dev)(indio_dev);
+		if (IS_ERR(claimed_dev))
+			return PTR_ERR(claimed_dev);
+		guard(mutex)(&st->lock);
+
 		switch (chan->type) {
 		case IIO_VOLTAGE:
 			if (chan->output) {
 				/* Set integer part to cached value */
 				*val = st->dac_val;
-				ret = IIO_VAL_INT;
+				return IIO_VAL_INT;
 			} else if (chan->differential) {
 				if (chan->channel == 1)
 					*val = st->differential_adc_val[0];
 				else
 					*val = st->differential_adc_val[1];
-				ret = IIO_VAL_INT;
+				return IIO_VAL_INT;
 			} else {
 				*val = st->single_ended_adc_val;
-				ret = IIO_VAL_INT;
+				return IIO_VAL_INT;
 			}
-			break;
+
 		case IIO_ACCEL:
 			*val = st->accel_val;
-			ret = IIO_VAL_INT;
-			break;
+			return IIO_VAL_INT;
 		default:
-			break;
+			return -EINVAL;
 		}
-		break;
-	case IIO_CHAN_INFO_PROCESSED:
+	}
+	case IIO_CHAN_INFO_PROCESSED: {
+		CLASS(iio_claim_direct, claimed_dev)(indio_dev);
+		if (IS_ERR(claimed_dev))
+			return PTR_ERR(claimed_dev);
+		guard(mutex)(&st->lock);
 		switch (chan->type) {
 		case IIO_STEPS:
 			*val = st->steps;
-			ret = IIO_VAL_INT;
-			break;
+			return IIO_VAL_INT;
 		case IIO_ACTIVITY:
 			switch (chan->channel2) {
 			case IIO_MOD_RUNNING:
 				*val = st->activity_running;
-				ret = IIO_VAL_INT;
-				break;
+				return IIO_VAL_INT;
 			case IIO_MOD_WALKING:
 				*val = st->activity_walking;
-				ret = IIO_VAL_INT;
-				break;
+				return IIO_VAL_INT;
 			default:
-				break;
+				return -EINVAL;
 			}
-			break;
 		default:
-			break;
+			return -EINVAL;
 		}
-		break;
+	}
 	case IIO_CHAN_INFO_OFFSET:
 		/* only single ended adc -> 7 */
 		*val = 7;
-		ret = IIO_VAL_INT;
-		break;
+		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
 		switch (chan->type) {
 		case IIO_VOLTAGE:
@@ -350,60 +357,57 @@ static int iio_dummy_read_raw(struct iio_dev *indio_dev,
 				/* only single ended adc -> 0.001333 */
 				*val = 0;
 				*val2 = 1333;
-				ret = IIO_VAL_INT_PLUS_MICRO;
-				break;
+				return IIO_VAL_INT_PLUS_MICRO;
 			case 1:
 				/* all differential adc -> 0.000001344 */
 				*val = 0;
 				*val2 = 1344;
-				ret = IIO_VAL_INT_PLUS_NANO;
+				return IIO_VAL_INT_PLUS_NANO;
+			default:
+				return -EINVAL;
 			}
-			break;
 		default:
-			break;
+			return -EINVAL;
 		}
-		break;
-	case IIO_CHAN_INFO_CALIBBIAS:
+	case IIO_CHAN_INFO_CALIBBIAS: {
+		guard(mutex)(&st->lock);
 		/* only the acceleration axis - read from cache */
 		*val = st->accel_calibbias;
-		ret = IIO_VAL_INT;
-		break;
-	case IIO_CHAN_INFO_CALIBSCALE:
+		return IIO_VAL_INT;
+	}
+	case IIO_CHAN_INFO_CALIBSCALE: {
+		guard(mutex)(&st->lock);
 		*val = st->accel_calibscale->val;
 		*val2 = st->accel_calibscale->val2;
-		ret = IIO_VAL_INT_PLUS_MICRO;
-		break;
+		return IIO_VAL_INT_PLUS_MICRO;
+	}
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		*val = 3;
 		*val2 = 33;
-		ret = IIO_VAL_INT_PLUS_NANO;
-		break;
-	case IIO_CHAN_INFO_ENABLE:
+		return IIO_VAL_INT_PLUS_NANO;
+	case IIO_CHAN_INFO_ENABLE: {
+		guard(mutex)(&st->lock);
 		switch (chan->type) {
 		case IIO_STEPS:
 			*val = st->steps_enabled;
-			ret = IIO_VAL_INT;
-			break;
+			return IIO_VAL_INT;
 		default:
-			break;
+			return -EINVAL;
 		}
-		break;
-	case IIO_CHAN_INFO_CALIBHEIGHT:
+	}
+	case IIO_CHAN_INFO_CALIBHEIGHT: {
+		guard(mutex)(&st->lock);
 		switch (chan->type) {
 		case IIO_STEPS:
 			*val = st->height;
-			ret = IIO_VAL_INT;
-			break;
+			return IIO_VAL_INT;
 		default:
-			break;
+			return -EINVAL;
 		}
-		break;
-
+	}
 	default:
-		break;
+		return -EINVAL;
 	}
-	mutex_unlock(&st->lock);
-	return ret;
 }
 
 /**
@@ -436,10 +440,10 @@ static int iio_dummy_write_raw(struct iio_dev *indio_dev,
 			if (chan->output == 0)
 				return -EINVAL;
 
-			/* Locking not required as writing single value */
-			mutex_lock(&st->lock);
-			st->dac_val = val;
-			mutex_unlock(&st->lock);
+			scoped_guard(mutex, &st->lock) {
+				/* Locking not required as writing single value */
+				st->dac_val = val;
+			}
 			return 0;
 		default:
 			return -EINVAL;
@@ -447,9 +451,9 @@ static int iio_dummy_write_raw(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_PROCESSED:
 		switch (chan->type) {
 		case IIO_STEPS:
-			mutex_lock(&st->lock);
-			st->steps = val;
-			mutex_unlock(&st->lock);
+			scoped_guard(mutex, &st->lock) {
+				st->steps = val;
+			}
 			return 0;
 		case IIO_ACTIVITY:
 			if (val < 0)
@@ -470,30 +474,29 @@ static int iio_dummy_write_raw(struct iio_dev *indio_dev,
 		default:
 			return -EINVAL;
 		}
-	case IIO_CHAN_INFO_CALIBSCALE:
-		mutex_lock(&st->lock);
+	case IIO_CHAN_INFO_CALIBSCALE: {
+		guard(mutex)(&st->lock);
 		/* Compare against table - hard matching here */
 		for (i = 0; i < ARRAY_SIZE(dummy_scales); i++)
 			if (val == dummy_scales[i].val &&
 			    val2 == dummy_scales[i].val2)
 				break;
 		if (i == ARRAY_SIZE(dummy_scales))
-			ret = -EINVAL;
-		else
-			st->accel_calibscale = &dummy_scales[i];
-		mutex_unlock(&st->lock);
+			return  -EINVAL;
+		st->accel_calibscale = &dummy_scales[i];
 		return ret;
+	}
 	case IIO_CHAN_INFO_CALIBBIAS:
-		mutex_lock(&st->lock);
-		st->accel_calibbias = val;
-		mutex_unlock(&st->lock);
+		scoped_guard(mutex, &st->lock) {
+			st->accel_calibbias = val;
+		}
 		return 0;
 	case IIO_CHAN_INFO_ENABLE:
 		switch (chan->type) {
 		case IIO_STEPS:
-			mutex_lock(&st->lock);
-			st->steps_enabled = val;
-			mutex_unlock(&st->lock);
+			scoped_guard(mutex, &st->lock) {
+				st->steps_enabled = val;
+			}
 			return 0;
 		default:
 			return -EINVAL;
-- 
2.42.0

[RFC PATCH 3/8] iio: accel: adxl367: Use automated cleanup for locks and iio direct mode.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Switching to the CLASS based cleanup for iio_device_direct_mode_claim()
and to guard() based unlocking of mutexes simplifies error handling
by allowing direct returns when an error is encountered.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/accel/adxl367.c | 214 ++++++++++++++----------------------
 1 file changed, 80 insertions(+), 134 deletions(-)

diff --git a/drivers/iio/accel/adxl367.c b/drivers/iio/accel/adxl367.c
index 90b7ae6d42b7..50022f6b0f07 100644
--- a/drivers/iio/accel/adxl367.c
+++ b/drivers/iio/accel/adxl367.c
@@ -339,22 +339,17 @@ static int adxl367_set_act_threshold(struct adxl367_state *st,
 {
 	int ret;
 
-	mutex_lock(&st->lock);
+	guard(mutex)(&st->lock);
 
 	ret = adxl367_set_measure_en(st, false);
 	if (ret)
-		goto out;
+		return ret;
 
 	ret = _adxl367_set_act_threshold(st, act, threshold);
 	if (ret)
-		goto out;
-
-	ret = adxl367_set_measure_en(st, true);
-
-out:
-	mutex_unlock(&st->lock);
+		return ret;
 
-	return ret;
+	return adxl367_set_measure_en(st, true);
 }
 
 static int adxl367_set_act_proc_mode(struct adxl367_state *st,
@@ -482,25 +477,26 @@ static int adxl367_set_fifo_watermark(struct adxl367_state *st,
 static int adxl367_set_range(struct iio_dev *indio_dev,
 			     enum adxl367_range range)
 {
-	struct adxl367_state *st = iio_priv(indio_dev);
+	struct adxl367_state *st;
 	int ret;
 
-	ret = iio_device_claim_direct_mode(indio_dev);
-	if (ret)
-		return ret;
+	CLASS(iio_claim_direct, claimed_dev)(indio_dev);
+	if (IS_ERR(claimed_dev))
+		return PTR_ERR(claimed_dev);
 
-	mutex_lock(&st->lock);
+	st = iio_priv(claimed_dev);
+	guard(mutex)(&st->lock);
 
 	ret = adxl367_set_measure_en(st, false);
 	if (ret)
-		goto out;
+		return ret;
 
 	ret = regmap_update_bits(st->regmap, ADXL367_REG_FILTER_CTL,
 				 ADXL367_FILTER_CTL_RANGE_MASK,
 				 FIELD_PREP(ADXL367_FILTER_CTL_RANGE_MASK,
 					    range));
 	if (ret)
-		goto out;
+		return ret;
 
 	adxl367_scale_act_thresholds(st, st->range, range);
 
@@ -508,25 +504,20 @@ static int adxl367_set_range(struct iio_dev *indio_dev,
 	ret = _adxl367_set_act_threshold(st, ADXL367_ACTIVITY,
 					 st->act_threshold);
 	if (ret)
-		goto out;
+		return ret;
 
 	ret = _adxl367_set_act_threshold(st, ADXL367_INACTIVITY,
 					 st->inact_threshold);
 	if (ret)
-		goto out;
+		return ret;
 
 	ret = adxl367_set_measure_en(st, true);
 	if (ret)
-		goto out;
+		return ret;
 
 	st->range = range;
 
-out:
-	mutex_unlock(&st->lock);
-
-	iio_device_release_direct_mode(indio_dev);
-
-	return ret;
+	return 0;
 }
 
 static int adxl367_time_ms_to_samples(struct adxl367_state *st, unsigned int ms)
@@ -587,11 +578,11 @@ static int adxl367_set_act_time_ms(struct adxl367_state *st,
 {
 	int ret;
 
-	mutex_lock(&st->lock);
+	guard(mutex)(&st->lock);
 
 	ret = adxl367_set_measure_en(st, false);
 	if (ret)
-		goto out;
+		return ret;
 
 	if (act == ADXL367_ACTIVITY)
 		ret = _adxl367_set_act_time_ms(st, ms);
@@ -599,14 +590,9 @@ static int adxl367_set_act_time_ms(struct adxl367_state *st,
 		ret = _adxl367_set_inact_time_ms(st, ms);
 
 	if (ret)
-		goto out;
-
-	ret = adxl367_set_measure_en(st, true);
-
-out:
-	mutex_unlock(&st->lock);
+		return ret;
 
-	return ret;
+	return adxl367_set_measure_en(st, true);
 }
 
 static int _adxl367_set_odr(struct adxl367_state *st, enum adxl367_odr odr)
@@ -636,31 +622,25 @@ static int _adxl367_set_odr(struct adxl367_state *st, enum adxl367_odr odr)
 
 static int adxl367_set_odr(struct iio_dev *indio_dev, enum adxl367_odr odr)
 {
-	struct adxl367_state *st = iio_priv(indio_dev);
+	struct adxl367_state *st;
 	int ret;
 
-	ret = iio_device_claim_direct_mode(indio_dev);
-	if (ret)
-		return ret;
+	CLASS(iio_claim_direct, claimed_dev)(indio_dev);
+	if (IS_ERR(claimed_dev))
+		return PTR_ERR(claimed_dev);
+	st = iio_priv(claimed_dev);
 
-	mutex_lock(&st->lock);
+	guard(mutex)(&st->lock);
 
 	ret = adxl367_set_measure_en(st, false);
 	if (ret)
-		goto out;
+		return ret;
 
 	ret = _adxl367_set_odr(st, odr);
 	if (ret)
-		goto out;
-
-	ret = adxl367_set_measure_en(st, true);
-
-out:
-	mutex_unlock(&st->lock);
-
-	iio_device_release_direct_mode(indio_dev);
+		return ret;;
 
-	return ret;
+	return adxl367_set_measure_en(st, true);
 }
 
 static int adxl367_set_temp_adc_en(struct adxl367_state *st, unsigned int reg,
@@ -749,36 +729,34 @@ static int adxl367_read_sample(struct iio_dev *indio_dev,
 			       struct iio_chan_spec const *chan,
 			       int *val)
 {
-	struct adxl367_state *st = iio_priv(indio_dev);
+	struct adxl367_state *st;
 	u16 sample;
 	int ret;
 
-	ret = iio_device_claim_direct_mode(indio_dev);
-	if (ret)
-		return ret;
+	CLASS(iio_claim_direct, claimed_dev)(indio_dev);
+	if (IS_ERR(claimed_dev))
+		return PTR_ERR(claimed_dev);
+	st = iio_priv(claimed_dev);
 
-	mutex_lock(&st->lock);
+	guard(mutex)(&st->lock);
 
 	ret = adxl367_set_temp_adc_reg_en(st, chan->address, true);
 	if (ret)
-		goto out;
+		return ret;
 
 	ret = regmap_bulk_read(st->regmap, chan->address, &st->sample_buf,
 			       sizeof(st->sample_buf));
 	if (ret)
-		goto out;
+		return ret;
 
 	sample = FIELD_GET(ADXL367_DATA_MASK, be16_to_cpu(st->sample_buf));
 	*val = sign_extend32(sample, chan->scan_type.realbits - 1);
 
 	ret = adxl367_set_temp_adc_reg_en(st, chan->address, false);
+	if (ret)
+		return ret;
 
-out:
-	mutex_unlock(&st->lock);
-
-	iio_device_release_direct_mode(indio_dev);
-
-	return ret ?: IIO_VAL_INT;
+	return IIO_VAL_INT;
 }
 
 static int adxl367_get_status(struct adxl367_state *st, u8 *status,
@@ -886,12 +864,12 @@ static int adxl367_read_raw(struct iio_dev *indio_dev,
 		return adxl367_read_sample(indio_dev, chan, val);
 	case IIO_CHAN_INFO_SCALE:
 		switch (chan->type) {
-		case IIO_ACCEL:
-			mutex_lock(&st->lock);
+		case IIO_ACCEL: {
+			guard(mutex)(&st->lock);
 			*val = adxl367_range_scale_tbl[st->range][0];
 			*val2 = adxl367_range_scale_tbl[st->range][1];
-			mutex_unlock(&st->lock);
 			return IIO_VAL_INT_PLUS_NANO;
+		}
 		case IIO_TEMP:
 			*val = 1000;
 			*val2 = ADXL367_TEMP_PER_C;
@@ -914,12 +892,12 @@ static int adxl367_read_raw(struct iio_dev *indio_dev,
 		default:
 			return -EINVAL;
 		}
-	case IIO_CHAN_INFO_SAMP_FREQ:
-		mutex_lock(&st->lock);
+	case IIO_CHAN_INFO_SAMP_FREQ: {
+		guard(mutex)(&st->lock);
 		*val = adxl367_samp_freq_tbl[st->odr][0];
 		*val2 = adxl367_samp_freq_tbl[st->odr][1];
-		mutex_unlock(&st->lock);
 		return IIO_VAL_INT_PLUS_MICRO;
+	}
 	default:
 		return -EINVAL;
 	}
@@ -1004,18 +982,15 @@ static int adxl367_read_event_value(struct iio_dev *indio_dev,
 {
 	struct adxl367_state *st = iio_priv(indio_dev);
 
+	guard(mutex)(&st->lock);
 	switch (info) {
 	case IIO_EV_INFO_VALUE: {
 		switch (dir) {
 		case IIO_EV_DIR_RISING:
-			mutex_lock(&st->lock);
 			*val = st->act_threshold;
-			mutex_unlock(&st->lock);
 			return IIO_VAL_INT;
 		case IIO_EV_DIR_FALLING:
-			mutex_lock(&st->lock);
 			*val = st->inact_threshold;
-			mutex_unlock(&st->lock);
 			return IIO_VAL_INT;
 		default:
 			return -EINVAL;
@@ -1024,15 +999,11 @@ static int adxl367_read_event_value(struct iio_dev *indio_dev,
 	case IIO_EV_INFO_PERIOD:
 		switch (dir) {
 		case IIO_EV_DIR_RISING:
-			mutex_lock(&st->lock);
 			*val = st->act_time_ms;
-			mutex_unlock(&st->lock);
 			*val2 = 1000;
 			return IIO_VAL_FRACTIONAL;
 		case IIO_EV_DIR_FALLING:
-			mutex_lock(&st->lock);
 			*val = st->inact_time_ms;
-			mutex_unlock(&st->lock);
 			*val2 = 1000;
 			return IIO_VAL_FRACTIONAL;
 		default:
@@ -1110,7 +1081,7 @@ static int adxl367_write_event_config(struct iio_dev *indio_dev,
 				      enum iio_event_direction dir,
 				      int state)
 {
-	struct adxl367_state *st = iio_priv(indio_dev);
+	struct adxl367_state *st;
 	enum adxl367_activity_type act;
 	int ret;
 
@@ -1125,33 +1096,27 @@ static int adxl367_write_event_config(struct iio_dev *indio_dev,
 		return -EINVAL;
 	}
 
-	ret = iio_device_claim_direct_mode(indio_dev);
-	if (ret)
-		return ret;
+	CLASS(iio_claim_direct, claimed_dev)(indio_dev);
+	if (IS_ERR(claimed_dev))
+		return PTR_ERR(claimed_dev);
+	st = iio_priv(claimed_dev);
 
-	mutex_lock(&st->lock);
+	guard(mutex)(&st->lock);
 
 	ret = adxl367_set_measure_en(st, false);
 	if (ret)
-		goto out;
+		return ret;
 
 	ret = adxl367_set_act_interrupt_en(st, act, state);
 	if (ret)
-		goto out;
+		return ret;
 
 	ret = adxl367_set_act_en(st, act, state ? ADCL367_ACT_REF_ENABLED
 						: ADXL367_ACT_DISABLED);
 	if (ret)
-		goto out;
-
-	ret = adxl367_set_measure_en(st, true);
-
-out:
-	mutex_unlock(&st->lock);
-
-	iio_device_release_direct_mode(indio_dev);
+		return ret;
 
-	return ret;
+	return adxl367_set_measure_en(st, true);
 }
 
 static ssize_t adxl367_get_fifo_enabled(struct device *dev,
@@ -1176,9 +1141,8 @@ static ssize_t adxl367_get_fifo_watermark(struct device *dev,
 	struct adxl367_state *st = iio_priv(dev_to_iio_dev(dev));
 	unsigned int fifo_watermark;
 
-	mutex_lock(&st->lock);
+	guard(mutex)(&st->lock);
 	fifo_watermark = st->fifo_watermark;
-	mutex_unlock(&st->lock);
 
 	return sysfs_emit(buf, "%d\n", fifo_watermark);
 }
@@ -1207,22 +1171,17 @@ static int adxl367_set_watermark(struct iio_dev *indio_dev, unsigned int val)
 	if (val > ADXL367_FIFO_MAX_WATERMARK)
 		return -EINVAL;
 
-	mutex_lock(&st->lock);
+	guard(mutex)(&st->lock);
 
 	ret = adxl367_set_measure_en(st, false);
 	if (ret)
-		goto out;
+		return ret;
 
 	ret = adxl367_set_fifo_watermark(st, val);
 	if (ret)
-		goto out;
-
-	ret = adxl367_set_measure_en(st, true);
-
-out:
-	mutex_unlock(&st->lock);
+		return ret;
 
-	return ret;
+	return adxl367_set_measure_en(st, true);
 }
 
 static bool adxl367_find_mask_fifo_format(const unsigned long *scan_mask,
@@ -1253,27 +1212,24 @@ static int adxl367_update_scan_mode(struct iio_dev *indio_dev,
 	if (!adxl367_find_mask_fifo_format(active_scan_mask, &fifo_format))
 		return -EINVAL;
 
-	mutex_lock(&st->lock);
+	guard(mutex)(&st->lock);
 
 	ret = adxl367_set_measure_en(st, false);
 	if (ret)
-		goto out;
+		return ret;
 
 	ret = adxl367_set_fifo_format(st, fifo_format);
 	if (ret)
-		goto out;
+		return ret;
 
 	ret = adxl367_set_measure_en(st, true);
 	if (ret)
-		goto out;
+		return ret;
 
 	st->fifo_set_size = bitmap_weight(active_scan_mask,
 					  indio_dev->masklength);
 
-out:
-	mutex_unlock(&st->lock);
-
-	return ret;
+	return 0;
 }
 
 static int adxl367_buffer_postenable(struct iio_dev *indio_dev)
@@ -1281,31 +1237,26 @@ static int adxl367_buffer_postenable(struct iio_dev *indio_dev)
 	struct adxl367_state *st = iio_priv(indio_dev);
 	int ret;
 
-	mutex_lock(&st->lock);
+	guard(mutex)(&st->lock);
 
 	ret = adxl367_set_temp_adc_mask_en(st, indio_dev->active_scan_mask,
 					   true);
 	if (ret)
-		goto out;
+		return ret;
 
 	ret = adxl367_set_measure_en(st, false);
 	if (ret)
-		goto out;
+		return ret;
 
 	ret = adxl367_set_fifo_watermark_interrupt_en(st, true);
 	if (ret)
-		goto out;
+		return ret;
 
 	ret = adxl367_set_fifo_mode(st, ADXL367_FIFO_MODE_STREAM);
 	if (ret)
-		goto out;
-
-	ret = adxl367_set_measure_en(st, true);
-
-out:
-	mutex_unlock(&st->lock);
+		return ret;
 
-	return ret;
+	return adxl367_set_measure_en(st, true);
 }
 
 static int adxl367_buffer_predisable(struct iio_dev *indio_dev)
@@ -1313,31 +1264,26 @@ static int adxl367_buffer_predisable(struct iio_dev *indio_dev)
 	struct adxl367_state *st = iio_priv(indio_dev);
 	int ret;
 
-	mutex_lock(&st->lock);
+	guard(mutex)(&st->lock);
 
 	ret = adxl367_set_measure_en(st, false);
 	if (ret)
-		goto out;
+		return ret;
 
 	ret = adxl367_set_fifo_mode(st, ADXL367_FIFO_MODE_DISABLED);
 	if (ret)
-		goto out;
+		return ret;
 
 	ret = adxl367_set_fifo_watermark_interrupt_en(st, false);
 	if (ret)
-		goto out;
+		return ret;
 
 	ret = adxl367_set_measure_en(st, true);
 	if (ret)
-		goto out;
-
-	ret = adxl367_set_temp_adc_mask_en(st, indio_dev->active_scan_mask,
-					   false);
-
-out:
-	mutex_unlock(&st->lock);
+		return ret;
 
-	return ret;
+	return adxl367_set_temp_adc_mask_en(st, indio_dev->active_scan_mask,
+					    false);
 }
 
 static const struct iio_buffer_setup_ops adxl367_buffer_ops = {
-- 
2.42.0

[RFC PATCH 4/8] iio: imu: bmi323: Use cleanup handling for iio_device_claim_direct_mode()

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Similar to existing use of guard() in this driver,
CLASS(iio_claim_direct, claimed_dev)(indio_Dev);
if (IS_ERR(claimed_dev))
	return PTR_ERR(claimed_dev);

will ensure that scope based cleanup occurs.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/imu/bmi323/bmi323_core.c | 61 ++++++++++++----------------
 1 file changed, 27 insertions(+), 34 deletions(-)

diff --git a/drivers/iio/imu/bmi323/bmi323_core.c b/drivers/iio/imu/bmi323/bmi323_core.c
index 0bd5dedd9a63..c9784ad01d44 100644
--- a/drivers/iio/imu/bmi323/bmi323_core.c
+++ b/drivers/iio/imu/bmi323/bmi323_core.c
@@ -1671,34 +1671,30 @@ static int bmi323_write_raw(struct iio_dev *indio_dev,
 	int ret;
 
 	switch (mask) {
-	case IIO_CHAN_INFO_SAMP_FREQ:
-		ret = iio_device_claim_direct_mode(indio_dev);
-		if (ret)
-			return ret;
+	case IIO_CHAN_INFO_SAMP_FREQ: {
+		CLASS(iio_claim_direct, claimed_dev)(indio_dev);
+		if (IS_ERR(claimed_dev))
+			return PTR_ERR(claimed_dev);
 
-		ret = bmi323_set_odr(data, bmi323_iio_to_sensor(chan->type),
-				     val, val2);
-		iio_device_release_direct_mode(indio_dev);
-		return ret;
-	case IIO_CHAN_INFO_SCALE:
-		ret = iio_device_claim_direct_mode(indio_dev);
-		if (ret)
-			return ret;
-
-		ret = bmi323_set_scale(data, bmi323_iio_to_sensor(chan->type),
-				       val, val2);
-		iio_device_release_direct_mode(indio_dev);
-		return ret;
-	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
-		ret = iio_device_claim_direct_mode(indio_dev);
-		if (ret)
-			return ret;
+		return bmi323_set_odr(data, bmi323_iio_to_sensor(chan->type),
+				      val, val2);
+	}
+	case IIO_CHAN_INFO_SCALE: {
+		CLASS(iio_claim_direct, claimed_dev)(indio_dev);
+		if (IS_ERR(claimed_dev))
+			return PTR_ERR(claimed_dev);
 
-		ret = bmi323_set_average(data, bmi323_iio_to_sensor(chan->type),
-					 val);
+		return bmi323_set_scale(data, bmi323_iio_to_sensor(chan->type),
+					val, val2);
+	}
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO: {
+		CLASS(iio_claim_direct, claimed_dev)(indio_dev);
+		if (IS_ERR(claimed_dev))
+			return PTR_ERR(claimed_dev);
 
-		iio_device_release_direct_mode(indio_dev);
-		return ret;
+		return bmi323_set_average(data, bmi323_iio_to_sensor(chan->type),
+					  val);
+	}
 	case IIO_CHAN_INFO_ENABLE:
 		return bmi323_enable_steps(data, val);
 	case IIO_CHAN_INFO_PROCESSED:
@@ -1724,7 +1720,6 @@ static int bmi323_read_raw(struct iio_dev *indio_dev,
 			   int *val2, long mask)
 {
 	struct bmi323_data *data = iio_priv(indio_dev);
-	int ret;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_PROCESSED:
@@ -1732,15 +1727,13 @@ static int bmi323_read_raw(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_RAW:
 		switch (chan->type) {
 		case IIO_ACCEL:
-		case IIO_ANGL_VEL:
-			ret = iio_device_claim_direct_mode(indio_dev);
-			if (ret)
-				return ret;
+		case IIO_ANGL_VEL: {
+			CLASS(iio_claim_direct, claimed_dev)(indio_dev);
+			if (IS_ERR(claimed_dev))
+				return PTR_ERR(claimed_dev);
 
-			ret = bmi323_read_axis(data, chan, val);
-
-			iio_device_release_direct_mode(indio_dev);
-			return ret;
+			return bmi323_read_axis(data, chan, val);
+		}
 		case IIO_TEMP:
 			return bmi323_get_temp_data(data, val);
 		default:
-- 
2.42.0

[RFC PATCH 5/8] iio: adc: max1363: Use automatic cleanup for locks and iio mode claiming.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

This simplifies error return paths.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/adc/max1363.c | 63 ++++++++++++++++-----------------------
 1 file changed, 26 insertions(+), 37 deletions(-)

diff --git a/drivers/iio/adc/max1363.c b/drivers/iio/adc/max1363.c
index 7c2a98b8c3a9..d0f4302807d0 100644
--- a/drivers/iio/adc/max1363.c
+++ b/drivers/iio/adc/max1363.c
@@ -363,10 +363,11 @@ static int max1363_read_single_chan(struct iio_dev *indio_dev,
 	struct max1363_state *st = iio_priv(indio_dev);
 	struct i2c_client *client = st->client;
 
-	ret = iio_device_claim_direct_mode(indio_dev);
-	if (ret)
-		return ret;
-	mutex_lock(&st->lock);
+	CLASS(iio_claim_direct, claimed_dev)(indio_dev);
+	if (IS_ERR(claimed_dev))
+		return PTR_ERR(claimed_dev);
+
+	guard(mutex)(&st->lock);
 
 	/*
 	 * If monitor mode is enabled, the method for reading a single
@@ -375,10 +376,8 @@ static int max1363_read_single_chan(struct iio_dev *indio_dev,
 	 *
 	 * Also, cannot read directly if buffered capture enabled.
 	 */
-	if (st->monitor_on) {
-		ret = -EBUSY;
-		goto error_ret;
-	}
+	if (st->monitor_on)
+		return -EBUSY;
 
 	/* Check to see if current scan mode is correct */
 	if (st->current_mode != &max1363_mode_table[chan->address]) {
@@ -386,33 +385,27 @@ static int max1363_read_single_chan(struct iio_dev *indio_dev,
 		st->current_mode = &max1363_mode_table[chan->address];
 		ret = max1363_set_scan_mode(st);
 		if (ret < 0)
-			goto error_ret;
+			return ret;
 	}
 	if (st->chip_info->bits != 8) {
 		/* Get reading */
 		data = st->recv(client, rxbuf, 2);
-		if (data < 0) {
-			ret = data;
-			goto error_ret;
-		}
+		if (data < 0)
+			return data;
+
 		data = (rxbuf[1] | rxbuf[0] << 8) &
 		  ((1 << st->chip_info->bits) - 1);
 	} else {
 		/* Get reading */
 		data = st->recv(client, rxbuf, 1);
-		if (data < 0) {
-			ret = data;
-			goto error_ret;
-		}
+		if (data < 0)
+			return data;
+
 		data = rxbuf[0];
 	}
 	*val = data;
 
-error_ret:
-	mutex_unlock(&st->lock);
-	iio_device_release_direct_mode(indio_dev);
-	return ret;
-
+	return 0;
 }
 
 static int max1363_read_raw(struct iio_dev *indio_dev,
@@ -710,9 +703,8 @@ static ssize_t max1363_monitor_store_freq(struct device *dev,
 	if (!found)
 		return -EINVAL;
 
-	mutex_lock(&st->lock);
-	st->monitor_speed = i;
-	mutex_unlock(&st->lock);
+	scoped_guard(mutex, &st->lock)
+		st->monitor_speed = i;
 
 	return 0;
 }
@@ -815,12 +807,11 @@ static int max1363_read_event_config(struct iio_dev *indio_dev,
 	int val;
 	int number = chan->channel;
 
-	mutex_lock(&st->lock);
+	guard(mutex)(&st->lock);
 	if (dir == IIO_EV_DIR_FALLING)
 		val = (1 << number) & st->mask_low;
 	else
 		val = (1 << number) & st->mask_high;
-	mutex_unlock(&st->lock);
 
 	return val;
 }
@@ -967,10 +958,11 @@ static int max1363_write_event_config(struct iio_dev *indio_dev,
 	u16 unifiedmask;
 	int number = chan->channel;
 
-	ret = iio_device_claim_direct_mode(indio_dev);
-	if (ret)
-		return ret;
-	mutex_lock(&st->lock);
+	CLASS(iio_claim_direct, claimed_dev)(indio_dev);
+	if (IS_ERR(claimed_dev))
+		return PTR_ERR(claimed_dev);
+
+	guard(mutex)(&st->lock);
 
 	unifiedmask = st->mask_low | st->mask_high;
 	if (dir == IIO_EV_DIR_FALLING) {
@@ -981,7 +973,7 @@ static int max1363_write_event_config(struct iio_dev *indio_dev,
 			ret = __max1363_check_event_mask((1 << number),
 							 unifiedmask);
 			if (ret)
-				goto error_ret;
+				return ret;
 			st->mask_low |= (1 << number);
 		}
 	} else {
@@ -991,17 +983,14 @@ static int max1363_write_event_config(struct iio_dev *indio_dev,
 			ret = __max1363_check_event_mask((1 << number),
 							 unifiedmask);
 			if (ret)
-				goto error_ret;
+				return ret;
 			st->mask_high |= (1 << number);
 		}
 	}
 
 	max1363_monitor_mode_update(st, !!(st->mask_high | st->mask_low));
-error_ret:
-	mutex_unlock(&st->lock);
-	iio_device_release_direct_mode(indio_dev);
 
-	return ret;
+	return 0;
 }
 
 /*
-- 
2.42.0

[RFC PATCH 6/8] iio: proximity: sx9360: Use automated cleanup for locks and IIO mode claiming.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

This simplifies error handling paths and generallly removes a bunch
of boilerplate.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/proximity/sx9360.c | 117 ++++++++++++---------------------
 1 file changed, 43 insertions(+), 74 deletions(-)

diff --git a/drivers/iio/proximity/sx9360.c b/drivers/iio/proximity/sx9360.c
index 2c4e14a4fe9f..1ffb1abd1bfa 100644
--- a/drivers/iio/proximity/sx9360.c
+++ b/drivers/iio/proximity/sx9360.c
@@ -322,25 +322,22 @@ static int sx9360_read_raw(struct iio_dev *indio_dev,
 			   int *val, int *val2, long mask)
 {
 	struct sx_common_data *data = iio_priv(indio_dev);
-	int ret;
 
 	switch (mask) {
-	case IIO_CHAN_INFO_RAW:
-		ret = iio_device_claim_direct_mode(indio_dev);
-		if (ret)
-			return ret;
+	case IIO_CHAN_INFO_RAW: {
+		CLASS(iio_claim_direct, claimed_dev)(indio_dev);
+		if (IS_ERR(claimed_dev))
+			return PTR_ERR(claimed_dev);
 
-		ret = sx_common_read_proximity(data, chan, val);
-		iio_device_release_direct_mode(indio_dev);
-		return ret;
-	case IIO_CHAN_INFO_HARDWAREGAIN:
-		ret = iio_device_claim_direct_mode(indio_dev);
-		if (ret)
-			return ret;
+		return sx_common_read_proximity(data, chan, val);
+	}
+	case IIO_CHAN_INFO_HARDWAREGAIN: {
+		CLASS(iio_claim_direct, claimed_dev)(indio_dev);
+		if (IS_ERR(claimed_dev))
+			return PTR_ERR(claimed_dev);
 
-		ret = sx9360_read_gain(data, chan, val);
-		iio_device_release_direct_mode(indio_dev);
-		return ret;
+		return sx9360_read_gain(data, chan, val);
+	}
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		return sx9360_read_samp_freq(data, val, val2);
 	default:
@@ -387,19 +384,15 @@ static int sx9360_read_avail(struct iio_dev *indio_dev,
 static int sx9360_set_samp_freq(struct sx_common_data *data,
 				int val, int val2)
 {
-	int ret, reg;
+	int reg;
 	__be16 buf;
 
 	reg = val * 8192 / SX9360_FOSC_HZ + val2 * 8192 / (SX9360_FOSC_MHZ);
 	buf = cpu_to_be16(reg);
-	mutex_lock(&data->mutex);
-
-	ret = regmap_bulk_write(data->regmap, SX9360_REG_GNRL_CTRL1, &buf,
-				sizeof(buf));
-
-	mutex_unlock(&data->mutex);
+	guard(mutex)(&data->mutex);
 
-	return ret;
+	return regmap_bulk_write(data->regmap, SX9360_REG_GNRL_CTRL1, &buf,
+				 sizeof(buf));
 }
 
 static int sx9360_read_thresh(struct sx_common_data *data, int *val)
@@ -510,7 +503,6 @@ static int sx9360_read_event_val(struct iio_dev *indio_dev,
 static int sx9360_write_thresh(struct sx_common_data *data, int _val)
 {
 	unsigned int val = _val;
-	int ret;
 
 	if (val >= 1)
 		val = int_sqrt(2 * val);
@@ -518,11 +510,8 @@ static int sx9360_write_thresh(struct sx_common_data *data, int _val)
 	if (val > 0xff)
 		return -EINVAL;
 
-	mutex_lock(&data->mutex);
-	ret = regmap_write(data->regmap, SX9360_REG_PROX_CTRL5, val);
-	mutex_unlock(&data->mutex);
-
-	return ret;
+	guard(mutex)(&data->mutex);
+	return regmap_write(data->regmap, SX9360_REG_PROX_CTRL5, val);
 }
 
 static int sx9360_write_hysteresis(struct sx_common_data *data, int _val)
@@ -546,18 +535,14 @@ static int sx9360_write_hysteresis(struct sx_common_data *data, int _val)
 		return -EINVAL;
 
 	hyst = FIELD_PREP(SX9360_REG_PROX_CTRL4_HYST_MASK, hyst);
-	mutex_lock(&data->mutex);
-	ret = regmap_update_bits(data->regmap, SX9360_REG_PROX_CTRL4,
-				 SX9360_REG_PROX_CTRL4_HYST_MASK, hyst);
-	mutex_unlock(&data->mutex);
-
-	return ret;
+	guard(mutex)(&data->mutex);
+	return regmap_update_bits(data->regmap, SX9360_REG_PROX_CTRL4,
+				  SX9360_REG_PROX_CTRL4_HYST_MASK, hyst);
 }
 
 static int sx9360_write_far_debounce(struct sx_common_data *data, int _val)
 {
 	unsigned int regval, val = _val;
-	int ret;
 
 	if (val > 0)
 		val = ilog2(val);
@@ -566,19 +551,15 @@ static int sx9360_write_far_debounce(struct sx_common_data *data, int _val)
 
 	regval = FIELD_PREP(SX9360_REG_PROX_CTRL4_FAR_DEBOUNCE_MASK, val);
 
-	mutex_lock(&data->mutex);
-	ret = regmap_update_bits(data->regmap, SX9360_REG_PROX_CTRL4,
-				 SX9360_REG_PROX_CTRL4_FAR_DEBOUNCE_MASK,
-				 regval);
-	mutex_unlock(&data->mutex);
-
-	return ret;
+	guard(mutex)(&data->mutex);
+	return regmap_update_bits(data->regmap, SX9360_REG_PROX_CTRL4,
+				  SX9360_REG_PROX_CTRL4_FAR_DEBOUNCE_MASK,
+				  regval);
 }
 
 static int sx9360_write_close_debounce(struct sx_common_data *data, int _val)
 {
 	unsigned int regval, val = _val;
-	int ret;
 
 	if (val > 0)
 		val = ilog2(val);
@@ -587,13 +568,10 @@ static int sx9360_write_close_debounce(struct sx_common_data *data, int _val)
 
 	regval = FIELD_PREP(SX9360_REG_PROX_CTRL4_CLOSE_DEBOUNCE_MASK, val);
 
-	mutex_lock(&data->mutex);
-	ret = regmap_update_bits(data->regmap, SX9360_REG_PROX_CTRL4,
-				 SX9360_REG_PROX_CTRL4_CLOSE_DEBOUNCE_MASK,
-				 regval);
-	mutex_unlock(&data->mutex);
-
-	return ret;
+	guard(mutex)(&data->mutex);
+	return regmap_update_bits(data->regmap, SX9360_REG_PROX_CTRL4,
+				  SX9360_REG_PROX_CTRL4_CLOSE_DEBOUNCE_MASK,
+				  regval);
 }
 
 static int sx9360_write_event_val(struct iio_dev *indio_dev,
@@ -630,19 +608,15 @@ static int sx9360_write_gain(struct sx_common_data *data,
 			     const struct iio_chan_spec *chan, int val)
 {
 	unsigned int gain, reg;
-	int ret;
 
 	gain = ilog2(val);
 	reg = SX9360_REG_PROX_CTRL0_PHR + chan->channel;
 	gain = FIELD_PREP(SX9360_REG_PROX_CTRL0_GAIN_MASK, gain);
 
-	mutex_lock(&data->mutex);
-	ret = regmap_update_bits(data->regmap, reg,
-				 SX9360_REG_PROX_CTRL0_GAIN_MASK,
-				 gain);
-	mutex_unlock(&data->mutex);
-
-	return ret;
+	guard(mutex)(&data->mutex);
+	return regmap_update_bits(data->regmap, reg,
+				  SX9360_REG_PROX_CTRL0_GAIN_MASK,
+				  gain);
 }
 
 static int sx9360_write_raw(struct iio_dev *indio_dev,
@@ -827,21 +801,17 @@ static int sx9360_suspend(struct device *dev)
 
 	disable_irq_nosync(data->client->irq);
 
-	mutex_lock(&data->mutex);
+	guard(mutex)(&data->mutex);
 	ret = regmap_read(data->regmap, SX9360_REG_GNRL_CTRL0, &regval);
+	if (ret < 0)
+		return ret;
 
 	data->suspend_ctrl =
 		FIELD_GET(SX9360_REG_GNRL_CTRL0_PHEN_MASK, regval);
 
-	if (ret < 0)
-		goto out;
 
 	/* Disable all phases, send the device to sleep. */
-	ret = regmap_write(data->regmap, SX9360_REG_GNRL_CTRL0, 0);
-
-out:
-	mutex_unlock(&data->mutex);
-	return ret;
+	return regmap_write(data->regmap, SX9360_REG_GNRL_CTRL0, 0);
 }
 
 static int sx9360_resume(struct device *dev)
@@ -849,14 +819,13 @@ static int sx9360_resume(struct device *dev)
 	struct sx_common_data *data = iio_priv(dev_get_drvdata(dev));
 	int ret;
 
-	mutex_lock(&data->mutex);
-	ret = regmap_update_bits(data->regmap, SX9360_REG_GNRL_CTRL0,
-				 SX9360_REG_GNRL_CTRL0_PHEN_MASK,
+	scoped_guard(mutex, &data->mutex) {
+		ret = regmap_update_bits(data->regmap, SX9360_REG_GNRL_CTRL0,
+					 SX9360_REG_GNRL_CTRL0_PHEN_MASK,
 				 data->suspend_ctrl);
-	mutex_unlock(&data->mutex);
-	if (ret)
-		return ret;
-
+		if (ret)
+			return ret;
+	}
 	enable_irq(data->client->irq);
 	return 0;
 }
-- 
2.42.0

[RFC PATCH 7/8] iio: proximity: sx9324: Use automated cleanup for locks and IIO mode claiming.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

This simplifies error handling paths and generallly removes a bunch
of boilerplate.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/proximity/sx9324.c | 113 +++++++++++++--------------------
 1 file changed, 44 insertions(+), 69 deletions(-)

diff --git a/drivers/iio/proximity/sx9324.c b/drivers/iio/proximity/sx9324.c
index ac2ed2da21cc..836feb6ae0c4 100644
--- a/drivers/iio/proximity/sx9324.c
+++ b/drivers/iio/proximity/sx9324.c
@@ -429,25 +429,22 @@ static int sx9324_read_raw(struct iio_dev *indio_dev,
 			   int *val, int *val2, long mask)
 {
 	struct sx_common_data *data = iio_priv(indio_dev);
-	int ret;
 
 	switch (mask) {
-	case IIO_CHAN_INFO_RAW:
-		ret = iio_device_claim_direct_mode(indio_dev);
-		if (ret)
-			return ret;
+	case IIO_CHAN_INFO_RAW: {
+		CLASS(iio_claim_direct, claimed_dev)(indio_dev);
+		if (IS_ERR(claimed_dev))
+			return PTR_ERR(claimed_dev);
 
-		ret = sx_common_read_proximity(data, chan, val);
-		iio_device_release_direct_mode(indio_dev);
-		return ret;
-	case IIO_CHAN_INFO_HARDWAREGAIN:
-		ret = iio_device_claim_direct_mode(indio_dev);
-		if (ret)
-			return ret;
+		return sx_common_read_proximity(data, chan, val);
+	}
+	case IIO_CHAN_INFO_HARDWAREGAIN: {
+		CLASS(iio_claim_direct, claimed_dev)(indio_dev);
+		if (IS_ERR(claimed_dev))
+			return PTR_ERR(claimed_dev);
 
-		ret = sx9324_read_gain(data, chan, val);
-		iio_device_release_direct_mode(indio_dev);
-		return ret;
+		return sx9324_read_gain(data, chan, val);
+	}
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		return sx9324_read_samp_freq(data, val, val2);
 	default:
@@ -484,7 +481,7 @@ static int sx9324_read_avail(struct iio_dev *indio_dev,
 static int sx9324_set_samp_freq(struct sx_common_data *data,
 				int val, int val2)
 {
-	int i, ret;
+	int i;
 
 	for (i = 0; i < ARRAY_SIZE(sx9324_samp_freq_table); i++)
 		if (val == sx9324_samp_freq_table[i].val &&
@@ -494,15 +491,11 @@ static int sx9324_set_samp_freq(struct sx_common_data *data,
 	if (i == ARRAY_SIZE(sx9324_samp_freq_table))
 		return -EINVAL;
 
-	mutex_lock(&data->mutex);
+	guard(mutex)(&data->mutex);
 
-	ret = regmap_update_bits(data->regmap,
-				 SX9324_REG_GNRL_CTRL0,
-				 SX9324_REG_GNRL_CTRL0_SCANPERIOD_MASK, i);
-
-	mutex_unlock(&data->mutex);
-
-	return ret;
+	return regmap_update_bits(data->regmap,
+				  SX9324_REG_GNRL_CTRL0,
+				  SX9324_REG_GNRL_CTRL0_SCANPERIOD_MASK, i);
 }
 
 static int sx9324_read_thresh(struct sx_common_data *data,
@@ -623,7 +616,6 @@ static int sx9324_write_thresh(struct sx_common_data *data,
 			       const struct iio_chan_spec *chan, int _val)
 {
 	unsigned int reg, val = _val;
-	int ret;
 
 	reg = SX9324_REG_PROX_CTRL6 + chan->channel / 2;
 
@@ -633,11 +625,9 @@ static int sx9324_write_thresh(struct sx_common_data *data,
 	if (val > 0xff)
 		return -EINVAL;
 
-	mutex_lock(&data->mutex);
-	ret = regmap_write(data->regmap, reg, val);
-	mutex_unlock(&data->mutex);
+	guard(mutex)(&data->mutex);
 
-	return ret;
+	return regmap_write(data->regmap, reg, val);
 }
 
 static int sx9324_write_hysteresis(struct sx_common_data *data,
@@ -662,18 +652,15 @@ static int sx9324_write_hysteresis(struct sx_common_data *data,
 		return -EINVAL;
 
 	hyst = FIELD_PREP(SX9324_REG_PROX_CTRL5_HYST_MASK, hyst);
-	mutex_lock(&data->mutex);
-	ret = regmap_update_bits(data->regmap, SX9324_REG_PROX_CTRL5,
-				 SX9324_REG_PROX_CTRL5_HYST_MASK, hyst);
-	mutex_unlock(&data->mutex);
+	guard(mutex)(&data->mutex);
 
-	return ret;
+	return regmap_update_bits(data->regmap, SX9324_REG_PROX_CTRL5,
+				  SX9324_REG_PROX_CTRL5_HYST_MASK, hyst);
 }
 
 static int sx9324_write_far_debounce(struct sx_common_data *data, int _val)
 {
 	unsigned int regval, val = _val;
-	int ret;
 
 	if (val > 0)
 		val = ilog2(val);
@@ -682,19 +669,16 @@ static int sx9324_write_far_debounce(struct sx_common_data *data, int _val)
 
 	regval = FIELD_PREP(SX9324_REG_PROX_CTRL5_FAR_DEBOUNCE_MASK, val);
 
-	mutex_lock(&data->mutex);
-	ret = regmap_update_bits(data->regmap, SX9324_REG_PROX_CTRL5,
-				 SX9324_REG_PROX_CTRL5_FAR_DEBOUNCE_MASK,
-				 regval);
-	mutex_unlock(&data->mutex);
+	guard(mutex)(&data->mutex);
 
-	return ret;
+	return regmap_update_bits(data->regmap, SX9324_REG_PROX_CTRL5,
+				  SX9324_REG_PROX_CTRL5_FAR_DEBOUNCE_MASK,
+				  regval);
 }
 
 static int sx9324_write_close_debounce(struct sx_common_data *data, int _val)
 {
 	unsigned int regval, val = _val;
-	int ret;
 
 	if (val > 0)
 		val = ilog2(val);
@@ -703,13 +687,11 @@ static int sx9324_write_close_debounce(struct sx_common_data *data, int _val)
 
 	regval = FIELD_PREP(SX9324_REG_PROX_CTRL5_CLOSE_DEBOUNCE_MASK, val);
 
-	mutex_lock(&data->mutex);
-	ret = regmap_update_bits(data->regmap, SX9324_REG_PROX_CTRL5,
-				 SX9324_REG_PROX_CTRL5_CLOSE_DEBOUNCE_MASK,
-				 regval);
-	mutex_unlock(&data->mutex);
+	guard(mutex)(&data->mutex);
 
-	return ret;
+	return regmap_update_bits(data->regmap, SX9324_REG_PROX_CTRL5,
+				  SX9324_REG_PROX_CTRL5_CLOSE_DEBOUNCE_MASK,
+				  regval);
 }
 
 static int sx9324_write_event_val(struct iio_dev *indio_dev,
@@ -746,7 +728,6 @@ static int sx9324_write_gain(struct sx_common_data *data,
 			     const struct iio_chan_spec *chan, int val)
 {
 	unsigned int gain, reg;
-	int ret;
 
 	reg = SX9324_REG_PROX_CTRL0 + chan->channel / 2;
 
@@ -756,13 +737,11 @@ static int sx9324_write_gain(struct sx_common_data *data,
 
 	gain = FIELD_PREP(SX9324_REG_PROX_CTRL0_GAIN_MASK, gain);
 
-	mutex_lock(&data->mutex);
-	ret = regmap_update_bits(data->regmap, reg,
-				 SX9324_REG_PROX_CTRL0_GAIN_MASK,
-				 gain);
-	mutex_unlock(&data->mutex);
+	guard(mutex)(&data->mutex);
 
-	return ret;
+	return regmap_update_bits(data->regmap, reg,
+				  SX9324_REG_PROX_CTRL0_GAIN_MASK,
+				  gain);
 }
 
 static int sx9324_write_raw(struct iio_dev *indio_dev,
@@ -1081,21 +1060,17 @@ static int sx9324_suspend(struct device *dev)
 
 	disable_irq_nosync(data->client->irq);
 
-	mutex_lock(&data->mutex);
+	guard(mutex)(&data->mutex);
 	ret = regmap_read(data->regmap, SX9324_REG_GNRL_CTRL1, &regval);
+	if (ret < 0)
+		return ret;
 
 	data->suspend_ctrl =
 		FIELD_GET(SX9324_REG_GNRL_CTRL1_PHEN_MASK, regval);
 
-	if (ret < 0)
-		goto out;
 
 	/* Disable all phases, send the device to sleep. */
-	ret = regmap_write(data->regmap, SX9324_REG_GNRL_CTRL1, 0);
-
-out:
-	mutex_unlock(&data->mutex);
-	return ret;
+	return regmap_write(data->regmap, SX9324_REG_GNRL_CTRL1, 0);
 }
 
 static int sx9324_resume(struct device *dev)
@@ -1103,12 +1078,12 @@ static int sx9324_resume(struct device *dev)
 	struct sx_common_data *data = iio_priv(dev_get_drvdata(dev));
 	int ret;
 
-	mutex_lock(&data->mutex);
-	ret = regmap_write(data->regmap, SX9324_REG_GNRL_CTRL1,
-			   data->suspend_ctrl | SX9324_REG_GNRL_CTRL1_PAUSECTRL);
-	mutex_unlock(&data->mutex);
-	if (ret)
-		return ret;
+	scoped_guard(mutex, &data->mutex) {
+		ret = regmap_write(data->regmap, SX9324_REG_GNRL_CTRL1,
+				   data->suspend_ctrl | SX9324_REG_GNRL_CTRL1_PAUSECTRL);
+		if (ret)
+			return ret;
+	}
 
 	enable_irq(data->client->irq);
 	return 0;
-- 
2.42.0

[RFC PATCH 8/8] iio: proximity: sx9310: Use automated cleanup for locks and IIO mode claiming.

[Jonathan Cameron]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

This simplifies error handling paths and generallly removes a bunch
of boilerplate.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/proximity/sx9310.c | 120 +++++++++++++--------------------
 1 file changed, 45 insertions(+), 75 deletions(-)

diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
index 0d230a0dff56..5e5436791550 100644
--- a/drivers/iio/proximity/sx9310.c
+++ b/drivers/iio/proximity/sx9310.c
@@ -337,28 +337,25 @@ static int sx9310_read_raw(struct iio_dev *indio_dev,
 			   int *val2, long mask)
 {
 	struct sx_common_data *data = iio_priv(indio_dev);
-	int ret;
 
 	if (chan->type != IIO_PROXIMITY)
 		return -EINVAL;
 
 	switch (mask) {
-	case IIO_CHAN_INFO_RAW:
-		ret = iio_device_claim_direct_mode(indio_dev);
-		if (ret)
-			return ret;
+	case IIO_CHAN_INFO_RAW: {
+		CLASS(iio_claim_direct, claimed_dev)(indio_dev);
+		if (IS_ERR(claimed_dev))
+			return PTR_ERR(claimed_dev);
 
-		ret = sx_common_read_proximity(data, chan, val);
-		iio_device_release_direct_mode(indio_dev);
-		return ret;
-	case IIO_CHAN_INFO_HARDWAREGAIN:
-		ret = iio_device_claim_direct_mode(indio_dev);
-		if (ret)
-			return ret;
+		return sx_common_read_proximity(data, chan, val);
+	}
+	case IIO_CHAN_INFO_HARDWAREGAIN: {
+		CLASS(iio_claim_direct, claimed_dev)(indio_dev);
+		if (IS_ERR(claimed_dev))
+			return PTR_ERR(claimed_dev);
 
-		ret = sx9310_read_gain(data, chan, val);
-		iio_device_release_direct_mode(indio_dev);
-		return ret;
+		return sx9310_read_gain(data, chan, val);
+	}
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		return sx9310_read_samp_freq(data, val, val2);
 	default:
@@ -546,12 +543,10 @@ static int sx9310_write_thresh(struct sx_common_data *data,
 		return -EINVAL;
 
 	regval = FIELD_PREP(SX9310_REG_PROX_CTRL8_9_PTHRESH_MASK, regval);
-	mutex_lock(&data->mutex);
-	ret = regmap_update_bits(data->regmap, reg,
-				 SX9310_REG_PROX_CTRL8_9_PTHRESH_MASK, regval);
-	mutex_unlock(&data->mutex);
 
-	return ret;
+	guard(mutex)(&data->mutex);
+	return regmap_update_bits(data->regmap, reg,
+				  SX9310_REG_PROX_CTRL8_9_PTHRESH_MASK, regval);
 }
 
 static int sx9310_write_hysteresis(struct sx_common_data *data,
@@ -576,17 +571,14 @@ static int sx9310_write_hysteresis(struct sx_common_data *data,
 		return -EINVAL;
 
 	hyst = FIELD_PREP(SX9310_REG_PROX_CTRL10_HYST_MASK, hyst);
-	mutex_lock(&data->mutex);
-	ret = regmap_update_bits(data->regmap, SX9310_REG_PROX_CTRL10,
-				 SX9310_REG_PROX_CTRL10_HYST_MASK, hyst);
-	mutex_unlock(&data->mutex);
 
-	return ret;
+	guard(mutex)(&data->mutex);
+	return regmap_update_bits(data->regmap, SX9310_REG_PROX_CTRL10,
+				  SX9310_REG_PROX_CTRL10_HYST_MASK, hyst);
 }
 
 static int sx9310_write_far_debounce(struct sx_common_data *data, int val)
 {
-	int ret;
 	unsigned int regval;
 
 	if (val > 0)
@@ -596,18 +588,14 @@ static int sx9310_write_far_debounce(struct sx_common_data *data, int val)
 
 	regval = FIELD_PREP(SX9310_REG_PROX_CTRL10_FAR_DEBOUNCE_MASK, val);
 
-	mutex_lock(&data->mutex);
-	ret = regmap_update_bits(data->regmap, SX9310_REG_PROX_CTRL10,
-				 SX9310_REG_PROX_CTRL10_FAR_DEBOUNCE_MASK,
-				 regval);
-	mutex_unlock(&data->mutex);
-
-	return ret;
+	guard(mutex)(&data->mutex);
+	return regmap_update_bits(data->regmap, SX9310_REG_PROX_CTRL10,
+				  SX9310_REG_PROX_CTRL10_FAR_DEBOUNCE_MASK,
+				  regval);
 }
 
 static int sx9310_write_close_debounce(struct sx_common_data *data, int val)
 {
-	int ret;
 	unsigned int regval;
 
 	if (val > 0)
@@ -617,13 +605,10 @@ static int sx9310_write_close_debounce(struct sx_common_data *data, int val)
 
 	regval = FIELD_PREP(SX9310_REG_PROX_CTRL10_CLOSE_DEBOUNCE_MASK, val);
 
-	mutex_lock(&data->mutex);
-	ret = regmap_update_bits(data->regmap, SX9310_REG_PROX_CTRL10,
-				 SX9310_REG_PROX_CTRL10_CLOSE_DEBOUNCE_MASK,
-				 regval);
-	mutex_unlock(&data->mutex);
-
-	return ret;
+	guard(mutex)(&data->mutex);
+	return regmap_update_bits(data->regmap, SX9310_REG_PROX_CTRL10,
+				  SX9310_REG_PROX_CTRL10_CLOSE_DEBOUNCE_MASK,
+				  regval);
 }
 
 static int sx9310_write_event_val(struct iio_dev *indio_dev,
@@ -658,7 +643,7 @@ static int sx9310_write_event_val(struct iio_dev *indio_dev,
 
 static int sx9310_set_samp_freq(struct sx_common_data *data, int val, int val2)
 {
-	int i, ret;
+	int i;
 
 	for (i = 0; i < ARRAY_SIZE(sx9310_samp_freq_table); i++)
 		if (val == sx9310_samp_freq_table[i].val &&
@@ -668,23 +653,17 @@ static int sx9310_set_samp_freq(struct sx_common_data *data, int val, int val2)
 	if (i == ARRAY_SIZE(sx9310_samp_freq_table))
 		return -EINVAL;
 
-	mutex_lock(&data->mutex);
-
-	ret = regmap_update_bits(
+	guard(mutex)(&data->mutex);
+	return regmap_update_bits(
 		data->regmap, SX9310_REG_PROX_CTRL0,
 		SX9310_REG_PROX_CTRL0_SCANPERIOD_MASK,
 		FIELD_PREP(SX9310_REG_PROX_CTRL0_SCANPERIOD_MASK, i));
-
-	mutex_unlock(&data->mutex);
-
-	return ret;
 }
 
 static int sx9310_write_gain(struct sx_common_data *data,
 			     const struct iio_chan_spec *chan, int val)
 {
 	unsigned int gain, mask;
-	int ret;
 
 	gain = ilog2(val);
 
@@ -703,12 +682,9 @@ static int sx9310_write_gain(struct sx_common_data *data,
 		return -EINVAL;
 	}
 
-	mutex_lock(&data->mutex);
-	ret = regmap_update_bits(data->regmap, SX9310_REG_PROX_CTRL3, mask,
-				 gain);
-	mutex_unlock(&data->mutex);
-
-	return ret;
+	guard(mutex)(&data->mutex);
+	return regmap_update_bits(data->regmap, SX9310_REG_PROX_CTRL3, mask,
+				  gain);
 }
 
 static int sx9310_write_raw(struct iio_dev *indio_dev,
@@ -969,22 +945,18 @@ static int sx9310_suspend(struct device *dev)
 
 	disable_irq_nosync(data->client->irq);
 
-	mutex_lock(&data->mutex);
+	guard(mutex)(&data->mutex);
 	ret = regmap_read(data->regmap, SX9310_REG_PROX_CTRL0,
 			  &data->suspend_ctrl);
 	if (ret)
-		goto out;
+		return ret;
 
 	ctrl0 = data->suspend_ctrl & ~SX9310_REG_PROX_CTRL0_SENSOREN_MASK;
 	ret = regmap_write(data->regmap, SX9310_REG_PROX_CTRL0, ctrl0);
 	if (ret)
-		goto out;
-
-	ret = regmap_write(data->regmap, SX9310_REG_PAUSE, 0);
+		return ret;
 
-out:
-	mutex_unlock(&data->mutex);
-	return ret;
+	return regmap_write(data->regmap, SX9310_REG_PAUSE, 0);
 }
 
 static int sx9310_resume(struct device *dev)
@@ -992,18 +964,16 @@ static int sx9310_resume(struct device *dev)
 	struct sx_common_data *data = iio_priv(dev_get_drvdata(dev));
 	int ret;
 
-	mutex_lock(&data->mutex);
-	ret = regmap_write(data->regmap, SX9310_REG_PAUSE, 1);
-	if (ret)
-		goto out;
-
-	ret = regmap_write(data->regmap, SX9310_REG_PROX_CTRL0,
-			   data->suspend_ctrl);
+	scoped_guard(mutex, &data->mutex) {
+		ret = regmap_write(data->regmap, SX9310_REG_PAUSE, 1);
+		if (ret)
+			return ret;
 
-out:
-	mutex_unlock(&data->mutex);
-	if (ret)
-		return ret;
+		ret = regmap_write(data->regmap, SX9310_REG_PROX_CTRL0,
+				   data->suspend_ctrl);
+		if (ret)
+			return ret;
+	}
 
 	enable_irq(data->client->irq);
 	return 0;
-- 
2.42.0

Re: [RFC PATCH 1/8] iio: locking: introduce __cleanup() based direct mode claiming infrastructure

[David Lechner]

On Sun, Oct 22, 2023 at 10:47 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> Allows use of:
>
>         CLASS(iio_claim_direct, claimed_dev)(indio_dev);
>         if (IS_ERR(claimed_dev))
>                 return PTR_ERR(claimed_dev);
>
>         st = iio_priv(claimed_dev);
>
> to automatically call iio_device_release_direct_mode() based on scope.
> Typically seen in combination with local device specific locks which
> are already have automated cleanup options via guard(mutex)(&st->lock)
> and scoped_guard().  Using both together allows most error handling to
> be automated.
>
> Note that whilst this pattern results in a struct iio_dev *claimed_dev
> that can be used, it is not necessary to do so as long as that pointer
> has been checked for errors as in the example.
>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/iio/industrialio-core.c |  4 ++++
>  include/linux/iio/iio.h         | 25 +++++++++++++++++++++++++
>  2 files changed, 29 insertions(+)
>
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index c77745b594bd..93bfad105eb5 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -2065,6 +2065,10 @@ EXPORT_SYMBOL_GPL(iio_device_claim_direct_mode);
>   */
>  void iio_device_release_direct_mode(struct iio_dev *indio_dev)
>  {
> +       /* Auto cleanup can result in this being called with an ERR_PTR */
> +       if (IS_ERR(indio_dev))
> +               return;
> +
>         mutex_unlock(&to_iio_dev_opaque(indio_dev)->mlock);
>  }
>  EXPORT_SYMBOL_GPL(iio_device_release_direct_mode);
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index d0ce3b71106a..11c42170fda1 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -9,6 +9,7 @@
>
>  #include <linux/device.h>
>  #include <linux/cdev.h>
> +#include <linux/cleanup.h>
>  #include <linux/slab.h>
>  #include <linux/iio/types.h>
>  /* IIO TODO LIST */
> @@ -644,6 +645,30 @@ int __devm_iio_device_register(struct device *dev, struct iio_dev *indio_dev,
>  int iio_push_event(struct iio_dev *indio_dev, u64 ev_code, s64 timestamp);
>  int iio_device_claim_direct_mode(struct iio_dev *indio_dev);
>  void iio_device_release_direct_mode(struct iio_dev *indio_dev);
> +/*
> + * Auto cleanup version of iio_device_claim_direct_mode,
> + *
> + *     CLASS(iio_claim_direct, claimed_dev)(indio_dev);
> + *     if (IS_ERR(claimed_dev))
> + *             return PTR_ERR(claimed_dev);
> + *
> + *     st = iio_priv(claimed_dev);
> + *     ....
> + */
> +DEFINE_CLASS(iio_claim_direct, struct iio_dev *,
> +            iio_device_release_direct_mode(_T),
> +            ({
> +                       struct iio_dev *dev;
> +                       int d = iio_device_claim_direct_mode(_T);
> +
> +                       if (d < 0)
> +                               dev = ERR_PTR(d);
> +                       else
> +                               dev = _T;
> +                       dev;
> +            }),
> +            struct iio_dev *_T);
> +
>  int iio_device_claim_buffer_mode(struct iio_dev *indio_dev);
>  void iio_device_release_buffer_mode(struct iio_dev *indio_dev);
>
> --
> 2.42.0
>

What is the benefit of exposing `claimed_dev` rather than just the int
return value? It seems like it just makes more noise in the error
check.

Also, this seems like this is a pattern that could be generalized and
put in cleanup.h. For example, this pattern could be used with
mutex_trylock as well.

Basically we could create a variation of the current `guard` like:

#define DEFINE_CHECKED_GUARD(_name, _type, _lock, _unlock) ...
#define checked_guard(_name) ...

To be used like:

/* linux/mutex.h */
#define DEFINE_CHECKED_GUARD(mutex, struct mutex *, \
    mutex_trylock(_T), mutex_unlock(_T))

/* any/driver.c */
if (!checked_guard(mutex)(&thing->lock))
    return -EBUSY

/* linux/iio/iio.h */
#define DEFINE_CHECKED_GUARD(iio_claim_direct, struct iio_dev *indio_dev *, \
    iio_device_claim_direct_mode(_T), iio_device_release_direct_mode(_T))

/* iio/driver.c */
if (!checked_guard(iio_claim_direct)(indio_dev))
    return -EBUSY

Re: [RFC PATCH 1/8] iio: locking: introduce __cleanup() based direct mode claiming infrastructure

[Nuno Sá]

On Sun, 2023-10-22 at 16:10 -0500, David Lechner wrote:
> On Sun, Oct 22, 2023 at 10:47 AM Jonathan Cameron <jic23@kernel.org> wrote:
> > 
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > Allows use of:
> > 
> >         CLASS(iio_claim_direct, claimed_dev)(indio_dev);
> >         if (IS_ERR(claimed_dev))
> >                 return PTR_ERR(claimed_dev);
> > 
> >         st = iio_priv(claimed_dev);
> > 
> > to automatically call iio_device_release_direct_mode() based on scope.
> > Typically seen in combination with local device specific locks which
> > are already have automated cleanup options via guard(mutex)(&st->lock)
> > and scoped_guard().  Using both together allows most error handling to
> > be automated.
> > 
> > Note that whilst this pattern results in a struct iio_dev *claimed_dev
> > that can be used, it is not necessary to do so as long as that pointer
> > has been checked for errors as in the example.
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---
> >  drivers/iio/industrialio-core.c |  4 ++++
> >  include/linux/iio/iio.h         | 25 +++++++++++++++++++++++++
> >  2 files changed, 29 insertions(+)
> > 
> > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-
> > core.c
> > index c77745b594bd..93bfad105eb5 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -2065,6 +2065,10 @@ EXPORT_SYMBOL_GPL(iio_device_claim_direct_mode);
> >   */
> >  void iio_device_release_direct_mode(struct iio_dev *indio_dev)
> >  {
> > +       /* Auto cleanup can result in this being called with an ERR_PTR */
> > +       if (IS_ERR(indio_dev))
> > +               return;
> > +
> >         mutex_unlock(&to_iio_dev_opaque(indio_dev)->mlock);
> >  }
> >  EXPORT_SYMBOL_GPL(iio_device_release_direct_mode);
> > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> > index d0ce3b71106a..11c42170fda1 100644
> > --- a/include/linux/iio/iio.h
> > +++ b/include/linux/iio/iio.h
> > @@ -9,6 +9,7 @@
> > 
> >  #include <linux/device.h>
> >  #include <linux/cdev.h>
> > +#include <linux/cleanup.h>
> >  #include <linux/slab.h>
> >  #include <linux/iio/types.h>
> >  /* IIO TODO LIST */
> > @@ -644,6 +645,30 @@ int __devm_iio_device_register(struct device *dev,
> > struct iio_dev *indio_dev,
> >  int iio_push_event(struct iio_dev *indio_dev, u64 ev_code, s64 timestamp);
> >  int iio_device_claim_direct_mode(struct iio_dev *indio_dev);
> >  void iio_device_release_direct_mode(struct iio_dev *indio_dev);
> > +/*
> > + * Auto cleanup version of iio_device_claim_direct_mode,
> > + *
> > + *     CLASS(iio_claim_direct, claimed_dev)(indio_dev);
> > + *     if (IS_ERR(claimed_dev))
> > + *             return PTR_ERR(claimed_dev);
> > + *
> > + *     st = iio_priv(claimed_dev);
> > + *     ....
> > + */
> > +DEFINE_CLASS(iio_claim_direct, struct iio_dev *,
> > +            iio_device_release_direct_mode(_T),
> > +            ({
> > +                       struct iio_dev *dev;
> > +                       int d = iio_device_claim_direct_mode(_T);
> > +
> > +                       if (d < 0)
> > +                               dev = ERR_PTR(d);
> > +                       else
> > +                               dev = _T;
> > +                       dev;
> > +            }),
> > +            struct iio_dev *_T);
> > +
> >  int iio_device_claim_buffer_mode(struct iio_dev *indio_dev);
> >  void iio_device_release_buffer_mode(struct iio_dev *indio_dev);
> > 
> > --
> > 2.42.0
> > 
> 
> What is the benefit of exposing `claimed_dev` rather than just the int
> return value? It seems like it just makes more noise in the error
> check.
> 

I don't really have a very strong opinion on this but what I really don't like
much is the pattern:

CLASS(type, ret), where the return value is an argument of the macro... It would
be nice if we could just make it like:

ret = guard(type)(...); //or any other variation of the guard() macro
if (ret) 
	return ret;

the above could also be an error pointer or even have one variation of each. but
yeah, that likely means changing the cleanup.h file and that might be out of
scope for Jonathan's patch series. 

- Nuno Sá

Re: [RFC PATCH 1/8] iio: locking: introduce __cleanup() based direct mode claiming infrastructure

[Jonathan Cameron]

On Sun, 22 Oct 2023 16:10:48 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On Sun, Oct 22, 2023 at 10:47 AM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >
> > Allows use of:
> >
> >         CLASS(iio_claim_direct, claimed_dev)(indio_dev);
> >         if (IS_ERR(claimed_dev))
> >                 return PTR_ERR(claimed_dev);
> >
> >         st = iio_priv(claimed_dev);
> >
> > to automatically call iio_device_release_direct_mode() based on scope.
> > Typically seen in combination with local device specific locks which
> > are already have automated cleanup options via guard(mutex)(&st->lock)
> > and scoped_guard().  Using both together allows most error handling to
> > be automated.
> >
> > Note that whilst this pattern results in a struct iio_dev *claimed_dev
> > that can be used, it is not necessary to do so as long as that pointer
> > has been checked for errors as in the example.
> >
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---
> >  drivers/iio/industrialio-core.c |  4 ++++
> >  include/linux/iio/iio.h         | 25 +++++++++++++++++++++++++
> >  2 files changed, 29 insertions(+)
> >
> > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> > index c77745b594bd..93bfad105eb5 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -2065,6 +2065,10 @@ EXPORT_SYMBOL_GPL(iio_device_claim_direct_mode);
> >   */
> >  void iio_device_release_direct_mode(struct iio_dev *indio_dev)
> >  {
> > +       /* Auto cleanup can result in this being called with an ERR_PTR */
> > +       if (IS_ERR(indio_dev))
> > +               return;
> > +
> >         mutex_unlock(&to_iio_dev_opaque(indio_dev)->mlock);
> >  }
> >  EXPORT_SYMBOL_GPL(iio_device_release_direct_mode);
> > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> > index d0ce3b71106a..11c42170fda1 100644
> > --- a/include/linux/iio/iio.h
> > +++ b/include/linux/iio/iio.h
> > @@ -9,6 +9,7 @@
> >
> >  #include <linux/device.h>
> >  #include <linux/cdev.h>
> > +#include <linux/cleanup.h>
> >  #include <linux/slab.h>
> >  #include <linux/iio/types.h>
> >  /* IIO TODO LIST */
> > @@ -644,6 +645,30 @@ int __devm_iio_device_register(struct device *dev, struct iio_dev *indio_dev,
> >  int iio_push_event(struct iio_dev *indio_dev, u64 ev_code, s64 timestamp);
> >  int iio_device_claim_direct_mode(struct iio_dev *indio_dev);
> >  void iio_device_release_direct_mode(struct iio_dev *indio_dev);
> > +/*
> > + * Auto cleanup version of iio_device_claim_direct_mode,
> > + *
> > + *     CLASS(iio_claim_direct, claimed_dev)(indio_dev);
> > + *     if (IS_ERR(claimed_dev))
> > + *             return PTR_ERR(claimed_dev);
> > + *
> > + *     st = iio_priv(claimed_dev);
> > + *     ....
> > + */
> > +DEFINE_CLASS(iio_claim_direct, struct iio_dev *,
> > +            iio_device_release_direct_mode(_T),
> > +            ({
> > +                       struct iio_dev *dev;
> > +                       int d = iio_device_claim_direct_mode(_T);
> > +
> > +                       if (d < 0)
> > +                               dev = ERR_PTR(d);
> > +                       else
> > +                               dev = _T;
> > +                       dev;
> > +            }),
> > +            struct iio_dev *_T);
> > +
> >  int iio_device_claim_buffer_mode(struct iio_dev *indio_dev);
> >  void iio_device_release_buffer_mode(struct iio_dev *indio_dev);
> >
> > --
> > 2.42.0
> >  
> 
> What is the benefit of exposing `claimed_dev` rather than just the int
> return value? It seems like it just makes more noise in the error
> check.
> 
> Also, this seems like this is a pattern that could be generalized and
> put in cleanup.h. For example, this pattern could be used with
> mutex_trylock as well.
> 
> Basically we could create a variation of the current `guard` like:
> 
> #define DEFINE_CHECKED_GUARD(_name, _type, _lock, _unlock) ...
> #define checked_guard(_name) ...
> 
> To be used like:
> 
> /* linux/mutex.h */
> #define DEFINE_CHECKED_GUARD(mutex, struct mutex *, \
>     mutex_trylock(_T), mutex_unlock(_T))

My head hurt whilst digging through what the macros did, but
I don't think this can work this simply because we need to instantiate
a local variable that is then used for the cleanup.

When you call
guard(mutex)(&lock);

Expanding to
CLASS(mutex, __UNIQUE_ID(guard))(&lock)
to
class_mutex_t var __cleanup(class_mutex_destuctor) = class_mutex_constructor(&lock);

where
DEFINE_GUARD(mutex, struct mutex *, mutex_lock(_T), mutex_unlock(_T))
to
DEFINE_CLASS(mutex, struct mutex *, mutex_unlock, ({mutex_lock(_T), T;}), struct mutex *_T;
to 
typedef struct mutex * class_mutex_t;
...

Key being that it relies on the lock path returning the variable that you use as the
input for the cleanup.

If we return an int, we can't do that.

Unless I'm missing some magic that would make it work!

> 
> /* any/driver.c */
> if (!checked_guard(mutex)(&thing->lock))
>     return -EBUSY
> 
> /* linux/iio/iio.h */
> #define DEFINE_CHECKED_GUARD(iio_claim_direct, struct iio_dev *indio_dev *, \
>     iio_device_claim_direct_mode(_T), iio_device_release_direct_mode(_T))
> 
> /* iio/driver.c */
> if (!checked_guard(iio_claim_direct)(indio_dev))
>     return -EBUSY
> 

Re: [RFC PATCH 1/8] iio: locking: introduce __cleanup() based direct mode claiming infrastructure

[Jonathan Cameron]

On Mon, 23 Oct 2023 10:55:56 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Sun, 2023-10-22 at 16:10 -0500, David Lechner wrote:
> > On Sun, Oct 22, 2023 at 10:47 AM Jonathan Cameron <jic23@kernel.org> wrote:  
> > > 
> > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > 
> > > Allows use of:
> > > 
> > >         CLASS(iio_claim_direct, claimed_dev)(indio_dev);
> > >         if (IS_ERR(claimed_dev))
> > >                 return PTR_ERR(claimed_dev);
> > > 
> > >         st = iio_priv(claimed_dev);
> > > 
> > > to automatically call iio_device_release_direct_mode() based on scope.
> > > Typically seen in combination with local device specific locks which
> > > are already have automated cleanup options via guard(mutex)(&st->lock)
> > > and scoped_guard().  Using both together allows most error handling to
> > > be automated.
> > > 
> > > Note that whilst this pattern results in a struct iio_dev *claimed_dev
> > > that can be used, it is not necessary to do so as long as that pointer
> > > has been checked for errors as in the example.
> > > 
> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > ---
> > >  drivers/iio/industrialio-core.c |  4 ++++
> > >  include/linux/iio/iio.h         | 25 +++++++++++++++++++++++++
> > >  2 files changed, 29 insertions(+)
> > > 
> > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-
> > > core.c
> > > index c77745b594bd..93bfad105eb5 100644
> > > --- a/drivers/iio/industrialio-core.c
> > > +++ b/drivers/iio/industrialio-core.c
> > > @@ -2065,6 +2065,10 @@ EXPORT_SYMBOL_GPL(iio_device_claim_direct_mode);
> > >   */
> > >  void iio_device_release_direct_mode(struct iio_dev *indio_dev)
> > >  {
> > > +       /* Auto cleanup can result in this being called with an ERR_PTR */
> > > +       if (IS_ERR(indio_dev))
> > > +               return;
> > > +
> > >         mutex_unlock(&to_iio_dev_opaque(indio_dev)->mlock);
> > >  }
> > >  EXPORT_SYMBOL_GPL(iio_device_release_direct_mode);
> > > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> > > index d0ce3b71106a..11c42170fda1 100644
> > > --- a/include/linux/iio/iio.h
> > > +++ b/include/linux/iio/iio.h
> > > @@ -9,6 +9,7 @@
> > > 
> > >  #include <linux/device.h>
> > >  #include <linux/cdev.h>
> > > +#include <linux/cleanup.h>
> > >  #include <linux/slab.h>
> > >  #include <linux/iio/types.h>
> > >  /* IIO TODO LIST */
> > > @@ -644,6 +645,30 @@ int __devm_iio_device_register(struct device *dev,
> > > struct iio_dev *indio_dev,
> > >  int iio_push_event(struct iio_dev *indio_dev, u64 ev_code, s64 timestamp);
> > >  int iio_device_claim_direct_mode(struct iio_dev *indio_dev);
> > >  void iio_device_release_direct_mode(struct iio_dev *indio_dev);
> > > +/*
> > > + * Auto cleanup version of iio_device_claim_direct_mode,
> > > + *
> > > + *     CLASS(iio_claim_direct, claimed_dev)(indio_dev);
> > > + *     if (IS_ERR(claimed_dev))
> > > + *             return PTR_ERR(claimed_dev);
> > > + *
> > > + *     st = iio_priv(claimed_dev);
> > > + *     ....
> > > + */
> > > +DEFINE_CLASS(iio_claim_direct, struct iio_dev *,
> > > +            iio_device_release_direct_mode(_T),
> > > +            ({
> > > +                       struct iio_dev *dev;
> > > +                       int d = iio_device_claim_direct_mode(_T);
> > > +
> > > +                       if (d < 0)
> > > +                               dev = ERR_PTR(d);
> > > +                       else
> > > +                               dev = _T;
> > > +                       dev;
> > > +            }),
> > > +            struct iio_dev *_T);
> > > +
> > >  int iio_device_claim_buffer_mode(struct iio_dev *indio_dev);
> > >  void iio_device_release_buffer_mode(struct iio_dev *indio_dev);
> > > 
> > > --
> > > 2.42.0
> > >   
> > 
> > What is the benefit of exposing `claimed_dev` rather than just the int
> > return value? It seems like it just makes more noise in the error
> > check.
> >   
> 
> I don't really have a very strong opinion on this but what I really don't like
> much is the pattern:
> 
> CLASS(type, ret), where the return value is an argument of the macro... It would
> be nice if we could just make it like:
> 
> ret = guard(type)(...); //or any other variation of the guard() macro
> if (ret) 
> 	return ret;
> 
> the above could also be an error pointer or even have one variation of each. but
> yeah, that likely means changing the cleanup.h file and that might be out of
> scope for Jonathan's patch series. 
> 

I fully agree it's ugly and a little unintuitive but I don't see a way an "lvalue"
can work work cleanly (due to magic types under the hood) and I suspect we will
have to get used to this pattern.

There are lots of other examples in kernel that are similar DECLARE_BITMAP() etc
and we've kind of gotten used to those...

Jonathan


> - Nuno Sá
> 
> 

Re: [RFC PATCH 1/8] iio: locking: introduce __cleanup() based direct mode claiming infrastructure

[Nuno Sá]

On Mon, 2023-10-23 at 10:53 +0100, Jonathan Cameron wrote:
> On Mon, 23 Oct 2023 10:55:56 +0200
> Nuno Sá <noname.nuno@gmail.com> wrote:
> 
> > On Sun, 2023-10-22 at 16:10 -0500, David Lechner wrote:
> > > On Sun, Oct 22, 2023 at 10:47 AM Jonathan Cameron <jic23@kernel.org>
> > > wrote:  
> > > > 
> > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > 
> > > > Allows use of:
> > > > 
> > > >         CLASS(iio_claim_direct, claimed_dev)(indio_dev);
> > > >         if (IS_ERR(claimed_dev))
> > > >                 return PTR_ERR(claimed_dev);
> > > > 
> > > >         st = iio_priv(claimed_dev);
> > > > 
> > > > to automatically call iio_device_release_direct_mode() based on scope.
> > > > Typically seen in combination with local device specific locks which
> > > > are already have automated cleanup options via guard(mutex)(&st->lock)
> > > > and scoped_guard().  Using both together allows most error handling to
> > > > be automated.
> > > > 
> > > > Note that whilst this pattern results in a struct iio_dev *claimed_dev
> > > > that can be used, it is not necessary to do so as long as that pointer
> > > > has been checked for errors as in the example.
> > > > 
> > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > ---
> > > >  drivers/iio/industrialio-core.c |  4 ++++
> > > >  include/linux/iio/iio.h         | 25 +++++++++++++++++++++++++
> > > >  2 files changed, 29 insertions(+)
> > > > 
> > > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-
> > > > core.c
> > > > index c77745b594bd..93bfad105eb5 100644
> > > > --- a/drivers/iio/industrialio-core.c
> > > > +++ b/drivers/iio/industrialio-core.c
> > > > @@ -2065,6 +2065,10 @@ EXPORT_SYMBOL_GPL(iio_device_claim_direct_mode);
> > > >   */
> > > >  void iio_device_release_direct_mode(struct iio_dev *indio_dev)
> > > >  {
> > > > +       /* Auto cleanup can result in this being called with an ERR_PTR
> > > > */
> > > > +       if (IS_ERR(indio_dev))
> > > > +               return;
> > > > +
> > > >         mutex_unlock(&to_iio_dev_opaque(indio_dev)->mlock);
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(iio_device_release_direct_mode);
> > > > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> > > > index d0ce3b71106a..11c42170fda1 100644
> > > > --- a/include/linux/iio/iio.h
> > > > +++ b/include/linux/iio/iio.h
> > > > @@ -9,6 +9,7 @@
> > > > 
> > > >  #include <linux/device.h>
> > > >  #include <linux/cdev.h>
> > > > +#include <linux/cleanup.h>
> > > >  #include <linux/slab.h>
> > > >  #include <linux/iio/types.h>
> > > >  /* IIO TODO LIST */
> > > > @@ -644,6 +645,30 @@ int __devm_iio_device_register(struct device *dev,
> > > > struct iio_dev *indio_dev,
> > > >  int iio_push_event(struct iio_dev *indio_dev, u64 ev_code, s64
> > > > timestamp);
> > > >  int iio_device_claim_direct_mode(struct iio_dev *indio_dev);
> > > >  void iio_device_release_direct_mode(struct iio_dev *indio_dev);
> > > > +/*
> > > > + * Auto cleanup version of iio_device_claim_direct_mode,
> > > > + *
> > > > + *     CLASS(iio_claim_direct, claimed_dev)(indio_dev);
> > > > + *     if (IS_ERR(claimed_dev))
> > > > + *             return PTR_ERR(claimed_dev);
> > > > + *
> > > > + *     st = iio_priv(claimed_dev);
> > > > + *     ....
> > > > + */
> > > > +DEFINE_CLASS(iio_claim_direct, struct iio_dev *,
> > > > +            iio_device_release_direct_mode(_T),
> > > > +            ({
> > > > +                       struct iio_dev *dev;
> > > > +                       int d = iio_device_claim_direct_mode(_T);
> > > > +
> > > > +                       if (d < 0)
> > > > +                               dev = ERR_PTR(d);
> > > > +                       else
> > > > +                               dev = _T;
> > > > +                       dev;
> > > > +            }),
> > > > +            struct iio_dev *_T);
> > > > +
> > > >  int iio_device_claim_buffer_mode(struct iio_dev *indio_dev);
> > > >  void iio_device_release_buffer_mode(struct iio_dev *indio_dev);
> > > > 
> > > > --
> > > > 2.42.0
> > > >   
> > > 
> > > What is the benefit of exposing `claimed_dev` rather than just the int
> > > return value? It seems like it just makes more noise in the error
> > > check.
> > >   
> > 
> > I don't really have a very strong opinion on this but what I really don't
> > like
> > much is the pattern:
> > 
> > CLASS(type, ret), where the return value is an argument of the macro... It
> > would
> > be nice if we could just make it like:
> > 
> > ret = guard(type)(...); //or any other variation of the guard() macro
> > if (ret) 
> > 	return ret;
> > 
> > the above could also be an error pointer or even have one variation of each.
> > but
> > yeah, that likely means changing the cleanup.h file and that might be out of
> > scope for Jonathan's patch series. 
> > 
> 
> I fully agree it's ugly and a little unintuitive but I don't see a way an
> "lvalue"
> can work work cleanly (due to magic types under the hood) and I suspect we
> will
> have to get used to this pattern.
> 

Yeah, given the games being played with the constructor and the _lock definition
so we return the variable we want to "release" I agree it would be hard to have
anything clean and likely even harder to read (more than it is already :)).

However, I think users of the cleanup.h stuff could build on top of it... For
instance, in our case we could have something like:

#define IIO_CLAIM_DIRECT(dev) 
	int __ret = 0;
	CLASS(iio_claim_direct, claimed_dev)(dev);
	if ((IS_ERR(claimed_dev))
		__ret = PTR_ERR(claimed_dev);
	__ret

Then we could use it in the same way as before... Or at the very least I would
simply make it a bit more readable for IIO (rather than the plain CLASS() call):

#define IIO_CLAIM_DIRECT(claimed_dev, dev)
	CLASS(iio_claim_direct, claimed_dev)(dev)

Just some thoughts...

- Nuno Sá

Re: [RFC PATCH 1/8] iio: locking: introduce __cleanup() based direct mode claiming infrastructure

[Jonathan Cameron]

On Mon, 23 Oct 2023 13:51:04 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Mon, 2023-10-23 at 10:53 +0100, Jonathan Cameron wrote:
> > On Mon, 23 Oct 2023 10:55:56 +0200
> > Nuno Sá <noname.nuno@gmail.com> wrote:
> >   
> > > On Sun, 2023-10-22 at 16:10 -0500, David Lechner wrote:  
> > > > On Sun, Oct 22, 2023 at 10:47 AM Jonathan Cameron <jic23@kernel.org>
> > > > wrote:    
> > > > > 
> > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > 
> > > > > Allows use of:
> > > > > 
> > > > >         CLASS(iio_claim_direct, claimed_dev)(indio_dev);
> > > > >         if (IS_ERR(claimed_dev))
> > > > >                 return PTR_ERR(claimed_dev);
> > > > > 
> > > > >         st = iio_priv(claimed_dev);
> > > > > 
> > > > > to automatically call iio_device_release_direct_mode() based on scope.
> > > > > Typically seen in combination with local device specific locks which
> > > > > are already have automated cleanup options via guard(mutex)(&st->lock)
> > > > > and scoped_guard().  Using both together allows most error handling to
> > > > > be automated.
> > > > > 
> > > > > Note that whilst this pattern results in a struct iio_dev *claimed_dev
> > > > > that can be used, it is not necessary to do so as long as that pointer
> > > > > has been checked for errors as in the example.
> > > > > 
> > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > ---
> > > > >  drivers/iio/industrialio-core.c |  4 ++++
> > > > >  include/linux/iio/iio.h         | 25 +++++++++++++++++++++++++
> > > > >  2 files changed, 29 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-
> > > > > core.c
> > > > > index c77745b594bd..93bfad105eb5 100644
> > > > > --- a/drivers/iio/industrialio-core.c
> > > > > +++ b/drivers/iio/industrialio-core.c
> > > > > @@ -2065,6 +2065,10 @@ EXPORT_SYMBOL_GPL(iio_device_claim_direct_mode);
> > > > >   */
> > > > >  void iio_device_release_direct_mode(struct iio_dev *indio_dev)
> > > > >  {
> > > > > +       /* Auto cleanup can result in this being called with an ERR_PTR
> > > > > */
> > > > > +       if (IS_ERR(indio_dev))
> > > > > +               return;
> > > > > +
> > > > >         mutex_unlock(&to_iio_dev_opaque(indio_dev)->mlock);
> > > > >  }
> > > > >  EXPORT_SYMBOL_GPL(iio_device_release_direct_mode);
> > > > > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> > > > > index d0ce3b71106a..11c42170fda1 100644
> > > > > --- a/include/linux/iio/iio.h
> > > > > +++ b/include/linux/iio/iio.h
> > > > > @@ -9,6 +9,7 @@
> > > > > 
> > > > >  #include <linux/device.h>
> > > > >  #include <linux/cdev.h>
> > > > > +#include <linux/cleanup.h>
> > > > >  #include <linux/slab.h>
> > > > >  #include <linux/iio/types.h>
> > > > >  /* IIO TODO LIST */
> > > > > @@ -644,6 +645,30 @@ int __devm_iio_device_register(struct device *dev,
> > > > > struct iio_dev *indio_dev,
> > > > >  int iio_push_event(struct iio_dev *indio_dev, u64 ev_code, s64
> > > > > timestamp);
> > > > >  int iio_device_claim_direct_mode(struct iio_dev *indio_dev);
> > > > >  void iio_device_release_direct_mode(struct iio_dev *indio_dev);
> > > > > +/*
> > > > > + * Auto cleanup version of iio_device_claim_direct_mode,
> > > > > + *
> > > > > + *     CLASS(iio_claim_direct, claimed_dev)(indio_dev);
> > > > > + *     if (IS_ERR(claimed_dev))
> > > > > + *             return PTR_ERR(claimed_dev);
> > > > > + *
> > > > > + *     st = iio_priv(claimed_dev);
> > > > > + *     ....
> > > > > + */
> > > > > +DEFINE_CLASS(iio_claim_direct, struct iio_dev *,
> > > > > +            iio_device_release_direct_mode(_T),
> > > > > +            ({
> > > > > +                       struct iio_dev *dev;
> > > > > +                       int d = iio_device_claim_direct_mode(_T);
> > > > > +
> > > > > +                       if (d < 0)
> > > > > +                               dev = ERR_PTR(d);
> > > > > +                       else
> > > > > +                               dev = _T;
> > > > > +                       dev;
> > > > > +            }),
> > > > > +            struct iio_dev *_T);
> > > > > +
> > > > >  int iio_device_claim_buffer_mode(struct iio_dev *indio_dev);
> > > > >  void iio_device_release_buffer_mode(struct iio_dev *indio_dev);
> > > > > 
> > > > > --
> > > > > 2.42.0
> > > > >     
> > > > 
> > > > What is the benefit of exposing `claimed_dev` rather than just the int
> > > > return value? It seems like it just makes more noise in the error
> > > > check.
> > > >     
> > > 
> > > I don't really have a very strong opinion on this but what I really don't
> > > like
> > > much is the pattern:
> > > 
> > > CLASS(type, ret), where the return value is an argument of the macro... It
> > > would
> > > be nice if we could just make it like:
> > > 
> > > ret = guard(type)(...); //or any other variation of the guard() macro
> > > if (ret) 
> > > 	return ret;
> > > 
> > > the above could also be an error pointer or even have one variation of each.
> > > but
> > > yeah, that likely means changing the cleanup.h file and that might be out of
> > > scope for Jonathan's patch series. 
> > >   
> > 
> > I fully agree it's ugly and a little unintuitive but I don't see a way an
> > "lvalue"
> > can work work cleanly (due to magic types under the hood) and I suspect we
> > will
> > have to get used to this pattern.
> >   
> 
> Yeah, given the games being played with the constructor and the _lock definition
> so we return the variable we want to "release" I agree it would be hard to have
> anything clean and likely even harder to read (more than it is already :)).
> 
> However, I think users of the cleanup.h stuff could build on top of it... For
> instance, in our case we could have something like:
> 
> #define IIO_CLAIM_DIRECT(dev) 
> 	int __ret = 0;
> 	CLASS(iio_claim_direct, claimed_dev)(dev);
> 	if ((IS_ERR(claimed_dev))
> 		__ret = PTR_ERR(claimed_dev);
> 	__ret

Maybe, but we'll have to deal with people perpetually trying to brackets around
the complex macro... 
> 
> Then we could use it in the same way as before... Or at the very least I would
> simply make it a bit more readable for IIO (rather than the plain CLASS() call):
> 
> #define IIO_CLAIM_DIRECT(claimed_dev, dev)
> 	CLASS(iio_claim_direct, claimed_dev)(dev)
> 
> Just some thoughts...

Maybe. I'm not sure it's worth it though. This class stuff is
odd and I don't really want to hid it from people too much.

Sometimes better just to make people deal with the ugly on basis they hopefully
go figure out what it is doing.

Jonathan

> 
> - Nuno Sá
> 
> 

Re: [RFC PATCH 1/8] iio: locking: introduce __cleanup() based direct mode claiming infrastructure

[Nuno Sá]

On Mon, 2023-10-23 at 15:34 +0100, Jonathan Cameron wrote:
> On Mon, 23 Oct 2023 13:51:04 +0200
> Nuno Sá <noname.nuno@gmail.com> wrote:
> 
> > On Mon, 2023-10-23 at 10:53 +0100, Jonathan Cameron wrote:
> > > On Mon, 23 Oct 2023 10:55:56 +0200
> > > Nuno Sá <noname.nuno@gmail.com> wrote:
> > >   
> > > > On Sun, 2023-10-22 at 16:10 -0500, David Lechner wrote:  
> > > > > On Sun, Oct 22, 2023 at 10:47 AM Jonathan Cameron <jic23@kernel.org>
> > > > > wrote:    
> > > > > > 
> > > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > > 
> > > > > > Allows use of:
> > > > > > 
> > > > > >         CLASS(iio_claim_direct, claimed_dev)(indio_dev);
> > > > > >         if (IS_ERR(claimed_dev))
> > > > > >                 return PTR_ERR(claimed_dev);
> > > > > > 
> > > > > >         st = iio_priv(claimed_dev);
> > > > > > 
> > > > > > to automatically call iio_device_release_direct_mode() based on
> > > > > > scope.
> > > > > > Typically seen in combination with local device specific locks which
> > > > > > are already have automated cleanup options via guard(mutex)(&st-
> > > > > > >lock)
> > > > > > and scoped_guard().  Using both together allows most error handling
> > > > > > to
> > > > > > be automated.
> > > > > > 
> > > > > > Note that whilst this pattern results in a struct iio_dev
> > > > > > *claimed_dev
> > > > > > that can be used, it is not necessary to do so as long as that
> > > > > > pointer
> > > > > > has been checked for errors as in the example.
> > > > > > 
> > > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > > ---
> > > > > >  drivers/iio/industrialio-core.c |  4 ++++
> > > > > >  include/linux/iio/iio.h         | 25 +++++++++++++++++++++++++
> > > > > >  2 files changed, 29 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/iio/industrialio-core.c
> > > > > > b/drivers/iio/industrialio-
> > > > > > core.c
> > > > > > index c77745b594bd..93bfad105eb5 100644
> > > > > > --- a/drivers/iio/industrialio-core.c
> > > > > > +++ b/drivers/iio/industrialio-core.c
> > > > > > @@ -2065,6 +2065,10 @@
> > > > > > EXPORT_SYMBOL_GPL(iio_device_claim_direct_mode);
> > > > > >   */
> > > > > >  void iio_device_release_direct_mode(struct iio_dev *indio_dev)
> > > > > >  {
> > > > > > +       /* Auto cleanup can result in this being called with an
> > > > > > ERR_PTR
> > > > > > */
> > > > > > +       if (IS_ERR(indio_dev))
> > > > > > +               return;
> > > > > > +
> > > > > >         mutex_unlock(&to_iio_dev_opaque(indio_dev)->mlock);
> > > > > >  }
> > > > > >  EXPORT_SYMBOL_GPL(iio_device_release_direct_mode);
> > > > > > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> > > > > > index d0ce3b71106a..11c42170fda1 100644
> > > > > > --- a/include/linux/iio/iio.h
> > > > > > +++ b/include/linux/iio/iio.h
> > > > > > @@ -9,6 +9,7 @@
> > > > > > 
> > > > > >  #include <linux/device.h>
> > > > > >  #include <linux/cdev.h>
> > > > > > +#include <linux/cleanup.h>
> > > > > >  #include <linux/slab.h>
> > > > > >  #include <linux/iio/types.h>
> > > > > >  /* IIO TODO LIST */
> > > > > > @@ -644,6 +645,30 @@ int __devm_iio_device_register(struct device
> > > > > > *dev,
> > > > > > struct iio_dev *indio_dev,
> > > > > >  int iio_push_event(struct iio_dev *indio_dev, u64 ev_code, s64
> > > > > > timestamp);
> > > > > >  int iio_device_claim_direct_mode(struct iio_dev *indio_dev);
> > > > > >  void iio_device_release_direct_mode(struct iio_dev *indio_dev);
> > > > > > +/*
> > > > > > + * Auto cleanup version of iio_device_claim_direct_mode,
> > > > > > + *
> > > > > > + *     CLASS(iio_claim_direct, claimed_dev)(indio_dev);
> > > > > > + *     if (IS_ERR(claimed_dev))
> > > > > > + *             return PTR_ERR(claimed_dev);
> > > > > > + *
> > > > > > + *     st = iio_priv(claimed_dev);
> > > > > > + *     ....
> > > > > > + */
> > > > > > +DEFINE_CLASS(iio_claim_direct, struct iio_dev *,
> > > > > > +            iio_device_release_direct_mode(_T),
> > > > > > +            ({
> > > > > > +                       struct iio_dev *dev;
> > > > > > +                       int d = iio_device_claim_direct_mode(_T);
> > > > > > +
> > > > > > +                       if (d < 0)
> > > > > > +                               dev = ERR_PTR(d);
> > > > > > +                       else
> > > > > > +                               dev = _T;
> > > > > > +                       dev;
> > > > > > +            }),
> > > > > > +            struct iio_dev *_T);
> > > > > > +
> > > > > >  int iio_device_claim_buffer_mode(struct iio_dev *indio_dev);
> > > > > >  void iio_device_release_buffer_mode(struct iio_dev *indio_dev);
> > > > > > 
> > > > > > --
> > > > > > 2.42.0
> > > > > >     
> > > > > 
> > > > > What is the benefit of exposing `claimed_dev` rather than just the int
> > > > > return value? It seems like it just makes more noise in the error
> > > > > check.
> > > > >     
> > > > 
> > > > I don't really have a very strong opinion on this but what I really
> > > > don't
> > > > like
> > > > much is the pattern:
> > > > 
> > > > CLASS(type, ret), where the return value is an argument of the macro...
> > > > It
> > > > would
> > > > be nice if we could just make it like:
> > > > 
> > > > ret = guard(type)(...); //or any other variation of the guard() macro
> > > > if (ret) 
> > > > 	return ret;
> > > > 
> > > > the above could also be an error pointer or even have one variation of
> > > > each.
> > > > but
> > > > yeah, that likely means changing the cleanup.h file and that might be
> > > > out of
> > > > scope for Jonathan's patch series. 
> > > >   
> > > 
> > > I fully agree it's ugly and a little unintuitive but I don't see a way an
> > > "lvalue"
> > > can work work cleanly (due to magic types under the hood) and I suspect we
> > > will
> > > have to get used to this pattern.
> > >   
> > 
> > Yeah, given the games being played with the constructor and the _lock
> > definition
> > so we return the variable we want to "release" I agree it would be hard to
> > have
> > anything clean and likely even harder to read (more than it is already :)).
> > 
> > However, I think users of the cleanup.h stuff could build on top of it...
> > For
> > instance, in our case we could have something like:
> > 
> > #define IIO_CLAIM_DIRECT(dev) 
> > 	int __ret = 0;
> > 	CLASS(iio_claim_direct, claimed_dev)(dev);
> > 	if ((IS_ERR(claimed_dev))
> > 		__ret = PTR_ERR(claimed_dev);
> > 	__ret
> 
> Maybe, but we'll have to deal with people perpetually trying to brackets
> around
> the complex macro... 
> 
> 

Not sure what you mean here... you mean dealing with people coming up with funny
new macros around CLASS(). In IIO, this is very specific and If I'm not missing
anything the obvious, the above macro with give the same usage as 
iio_device_claim_direct_mode() but without caring about release() - so not sure
people could be that creative :).

Anyways, as I started to say in my first reply, I don't feel strong about this
at all, so feel free to add:

Reviewed-by: Nuno Sa <nuno.sa@analog.com>

Re: [RFC PATCH 1/8] iio: locking: introduce __cleanup() based direct mode claiming infrastructure

[Jonathan Cameron]

On Mon, 23 Oct 2023 16:58:48 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Mon, 2023-10-23 at 15:34 +0100, Jonathan Cameron wrote:
> > On Mon, 23 Oct 2023 13:51:04 +0200
> > Nuno Sá <noname.nuno@gmail.com> wrote:
> >   
> > > On Mon, 2023-10-23 at 10:53 +0100, Jonathan Cameron wrote:  
> > > > On Mon, 23 Oct 2023 10:55:56 +0200
> > > > Nuno Sá <noname.nuno@gmail.com> wrote:
> > > >     
> > > > > On Sun, 2023-10-22 at 16:10 -0500, David Lechner wrote:    
> > > > > > On Sun, Oct 22, 2023 at 10:47 AM Jonathan Cameron <jic23@kernel.org>
> > > > > > wrote:      
> > > > > > > 
> > > > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > > > 
> > > > > > > Allows use of:
> > > > > > > 
> > > > > > >         CLASS(iio_claim_direct, claimed_dev)(indio_dev);
> > > > > > >         if (IS_ERR(claimed_dev))
> > > > > > >                 return PTR_ERR(claimed_dev);
> > > > > > > 
> > > > > > >         st = iio_priv(claimed_dev);
> > > > > > > 
> > > > > > > to automatically call iio_device_release_direct_mode() based on
> > > > > > > scope.
> > > > > > > Typically seen in combination with local device specific locks which
> > > > > > > are already have automated cleanup options via guard(mutex)(&st-  
> > > > > > > >lock)  
> > > > > > > and scoped_guard().  Using both together allows most error handling
> > > > > > > to
> > > > > > > be automated.
> > > > > > > 
> > > > > > > Note that whilst this pattern results in a struct iio_dev
> > > > > > > *claimed_dev
> > > > > > > that can be used, it is not necessary to do so as long as that
> > > > > > > pointer
> > > > > > > has been checked for errors as in the example.
> > > > > > > 
> > > > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > > > ---
> > > > > > >  drivers/iio/industrialio-core.c |  4 ++++
> > > > > > >  include/linux/iio/iio.h         | 25 +++++++++++++++++++++++++
> > > > > > >  2 files changed, 29 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/drivers/iio/industrialio-core.c
> > > > > > > b/drivers/iio/industrialio-
> > > > > > > core.c
> > > > > > > index c77745b594bd..93bfad105eb5 100644
> > > > > > > --- a/drivers/iio/industrialio-core.c
> > > > > > > +++ b/drivers/iio/industrialio-core.c
> > > > > > > @@ -2065,6 +2065,10 @@
> > > > > > > EXPORT_SYMBOL_GPL(iio_device_claim_direct_mode);
> > > > > > >   */
> > > > > > >  void iio_device_release_direct_mode(struct iio_dev *indio_dev)
> > > > > > >  {
> > > > > > > +       /* Auto cleanup can result in this being called with an
> > > > > > > ERR_PTR
> > > > > > > */
> > > > > > > +       if (IS_ERR(indio_dev))
> > > > > > > +               return;
> > > > > > > +
> > > > > > >         mutex_unlock(&to_iio_dev_opaque(indio_dev)->mlock);
> > > > > > >  }
> > > > > > >  EXPORT_SYMBOL_GPL(iio_device_release_direct_mode);
> > > > > > > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> > > > > > > index d0ce3b71106a..11c42170fda1 100644
> > > > > > > --- a/include/linux/iio/iio.h
> > > > > > > +++ b/include/linux/iio/iio.h
> > > > > > > @@ -9,6 +9,7 @@
> > > > > > > 
> > > > > > >  #include <linux/device.h>
> > > > > > >  #include <linux/cdev.h>
> > > > > > > +#include <linux/cleanup.h>
> > > > > > >  #include <linux/slab.h>
> > > > > > >  #include <linux/iio/types.h>
> > > > > > >  /* IIO TODO LIST */
> > > > > > > @@ -644,6 +645,30 @@ int __devm_iio_device_register(struct device
> > > > > > > *dev,
> > > > > > > struct iio_dev *indio_dev,
> > > > > > >  int iio_push_event(struct iio_dev *indio_dev, u64 ev_code, s64
> > > > > > > timestamp);
> > > > > > >  int iio_device_claim_direct_mode(struct iio_dev *indio_dev);
> > > > > > >  void iio_device_release_direct_mode(struct iio_dev *indio_dev);
> > > > > > > +/*
> > > > > > > + * Auto cleanup version of iio_device_claim_direct_mode,
> > > > > > > + *
> > > > > > > + *     CLASS(iio_claim_direct, claimed_dev)(indio_dev);
> > > > > > > + *     if (IS_ERR(claimed_dev))
> > > > > > > + *             return PTR_ERR(claimed_dev);
> > > > > > > + *
> > > > > > > + *     st = iio_priv(claimed_dev);
> > > > > > > + *     ....
> > > > > > > + */
> > > > > > > +DEFINE_CLASS(iio_claim_direct, struct iio_dev *,
> > > > > > > +            iio_device_release_direct_mode(_T),
> > > > > > > +            ({
> > > > > > > +                       struct iio_dev *dev;
> > > > > > > +                       int d = iio_device_claim_direct_mode(_T);
> > > > > > > +
> > > > > > > +                       if (d < 0)
> > > > > > > +                               dev = ERR_PTR(d);
> > > > > > > +                       else
> > > > > > > +                               dev = _T;
> > > > > > > +                       dev;
> > > > > > > +            }),
> > > > > > > +            struct iio_dev *_T);
> > > > > > > +
> > > > > > >  int iio_device_claim_buffer_mode(struct iio_dev *indio_dev);
> > > > > > >  void iio_device_release_buffer_mode(struct iio_dev *indio_dev);
> > > > > > > 
> > > > > > > --
> > > > > > > 2.42.0
> > > > > > >       
> > > > > > 
> > > > > > What is the benefit of exposing `claimed_dev` rather than just the int
> > > > > > return value? It seems like it just makes more noise in the error
> > > > > > check.
> > > > > >       
> > > > > 
> > > > > I don't really have a very strong opinion on this but what I really
> > > > > don't
> > > > > like
> > > > > much is the pattern:
> > > > > 
> > > > > CLASS(type, ret), where the return value is an argument of the macro...
> > > > > It
> > > > > would
> > > > > be nice if we could just make it like:
> > > > > 
> > > > > ret = guard(type)(...); //or any other variation of the guard() macro
> > > > > if (ret) 
> > > > > 	return ret;
> > > > > 
> > > > > the above could also be an error pointer or even have one variation of
> > > > > each.
> > > > > but
> > > > > yeah, that likely means changing the cleanup.h file and that might be
> > > > > out of
> > > > > scope for Jonathan's patch series. 
> > > > >     
> > > > 
> > > > I fully agree it's ugly and a little unintuitive but I don't see a way an
> > > > "lvalue"
> > > > can work work cleanly (due to magic types under the hood) and I suspect we
> > > > will
> > > > have to get used to this pattern.
> > > >     
> > > 
> > > Yeah, given the games being played with the constructor and the _lock
> > > definition
> > > so we return the variable we want to "release" I agree it would be hard to
> > > have
> > > anything clean and likely even harder to read (more than it is already :)).
> > > 
> > > However, I think users of the cleanup.h stuff could build on top of it...
> > > For
> > > instance, in our case we could have something like:
> > > 
> > > #define IIO_CLAIM_DIRECT(dev) 
> > > 	int __ret = 0;
> > > 	CLASS(iio_claim_direct, claimed_dev)(dev);
> > > 	if ((IS_ERR(claimed_dev))
> > > 		__ret = PTR_ERR(claimed_dev);
> > > 	__ret  
> > 
> > Maybe, but we'll have to deal with people perpetually trying to brackets
> > around
> > the complex macro... 
> > 
> >   
> 
> Not sure what you mean here... you mean dealing with people coming up with funny
> new macros around CLASS(). In IIO, this is very specific and If I'm not missing
> anything the obvious, the above macro with give the same usage as 
> iio_device_claim_direct_mode() but without caring about release() - so not sure
> people could be that creative :)
Checkpatch will warn something along the lines of complex macros should be contained
in brackets / or do while()

So the class would go out of scope and be freed at the end of the macro :)

> 
> Anyways, as I started to say in my first reply, I don't feel strong about this
> at all, so feel free to add:
> 
> Reviewed-by: Nuno Sa <nuno.sa@analog.com>

Thanks,

I'm not going to rush with this set anyway given merge window about to open
However I do have a few long flights coming up so might use it in a lot more
drivers for v2.

Jonathan


> 
> 
> 

Re: [RFC PATCH 1/8] iio: locking: introduce __cleanup() based direct mode claiming infrastructure

[Peter Zijlstra]

On Mon, Oct 23, 2023 at 10:55:56AM +0200, Nuno Sá wrote:

> > > +/*
> > > + * Auto cleanup version of iio_device_claim_direct_mode,
> > > + *
> > > + *     CLASS(iio_claim_direct, claimed_dev)(indio_dev);
> > > + *     if (IS_ERR(claimed_dev))
> > > + *             return PTR_ERR(claimed_dev);
> > > + *
> > > + *     st = iio_priv(claimed_dev);
> > > + *     ....
> > > + */
> > > +DEFINE_CLASS(iio_claim_direct, struct iio_dev *,
> > > +            iio_device_release_direct_mode(_T),
> > > +            ({
> > > +                       struct iio_dev *dev;
> > > +                       int d = iio_device_claim_direct_mode(_T);
> > > +
> > > +                       if (d < 0)
> > > +                               dev = ERR_PTR(d);
> > > +                       else
> > > +                               dev = _T;
> > > +                       dev;
> > > +            }),
> > > +            struct iio_dev *_T);
> > > +

> I don't really have a very strong opinion on this but what I really don't like
> much is the pattern:
> 
> CLASS(type, ret), where the return value is an argument of the macro... It would
> be nice if we could just make it like:
> 
> ret = guard(type)(...); //or any other variation of the guard() macro
> if (ret) 
> 	return ret;
> 
> the above could also be an error pointer or even have one variation of each. but
> yeah, that likely means changing the cleanup.h file and that might be out of
> scope for Jonathan's patch series. 

So I have a version for trylocks that I've not managed to send out.. it
doesn't deal with arbitrary error codes, and as someone else down-thread
noted, the guard() thing is not really suited for tricks like this.

Notably I have a patch that converts ptrace_attach() to have a loop like:

	scoped_guard (mutex_intr, &task->signal->cred_guard_mutex) {

		goto success;
	}
	return -ERESTARTNOINTR;

success:
	...
	return 0;


And another patch that does something like:

	scoped_cond_guard (rwsem_read_intr, no_lock,
			   task ? &task->signal->exec_update_lock : NULL) {

		if (0) {
no_lock:
			if (task)
				return -EINTR;
		}
		
		...
	}


---
Subject: cleanup: Add conditional guard support
From: Peter Zijlstra <peterz@infradead.org>
Date: Sun Sep 17 13:22:17 CEST 2023

Adds:

 - DEFINE_GUARD_COND() / DEFINE_LOCK_GUARD_1_COND() to extend existing
   guards with conditional lock primitives, eg. mutex_trylock(),
   mutex_lock_interruptible().

   nb. both primitives allow NULL 'locks', which cause the lock to
       fail (obviously).

 - extends scoped_guard() to not take the body when the the
   conditional guard 'fails'. eg.

     scoped_guard (mutex_intr, &task->signal_cred_guard_mutex) {
	...
     }

   will only execute the body when the mutex is held.

 - provides scoped_cond_guard(name, label, args...); which extends
   scoped_guard() to jump to @label when the lock fails.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
Index: linux-2.6/include/linux/cleanup.h
===================================================================
--- linux-2.6.orig/include/linux/cleanup.h
+++ linux-2.6/include/linux/cleanup.h
@@ -136,14 +136,35 @@ static inline class_##_name##_t class_##
  */
 
 #define DEFINE_GUARD(_name, _type, _lock, _unlock) \
-	DEFINE_CLASS(_name, _type, _unlock, ({ _lock; _T; }), _type _T)
+	DEFINE_CLASS(_name, _type, if (_T) { _unlock; }, ({ _lock; _T; }), _type _T); \
+	static inline void * class_##_name##_lock_ptr(class_##_name##_t *_T) \
+	{ return *_T; }
+
+#define DEFINE_GUARD_COND(_name, _ext, _condlock) \
+	EXTEND_CLASS(_name, _ext, \
+		     ({ void *_t = _T; if (_T && !(_condlock)) _t = NULL; _t; }), \
+		     class_##_name##_t _T) \
+	static inline void * class_##_name##_ext##_lock_ptr(class_##_name##_t *_T) \
+	{ return class_##_name##_lock_ptr(_T); }
+
+#define _guard(_name, var)						\
+	class_##_name##_t var __cleanup(class_##_name##_destructor) =	\
+		class_##_name##_constructor
 
 #define guard(_name) \
-	CLASS(_name, __UNIQUE_ID(guard))
+	_guard(_name, __UNIQUE_ID(guard))
+
+#define __guard_ptr(_name) class_##_name##_lock_ptr
 
 #define scoped_guard(_name, args...)					\
 	for (CLASS(_name, scope)(args),					\
-	     *done = NULL; !done; done = (void *)1)
+	     *done = NULL; __guard_ptr(_name)(&scope) && !done; done = (void *)1)
+
+#define scoped_cond_guard(_name, _label, args...) \
+	for (CLASS(_name, scope)(args), \
+	     *done = NULL; !done; done = (void *)1) \
+		if (!__guard_ptr(_name)(&scope)) goto _label; \
+		else
 
 /*
  * Additional helper macros for generating lock guards with types, either for
@@ -173,6 +194,11 @@ typedef struct {							\
 static inline void class_##_name##_destructor(class_##_name##_t *_T)	\
 {									\
 	if (_T->lock) { _unlock; }					\
+}									\
+									\
+static inline void *class_##_name##_lock_ptr(class_##_name##_t *_T)	\
+{									\
+	return _T->lock;						\
 }
 
 
@@ -201,4 +227,14 @@ __DEFINE_LOCK_GUARD_1(_name, _type, _loc
 __DEFINE_UNLOCK_GUARD(_name, void, _unlock, __VA_ARGS__)		\
 __DEFINE_LOCK_GUARD_0(_name, _lock)
 
+#define DEFINE_LOCK_GUARD_1_COND(_name, _ext, _condlock)		\
+	EXTEND_CLASS(_name, _ext,					\
+		     ({ class_##_name##_t _t = { .lock = l }, *_T = &_t;\
+		        if (_T->lock && !(_condlock)) _T->lock = NULL;	\
+			_t; }),						\
+		     typeof_member(class_##_name##_t, lock) l)		\
+	static inline void * class_##_name##_ext##_lock_ptr(class_##_name##_t *_T) \
+	{ return class_##_name##_lock_ptr(_T); }
+
+
 #endif /* __LINUX_GUARDS_H */
Index: linux-2.6/include/linux/mutex.h
===================================================================
--- linux-2.6.orig/include/linux/mutex.h
+++ linux-2.6/include/linux/mutex.h
@@ -221,6 +221,7 @@ extern void mutex_unlock(struct mutex *l
 extern int atomic_dec_and_mutex_lock(atomic_t *cnt, struct mutex *lock);
 
 DEFINE_GUARD(mutex, struct mutex *, mutex_lock(_T), mutex_unlock(_T))
-DEFINE_FREE(mutex, struct mutex *, if (_T) mutex_unlock(_T))
+DEFINE_GUARD_COND(mutex, _try, mutex_trylock(_T))
+DEFINE_GUARD_COND(mutex, _intr, mutex_lock_interruptible(_T) == 0)
 
 #endif /* __LINUX_MUTEX_H */
Index: linux-2.6/include/linux/spinlock.h
===================================================================
--- linux-2.6.orig/include/linux/spinlock.h
+++ linux-2.6/include/linux/spinlock.h
@@ -507,6 +507,8 @@ DEFINE_LOCK_GUARD_1(raw_spinlock, raw_sp
 		    raw_spin_lock(_T->lock),
 		    raw_spin_unlock(_T->lock))
 
+DEFINE_LOCK_GUARD_1_COND(raw_spinlock, _try, raw_spin_trylock(_T->lock))
+
 DEFINE_LOCK_GUARD_1(raw_spinlock_nested, raw_spinlock_t,
 		    raw_spin_lock_nested(_T->lock, SINGLE_DEPTH_NESTING),
 		    raw_spin_unlock(_T->lock))
@@ -515,23 +517,36 @@ DEFINE_LOCK_GUARD_1(raw_spinlock_irq, ra
 		    raw_spin_lock_irq(_T->lock),
 		    raw_spin_unlock_irq(_T->lock))
 
+DEFINE_LOCK_GUARD_1_COND(raw_spinlock_irq, _try, raw_spin_trylock_irq(_T->lock))
+
 DEFINE_LOCK_GUARD_1(raw_spinlock_irqsave, raw_spinlock_t,
 		    raw_spin_lock_irqsave(_T->lock, _T->flags),
 		    raw_spin_unlock_irqrestore(_T->lock, _T->flags),
 		    unsigned long flags)
 
+DEFINE_LOCK_GUARD_1_COND(raw_spinlock_irqsave, _try,
+			 raw_spin_trylock_irqsave(_T->lock, _T->flags))
+
 DEFINE_LOCK_GUARD_1(spinlock, spinlock_t,
 		    spin_lock(_T->lock),
 		    spin_unlock(_T->lock))
 
+DEFINE_LOCK_GUARD_1_COND(spinlock, _try, spin_trylock(_T->lock))
+
 DEFINE_LOCK_GUARD_1(spinlock_irq, spinlock_t,
 		    spin_lock_irq(_T->lock),
 		    spin_unlock_irq(_T->lock))
 
+DEFINE_LOCK_GUARD_1_COND(spinlock_irq, _try,
+			 spin_trylock_irq(_T->lock))
+
 DEFINE_LOCK_GUARD_1(spinlock_irqsave, spinlock_t,
 		    spin_lock_irqsave(_T->lock, _T->flags),
 		    spin_unlock_irqrestore(_T->lock, _T->flags),
 		    unsigned long flags)
 
+DEFINE_LOCK_GUARD_1_COND(spinlock_irqsave, _try,
+			 spin_trylock_irqsave(_T->lock, _T->flags))
+
 #undef __LINUX_INSIDE_SPINLOCK_H
 #endif /* __LINUX_SPINLOCK_H */
Index: linux-2.6/include/linux/rwsem.h
===================================================================
--- linux-2.6.orig/include/linux/rwsem.h
+++ linux-2.6/include/linux/rwsem.h
@@ -203,11 +203,11 @@ extern void up_read(struct rw_semaphore
 extern void up_write(struct rw_semaphore *sem);
 
 DEFINE_GUARD(rwsem_read, struct rw_semaphore *, down_read(_T), up_read(_T))
-DEFINE_GUARD(rwsem_write, struct rw_semaphore *, down_write(_T), up_write(_T))
-
-DEFINE_FREE(up_read, struct rw_semaphore *, if (_T) up_read(_T))
-DEFINE_FREE(up_write, struct rw_semaphore *, if (_T) up_write(_T))
+DEFINE_GUARD_COND(rwsem_read, _try, down_read_trylock(_T))
+DEFINE_GUARD_COND(rwsem_read, _intr, down_read_interruptible(_T) == 0)
 
+DEFINE_GUARD(rwsem_write, struct rw_semaphore *, down_write(_T), up_write(_T))
+DEFINE_GUARD_COND(rwsem_write, _try, down_write_trylock(_T))
 
 /*
  * downgrade write lock to read lock

Re: [RFC PATCH 1/8] iio: locking: introduce __cleanup() based direct mode claiming infrastructure

[Peter Zijlstra]

On Tue, Oct 24, 2023 at 05:11:23PM +0200, Peter Zijlstra wrote:
> On Mon, Oct 23, 2023 at 10:55:56AM +0200, Nuno Sá wrote:
> 
> > > > +/*
> > > > + * Auto cleanup version of iio_device_claim_direct_mode,
> > > > + *
> > > > + *     CLASS(iio_claim_direct, claimed_dev)(indio_dev);
> > > > + *     if (IS_ERR(claimed_dev))
> > > > + *             return PTR_ERR(claimed_dev);
> > > > + *
> > > > + *     st = iio_priv(claimed_dev);
> > > > + *     ....
> > > > + */
> > > > +DEFINE_CLASS(iio_claim_direct, struct iio_dev *,
> > > > +            iio_device_release_direct_mode(_T),
> > > > +            ({
> > > > +                       struct iio_dev *dev;
> > > > +                       int d = iio_device_claim_direct_mode(_T);
> > > > +
> > > > +                       if (d < 0)
> > > > +                               dev = ERR_PTR(d);
> > > > +                       else
> > > > +                               dev = _T;
> > > > +                       dev;
> > > > +            }),
> > > > +            struct iio_dev *_T);
> > > > +
> 
> > I don't really have a very strong opinion on this but what I really don't like
> > much is the pattern:
> > 
> > CLASS(type, ret), where the return value is an argument of the macro... It would
> > be nice if we could just make it like:
> > 
> > ret = guard(type)(...); //or any other variation of the guard() macro
> > if (ret) 
> > 	return ret;
> > 
> > the above could also be an error pointer or even have one variation of each. but
> > yeah, that likely means changing the cleanup.h file and that might be out of
> > scope for Jonathan's patch series. 
> 
> So I have a version for trylocks that I've not managed to send out.. it
> doesn't deal with arbitrary error codes, and as someone else down-thread
> noted, the guard() thing is not really suited for tricks like this.
> 
> Notably I have a patch that converts ptrace_attach() to have a loop like:
> 
> 	scoped_guard (mutex_intr, &task->signal->cred_guard_mutex) {
> 
> 		goto success;
> 	}
> 	return -ERESTARTNOINTR;
> 
> success:
> 	...
> 	return 0;
> 
> 
> And another patch that does something like:
> 
> 	scoped_cond_guard (rwsem_read_intr, no_lock,
> 			   task ? &task->signal->exec_update_lock : NULL) {
> 
> 		if (0) {
> no_lock:
> 			if (task)
> 				return -EINTR;
> 		}
> 		
> 		...
> 	}
> 

Hmm, looking at:

+       case IIO_CHAN_INFO_RAW: { /* magic value - channel value read */
+               CLASS(iio_claim_direct, claimed_dev)(indio_dev);
+               if (IS_ERR(claimed_dev))
+                       return PTR_ERR(claimed_dev);
+               guard(mutex)(&st->lock);
+
                switch (chan->type) {
                case IIO_VOLTAGE:
                        if (chan->output) {
                                /* Set integer part to cached value */
                                *val = st->dac_val;
+                               return IIO_VAL_INT;
                        } else if (chan->differential) {
                                if (chan->channel == 1)
                                        *val = st->differential_adc_val[0];
                                else
                                        *val = st->differential_adc_val[1];
+                               return IIO_VAL_INT;
                        } else {
                                *val = st->single_ended_adc_val;
+                               return IIO_VAL_INT;
                        }
+
                case IIO_ACCEL:
                        *val = st->accel_val;
+                       return IIO_VAL_INT;
                default:
+                       return -EINVAL;
                }
+       }


And your iio_device_claim_direct_mode(), that is basically a trylock,
either it succeeds (and returns 0) or fails with -EBUSY.

Which means you could write your thing above like:

DEFINE_CLASS(iio_claim_direct, struct iio_dev *,
            iio_device_release_direct_mode(_T),
            ({
                       struct iio_dev *dev;
                       int d = iio_device_claim_direct_mode(_T);

                       if (d < 0)
                               dev = NULL;
                       else
                               dev = _T;
                       dev;
            }),
            struct iio_dev *_T);

static inline void *
class_iio_claim_direct_lock_ptr(class_iio_claim_direct_t *_T)
{ return *_T; }



	case IIO_CHAN_INFO_RAW: /* magic value - channel value read */
		scoped_guard (iio_device_claim, indio_dev) {
			guard(mutex)(&st->lock);
			switch (chan->type) {
			case ..:
				return IIO_VAL_INT;
			default:
				return -EINVAL;
			}
		}
		return -EBUSY;

and it would all just work, no?

Re: [RFC PATCH 1/8] iio: locking: introduce __cleanup() based direct mode claiming infrastructure

[Nuno Sá]

On Tue, 2023-10-24 at 13:23 +0100, Jonathan Cameron wrote:
> On Mon, 23 Oct 2023 16:58:48 +0200
> Nuno Sá <noname.nuno@gmail.com> wrote:
> 
> > On Mon, 2023-10-23 at 15:34 +0100, Jonathan Cameron wrote:
> > > On Mon, 23 Oct 2023 13:51:04 +0200
> > > Nuno Sá <noname.nuno@gmail.com> wrote:
> > >   
> > > > On Mon, 2023-10-23 at 10:53 +0100, Jonathan Cameron wrote:  
> > > > > On Mon, 23 Oct 2023 10:55:56 +0200
> > > > > Nuno Sá <noname.nuno@gmail.com> wrote:
> > > > >     
> > > > > > On Sun, 2023-10-22 at 16:10 -0500, David Lechner wrote:    
> > > > > > > On Sun, Oct 22, 2023 at 10:47 AM Jonathan Cameron
> > > > > > > <jic23@kernel.org>
> > > > > > > wrote:      
> > > > > > > > 
> > > > > > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > > > > 
> > > > > > > > Allows use of:
> > > > > > > > 
> > > > > > > >         CLASS(iio_claim_direct, claimed_dev)(indio_dev);
> > > > > > > >         if (IS_ERR(claimed_dev))
> > > > > > > >                 return PTR_ERR(claimed_dev);
> > > > > > > > 
> > > > > > > >         st = iio_priv(claimed_dev);
> > > > > > > > 
> > > > > > > > to automatically call iio_device_release_direct_mode() based on
> > > > > > > > scope.
> > > > > > > > Typically seen in combination with local device specific locks
> > > > > > > > which
> > > > > > > > are already have automated cleanup options via guard(mutex)(&st-
> > > > > > > >   
> > > > > > > > > lock)  
> > > > > > > > and scoped_guard().  Using both together allows most error
> > > > > > > > handling
> > > > > > > > to
> > > > > > > > be automated.
> > > > > > > > 
> > > > > > > > Note that whilst this pattern results in a struct iio_dev
> > > > > > > > *claimed_dev
> > > > > > > > that can be used, it is not necessary to do so as long as that
> > > > > > > > pointer
> > > > > > > > has been checked for errors as in the example.
> > > > > > > > 
> > > > > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > > > > ---
> > > > > > > >  drivers/iio/industrialio-core.c |  4 ++++
> > > > > > > >  include/linux/iio/iio.h         | 25 +++++++++++++++++++++++++
> > > > > > > >  2 files changed, 29 insertions(+)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/iio/industrialio-core.c
> > > > > > > > b/drivers/iio/industrialio-
> > > > > > > > core.c
> > > > > > > > index c77745b594bd..93bfad105eb5 100644
> > > > > > > > --- a/drivers/iio/industrialio-core.c
> > > > > > > > +++ b/drivers/iio/industrialio-core.c
> > > > > > > > @@ -2065,6 +2065,10 @@
> > > > > > > > EXPORT_SYMBOL_GPL(iio_device_claim_direct_mode);
> > > > > > > >   */
> > > > > > > >  void iio_device_release_direct_mode(struct iio_dev *indio_dev)
> > > > > > > >  {
> > > > > > > > +       /* Auto cleanup can result in this being called with an
> > > > > > > > ERR_PTR
> > > > > > > > */
> > > > > > > > +       if (IS_ERR(indio_dev))
> > > > > > > > +               return;
> > > > > > > > +
> > > > > > > >         mutex_unlock(&to_iio_dev_opaque(indio_dev)->mlock);
> > > > > > > >  }
> > > > > > > >  EXPORT_SYMBOL_GPL(iio_device_release_direct_mode);
> > > > > > > > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> > > > > > > > index d0ce3b71106a..11c42170fda1 100644
> > > > > > > > --- a/include/linux/iio/iio.h
> > > > > > > > +++ b/include/linux/iio/iio.h
> > > > > > > > @@ -9,6 +9,7 @@
> > > > > > > > 
> > > > > > > >  #include <linux/device.h>
> > > > > > > >  #include <linux/cdev.h>
> > > > > > > > +#include <linux/cleanup.h>
> > > > > > > >  #include <linux/slab.h>
> > > > > > > >  #include <linux/iio/types.h>
> > > > > > > >  /* IIO TODO LIST */
> > > > > > > > @@ -644,6 +645,30 @@ int __devm_iio_device_register(struct
> > > > > > > > device
> > > > > > > > *dev,
> > > > > > > > struct iio_dev *indio_dev,
> > > > > > > >  int iio_push_event(struct iio_dev *indio_dev, u64 ev_code, s64
> > > > > > > > timestamp);
> > > > > > > >  int iio_device_claim_direct_mode(struct iio_dev *indio_dev);
> > > > > > > >  void iio_device_release_direct_mode(struct iio_dev *indio_dev);
> > > > > > > > +/*
> > > > > > > > + * Auto cleanup version of iio_device_claim_direct_mode,
> > > > > > > > + *
> > > > > > > > + *     CLASS(iio_claim_direct, claimed_dev)(indio_dev);
> > > > > > > > + *     if (IS_ERR(claimed_dev))
> > > > > > > > + *             return PTR_ERR(claimed_dev);
> > > > > > > > + *
> > > > > > > > + *     st = iio_priv(claimed_dev);
> > > > > > > > + *     ....
> > > > > > > > + */
> > > > > > > > +DEFINE_CLASS(iio_claim_direct, struct iio_dev *,
> > > > > > > > +            iio_device_release_direct_mode(_T),
> > > > > > > > +            ({
> > > > > > > > +                       struct iio_dev *dev;
> > > > > > > > +                       int d =
> > > > > > > > iio_device_claim_direct_mode(_T);
> > > > > > > > +
> > > > > > > > +                       if (d < 0)
> > > > > > > > +                               dev = ERR_PTR(d);
> > > > > > > > +                       else
> > > > > > > > +                               dev = _T;
> > > > > > > > +                       dev;
> > > > > > > > +            }),
> > > > > > > > +            struct iio_dev *_T);
> > > > > > > > +
> > > > > > > >  int iio_device_claim_buffer_mode(struct iio_dev *indio_dev);
> > > > > > > >  void iio_device_release_buffer_mode(struct iio_dev *indio_dev);
> > > > > > > > 
> > > > > > > > --
> > > > > > > > 2.42.0
> > > > > > > >       
> > > > > > > 
> > > > > > > What is the benefit of exposing `claimed_dev` rather than just the
> > > > > > > int
> > > > > > > return value? It seems like it just makes more noise in the error
> > > > > > > check.
> > > > > > >       
> > > > > > 
> > > > > > I don't really have a very strong opinion on this but what I really
> > > > > > don't
> > > > > > like
> > > > > > much is the pattern:
> > > > > > 
> > > > > > CLASS(type, ret), where the return value is an argument of the
> > > > > > macro...
> > > > > > It
> > > > > > would
> > > > > > be nice if we could just make it like:
> > > > > > 
> > > > > > ret = guard(type)(...); //or any other variation of the guard()
> > > > > > macro
> > > > > > if (ret) 
> > > > > > 	return ret;
> > > > > > 
> > > > > > the above could also be an error pointer or even have one variation
> > > > > > of
> > > > > > each.
> > > > > > but
> > > > > > yeah, that likely means changing the cleanup.h file and that might
> > > > > > be
> > > > > > out of
> > > > > > scope for Jonathan's patch series. 
> > > > > >     
> > > > > 
> > > > > I fully agree it's ugly and a little unintuitive but I don't see a way
> > > > > an
> > > > > "lvalue"
> > > > > can work work cleanly (due to magic types under the hood) and I
> > > > > suspect we
> > > > > will
> > > > > have to get used to this pattern.
> > > > >     
> > > > 
> > > > Yeah, given the games being played with the constructor and the _lock
> > > > definition
> > > > so we return the variable we want to "release" I agree it would be hard
> > > > to
> > > > have
> > > > anything clean and likely even harder to read (more than it is already
> > > > :)).
> > > > 
> > > > However, I think users of the cleanup.h stuff could build on top of
> > > > it...
> > > > For
> > > > instance, in our case we could have something like:
> > > > 
> > > > #define IIO_CLAIM_DIRECT(dev) 
> > > > 	int __ret = 0;
> > > > 	CLASS(iio_claim_direct, claimed_dev)(dev);
> > > > 	if ((IS_ERR(claimed_dev))
> > > > 		__ret = PTR_ERR(claimed_dev);
> > > > 	__ret  
> > > 
> > > Maybe, but we'll have to deal with people perpetually trying to brackets
> > > around
> > > the complex macro... 
> > > 
> > >   
> > 
> > Not sure what you mean here... you mean dealing with people coming up with
> > funny
> > new macros around CLASS(). In IIO, this is very specific and If I'm not
> > missing
> > anything the obvious, the above macro with give the same usage as 
> > iio_device_claim_direct_mode() but without caring about release() - so not
> > sure
> > people could be that creative :)
> Checkpatch will warn something along the lines of complex macros should be
> contained
> in brackets / or do while()
> 
> So the class would go out of scope and be freed at the end of the macro :)
> 

Dohh! Tbh, I was not  being "smart" by not putting the brackets in my example
macro. I was just making it simple. For the real thing I had the brackets in my
mind and completely forgot about the scope nature of the cleanup attr.

Anyways, I very much like all of this stuff and I'm starting to use it in all
the places I can...

- Nuno Sá

Re: [RFC PATCH 1/8] iio: locking: introduce __cleanup() based direct mode claiming infrastructure

[Jonathan Cameron]

On Tue, 24 Oct 2023 17:28:00 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Tue, Oct 24, 2023 at 05:11:23PM +0200, Peter Zijlstra wrote:
> > On Mon, Oct 23, 2023 at 10:55:56AM +0200, Nuno Sá wrote:
> >   
> > > > > +/*
> > > > > + * Auto cleanup version of iio_device_claim_direct_mode,
> > > > > + *
> > > > > + *     CLASS(iio_claim_direct, claimed_dev)(indio_dev);
> > > > > + *     if (IS_ERR(claimed_dev))
> > > > > + *             return PTR_ERR(claimed_dev);
> > > > > + *
> > > > > + *     st = iio_priv(claimed_dev);
> > > > > + *     ....
> > > > > + */
> > > > > +DEFINE_CLASS(iio_claim_direct, struct iio_dev *,
> > > > > +            iio_device_release_direct_mode(_T),
> > > > > +            ({
> > > > > +                       struct iio_dev *dev;
> > > > > +                       int d = iio_device_claim_direct_mode(_T);
> > > > > +
> > > > > +                       if (d < 0)
> > > > > +                               dev = ERR_PTR(d);
> > > > > +                       else
> > > > > +                               dev = _T;
> > > > > +                       dev;
> > > > > +            }),
> > > > > +            struct iio_dev *_T);
> > > > > +  
> >   
> > > I don't really have a very strong opinion on this but what I really don't like
> > > much is the pattern:
> > > 
> > > CLASS(type, ret), where the return value is an argument of the macro... It would
> > > be nice if we could just make it like:
> > > 
> > > ret = guard(type)(...); //or any other variation of the guard() macro
> > > if (ret) 
> > > 	return ret;
> > > 
> > > the above could also be an error pointer or even have one variation of each. but
> > > yeah, that likely means changing the cleanup.h file and that might be out of
> > > scope for Jonathan's patch series.   
> > 
> > So I have a version for trylocks that I've not managed to send out.. it
> > doesn't deal with arbitrary error codes, and as someone else down-thread
> > noted, the guard() thing is not really suited for tricks like this.
> > 
> > Notably I have a patch that converts ptrace_attach() to have a loop like:
> > 
> > 	scoped_guard (mutex_intr, &task->signal->cred_guard_mutex) {
> > 
> > 		goto success;
> > 	}
> > 	return -ERESTARTNOINTR;
> > 
> > success:
> > 	...
> > 	return 0;
> > 
> > 
> > And another patch that does something like:
> > 
> > 	scoped_cond_guard (rwsem_read_intr, no_lock,
> > 			   task ? &task->signal->exec_update_lock : NULL) {
> > 
> > 		if (0) {
> > no_lock:
> > 			if (task)
> > 				return -EINTR;
> > 		}
> > 		
> > 		...
> > 	}
> >   
> 
> Hmm, looking at:
> 
> +       case IIO_CHAN_INFO_RAW: { /* magic value - channel value read */
> +               CLASS(iio_claim_direct, claimed_dev)(indio_dev);
> +               if (IS_ERR(claimed_dev))
> +                       return PTR_ERR(claimed_dev);
> +               guard(mutex)(&st->lock);
> +
>                 switch (chan->type) {
>                 case IIO_VOLTAGE:
>                         if (chan->output) {
>                                 /* Set integer part to cached value */
>                                 *val = st->dac_val;
> +                               return IIO_VAL_INT;
>                         } else if (chan->differential) {
>                                 if (chan->channel == 1)
>                                         *val = st->differential_adc_val[0];
>                                 else
>                                         *val = st->differential_adc_val[1];
> +                               return IIO_VAL_INT;
>                         } else {
>                                 *val = st->single_ended_adc_val;
> +                               return IIO_VAL_INT;
>                         }
> +
>                 case IIO_ACCEL:
>                         *val = st->accel_val;
> +                       return IIO_VAL_INT;
>                 default:
> +                       return -EINVAL;
>                 }
> +       }
> 
> 
> And your iio_device_claim_direct_mode(), that is basically a trylock,
> either it succeeds (and returns 0) or fails with -EBUSY.
> 
> Which means you could write your thing above like:
> 
> DEFINE_CLASS(iio_claim_direct, struct iio_dev *,
>             iio_device_release_direct_mode(_T),
>             ({
>                        struct iio_dev *dev;
>                        int d = iio_device_claim_direct_mode(_T);
> 
>                        if (d < 0)
>                                dev = NULL;
>                        else
>                                dev = _T;
>                        dev;
>             }),
>             struct iio_dev *_T);
> 
> static inline void *
> class_iio_claim_direct_lock_ptr(class_iio_claim_direct_t *_T)
> { return *_T; }
> 
> 
> 
> 	case IIO_CHAN_INFO_RAW: /* magic value - channel value read */
> 		scoped_guard (iio_device_claim, indio_dev) {
> 			guard(mutex)(&st->lock);
> 			switch (chan->type) {
> 			case ..:
> 				return IIO_VAL_INT;
> 			default:
> 				return -EINVAL;
> 			}
> 		}
> 		return -EBUSY;
> 
> and it would all just work, no?
With the scoped_guard change you mention in previous
email, this should work.

I'm not that keen on it from a readability point of view as it puts
the 'good' path inline and if we do have anything to do after the
scoped_guard() we need to engage in setting a condition variable to
indicate we'd been in the loop.

Maybe we can extend your other case though for cases where
an early exit fits or make it more general to incorporate this?

scoped_cond_guard_ret(mutex_intr, &mutex, -EINTR)?
or (naming needs work
scoped_cond_guard_call(mutex_intr, &mutex, ({ return -EINTR; }));

#define scoped_cond_guard_call(_name, _call, args...) \
+	for (CLASS(_name, scope)(args), \
+	     *done = NULL; !done; done = (void *)1) \
+		if (!__guard_ptr(_name)(&scope)) _call; \
+		else

Totally untested but hopefully conveys the idea (which is a tiny
extension of your scoped_cond_guard)

Jonathan

Re: [RFC PATCH 1/8] iio: locking: introduce __cleanup() based direct mode claiming infrastructure

[Peter Zijlstra]

On Sat, Oct 28, 2023 at 05:59:28PM +0100, Jonathan Cameron wrote:

> #define scoped_cond_guard_call(_name, _call, args...) \
> +	for (CLASS(_name, scope)(args), \
> +	     *done = NULL; !done; done = (void *)1) \
> +		if (!__guard_ptr(_name)(&scope)) _call; \
> +		else
> 
> Totally untested but hopefully conveys the idea (which is a tiny
> extension of your scoped_cond_guard)

It's a statement not a call, but yeah, I've done something like this,
even made one of my cases simpler too,

Let me got post this stuff so we can get it merged.

Re: [RFC PATCH 1/8] iio: locking: introduce __cleanup() based direct mode claiming infrastructure

[Jonathan Cameron]

On Thu, 2 Nov 2023 11:48:48 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> On Sat, Oct 28, 2023 at 05:59:28PM +0100, Jonathan Cameron wrote:
> 
> > #define scoped_cond_guard_call(_name, _call, args...) \
> > +	for (CLASS(_name, scope)(args), \
> > +	     *done = NULL; !done; done = (void *)1) \
> > +		if (!__guard_ptr(_name)(&scope)) _call; \
> > +		else
> > 
> > Totally untested but hopefully conveys the idea (which is a tiny
> > extension of your scoped_cond_guard)  
> 
> It's a statement not a call, but yeah, I've done something like this,
> even made one of my cases simpler too,

Good point.

> 
> Let me got post this stuff so we can get it merged.
> 

Great.  I really like this whole effort, so thanks for working on it
and your feedback here.

Jonathan