[PATCH 0/3] cleanup: add if_not_cond_guard macro

[PATCH 0/3] cleanup: add if_not_cond_guard macro

[David Lechner]

So far, I have not found scoped_cond_guard() to be nice to work with.
We have been using it quite a bit in the IIO subsystem via the
iio_device_claim_direct_scoped() macro.

The main thing I don't like is that scoped_cond_guard() uses a for loop
internally. In the IIO subsystem, we usually try to return as early as
possible, so often we are returning from all paths from withing this
hidden for loop. However, since it is a for loop, the compiler thinks
that it possible to exit the for loop and so we end up having to use
unreachable() after the end of the scope to avoid a compiler warning.
This is illustrated in the ad7380 patch in this series and there are 36
more instance of unreachable() already introduced in the IIO subsystem
because of this.

Also, scoped_cond_guard() is they only macro for conditional guards in
cleanup.h currently. This means that so far, patches adopting this are
generally converting something that wasn't scoped to be scoped. This
results in changing the indentation of a lot of lines of code, which is
just noise in the patches.

To avoid these issues, the natural thing to do would be to have a
non-scoped version of the scoped_cond_guard() macro. There was was a
rejected attempt to do this in [1], where one of the complaints was:

> > -       rc = down_read_interruptible(&cxl_region_rwsem);
> > -       if (rc)
> > -               return rc;
> > +       cond_guard(rwsem_read_intr, return -EINTR, &cxl_region_rwsem);
>
> Yeah, this is an example of how NOT to do things.
>
> If you can't make the syntax be something clean and sane like
>
>         if (!cond_guard(rwsem_read_intr, &cxl_region_rwsem))
>                 return -EINTR;
>
> then this code should simply not be converted to guards AT ALL.

[1]: https://lore.kernel.org/all/170905252721.2268463.6714121678946763402.stgit@dwillia2-xfh.jf.intel.com/

I couldn't find a way to make a cond_guard() macro that would work like
exactly as suggested (the problem is that you can't declare a variable
in the condition expression of an if statement in C). So I am proposing
a macro that reads basically the same as the above so it still reads
almost like normal C code even though it hides the if statement a bit.

        if_not_cond_guard(rwsem_read_intr, &cxl_region_rwsem)
                return -EINTR;

The "not" is baked into the macro because in most cases, failing to
obtain the lock is the abnormal condition and generally we want to have
the abnormal path be the indented one.

As example users, I've include a modified version of [2] from the
rejected series and an ADC patch that shows how this avoids the
unreachable() and too much indentation issues in the IIO subsystem.

[2]: https://lore.kernel.org/all/170905254443.2268463.935306988251313983.stgit@dwillia2-xfh.jf.intel.com/

---
David Lechner (3):
      cleanup: add conditional guard helper
      iio: adc: ad7380: use if_not_cond_guard for claim direct
      cxl/region: Use cond_guard() in show_targetN()

 drivers/cxl/core/region.c | 18 ++++--------
 drivers/iio/adc/ad7380.c  | 70 +++++++++++++++++++++++------------------------
 include/linux/cleanup.h   | 11 ++++++++
 3 files changed, 51 insertions(+), 48 deletions(-)
---
base-commit: 431c39f6d3edbab14f48dbf37a58ccdc0ac3be1e
change-id: 20241001-cleanup-if_not_cond_guard-0981d867ddf8

Best regards,
-- 
David Lechner <dlechner@baylibre.com>

[PATCH 1/3] cleanup: add conditional guard helper

[David Lechner]

Add a new if_not_cond_guard() macro to cleanup.h for handling
conditional guards such as mutext_trylock().

This is more ergonomic than scoped_cond_guard() for most use cases.
Instead of hiding the error handling statement in the macro args, it
works like a normal if statement and allow the error path to be indented
while the normal code flow path is not indented. And it avoid unwanted
side-effect from hidden for loop in scoped_cond_guard().

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 include/linux/cleanup.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index 038b2d523bf8..682bb3fadfc9 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -273,6 +273,10 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
  *	an anonymous instance of the (guard) class, not recommended for
  *	conditional locks.
  *
+ * if_not_cond_guard(name, args...) { <error handling> }:
+ *	convenience macro for conditional guards that calls the statement that
+ *	follows only if the lock was not acquired (typically an error return).
+ *
  * scoped_guard (name, args...) { }:
  *	similar to CLASS(name, scope)(args), except the variable (with the
  *	explicit name 'scope') is declard in a for-loop such that its scope is
@@ -304,6 +308,13 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
 
 #define __guard_ptr(_name) class_##_name##_lock_ptr
 
+#define __if_not_cond_guard(_name, _id, args...)	\
+	CLASS(_name, _id)(args);			\
+	if (!__guard_ptr(_name)(&_id))
+
+#define if_not_cond_guard(_name, args...) \
+	__if_not_cond_guard(_name, __UNIQUE_ID(guard), args)
+
 #define scoped_guard(_name, args...)					\
 	for (CLASS(_name, scope)(args),					\
 	     *done = NULL; __guard_ptr(_name)(&scope) && !done; done = (void *)1)

-- 
2.43.0

[PATCH 2/3] iio: adc: ad7380: use if_not_cond_guard for claim direct

[David Lechner]

Replace usage of iio_device_claim_direct_scoped() with
if_not_cond_guard().

This makes fewer lines of code, less indentation, avoids having the
error return statement in the macro args, and avoids needing to use
unreachable().

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/adc/ad7380.c | 70 +++++++++++++++++++++++-------------------------
 1 file changed, 34 insertions(+), 36 deletions(-)

diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
index e8bddfb0d07d..19a942035e93 100644
--- a/drivers/iio/adc/ad7380.c
+++ b/drivers/iio/adc/ad7380.c
@@ -569,15 +569,15 @@ static const struct regmap_config ad7380_regmap_config = {
 static int ad7380_debugfs_reg_access(struct iio_dev *indio_dev, u32 reg,
 				     u32 writeval, u32 *readval)
 {
-	iio_device_claim_direct_scoped(return  -EBUSY, indio_dev) {
-		struct ad7380_state *st = iio_priv(indio_dev);
+	struct ad7380_state *st = iio_priv(indio_dev);
 
-		if (readval)
-			return regmap_read(st->regmap, reg, readval);
-		else
-			return regmap_write(st->regmap, reg, writeval);
-	}
-	unreachable();
+	if_not_cond_guard(iio_claim_direct_try, indio_dev)
+		return -EBUSY;
+
+	if (readval)
+		return regmap_read(st->regmap, reg, readval);
+
+	return regmap_write(st->regmap, reg, writeval);
 }
 
 /*
@@ -820,11 +820,10 @@ static int ad7380_read_raw(struct iio_dev *indio_dev,
 
 	switch (info) {
 	case IIO_CHAN_INFO_RAW:
-		iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
-			return ad7380_read_direct(st, chan->scan_index,
-						  scan_type, val);
-		}
-		unreachable();
+		if_not_cond_guard(iio_claim_direct_try, indio_dev)
+			return -EBUSY;
+
+		return ad7380_read_direct(st, chan->scan_index, scan_type, val);
 	case IIO_CHAN_INFO_SCALE:
 		/*
 		 * According to the datasheet, the LSB size is:
@@ -909,31 +908,30 @@ static int ad7380_write_raw(struct iio_dev *indio_dev,
 		/* always enable resolution boost when oversampling is enabled */
 		boost = osr > 0 ? 1 : 0;
 
-		iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
-			ret = regmap_update_bits(st->regmap,
-					AD7380_REG_ADDR_CONFIG1,
-					AD7380_CONFIG1_OSR | AD7380_CONFIG1_RES,
-					FIELD_PREP(AD7380_CONFIG1_OSR, osr) |
-					FIELD_PREP(AD7380_CONFIG1_RES, boost));
+		if_not_cond_guard(iio_claim_direct_try, indio_dev)
+			return -EBUSY;
 
-			if (ret)
-				return ret;
+		ret = regmap_update_bits(st->regmap,
+					 AD7380_REG_ADDR_CONFIG1,
+					 AD7380_CONFIG1_OSR | AD7380_CONFIG1_RES,
+					 FIELD_PREP(AD7380_CONFIG1_OSR, osr) |
+					 FIELD_PREP(AD7380_CONFIG1_RES, boost));
 
-			st->oversampling_ratio = val;
-			st->resolution_boost_enabled = boost;
-
-			/*
-			 * Perform a soft reset. This will flush the oversampling
-			 * block and FIFO but will maintain the content of the
-			 * configurable registers.
-			 */
-			return regmap_update_bits(st->regmap,
-					AD7380_REG_ADDR_CONFIG2,
-					AD7380_CONFIG2_RESET,
-					FIELD_PREP(AD7380_CONFIG2_RESET,
-						   AD7380_CONFIG2_RESET_SOFT));
-		}
-		unreachable();
+		if (ret)
+			return ret;
+
+		st->oversampling_ratio = val;
+		st->resolution_boost_enabled = boost;
+
+		/*
+		 * Perform a soft reset. This will flush the oversampling block
+		 * and FIFO but will maintain the content of the configurable
+		 * registers.
+		 */
+		return regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG2,
+					  AD7380_CONFIG2_RESET,
+					  FIELD_PREP(AD7380_CONFIG2_RESET,
+						     AD7380_CONFIG2_RESET_SOFT));
 	default:
 		return -EINVAL;
 	}

-- 
2.43.0

[PATCH 3/3] cxl/region: Use cond_guard() in show_targetN()

[David Lechner]

Use if_not_cond_guard() to avoid goto and rc variable in the
show_targetN() function.

Suggested-by: Dan Williams <dan.j.williams@intel.com>
Suggested-by: Ira Weiny <ira.weiny@intel.com>
Co-developed-by: Fabio M. De Francesco <fabio.maria.de.francesco@linux.intel.com>
Signed-off-by: Fabio M. De Francesco <fabio.maria.de.francesco@linux.intel.com>
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/cxl/core/region.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index e701e4b04032..e7c0221edeae 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /* Copyright(c) 2022 Intel Corporation. All rights reserved. */
+#include <linux/cleanup.h>
 #include <linux/memregion.h>
 #include <linux/genalloc.h>
 #include <linux/device.h>
@@ -770,28 +771,21 @@ static size_t show_targetN(struct cxl_region *cxlr, char *buf, int pos)
 {
 	struct cxl_region_params *p = &cxlr->params;
 	struct cxl_endpoint_decoder *cxled;
-	int rc;
 
-	rc = down_read_interruptible(&cxl_region_rwsem);
-	if (rc)
-		return rc;
+	if_not_cond_guard(rwsem_read_intr, &cxl_region_rwsem)
+		return -EINTR;
 
 	if (pos >= p->interleave_ways) {
 		dev_dbg(&cxlr->dev, "position %d out of range %d\n", pos,
 			p->interleave_ways);
-		rc = -ENXIO;
-		goto out;
+		return -ENXIO;
 	}
 
 	cxled = p->targets[pos];
 	if (!cxled)
-		rc = sysfs_emit(buf, "\n");
-	else
-		rc = sysfs_emit(buf, "%s\n", dev_name(&cxled->cxld.dev));
-out:
-	up_read(&cxl_region_rwsem);
+		return sysfs_emit(buf, "\n");
 
-	return rc;
+	return sysfs_emit(buf, "%s\n", dev_name(&cxled->cxld.dev));
 }
 
 static int match_free_decoder(struct device *dev, void *data)

-- 
2.43.0

Re: [PATCH 0/3] cleanup: add if_not_cond_guard macro

[Dan Williams]

David Lechner wrote:
> So far, I have not found scoped_cond_guard() to be nice to work with.
> We have been using it quite a bit in the IIO subsystem via the
> iio_device_claim_direct_scoped() macro.
> 
> The main thing I don't like is that scoped_cond_guard() uses a for loop
> internally. In the IIO subsystem, we usually try to return as early as
> possible, so often we are returning from all paths from withing this
> hidden for loop. However, since it is a for loop, the compiler thinks
> that it possible to exit the for loop and so we end up having to use
> unreachable() after the end of the scope to avoid a compiler warning.
> This is illustrated in the ad7380 patch in this series and there are 36
> more instance of unreachable() already introduced in the IIO subsystem
> because of this.
> 
> Also, scoped_cond_guard() is they only macro for conditional guards in
> cleanup.h currently. This means that so far, patches adopting this are
> generally converting something that wasn't scoped to be scoped. This
> results in changing the indentation of a lot of lines of code, which is
> just noise in the patches.
> 
> To avoid these issues, the natural thing to do would be to have a
> non-scoped version of the scoped_cond_guard() macro. There was was a
> rejected attempt to do this in [1], where one of the complaints was:
> 
> > > -       rc = down_read_interruptible(&cxl_region_rwsem);
> > > -       if (rc)
> > > -               return rc;
> > > +       cond_guard(rwsem_read_intr, return -EINTR, &cxl_region_rwsem);
> >
> > Yeah, this is an example of how NOT to do things.
> >
> > If you can't make the syntax be something clean and sane like
> >
> >         if (!cond_guard(rwsem_read_intr, &cxl_region_rwsem))
> >                 return -EINTR;
> >
> > then this code should simply not be converted to guards AT ALL.
> 
> [1]: https://lore.kernel.org/all/170905252721.2268463.6714121678946763402.stgit@dwillia2-xfh.jf.intel.com/
> 
> I couldn't find a way to make a cond_guard() macro that would work like
> exactly as suggested (the problem is that you can't declare a variable
> in the condition expression of an if statement in C). So I am proposing
> a macro that reads basically the same as the above so it still reads
> almost like normal C code even though it hides the if statement a bit.
> 
>         if_not_cond_guard(rwsem_read_intr, &cxl_region_rwsem)
>                 return -EINTR;
> 
> The "not" is baked into the macro because in most cases, failing to
> obtain the lock is the abnormal condition and generally we want to have
> the abnormal path be the indented one.

I think you could also take the "cond" out of the name because that is
implied by the fact it's an 'if'.

So, calling this "if_not_guard ()", for the series, you can add:

Reviewed-by: Dan Williams <dan.j.williams@intel.com>

...but it's ultimately up to Peter and Linus if they find this "if ()"
rename acceptable. If it is I would suggest the style should be treat it
as an "if ()" statement and add this to .clang-format:

diff --git a/.clang-format b/.clang-format
index 252820d9c80a..ae3511a69896 100644
--- a/.clang-format
+++ b/.clang-format
@@ -63,6 +63,8 @@ DerivePointerAlignment: false
 DisableFormat: false
 ExperimentalAutoDetectBinPacking: false
 FixNamespaceComments: false
+IfMacros:
+  - 'if_not_guard'
 
 # Taken from:
 #   git grep -h '^#define [^[:space:]]*for_each[^[:space:]]*(' include/ tools/ \


Last note, while the iio conversion looks correct to me, I would feel
more comfortable if there was a way to have the compiler catch that
plain "guard(iio_claim_direct)" usage is broken. Perhaps declaring
iio_device_claim_direct_mode() as __must_check achieves that effect?

Re: [PATCH 2/3] iio: adc: ad7380: use if_not_cond_guard for claim direct

[kernel test robot]

Hi David,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 431c39f6d3edbab14f48dbf37a58ccdc0ac3be1e]

url:    https://github.com/intel-lab-lkp/linux/commits/David-Lechner/cleanup-add-conditional-guard-helper/20241002-063209
base:   431c39f6d3edbab14f48dbf37a58ccdc0ac3be1e
patch link:    https://lore.kernel.org/r/20241001-cleanup-if_not_cond_guard-v1-2-7753810b0f7a%40baylibre.com
patch subject: [PATCH 2/3] iio: adc: ad7380: use if_not_cond_guard for claim direct
config: x86_64-randconfig-011-20241003 (https://download.01.org/0day-ci/archive/20241003/202410031215.IKHRIsat-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241003/202410031215.IKHRIsat-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410031215.IKHRIsat-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/iio/adc/ad7380.c:823:3: warning: label followed by a declaration is a C23 extension [-Wc23-extensions]
     823 |                 if_not_cond_guard(iio_claim_direct_try, indio_dev)
         |                 ^
   include/linux/cleanup.h:316:2: note: expanded from macro 'if_not_cond_guard'
     316 |         __if_not_cond_guard(_name, __UNIQUE_ID(guard), args)
         |         ^
   include/linux/cleanup.h:312:2: note: expanded from macro '__if_not_cond_guard'
     312 |         CLASS(_name, _id)(args);                        \
         |         ^
   include/linux/cleanup.h:258:2: note: expanded from macro 'CLASS'
     258 |         class_##_name##_t var __cleanup(class_##_name##_destructor) =   \
         |         ^
   <scratch space>:67:1: note: expanded from here
      67 | class_iio_claim_direct_try_t
         | ^
   drivers/iio/adc/ad7380.c:851:2: error: cannot jump from switch statement to this case label
     851 |         default:
         |         ^
   drivers/iio/adc/ad7380.c:823:3: note: jump bypasses initialization of variable with __attribute__((cleanup))
     823 |                 if_not_cond_guard(iio_claim_direct_try, indio_dev)
         |                 ^
   include/linux/cleanup.h:316:29: note: expanded from macro 'if_not_cond_guard'
     316 |         __if_not_cond_guard(_name, __UNIQUE_ID(guard), args)
         |                                    ^
   include/linux/compiler.h:189:29: note: expanded from macro '__UNIQUE_ID'
     189 | #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
         |                             ^
   include/linux/compiler_types.h:84:22: note: expanded from macro '__PASTE'
      84 | #define __PASTE(a,b) ___PASTE(a,b)
         |                      ^
   include/linux/compiler_types.h:83:23: note: expanded from macro '___PASTE'
      83 | #define ___PASTE(a,b) a##b
         |                       ^
   <scratch space>:65:1: note: expanded from here
      65 | __UNIQUE_ID_guard837
         | ^
   drivers/iio/adc/ad7380.c:847:2: error: cannot jump from switch statement to this case label
     847 |         case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
         |         ^
   drivers/iio/adc/ad7380.c:823:3: note: jump bypasses initialization of variable with __attribute__((cleanup))
     823 |                 if_not_cond_guard(iio_claim_direct_try, indio_dev)
         |                 ^
   include/linux/cleanup.h:316:29: note: expanded from macro 'if_not_cond_guard'
     316 |         __if_not_cond_guard(_name, __UNIQUE_ID(guard), args)
         |                                    ^
   include/linux/compiler.h:189:29: note: expanded from macro '__UNIQUE_ID'
     189 | #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
         |                             ^
   include/linux/compiler_types.h:84:22: note: expanded from macro '__PASTE'
      84 | #define __PASTE(a,b) ___PASTE(a,b)
         |                      ^
   include/linux/compiler_types.h:83:23: note: expanded from macro '___PASTE'
      83 | #define ___PASTE(a,b) a##b
         |                       ^
   <scratch space>:65:1: note: expanded from here
      65 | __UNIQUE_ID_guard837
         | ^
   drivers/iio/adc/ad7380.c:838:2: error: cannot jump from switch statement to this case label
     838 |         case IIO_CHAN_INFO_OFFSET:
         |         ^
   drivers/iio/adc/ad7380.c:823:3: note: jump bypasses initialization of variable with __attribute__((cleanup))
     823 |                 if_not_cond_guard(iio_claim_direct_try, indio_dev)
         |                 ^
   include/linux/cleanup.h:316:29: note: expanded from macro 'if_not_cond_guard'
     316 |         __if_not_cond_guard(_name, __UNIQUE_ID(guard), args)
         |                                    ^
   include/linux/compiler.h:189:29: note: expanded from macro '__UNIQUE_ID'
     189 | #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
         |                             ^
   include/linux/compiler_types.h:84:22: note: expanded from macro '__PASTE'
      84 | #define __PASTE(a,b) ___PASTE(a,b)
         |                      ^
   include/linux/compiler_types.h:83:23: note: expanded from macro '___PASTE'
      83 | #define ___PASTE(a,b) a##b
         |                       ^
   <scratch space>:65:1: note: expanded from here
      65 | __UNIQUE_ID_guard837
         | ^
   drivers/iio/adc/ad7380.c:827:2: error: cannot jump from switch statement to this case label
     827 |         case IIO_CHAN_INFO_SCALE:
         |         ^
   drivers/iio/adc/ad7380.c:823:3: note: jump bypasses initialization of variable with __attribute__((cleanup))
     823 |                 if_not_cond_guard(iio_claim_direct_try, indio_dev)
         |                 ^
   include/linux/cleanup.h:316:29: note: expanded from macro 'if_not_cond_guard'
     316 |         __if_not_cond_guard(_name, __UNIQUE_ID(guard), args)
         |                                    ^
   include/linux/compiler.h:189:29: note: expanded from macro '__UNIQUE_ID'
     189 | #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
         |                             ^
   include/linux/compiler_types.h:84:22: note: expanded from macro '__PASTE'
      84 | #define __PASTE(a,b) ___PASTE(a,b)
         |                      ^
   include/linux/compiler_types.h:83:23: note: expanded from macro '___PASTE'
      83 | #define ___PASTE(a,b) a##b
         |                       ^
   <scratch space>:65:1: note: expanded from here
      65 | __UNIQUE_ID_guard837
         | ^
   drivers/iio/adc/ad7380.c:935:2: error: cannot jump from switch statement to this case label
     935 |         default:


vim +823 drivers/iio/adc/ad7380.c

   808	
   809	static int ad7380_read_raw(struct iio_dev *indio_dev,
   810				   struct iio_chan_spec const *chan,
   811				   int *val, int *val2, long info)
   812	{
   813		struct ad7380_state *st = iio_priv(indio_dev);
   814		const struct iio_scan_type *scan_type;
   815	
   816		scan_type = iio_get_current_scan_type(indio_dev, chan);
   817	
   818		if (IS_ERR(scan_type))
   819			return PTR_ERR(scan_type);
   820	
   821		switch (info) {
   822		case IIO_CHAN_INFO_RAW:
 > 823			if_not_cond_guard(iio_claim_direct_try, indio_dev)
   824				return -EBUSY;
   825	
   826			return ad7380_read_direct(st, chan->scan_index, scan_type, val);
   827		case IIO_CHAN_INFO_SCALE:
   828			/*
   829			 * According to the datasheet, the LSB size is:
   830			 *    * (2 × VREF) / 2^N, for differential chips
   831			 *    * VREF / 2^N, for pseudo-differential chips
   832			 * where N is the ADC resolution (i.e realbits)
   833			 */
   834			*val = st->vref_mv;
   835			*val2 = scan_type->realbits - chan->differential;
   836	
   837			return IIO_VAL_FRACTIONAL_LOG2;
   838		case IIO_CHAN_INFO_OFFSET:
   839			/*
   840			 * According to IIO ABI, offset is applied before scale,
   841			 * so offset is: vcm_mv / scale
   842			 */
   843			*val = st->vcm_mv[chan->channel] * (1 << scan_type->realbits)
   844				/ st->vref_mv;
   845	
   846			return IIO_VAL_INT;
   847		case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
   848			*val = st->oversampling_ratio;
   849	
   850			return IIO_VAL_INT;
   851		default:
   852			return -EINVAL;
   853		}
   854	}
   855	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 2/3] iio: adc: ad7380: use if_not_cond_guard for claim direct

[kernel test robot]

Hi David,

kernel test robot noticed the following build errors:

[auto build test ERROR on 431c39f6d3edbab14f48dbf37a58ccdc0ac3be1e]

url:    https://github.com/intel-lab-lkp/linux/commits/David-Lechner/cleanup-add-conditional-guard-helper/20241002-063209
base:   431c39f6d3edbab14f48dbf37a58ccdc0ac3be1e
patch link:    https://lore.kernel.org/r/20241001-cleanup-if_not_cond_guard-v1-2-7753810b0f7a%40baylibre.com
patch subject: [PATCH 2/3] iio: adc: ad7380: use if_not_cond_guard for claim direct
config: arm64-randconfig-001-20241003 (https://download.01.org/0day-ci/archive/20241003/202410031253.vjAMDYuX-lkp@intel.com/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241003/202410031253.vjAMDYuX-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410031253.vjAMDYuX-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/iio/adc/ad7380.c:823:3: error: expected expression
     823 |                 if_not_cond_guard(iio_claim_direct_try, indio_dev)
         |                 ^
   include/linux/cleanup.h:316:2: note: expanded from macro 'if_not_cond_guard'
     316 |         __if_not_cond_guard(_name, __UNIQUE_ID(guard), args)
         |         ^
   include/linux/cleanup.h:312:2: note: expanded from macro '__if_not_cond_guard'
     312 |         CLASS(_name, _id)(args);                        \
         |         ^
   include/linux/cleanup.h:258:2: note: expanded from macro 'CLASS'
     258 |         class_##_name##_t var __cleanup(class_##_name##_destructor) =   \
         |         ^
   <scratch space>:81:1: note: expanded from here
      81 | class_iio_claim_direct_try_t
         | ^
   drivers/iio/adc/ad7380.c:823:3: error: use of undeclared identifier '__UNIQUE_ID_guard799'
   include/linux/cleanup.h:316:29: note: expanded from macro 'if_not_cond_guard'
     316 |         __if_not_cond_guard(_name, __UNIQUE_ID(guard), args)
         |                                    ^
   include/linux/compiler.h:189:29: note: expanded from macro '__UNIQUE_ID'
     189 | #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
         |                             ^
   include/linux/compiler_types.h:84:22: note: expanded from macro '__PASTE'
      84 | #define __PASTE(a,b) ___PASTE(a,b)
         |                      ^
   include/linux/compiler_types.h:83:23: note: expanded from macro '___PASTE'
      83 | #define ___PASTE(a,b) a##b
         |                       ^
   <scratch space>:79:1: note: expanded from here
      79 | __UNIQUE_ID_guard799
         | ^
>> drivers/iio/adc/ad7380.c:935:2: error: cannot jump from switch statement to this case label
     935 |         default:
         |         ^
   drivers/iio/adc/ad7380.c:911:3: note: jump bypasses initialization of variable with __attribute__((cleanup))
     911 |                 if_not_cond_guard(iio_claim_direct_try, indio_dev)
         |                 ^
   include/linux/cleanup.h:316:29: note: expanded from macro 'if_not_cond_guard'
     316 |         __if_not_cond_guard(_name, __UNIQUE_ID(guard), args)
         |                                    ^
   include/linux/compiler.h:189:29: note: expanded from macro '__UNIQUE_ID'
     189 | #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
         |                             ^
   include/linux/compiler_types.h:84:22: note: expanded from macro '__PASTE'
      84 | #define __PASTE(a,b) ___PASTE(a,b)
         |                      ^
   include/linux/compiler_types.h:83:23: note: expanded from macro '___PASTE'
      83 | #define ___PASTE(a,b) a##b
         |                       ^
   <scratch space>:90:1: note: expanded from here
      90 | __UNIQUE_ID_guard800
         | ^
   3 errors generated.

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for MODVERSIONS
   Depends on [n]: MODULES [=y] && !COMPILE_TEST [=y]
   Selected by [y]:
   - RANDSTRUCT_FULL [=y] && (CC_HAS_RANDSTRUCT [=y] || GCC_PLUGINS [=n]) && MODULES [=y]


vim +823 drivers/iio/adc/ad7380.c

   808	
   809	static int ad7380_read_raw(struct iio_dev *indio_dev,
   810				   struct iio_chan_spec const *chan,
   811				   int *val, int *val2, long info)
   812	{
   813		struct ad7380_state *st = iio_priv(indio_dev);
   814		const struct iio_scan_type *scan_type;
   815	
   816		scan_type = iio_get_current_scan_type(indio_dev, chan);
   817	
   818		if (IS_ERR(scan_type))
   819			return PTR_ERR(scan_type);
   820	
   821		switch (info) {
   822		case IIO_CHAN_INFO_RAW:
 > 823			if_not_cond_guard(iio_claim_direct_try, indio_dev)
   824				return -EBUSY;
   825	
   826			return ad7380_read_direct(st, chan->scan_index, scan_type, val);
   827		case IIO_CHAN_INFO_SCALE:
   828			/*
   829			 * According to the datasheet, the LSB size is:
   830			 *    * (2 × VREF) / 2^N, for differential chips
   831			 *    * VREF / 2^N, for pseudo-differential chips
   832			 * where N is the ADC resolution (i.e realbits)
   833			 */
   834			*val = st->vref_mv;
   835			*val2 = scan_type->realbits - chan->differential;
   836	
   837			return IIO_VAL_FRACTIONAL_LOG2;
   838		case IIO_CHAN_INFO_OFFSET:
   839			/*
   840			 * According to IIO ABI, offset is applied before scale,
   841			 * so offset is: vcm_mv / scale
   842			 */
   843			*val = st->vcm_mv[chan->channel] * (1 << scan_type->realbits)
   844				/ st->vref_mv;
   845	
   846			return IIO_VAL_INT;
   847		case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
   848			*val = st->oversampling_ratio;
   849	
   850			return IIO_VAL_INT;
   851		default:
   852			return -EINVAL;
   853		}
   854	}
   855	
   856	static int ad7380_read_avail(struct iio_dev *indio_dev,
   857				     struct iio_chan_spec const *chan,
   858				     const int **vals, int *type, int *length,
   859				     long mask)
   860	{
   861		switch (mask) {
   862		case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
   863			*vals = ad7380_oversampling_ratios;
   864			*length = ARRAY_SIZE(ad7380_oversampling_ratios);
   865			*type = IIO_VAL_INT;
   866	
   867			return IIO_AVAIL_LIST;
   868		default:
   869			return -EINVAL;
   870		}
   871	}
   872	
   873	/**
   874	 * ad7380_osr_to_regval - convert ratio to OSR register value
   875	 * @ratio: ratio to check
   876	 *
   877	 * Check if ratio is present in the list of available ratios and return the
   878	 * corresponding value that needs to be written to the register to select that
   879	 * ratio.
   880	 *
   881	 * Returns: register value (0 to 7) or -EINVAL if there is not an exact match
   882	 */
   883	static int ad7380_osr_to_regval(int ratio)
   884	{
   885		int i;
   886	
   887		for (i = 0; i < ARRAY_SIZE(ad7380_oversampling_ratios); i++) {
   888			if (ratio == ad7380_oversampling_ratios[i])
   889				return i;
   890		}
   891	
   892		return -EINVAL;
   893	}
   894	
   895	static int ad7380_write_raw(struct iio_dev *indio_dev,
   896				    struct iio_chan_spec const *chan, int val,
   897				    int val2, long mask)
   898	{
   899		struct ad7380_state *st = iio_priv(indio_dev);
   900		int ret, osr, boost;
   901	
   902		switch (mask) {
   903		case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
   904			osr = ad7380_osr_to_regval(val);
   905			if (osr < 0)
   906				return osr;
   907	
   908			/* always enable resolution boost when oversampling is enabled */
   909			boost = osr > 0 ? 1 : 0;
   910	
   911			if_not_cond_guard(iio_claim_direct_try, indio_dev)
   912				return -EBUSY;
   913	
   914			ret = regmap_update_bits(st->regmap,
   915						 AD7380_REG_ADDR_CONFIG1,
   916						 AD7380_CONFIG1_OSR | AD7380_CONFIG1_RES,
   917						 FIELD_PREP(AD7380_CONFIG1_OSR, osr) |
   918						 FIELD_PREP(AD7380_CONFIG1_RES, boost));
   919	
   920			if (ret)
   921				return ret;
   922	
   923			st->oversampling_ratio = val;
   924			st->resolution_boost_enabled = boost;
   925	
   926			/*
   927			 * Perform a soft reset. This will flush the oversampling block
   928			 * and FIFO but will maintain the content of the configurable
   929			 * registers.
   930			 */
   931			return regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG2,
   932						  AD7380_CONFIG2_RESET,
   933						  FIELD_PREP(AD7380_CONFIG2_RESET,
   934							     AD7380_CONFIG2_RESET_SOFT));
 > 935		default:
   936			return -EINVAL;
   937		}
   938	}
   939	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 2/3] iio: adc: ad7380: use if_not_cond_guard for claim direct

[David Lechner]

On 10/3/24 12:35 AM, kernel test robot wrote:
> Hi David,
> 
> kernel test robot noticed the following build errors:
> 
> [auto build test ERROR on 431c39f6d3edbab14f48dbf37a58ccdc0ac3be1e]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/David-Lechner/cleanup-add-conditional-guard-helper/20241002-063209
> base:   431c39f6d3edbab14f48dbf37a58ccdc0ac3be1e
> patch link:    https://lore.kernel.org/r/20241001-cleanup-if_not_cond_guard-v1-2-7753810b0f7a%40baylibre.com
> patch subject: [PATCH 2/3] iio: adc: ad7380: use if_not_cond_guard for claim direct
> config: arm64-randconfig-001-20241003 (https://download.01.org/0day-ci/archive/20241003/202410031253.vjAMDYuX-lkp@intel.com/config)
> compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241003/202410031253.vjAMDYuX-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202410031253.vjAMDYuX-lkp@intel.com/
> 
> All errors (new ones prefixed by >>):
> 
>>> drivers/iio/adc/ad7380.c:823:3: error: expected expression
>      823 |                 if_not_cond_guard(iio_claim_direct_try, indio_dev)
>          |                 ^
>    include/linux/cleanup.h:316:2: note: expanded from macro 'if_not_cond_guard'
>      316 |         __if_not_cond_guard(_name, __UNIQUE_ID(guard), args)
>          |         ^
>    include/linux/cleanup.h:312:2: note: expanded from macro '__if_not_cond_guard'
>      312 |         CLASS(_name, _id)(args);                        \
>          |         ^
>    include/linux/cleanup.h:258:2: note: expanded from macro 'CLASS'
>      258 |         class_##_name##_t var __cleanup(class_##_name##_destructor) =   \
>          |         ^
>    <scratch space>:81:1: note: expanded from here
>       81 | class_iio_claim_direct_try_t
>          | ^
>    drivers/iio/adc/ad7380.c:823:3: error: use of undeclared identifier '__UNIQUE_ID_guard799'
>    include/linux/cleanup.h:316:29: note: expanded from macro 'if_not_cond_guard'
>      316 |         __if_not_cond_guard(_name, __UNIQUE_ID(guard), args)
>          |                                    ^
>    include/linux/compiler.h:189:29: note: expanded from macro '__UNIQUE_ID'
>      189 | #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
>          |                             ^
>    include/linux/compiler_types.h:84:22: note: expanded from macro '__PASTE'
>       84 | #define __PASTE(a,b) ___PASTE(a,b)
>          |                      ^
>    include/linux/compiler_types.h:83:23: note: expanded from macro '___PASTE'
>       83 | #define ___PASTE(a,b) a##b
>          |                       ^
>    <scratch space>:79:1: note: expanded from here
>       79 | __UNIQUE_ID_guard799
>          | ^
>>> drivers/iio/adc/ad7380.c:935:2: error: cannot jump from switch statement to this case label
>      935 |         default:
>          |         ^
>    drivers/iio/adc/ad7380.c:911:3: note: jump bypasses initialization of variable with __attribute__((cleanup))
>      911 |                 if_not_cond_guard(iio_claim_direct_try, indio_dev)
>          |                 ^
>    include/linux/cleanup.h:316:29: note: expanded from macro 'if_not_cond_guard'
>      316 |         __if_not_cond_guard(_name, __UNIQUE_ID(guard), args)
>          |                                    ^
>    include/linux/compiler.h:189:29: note: expanded from macro '__UNIQUE_ID'
>      189 | #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
>          |                             ^
>    include/linux/compiler_types.h:84:22: note: expanded from macro '__PASTE'
>       84 | #define __PASTE(a,b) ___PASTE(a,b)
>          |                      ^
>    include/linux/compiler_types.h:83:23: note: expanded from macro '___PASTE'
>       83 | #define ___PASTE(a,b) a##b
>          |                       ^
>    <scratch space>:90:1: note: expanded from here
>       90 | __UNIQUE_ID_guard800
>          | ^
>    3 errors generated.
> 
> Kconfig warnings: (for reference only)
>    WARNING: unmet direct dependencies detected for MODVERSIONS
>    Depends on [n]: MODULES [=y] && !COMPILE_TEST [=y]
>    Selected by [y]:
>    - RANDSTRUCT_FULL [=y] && (CC_HAS_RANDSTRUCT [=y] || GCC_PLUGINS [=n]) && MODULES [=y]
> 
> 
> vim +823 drivers/iio/adc/ad7380.c
> 
>    808	
>    809	static int ad7380_read_raw(struct iio_dev *indio_dev,
>    810				   struct iio_chan_spec const *chan,
>    811				   int *val, int *val2, long info)
>    812	{
>    813		struct ad7380_state *st = iio_priv(indio_dev);
>    814		const struct iio_scan_type *scan_type;
>    815	
>    816		scan_type = iio_get_current_scan_type(indio_dev, chan);
>    817	
>    818		if (IS_ERR(scan_type))
>    819			return PTR_ERR(scan_type);
>    820	
>    821		switch (info) {
>    822		case IIO_CHAN_INFO_RAW:
>  > 823			if_not_cond_guard(iio_claim_direct_try, indio_dev)
>    824				return -EBUSY;
>    825	
>    826			return ad7380_read_direct(st, chan->scan_index, scan_type, val);
>    827		case IIO_CHAN_INFO_SCALE:
>    828			/*
>    829			 * According to the datasheet, the LSB size is:
>    830			 *    * (2 × VREF) / 2^N, for differential chips
>    831			 *    * VREF / 2^N, for pseudo-differential chips
>    832			 * where N is the ADC resolution (i.e realbits)
>    833			 */
>    834			*val = st->vref_mv;
>    835			*val2 = scan_type->realbits - chan->differential;
>    836	
>    837			return IIO_VAL_FRACTIONAL_LOG2;
>    838		case IIO_CHAN_INFO_OFFSET:
>    839			/*
>    840			 * According to IIO ABI, offset is applied before scale,
>    841			 * so offset is: vcm_mv / scale
>    842			 */
>    843			*val = st->vcm_mv[chan->channel] * (1 << scan_type->realbits)
>    844				/ st->vref_mv;
>    845	
>    846			return IIO_VAL_INT;
>    847		case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>    848			*val = st->oversampling_ratio;
>    849	
>    850			return IIO_VAL_INT;
>    851		default:
>    852			return -EINVAL;
>    853		}
>    854	}
>    855	
>    856	static int ad7380_read_avail(struct iio_dev *indio_dev,
>    857				     struct iio_chan_spec const *chan,
>    858				     const int **vals, int *type, int *length,
>    859				     long mask)
>    860	{
>    861		switch (mask) {
>    862		case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>    863			*vals = ad7380_oversampling_ratios;
>    864			*length = ARRAY_SIZE(ad7380_oversampling_ratios);
>    865			*type = IIO_VAL_INT;
>    866	
>    867			return IIO_AVAIL_LIST;
>    868		default:
>    869			return -EINVAL;
>    870		}
>    871	}
>    872	
>    873	/**
>    874	 * ad7380_osr_to_regval - convert ratio to OSR register value
>    875	 * @ratio: ratio to check
>    876	 *
>    877	 * Check if ratio is present in the list of available ratios and return the
>    878	 * corresponding value that needs to be written to the register to select that
>    879	 * ratio.
>    880	 *
>    881	 * Returns: register value (0 to 7) or -EINVAL if there is not an exact match
>    882	 */
>    883	static int ad7380_osr_to_regval(int ratio)
>    884	{
>    885		int i;
>    886	
>    887		for (i = 0; i < ARRAY_SIZE(ad7380_oversampling_ratios); i++) {
>    888			if (ratio == ad7380_oversampling_ratios[i])
>    889				return i;
>    890		}
>    891	
>    892		return -EINVAL;
>    893	}
>    894	
>    895	static int ad7380_write_raw(struct iio_dev *indio_dev,
>    896				    struct iio_chan_spec const *chan, int val,
>    897				    int val2, long mask)
>    898	{
>    899		struct ad7380_state *st = iio_priv(indio_dev);
>    900		int ret, osr, boost;
>    901	
>    902		switch (mask) {
>    903		case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>    904			osr = ad7380_osr_to_regval(val);
>    905			if (osr < 0)
>    906				return osr;
>    907	
>    908			/* always enable resolution boost when oversampling is enabled */
>    909			boost = osr > 0 ? 1 : 0;
>    910	
>    911			if_not_cond_guard(iio_claim_direct_try, indio_dev)
>    912				return -EBUSY;
>    913	
>    914			ret = regmap_update_bits(st->regmap,
>    915						 AD7380_REG_ADDR_CONFIG1,
>    916						 AD7380_CONFIG1_OSR | AD7380_CONFIG1_RES,
>    917						 FIELD_PREP(AD7380_CONFIG1_OSR, osr) |
>    918						 FIELD_PREP(AD7380_CONFIG1_RES, boost));
>    919	
>    920			if (ret)
>    921				return ret;
>    922	
>    923			st->oversampling_ratio = val;
>    924			st->resolution_boost_enabled = boost;
>    925	
>    926			/*
>    927			 * Perform a soft reset. This will flush the oversampling block
>    928			 * and FIFO but will maintain the content of the configurable
>    929			 * registers.
>    930			 */
>    931			return regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG2,
>    932						  AD7380_CONFIG2_RESET,
>    933						  FIELD_PREP(AD7380_CONFIG2_RESET,
>    934							     AD7380_CONFIG2_RESET_SOFT));
>  > 935		default:
>    936			return -EINVAL;
>    937		}
>    938	}
>    939	
> 

This warning and the "warning: label followed by a declaration
is a C23 extension" from the other bot message are both fixed
with the following modification:


diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
index 19a942035e93..34adc5aeb6f3 100644
--- a/drivers/iio/adc/ad7380.c
+++ b/drivers/iio/adc/ad7380.c
@@ -819,11 +819,12 @@ static int ad7380_read_raw(struct iio_dev *indio_dev,
 		return PTR_ERR(scan_type);
 
 	switch (info) {
-	case IIO_CHAN_INFO_RAW:
+	case IIO_CHAN_INFO_RAW: {
 		if_not_cond_guard(iio_claim_direct_try, indio_dev)
 			return -EBUSY;
 
 		return ad7380_read_direct(st, chan->scan_index, scan_type, val);
+	}
 	case IIO_CHAN_INFO_SCALE:
 		/*
 		 * According to the datasheet, the LSB size is:
@@ -900,7 +901,7 @@ static int ad7380_write_raw(struct iio_dev *indio_dev,
 	int ret, osr, boost;
 
 	switch (mask) {
-	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO: {
 		osr = ad7380_osr_to_regval(val);
 		if (osr < 0)
 			return osr;
@@ -932,6 +933,7 @@ static int ad7380_write_raw(struct iio_dev *indio_dev,
 					  AD7380_CONFIG2_RESET,
 					  FIELD_PREP(AD7380_CONFIG2_RESET,
 						     AD7380_CONFIG2_RESET_SOFT));
+	}
 	default:
 		return -EINVAL;
 	}

Re: [PATCH 1/3] cleanup: add conditional guard helper

[Dan Williams]

[ add Fabio ]

David Lechner wrote:
> Add a new if_not_cond_guard() macro to cleanup.h for handling
> conditional guards such as mutext_trylock().
> 
> This is more ergonomic than scoped_cond_guard() for most use cases.
> Instead of hiding the error handling statement in the macro args, it
> works like a normal if statement and allow the error path to be indented
> while the normal code flow path is not indented. And it avoid unwanted
> side-effect from hidden for loop in scoped_cond_guard().
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>

Hi David,

When you update this to the if_not_guard() name can you also add Fabio as a
co-developer? His work [1] contributed to eliciting the response from Linus,
and then this patch takes the novel additional step to create an "if ()" macro.

Thanks for pushing this forward!

[1]: http://lore.kernel.org/20240130164059.25130-1-fabio.maria.de.francesco@linux.intel.com

Re: [PATCH 1/3] cleanup: add conditional guard helper

[David Lechner]

On 10/4/24 12:34 PM, Dan Williams wrote:
> [ add Fabio ]
> 
> David Lechner wrote:
>> Add a new if_not_cond_guard() macro to cleanup.h for handling
>> conditional guards such as mutext_trylock().
>>
>> This is more ergonomic than scoped_cond_guard() for most use cases.
>> Instead of hiding the error handling statement in the macro args, it
>> works like a normal if statement and allow the error path to be indented
>> while the normal code flow path is not indented. And it avoid unwanted
>> side-effect from hidden for loop in scoped_cond_guard().
>>
>> Signed-off-by: David Lechner <dlechner@baylibre.com>
> 
> Hi David,
> 
> When you update this to the if_not_guard() name can you also add Fabio as a
> co-developer? His work [1] contributed to eliciting the response from Linus,
> and then this patch takes the novel additional step to create an "if ()" macro.
> 
> Thanks for pushing this forward!
> 
> [1]: http://lore.kernel.org/20240130164059.25130-1-fabio.maria.de.francesco@linux.intel.com

Sure, I didn't dig deep enough to find that patch, but basically
the same idea. :-)

Re: [PATCH 0/3] cleanup: add if_not_cond_guard macro

[Jonathan Cameron]

On Tue, 1 Oct 2024 19:13:01 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> David Lechner wrote:
> > So far, I have not found scoped_cond_guard() to be nice to work with.
> > We have been using it quite a bit in the IIO subsystem via the
> > iio_device_claim_direct_scoped() macro.
> > 
> > The main thing I don't like is that scoped_cond_guard() uses a for loop
> > internally. In the IIO subsystem, we usually try to return as early as
> > possible, so often we are returning from all paths from withing this
> > hidden for loop. However, since it is a for loop, the compiler thinks
> > that it possible to exit the for loop and so we end up having to use
> > unreachable() after the end of the scope to avoid a compiler warning.
> > This is illustrated in the ad7380 patch in this series and there are 36
> > more instance of unreachable() already introduced in the IIO subsystem
> > because of this.
> > 
> > Also, scoped_cond_guard() is they only macro for conditional guards in
> > cleanup.h currently. This means that so far, patches adopting this are
> > generally converting something that wasn't scoped to be scoped. This
> > results in changing the indentation of a lot of lines of code, which is
> > just noise in the patches.
> > 
> > To avoid these issues, the natural thing to do would be to have a
> > non-scoped version of the scoped_cond_guard() macro. There was was a
> > rejected attempt to do this in [1], where one of the complaints was:
> >   
> > > > -       rc = down_read_interruptible(&cxl_region_rwsem);
> > > > -       if (rc)
> > > > -               return rc;
> > > > +       cond_guard(rwsem_read_intr, return -EINTR, &cxl_region_rwsem);  
> > >
> > > Yeah, this is an example of how NOT to do things.
> > >
> > > If you can't make the syntax be something clean and sane like
> > >
> > >         if (!cond_guard(rwsem_read_intr, &cxl_region_rwsem))
> > >                 return -EINTR;
> > >
> > > then this code should simply not be converted to guards AT ALL.  
> > 
> > [1]: https://lore.kernel.org/all/170905252721.2268463.6714121678946763402.stgit@dwillia2-xfh.jf.intel.com/
> > 
> > I couldn't find a way to make a cond_guard() macro that would work like
> > exactly as suggested (the problem is that you can't declare a variable
> > in the condition expression of an if statement in C). So I am proposing
> > a macro that reads basically the same as the above so it still reads
> > almost like normal C code even though it hides the if statement a bit.
> > 
> >         if_not_cond_guard(rwsem_read_intr, &cxl_region_rwsem)
> >                 return -EINTR;
> > 
> > The "not" is baked into the macro because in most cases, failing to
> > obtain the lock is the abnormal condition and generally we want to have
> > the abnormal path be the indented one.  
> 
> I think you could also take the "cond" out of the name because that is
> implied by the fact it's an 'if'.
> 
> So, calling this "if_not_guard ()", for the series, you can add:
> 
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> 
> ...but it's ultimately up to Peter and Linus if they find this "if ()"
> rename acceptable. 

This is a nice improvement to my eyes anyway and I hope will be fine
with Linus and Peter.  Whilst I like the cond guard stuff for the
simplifications it has brought in the IIO code, it is clunky in
some cases as you've pointed out.

Thanks for driving this forwards.


> If it is I would suggest the style should be treat it
> as an "if ()" statement and add this to .clang-format:
> 
> diff --git a/.clang-format b/.clang-format
> index 252820d9c80a..ae3511a69896 100644
> --- a/.clang-format
> +++ b/.clang-format
> @@ -63,6 +63,8 @@ DerivePointerAlignment: false
>  DisableFormat: false
>  ExperimentalAutoDetectBinPacking: false
>  FixNamespaceComments: false
> +IfMacros:
> +  - 'if_not_guard'
>  
>  # Taken from:
>  #   git grep -h '^#define [^[:space:]]*for_each[^[:space:]]*(' include/ tools/ \
> 
> 
> Last note, while the iio conversion looks correct to me, I would feel
> more comfortable if there was a way to have the compiler catch that
> plain "guard(iio_claim_direct)" usage is broken. Perhaps declaring
> iio_device_claim_direct_mode() as __must_check achieves that effect?

That would be good to catch. We've not had many missuses but there
have been one or two that have shown up in review.

Jonathan

Re: [PATCH 1/3] cleanup: add conditional guard helper

[Peter Zijlstra]

On Tue, Oct 01, 2024 at 05:30:18PM -0500, David Lechner wrote:
> Add a new if_not_cond_guard() macro to cleanup.h for handling
> conditional guards such as mutext_trylock().
> 
> This is more ergonomic than scoped_cond_guard() for most use cases.
> Instead of hiding the error handling statement in the macro args, it
> works like a normal if statement and allow the error path to be indented
> while the normal code flow path is not indented. And it avoid unwanted
> side-effect from hidden for loop in scoped_cond_guard().
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
>  include/linux/cleanup.h | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
> index 038b2d523bf8..682bb3fadfc9 100644
> --- a/include/linux/cleanup.h
> +++ b/include/linux/cleanup.h
> @@ -273,6 +273,10 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
>   *	an anonymous instance of the (guard) class, not recommended for
>   *	conditional locks.
>   *
> + * if_not_cond_guard(name, args...) { <error handling> }:
> + *	convenience macro for conditional guards that calls the statement that
> + *	follows only if the lock was not acquired (typically an error return).
> + *
>   * scoped_guard (name, args...) { }:
>   *	similar to CLASS(name, scope)(args), except the variable (with the
>   *	explicit name 'scope') is declard in a for-loop such that its scope is
> @@ -304,6 +308,13 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
>  
>  #define __guard_ptr(_name) class_##_name##_lock_ptr
>  
> +#define __if_not_cond_guard(_name, _id, args...)	\
> +	CLASS(_name, _id)(args);			\
> +	if (!__guard_ptr(_name)(&_id))
> +
> +#define if_not_cond_guard(_name, args...) \
> +	__if_not_cond_guard(_name, __UNIQUE_ID(guard), args)
> +
>  #define scoped_guard(_name, args...)					\
>  	for (CLASS(_name, scope)(args),					\
>  	     *done = NULL; __guard_ptr(_name)(&scope) && !done; done = (void *)1)


So if I stick this on top of:

  https://lkml.kernel.org/r/20241011121535.28049-1-przemyslaw.kitszel@intel.com

then I can add the below:

--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -277,6 +277,8 @@ static inline class_##_name##_t class_##
  *	convenience macro for conditional guards that calls the statement that
  *	follows only if the lock was not acquired (typically an error return).
  *
+ *	Only for conditional locks.
+ *
  * scoped_guard (name, args...) { }:
  *	similar to CLASS(name, scope)(args), except the variable (with the
  *	explicit name 'scope') is declard in a for-loop such that its scope is
@@ -290,7 +292,6 @@ static inline class_##_name##_t class_##
  *      acquire fails.
  *
  *	Only for conditional locks.
- *
  */
 
 #define __DEFINE_CLASS_IS_CONDITIONAL(_name, _is_cond)	\
@@ -342,6 +343,7 @@ _label:										\
 		       __UNIQUE_ID(label), args)
 
 #define __if_not_guard(_name, _id, args...)		\
+	BUILD_BUG_ON(!__is_cond_ptr(_name));		\
 	CLASS(_name, _id)(args);			\
 	if (!__guard_ptr(_name)(&_id))
 

That make sense to people?

I've queued these two patches:

  git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git locking/core

But lacking if_not_guard() users, the robot isn't really going to give
me much feedback there, I suppose...

Re: [PATCH 1/3] cleanup: add conditional guard helper

[Przemek Kitszel]

On 10/18/24 13:15, Peter Zijlstra wrote:
> On Tue, Oct 01, 2024 at 05:30:18PM -0500, David Lechner wrote:
>> Add a new if_not_cond_guard() macro to cleanup.h for handling
>> conditional guards such as mutext_trylock().
>>
>> This is more ergonomic than scoped_cond_guard() for most use cases.
>> Instead of hiding the error handling statement in the macro args, it
>> works like a normal if statement and allow the error path to be indented
>> while the normal code flow path is not indented. And it avoid unwanted
>> side-effect from hidden for loop in scoped_cond_guard().
>>
>> Signed-off-by: David Lechner <dlechner@baylibre.com>

So this is guard()() with error handler for cond class of locks.
I would name such guard_or_err(), or guard_or_err_block(), to make it
obvious why there is a block attached (so bad we could not ENFORCE that
there is a block atached).

Then, having it, it would make sense to not only limit guard_or_err() to
cond class of locks, but also forbid plain guard() with cond locks
(instead just discouraging it in the doc).

>> ---
>>   include/linux/cleanup.h | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
>> index 038b2d523bf8..682bb3fadfc9 100644
>> --- a/include/linux/cleanup.h
>> +++ b/include/linux/cleanup.h
>> @@ -273,6 +273,10 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
>>    *	an anonymous instance of the (guard) class, not recommended for
>>    *	conditional locks.
>>    *
>> + * if_not_cond_guard(name, args...) { <error handling> }:
>> + *	convenience macro for conditional guards that calls the statement that
>> + *	follows only if the lock was not acquired (typically an error return).
>> + *
>>    * scoped_guard (name, args...) { }:
>>    *	similar to CLASS(name, scope)(args), except the variable (with the
>>    *	explicit name 'scope') is declard in a for-loop such that its scope is
>> @@ -304,6 +308,13 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
>>   
>>   #define __guard_ptr(_name) class_##_name##_lock_ptr
>>   
>> +#define __if_not_cond_guard(_name, _id, args...)	\
>> +	CLASS(_name, _id)(args);			\
>> +	if (!__guard_ptr(_name)(&_id))
>> +
>> +#define if_not_cond_guard(_name, args...) \
>> +	__if_not_cond_guard(_name, __UNIQUE_ID(guard), args)
>> +
>>   #define scoped_guard(_name, args...)					\
>>   	for (CLASS(_name, scope)(args),					\
>>   	     *done = NULL; __guard_ptr(_name)(&scope) && !done; done = (void *)1)
> 
> 
> So if I stick this on top of:
> 
>    https://lkml.kernel.org/r/20241011121535.28049-1-przemyslaw.kitszel@intel.com

I have v4 that fixes non-cond version. Apologies it took me that long.
[v4] 
https://lore.kernel.org/netdev/20241018113823.171256-1-przemyslaw.kitszel@intel.com

I have tested it also with the unrechable() calls removed, as suggested
by David Lechner here:
https://lore.kernel.org/netdev/0f4786e9-d738-435d-afb9-8c0c4a028ddb@baylibre.com

> 
> then I can add the below:
> 
> --- a/include/linux/cleanup.h
> +++ b/include/linux/cleanup.h
> @@ -277,6 +277,8 @@ static inline class_##_name##_t class_##
>    *	convenience macro for conditional guards that calls the statement that
>    *	follows only if the lock was not acquired (typically an error return).
>    *
> + *	Only for conditional locks.
> + *
>    * scoped_guard (name, args...) { }:
>    *	similar to CLASS(name, scope)(args), except the variable (with the
>    *	explicit name 'scope') is declard in a for-loop such that its scope is
> @@ -290,7 +292,6 @@ static inline class_##_name##_t class_##
>    *      acquire fails.
>    *
>    *	Only for conditional locks.
> - *
>    */
>   
>   #define __DEFINE_CLASS_IS_CONDITIONAL(_name, _is_cond)	\
> @@ -342,6 +343,7 @@ _label:										\
>   		       __UNIQUE_ID(label), args)
>   
>   #define __if_not_guard(_name, _id, args...)		\
> +	BUILD_BUG_ON(!__is_cond_ptr(_name));		\
>   	CLASS(_name, _id)(args);			\
>   	if (!__guard_ptr(_name)(&_id))
>   
> 
> That make sense to people?

despite name, looks promising!

> 
> I've queued these two patches:
> 
>    git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git locking/core
> 
> But lacking if_not_guard() users, the robot isn't really going to give
> me much feedback there, I suppose...

Couldn't you just pick the other patches, that use the newly introduced
macro?

Re: [PATCH 1/3] cleanup: add conditional guard helper

[Jonathan Cameron]

On Fri, 18 Oct 2024 14:31:43 +0200
Przemek Kitszel <przemyslaw.kitszel@intel.com> wrote:

> On 10/18/24 13:15, Peter Zijlstra wrote:
> > On Tue, Oct 01, 2024 at 05:30:18PM -0500, David Lechner wrote:  
> >> Add a new if_not_cond_guard() macro to cleanup.h for handling
> >> conditional guards such as mutext_trylock().
> >>
> >> This is more ergonomic than scoped_cond_guard() for most use cases.
> >> Instead of hiding the error handling statement in the macro args, it
> >> works like a normal if statement and allow the error path to be indented
> >> while the normal code flow path is not indented. And it avoid unwanted
> >> side-effect from hidden for loop in scoped_cond_guard().
> >>
> >> Signed-off-by: David Lechner <dlechner@baylibre.com>  
> 
> So this is guard()() with error handler for cond class of locks.
> I would name such guard_or_err(), or guard_or_err_block(), to make it
> obvious why there is a block attached (so bad we could not ENFORCE that
> there is a block atached).
> 
> Then, having it, it would make sense to not only limit guard_or_err() to
> cond class of locks, but also forbid plain guard() with cond locks
> (instead just discouraging it in the doc).
> 
> >> ---
> >>   include/linux/cleanup.h | 11 +++++++++++
> >>   1 file changed, 11 insertions(+)
> >>
> >> diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
> >> index 038b2d523bf8..682bb3fadfc9 100644
> >> --- a/include/linux/cleanup.h
> >> +++ b/include/linux/cleanup.h
> >> @@ -273,6 +273,10 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
> >>    *	an anonymous instance of the (guard) class, not recommended for
> >>    *	conditional locks.
> >>    *
> >> + * if_not_cond_guard(name, args...) { <error handling> }:
> >> + *	convenience macro for conditional guards that calls the statement that
> >> + *	follows only if the lock was not acquired (typically an error return).
> >> + *
> >>    * scoped_guard (name, args...) { }:
> >>    *	similar to CLASS(name, scope)(args), except the variable (with the
> >>    *	explicit name 'scope') is declard in a for-loop such that its scope is
> >> @@ -304,6 +308,13 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
> >>   
> >>   #define __guard_ptr(_name) class_##_name##_lock_ptr
> >>   
> >> +#define __if_not_cond_guard(_name, _id, args...)	\
> >> +	CLASS(_name, _id)(args);			\
> >> +	if (!__guard_ptr(_name)(&_id))
> >> +
> >> +#define if_not_cond_guard(_name, args...) \
> >> +	__if_not_cond_guard(_name, __UNIQUE_ID(guard), args)
> >> +
> >>   #define scoped_guard(_name, args...)					\
> >>   	for (CLASS(_name, scope)(args),					\
> >>   	     *done = NULL; __guard_ptr(_name)(&scope) && !done; done = (void *)1)  
> > 
> > 
> > So if I stick this on top of:
> > 
> >    https://lkml.kernel.org/r/20241011121535.28049-1-przemyslaw.kitszel@intel.com  
> 
> I have v4 that fixes non-cond version. Apologies it took me that long.
> [v4] 
> https://lore.kernel.org/netdev/20241018113823.171256-1-przemyslaw.kitszel@intel.com
> 
> I have tested it also with the unrechable() calls removed, as suggested
> by David Lechner here:
> https://lore.kernel.org/netdev/0f4786e9-d738-435d-afb9-8c0c4a028ddb@baylibre.com
> 
> > 
> > then I can add the below:
> > 
> > --- a/include/linux/cleanup.h
> > +++ b/include/linux/cleanup.h
> > @@ -277,6 +277,8 @@ static inline class_##_name##_t class_##
> >    *	convenience macro for conditional guards that calls the statement that
> >    *	follows only if the lock was not acquired (typically an error return).
> >    *
> > + *	Only for conditional locks.
> > + *
> >    * scoped_guard (name, args...) { }:
> >    *	similar to CLASS(name, scope)(args), except the variable (with the
> >    *	explicit name 'scope') is declard in a for-loop such that its scope is
> > @@ -290,7 +292,6 @@ static inline class_##_name##_t class_##
> >    *      acquire fails.
> >    *
> >    *	Only for conditional locks.
> > - *
> >    */
> >   
> >   #define __DEFINE_CLASS_IS_CONDITIONAL(_name, _is_cond)	\
> > @@ -342,6 +343,7 @@ _label:										\
> >   		       __UNIQUE_ID(label), args)
> >   
> >   #define __if_not_guard(_name, _id, args...)		\
> > +	BUILD_BUG_ON(!__is_cond_ptr(_name));		\
> >   	CLASS(_name, _id)(args);			\
> >   	if (!__guard_ptr(_name)(&_id))
> >   
> > 
> > That make sense to people?  
> 
> despite name, looks promising!
> 
> > 
> > I've queued these two patches:
> > 
> >    git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git locking/core
> > 
> > But lacking if_not_guard() users, the robot isn't really going to give
> > me much feedback there, I suppose...  
> 
> Couldn't you just pick the other patches, that use the newly introduced
> macro?

For a test, sure, but there is a lot of ad7380 work in flight and I'd rather
not push that back a cycle for this improvement (nice though it is!)

If it looks good, an immutable branch would be great, or I could just merge
from Peter's tree if that is stable.

Similarly there is a high risk of the CXL code changing for other reasons
this cycle, but same solution would work.

Jonathan




> 
> 
> 

Re: [PATCH 1/3] cleanup: add conditional guard helper

[Dan Williams]

Peter Zijlstra wrote:
> On Tue, Oct 01, 2024 at 05:30:18PM -0500, David Lechner wrote:
> > Add a new if_not_cond_guard() macro to cleanup.h for handling
> > conditional guards such as mutext_trylock().
> > 
> > This is more ergonomic than scoped_cond_guard() for most use cases.
> > Instead of hiding the error handling statement in the macro args, it
> > works like a normal if statement and allow the error path to be indented
> > while the normal code flow path is not indented. And it avoid unwanted
> > side-effect from hidden for loop in scoped_cond_guard().
> > 
> > Signed-off-by: David Lechner <dlechner@baylibre.com>
> > ---
> >  include/linux/cleanup.h | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
[..]
> I've queued these two patches:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git locking/core
> 
> But lacking if_not_guard() users, the robot isn't really going to give
> me much feedback there, I suppose...

Looks good. If that branch is rebase-able it would be nice to add some
credit tags to "cleanup: Add conditional guard helper":

Co-developed-by: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>
Signed-off-by: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>

David and I talked about that here:

http://lore.kernel.org/f4cc471a-b602-48d8-8323-15efcd602814@baylibre.com

Also feel free to add:

Reviewed-by: Dan Williams <dan.j.williams@intel.com>

Thanks, Peter!

Re: [PATCH 1/3] cleanup: add conditional guard helper

[Peter Zijlstra]

On Fri, Oct 18, 2024 at 12:29:27PM -0700, Dan Williams wrote:
> Peter Zijlstra wrote:
> > On Tue, Oct 01, 2024 at 05:30:18PM -0500, David Lechner wrote:
> > > Add a new if_not_cond_guard() macro to cleanup.h for handling
> > > conditional guards such as mutext_trylock().
> > > 
> > > This is more ergonomic than scoped_cond_guard() for most use cases.
> > > Instead of hiding the error handling statement in the macro args, it
> > > works like a normal if statement and allow the error path to be indented
> > > while the normal code flow path is not indented. And it avoid unwanted
> > > side-effect from hidden for loop in scoped_cond_guard().
> > > 
> > > Signed-off-by: David Lechner <dlechner@baylibre.com>
> > > ---
> > >  include/linux/cleanup.h | 11 +++++++++++
> > >  1 file changed, 11 insertions(+)
> > > 
> [..]
> > I've queued these two patches:
> > 
> >   git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git locking/core
> > 
> > But lacking if_not_guard() users, the robot isn't really going to give
> > me much feedback there, I suppose...
> 
> Looks good. If that branch is rebase-able it would be nice to add some
> credit tags to "cleanup: Add conditional guard helper":
> 
> Co-developed-by: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>
> Signed-off-by: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>
> 
> David and I talked about that here:
> 
> http://lore.kernel.org/f4cc471a-b602-48d8-8323-15efcd602814@baylibre.com
> 
> Also feel free to add:
> 
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>

I rebased because I had to magic in the v4 from Przemek, and I added the
above tags to the if_not_guard() thing.

I've also pushed out a locking/test branch that includes the iio
conversion for the robots.

Once I push to tip/locking/core (people will get robot mail) the commits
should be stable and can be used in other branches if so desired.