[RFC PATCH] cleanup: make scoped_guard() to be return-friendly

[RFC PATCH] cleanup: make scoped_guard() to be return-friendly

[Przemek Kitszel]

Simply enable one to write code like:

int foo(struct my_drv *adapter)
{
	scoped_guard(spinlock, &adapter->some_spinlock)
		return adapter->spinlock_protected_var;
}

Current scoped_guard() implementation does not support that,
due to compiler complaining:
error: control reaches end of non-void function [-Werror=return-type]

One could argue that for such use case it would be better to use
guard(spinlock)(&adapter->some_spinlock), I disagree. I could also say
that coding with my proposed locking style is also very pleasant, as I'm
doing so for a few weeks already.

Technical stuff about the change:
scoped_guard() macro uses common idiom of using "for" statement to declare
a scoped variable. Unfortunately, current logic is too hard for compiler
diagnostics to be sure that there is exactly one loop step; fix that.

To make any loop so trivial that there is no above warning, it must not
depend on any variable to tell if there are more steps. There is no
obvious solution for that in C, but one could use the compound statement
expression with "goto" jumping past the "loop", effectively leaving only
the subscope part of the loop semantics.

More impl details:
one more level of macro indirection is now needed to avoid duplicating
label names;
I didn't spot any other place that is using
	if (0) past_the_loop:; else for (...; 1; ({goto past_the_loop}))
idiom, so it's not packed for reuse what makes actual macros code cleaner.

NAKed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
---
Andy believes that this change is completely wrong C, and wants me
to keep the following 4 corncers attached (I either don't agree
or they are irrelevant), but here we go:
1. wrong usage of scoped_guard().
   In the described cases the guard() needs to be used.
2. the code like:
	int foo(...)
	{
		my_macro(...)
			return X;
	}
   without return 0; (which is a known dead code) is counter intuitive
   from the C language perspective.
3. [about netdev not liking guard()]
   I do not buy "too hard" when it's too easy to get a preprocessed *.i
   file if needed for any diagnosis which makes things quite clear.
   Moreover, once done the developer will much easier understands how this
   "magic" works (there is no rocket science, but yes, the initial
   threshold probably a bit higher than just pure C).
4. Besides that (if you was following the minmax discussion in LKML) the
   macro expansion may be problematic and lead to the unbelievable huge .i
   files that compiles dozens of seconds on modern CPUs (I experienced
   myself that with AtomISP driver which drove the above mentioned minmax
   discussion).
   [Przemek - nested scoped_guard() usage expands linearly]
---
 include/linux/cleanup.h | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index d9e613803df1..6b568a8a7f9c 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -168,9 +168,16 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
 
 #define __guard_ptr(_name) class_##_name##_lock_ptr
 
-#define scoped_guard(_name, args...)					\
-	for (CLASS(_name, scope)(args),					\
-	     *done = NULL; __guard_ptr(_name)(&scope) && !done; done = (void *)1)
+#define scoped_guard(_name, args...)	\
+	__scoped_guard_labeled(__UNIQUE_ID(label), _name, args)
+
+#define __scoped_guard_labeled(_label, _name, args...)	\
+	if (0)						\
+		_label: ;				\
+	else						\
+		for (CLASS(_name, scope)(args);		\
+		     __guard_ptr(_name)(&scope), 1;	\
+		     ({ goto _label; }))
 
 #define scoped_cond_guard(_name, _fail, args...) \
 	for (CLASS(_name, scope)(args), \

base-commit: 151ac45348afc5b56baa584c7cd4876addf461ff
-- 
2.46.0

Re: [RFC PATCH] cleanup: make scoped_guard() to be return-friendly

[kernel test robot]

Hi Przemek,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build warnings:

[auto build test WARNING on 151ac45348afc5b56baa584c7cd4876addf461ff]

url:    https://github.com/intel-lab-lkp/linux/commits/Przemek-Kitszel/cleanup-make-scoped_guard-to-be-return-friendly/20240926-214521
base:   151ac45348afc5b56baa584c7cd4876addf461ff
patch link:    https://lore.kernel.org/r/20240926134347.19371-1-przemyslaw.kitszel%40intel.com
patch subject: [RFC PATCH] cleanup: make scoped_guard() to be return-friendly
config: i386-allmodconfig (https://download.01.org/0day-ci/archive/20240927/202409270859.IwkSNGqY-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240927/202409270859.IwkSNGqY-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/202409270859.IwkSNGqY-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/iio/magnetometer/af8133j.c: In function 'af8133j_set_scale':
>> drivers/iio/magnetometer/af8133j.c:315:12: warning: suggest explicit braces to avoid ambiguous 'else' [-Wdangling-else]
     315 |         if (!pm_runtime_status_suspended(dev))
         |            ^


vim +/else +315 drivers/iio/magnetometer/af8133j.c

1d8f4b04621f0f3 Icenowy Zheng 2024-02-22  294  
1d8f4b04621f0f3 Icenowy Zheng 2024-02-22  295  static int af8133j_set_scale(struct af8133j_data *data,
1d8f4b04621f0f3 Icenowy Zheng 2024-02-22  296  			     unsigned int val, unsigned int val2)
1d8f4b04621f0f3 Icenowy Zheng 2024-02-22  297  {
1d8f4b04621f0f3 Icenowy Zheng 2024-02-22  298  	struct device *dev = &data->client->dev;
1d8f4b04621f0f3 Icenowy Zheng 2024-02-22  299  	u8 range;
1d8f4b04621f0f3 Icenowy Zheng 2024-02-22  300  	int ret = 0;
1d8f4b04621f0f3 Icenowy Zheng 2024-02-22  301  
1d8f4b04621f0f3 Icenowy Zheng 2024-02-22  302  	if (af8133j_scales[0][0] == val && af8133j_scales[0][1] == val2)
1d8f4b04621f0f3 Icenowy Zheng 2024-02-22  303  		range = AF8133J_REG_RANGE_12G;
1d8f4b04621f0f3 Icenowy Zheng 2024-02-22  304  	else if (af8133j_scales[1][0] == val && af8133j_scales[1][1] == val2)
1d8f4b04621f0f3 Icenowy Zheng 2024-02-22  305  		range = AF8133J_REG_RANGE_22G;
1d8f4b04621f0f3 Icenowy Zheng 2024-02-22  306  	else
1d8f4b04621f0f3 Icenowy Zheng 2024-02-22  307  		return -EINVAL;
1d8f4b04621f0f3 Icenowy Zheng 2024-02-22  308  
1d8f4b04621f0f3 Icenowy Zheng 2024-02-22  309  	pm_runtime_disable(dev);
1d8f4b04621f0f3 Icenowy Zheng 2024-02-22  310  
1d8f4b04621f0f3 Icenowy Zheng 2024-02-22  311  	/*
1d8f4b04621f0f3 Icenowy Zheng 2024-02-22  312  	 * When suspended, just store the new range to data->range to be
1d8f4b04621f0f3 Icenowy Zheng 2024-02-22  313  	 * applied later during power up.
1d8f4b04621f0f3 Icenowy Zheng 2024-02-22  314  	 */
1d8f4b04621f0f3 Icenowy Zheng 2024-02-22 @315  	if (!pm_runtime_status_suspended(dev))
1d8f4b04621f0f3 Icenowy Zheng 2024-02-22  316  		scoped_guard(mutex, &data->mutex)
1d8f4b04621f0f3 Icenowy Zheng 2024-02-22  317  			ret = regmap_write(data->regmap,
1d8f4b04621f0f3 Icenowy Zheng 2024-02-22  318  					   AF8133J_REG_RANGE, range);
1d8f4b04621f0f3 Icenowy Zheng 2024-02-22  319  
1d8f4b04621f0f3 Icenowy Zheng 2024-02-22  320  	pm_runtime_enable(dev);
1d8f4b04621f0f3 Icenowy Zheng 2024-02-22  321  
1d8f4b04621f0f3 Icenowy Zheng 2024-02-22  322  	data->range = range;
1d8f4b04621f0f3 Icenowy Zheng 2024-02-22  323  	return ret;
1d8f4b04621f0f3 Icenowy Zheng 2024-02-22  324  }
1d8f4b04621f0f3 Icenowy Zheng 2024-02-22  325  

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

Re: [RFC PATCH] cleanup: make scoped_guard() to be return-friendly

[kernel test robot]

Hi Przemek,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build warnings:

[auto build test WARNING on 151ac45348afc5b56baa584c7cd4876addf461ff]

url:    https://github.com/intel-lab-lkp/linux/commits/Przemek-Kitszel/cleanup-make-scoped_guard-to-be-return-friendly/20240926-214521
base:   151ac45348afc5b56baa584c7cd4876addf461ff
patch link:    https://lore.kernel.org/r/20240926134347.19371-1-przemyslaw.kitszel%40intel.com
patch subject: [RFC PATCH] cleanup: make scoped_guard() to be return-friendly
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20240927/202409270848.tTpyEAR7-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/20240927/202409270848.tTpyEAR7-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/202409270848.tTpyEAR7-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/iio/magnetometer/af8133j.c:316:3: warning: add explicit braces to avoid dangling else [-Wdangling-else]
     316 |                 scoped_guard(mutex, &data->mutex)
         |                 ^
   include/linux/cleanup.h:172:2: note: expanded from macro 'scoped_guard'
     172 |         __scoped_guard_labeled(__UNIQUE_ID(label), _name, args)
         |         ^
   include/linux/cleanup.h:177:2: note: expanded from macro '__scoped_guard_labeled'
     177 |         else                                            \
         |         ^
   1 warning generated.


vim +316 drivers/iio/magnetometer/af8133j.c

1d8f4b04621f0f Icenowy Zheng 2024-02-22  294  
1d8f4b04621f0f Icenowy Zheng 2024-02-22  295  static int af8133j_set_scale(struct af8133j_data *data,
1d8f4b04621f0f Icenowy Zheng 2024-02-22  296  			     unsigned int val, unsigned int val2)
1d8f4b04621f0f Icenowy Zheng 2024-02-22  297  {
1d8f4b04621f0f Icenowy Zheng 2024-02-22  298  	struct device *dev = &data->client->dev;
1d8f4b04621f0f Icenowy Zheng 2024-02-22  299  	u8 range;
1d8f4b04621f0f Icenowy Zheng 2024-02-22  300  	int ret = 0;
1d8f4b04621f0f Icenowy Zheng 2024-02-22  301  
1d8f4b04621f0f Icenowy Zheng 2024-02-22  302  	if (af8133j_scales[0][0] == val && af8133j_scales[0][1] == val2)
1d8f4b04621f0f Icenowy Zheng 2024-02-22  303  		range = AF8133J_REG_RANGE_12G;
1d8f4b04621f0f Icenowy Zheng 2024-02-22  304  	else if (af8133j_scales[1][0] == val && af8133j_scales[1][1] == val2)
1d8f4b04621f0f Icenowy Zheng 2024-02-22  305  		range = AF8133J_REG_RANGE_22G;
1d8f4b04621f0f Icenowy Zheng 2024-02-22  306  	else
1d8f4b04621f0f Icenowy Zheng 2024-02-22  307  		return -EINVAL;
1d8f4b04621f0f Icenowy Zheng 2024-02-22  308  
1d8f4b04621f0f Icenowy Zheng 2024-02-22  309  	pm_runtime_disable(dev);
1d8f4b04621f0f Icenowy Zheng 2024-02-22  310  
1d8f4b04621f0f Icenowy Zheng 2024-02-22  311  	/*
1d8f4b04621f0f Icenowy Zheng 2024-02-22  312  	 * When suspended, just store the new range to data->range to be
1d8f4b04621f0f Icenowy Zheng 2024-02-22  313  	 * applied later during power up.
1d8f4b04621f0f Icenowy Zheng 2024-02-22  314  	 */
1d8f4b04621f0f Icenowy Zheng 2024-02-22  315  	if (!pm_runtime_status_suspended(dev))
1d8f4b04621f0f Icenowy Zheng 2024-02-22 @316  		scoped_guard(mutex, &data->mutex)
1d8f4b04621f0f Icenowy Zheng 2024-02-22  317  			ret = regmap_write(data->regmap,
1d8f4b04621f0f Icenowy Zheng 2024-02-22  318  					   AF8133J_REG_RANGE, range);
1d8f4b04621f0f Icenowy Zheng 2024-02-22  319  
1d8f4b04621f0f Icenowy Zheng 2024-02-22  320  	pm_runtime_enable(dev);
1d8f4b04621f0f Icenowy Zheng 2024-02-22  321  
1d8f4b04621f0f Icenowy Zheng 2024-02-22  322  	data->range = range;
1d8f4b04621f0f Icenowy Zheng 2024-02-22  323  	return ret;
1d8f4b04621f0f Icenowy Zheng 2024-02-22  324  }
1d8f4b04621f0f Icenowy Zheng 2024-02-22  325  

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

Re: [RFC PATCH] cleanup: make scoped_guard() to be return-friendly

[kernel test robot]

BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20240926134347.19371-1-przemyslaw.kitszel@intel.com>
References: <20240926134347.19371-1-przemyslaw.kitszel@intel.com>
TO: Przemek Kitszel <przemyslaw.kitszel@intel.com>

Hi Przemek,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build warnings:

[auto build test WARNING on 151ac45348afc5b56baa584c7cd4876addf461ff]

url:    https://github.com/intel-lab-lkp/linux/commits/Przemek-Kitszel/cleanup-make-scoped_guard-to-be-return-friendly/20240926-214521
base:   151ac45348afc5b56baa584c7cd4876addf461ff
patch link:    https://lore.kernel.org/r/20240926134347.19371-1-przemyslaw.kitszel%40intel.com
patch subject: [RFC PATCH] cleanup: make scoped_guard() to be return-friendly
:::::: branch date: 16 hours ago
:::::: commit date: 16 hours ago
config: i386-randconfig-141-20240927 (https://download.01.org/0day-ci/archive/20240927/202409271312.hshEAZox-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0

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>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Closes: https://lore.kernel.org/r/202409271312.hshEAZox-lkp@intel.com/

smatch warnings:
drivers/iio/imu/adis16480.c:1341 adis16480_trigger_handler() warn: iterator 'i' not incremented

vim +/i +1341 drivers/iio/imu/adis16480.c

941f130881fa90 Nuno Sa           2021-04-22  1333  
941f130881fa90 Nuno Sa           2021-04-22  1334  static irqreturn_t adis16480_trigger_handler(int irq, void *p)
941f130881fa90 Nuno Sa           2021-04-22  1335  {
941f130881fa90 Nuno Sa           2021-04-22  1336  	struct iio_poll_func *pf = p;
941f130881fa90 Nuno Sa           2021-04-22  1337  	struct iio_dev *indio_dev = pf->indio_dev;
941f130881fa90 Nuno Sa           2021-04-22  1338  	struct adis16480 *st = iio_priv(indio_dev);
941f130881fa90 Nuno Sa           2021-04-22  1339  	struct adis *adis = &st->adis;
79f4dc9dec0e0b Andy Shevchenko   2022-04-14  1340  	struct device *dev = &adis->spi->dev;
85b2aeaa2f4cb4 Ramona Gradinariu 2024-05-28 @1341  	int ret, bit, offset, i = 0, buff_offset = 0;
941f130881fa90 Nuno Sa           2021-04-22  1342  	__be16 *buffer;
941f130881fa90 Nuno Sa           2021-04-22  1343  	u32 crc;
941f130881fa90 Nuno Sa           2021-04-22  1344  	bool valid;
941f130881fa90 Nuno Sa           2021-04-22  1345  
d6a60d76173d47 Nuno Sa           2024-06-18  1346  	adis_dev_auto_scoped_lock(adis) {
941f130881fa90 Nuno Sa           2021-04-22  1347  		if (adis->current_page != 0) {
941f130881fa90 Nuno Sa           2021-04-22  1348  			adis->tx[0] = ADIS_WRITE_REG(ADIS_REG_PAGE_ID);
941f130881fa90 Nuno Sa           2021-04-22  1349  			adis->tx[1] = 0;
941f130881fa90 Nuno Sa           2021-04-22  1350  			ret = spi_write(adis->spi, adis->tx, 2);
941f130881fa90 Nuno Sa           2021-04-22  1351  			if (ret) {
79f4dc9dec0e0b Andy Shevchenko   2022-04-14  1352  				dev_err(dev, "Failed to change device page: %d\n", ret);
941f130881fa90 Nuno Sa           2021-04-22  1353  				goto irq_done;
941f130881fa90 Nuno Sa           2021-04-22  1354  			}
941f130881fa90 Nuno Sa           2021-04-22  1355  
941f130881fa90 Nuno Sa           2021-04-22  1356  			adis->current_page = 0;
941f130881fa90 Nuno Sa           2021-04-22  1357  		}
941f130881fa90 Nuno Sa           2021-04-22  1358  
941f130881fa90 Nuno Sa           2021-04-22  1359  		ret = spi_sync(adis->spi, &adis->msg);
941f130881fa90 Nuno Sa           2021-04-22  1360  		if (ret) {
79f4dc9dec0e0b Andy Shevchenko   2022-04-14  1361  			dev_err(dev, "Failed to read data: %d\n", ret);
941f130881fa90 Nuno Sa           2021-04-22  1362  			goto irq_done;
941f130881fa90 Nuno Sa           2021-04-22  1363  		}
d6a60d76173d47 Nuno Sa           2024-06-18  1364  	}
941f130881fa90 Nuno Sa           2021-04-22  1365  
941f130881fa90 Nuno Sa           2021-04-22  1366  	/*
941f130881fa90 Nuno Sa           2021-04-22  1367  	 * After making the burst request, the response can have one or two
941f130881fa90 Nuno Sa           2021-04-22  1368  	 * 16-bit responses containing the BURST_ID depending on the sclk. If
941f130881fa90 Nuno Sa           2021-04-22  1369  	 * clk > 3.6MHz, then we will have two BURST_ID in a row. If clk < 3MHZ,
941f130881fa90 Nuno Sa           2021-04-22  1370  	 * we have only one. To manage that variation, we use the transition from the
85b2aeaa2f4cb4 Ramona Gradinariu 2024-05-28  1371  	 * BURST_ID to the SYS_E_FLAG register, which will not be equal to 0xA5A5/0xC3C3.
85b2aeaa2f4cb4 Ramona Gradinariu 2024-05-28  1372  	 * If we not find this variation in the first 4 segments, then the data should
941f130881fa90 Nuno Sa           2021-04-22  1373  	 * not be valid.
941f130881fa90 Nuno Sa           2021-04-22  1374  	 */
941f130881fa90 Nuno Sa           2021-04-22  1375  	buffer = adis->buffer;
941f130881fa90 Nuno Sa           2021-04-22  1376  	for (offset = 0; offset < 4; offset++) {
941f130881fa90 Nuno Sa           2021-04-22  1377  		u16 curr = be16_to_cpu(buffer[offset]);
941f130881fa90 Nuno Sa           2021-04-22  1378  		u16 next = be16_to_cpu(buffer[offset + 1]);
941f130881fa90 Nuno Sa           2021-04-22  1379  
85b2aeaa2f4cb4 Ramona Gradinariu 2024-05-28  1380  		if (curr == st->burst_id && next != st->burst_id) {
941f130881fa90 Nuno Sa           2021-04-22  1381  			offset++;
941f130881fa90 Nuno Sa           2021-04-22  1382  			break;
941f130881fa90 Nuno Sa           2021-04-22  1383  		}
941f130881fa90 Nuno Sa           2021-04-22  1384  	}
941f130881fa90 Nuno Sa           2021-04-22  1385  
941f130881fa90 Nuno Sa           2021-04-22  1386  	if (offset == 4) {
79f4dc9dec0e0b Andy Shevchenko   2022-04-14  1387  		dev_err(dev, "Invalid burst data\n");
941f130881fa90 Nuno Sa           2021-04-22  1388  		goto irq_done;
941f130881fa90 Nuno Sa           2021-04-22  1389  	}
941f130881fa90 Nuno Sa           2021-04-22  1390  
941f130881fa90 Nuno Sa           2021-04-22  1391  	crc = be16_to_cpu(buffer[offset + 16]) << 16 | be16_to_cpu(buffer[offset + 15]);
941f130881fa90 Nuno Sa           2021-04-22  1392  	valid = adis16480_validate_crc((u16 *)&buffer[offset], 15, crc);
941f130881fa90 Nuno Sa           2021-04-22  1393  	if (!valid) {
79f4dc9dec0e0b Andy Shevchenko   2022-04-14  1394  		dev_err(dev, "Invalid crc\n");
941f130881fa90 Nuno Sa           2021-04-22  1395  		goto irq_done;
941f130881fa90 Nuno Sa           2021-04-22  1396  	}
941f130881fa90 Nuno Sa           2021-04-22  1397  
941f130881fa90 Nuno Sa           2021-04-22  1398  	for_each_set_bit(bit, indio_dev->active_scan_mask, indio_dev->masklength) {
941f130881fa90 Nuno Sa           2021-04-22  1399  		/*
941f130881fa90 Nuno Sa           2021-04-22  1400  		 * When burst mode is used, temperature is the first data
941f130881fa90 Nuno Sa           2021-04-22  1401  		 * channel in the sequence, but the temperature scan index
941f130881fa90 Nuno Sa           2021-04-22  1402  		 * is 10.
941f130881fa90 Nuno Sa           2021-04-22  1403  		 */
941f130881fa90 Nuno Sa           2021-04-22  1404  		switch (bit) {
941f130881fa90 Nuno Sa           2021-04-22  1405  		case ADIS16480_SCAN_TEMP:
941f130881fa90 Nuno Sa           2021-04-22  1406  			st->data[i++] = buffer[offset + 1];
85b2aeaa2f4cb4 Ramona Gradinariu 2024-05-28  1407  			/*
85b2aeaa2f4cb4 Ramona Gradinariu 2024-05-28  1408  			 * The temperature channel has 16-bit storage size.
85b2aeaa2f4cb4 Ramona Gradinariu 2024-05-28  1409  			 * We need to perform the padding to have the buffer
85b2aeaa2f4cb4 Ramona Gradinariu 2024-05-28  1410  			 * elements naturally aligned in case there are any
85b2aeaa2f4cb4 Ramona Gradinariu 2024-05-28  1411  			 * 32-bit storage size channels enabled which are added
85b2aeaa2f4cb4 Ramona Gradinariu 2024-05-28  1412  			 * in the buffer after the temprature data. In case
85b2aeaa2f4cb4 Ramona Gradinariu 2024-05-28  1413  			 * there is no data being added after the temperature
85b2aeaa2f4cb4 Ramona Gradinariu 2024-05-28  1414  			 * data, the padding is harmless.
85b2aeaa2f4cb4 Ramona Gradinariu 2024-05-28  1415  			 */
85b2aeaa2f4cb4 Ramona Gradinariu 2024-05-28  1416  			st->data[i++] = 0;
941f130881fa90 Nuno Sa           2021-04-22  1417  			break;
85b2aeaa2f4cb4 Ramona Gradinariu 2024-05-28  1418  		case ADIS16480_SCAN_DELTANG_X ... ADIS16480_SCAN_DELTVEL_Z:
85b2aeaa2f4cb4 Ramona Gradinariu 2024-05-28  1419  			buff_offset = ADIS16480_SCAN_DELTANG_X;
85b2aeaa2f4cb4 Ramona Gradinariu 2024-05-28  1420  			fallthrough;
941f130881fa90 Nuno Sa           2021-04-22  1421  		case ADIS16480_SCAN_GYRO_X ... ADIS16480_SCAN_ACCEL_Z:
941f130881fa90 Nuno Sa           2021-04-22  1422  			/* The lower register data is sequenced first */
85b2aeaa2f4cb4 Ramona Gradinariu 2024-05-28  1423  			st->data[i++] = buffer[2 * (bit - buff_offset) + offset + 3];
85b2aeaa2f4cb4 Ramona Gradinariu 2024-05-28  1424  			st->data[i++] = buffer[2 * (bit - buff_offset) + offset + 2];
941f130881fa90 Nuno Sa           2021-04-22  1425  			break;
941f130881fa90 Nuno Sa           2021-04-22  1426  		}
941f130881fa90 Nuno Sa           2021-04-22  1427  	}
941f130881fa90 Nuno Sa           2021-04-22  1428  
941f130881fa90 Nuno Sa           2021-04-22  1429  	iio_push_to_buffers_with_timestamp(indio_dev, st->data, pf->timestamp);
941f130881fa90 Nuno Sa           2021-04-22  1430  irq_done:
941f130881fa90 Nuno Sa           2021-04-22  1431  	iio_trigger_notify_done(indio_dev->trig);
941f130881fa90 Nuno Sa           2021-04-22  1432  
941f130881fa90 Nuno Sa           2021-04-22  1433  	return IRQ_HANDLED;
941f130881fa90 Nuno Sa           2021-04-22  1434  }
941f130881fa90 Nuno Sa           2021-04-22  1435  

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

Re: [RFC PATCH] cleanup: make scoped_guard() to be return-friendly

[Dan Carpenter]

On Thu, Sep 26, 2024 at 03:41:38PM +0200, Przemek Kitszel wrote:
> diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
> index d9e613803df1..6b568a8a7f9c 100644
> --- a/include/linux/cleanup.h
> +++ b/include/linux/cleanup.h
> @@ -168,9 +168,16 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
>  
>  #define __guard_ptr(_name) class_##_name##_lock_ptr
>  
> -#define scoped_guard(_name, args...)					\
> -	for (CLASS(_name, scope)(args),					\
> -	     *done = NULL; __guard_ptr(_name)(&scope) && !done; done = (void *)1)
> +#define scoped_guard(_name, args...)	\
> +	__scoped_guard_labeled(__UNIQUE_ID(label), _name, args)
> +
> +#define __scoped_guard_labeled(_label, _name, args...)	\
> +	if (0)						\
> +		_label: ;				\
> +	else						\
> +		for (CLASS(_name, scope)(args);		\
> +		     __guard_ptr(_name)(&scope), 1;	\
                                               ^^^
> +		     ({ goto _label; }))
>  

Remove the ", 1".  The point of the __guard_ptr() condition is for try_locks
but the ", 1" means they always succeed.  The only try lock I can find in
the current tree is tsc200x_esd_work().

regards,
dan carpenter

Re: [RFC PATCH] cleanup: make scoped_guard() to be return-friendly

[kernel test robot]

BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20240926134347.19371-1-przemyslaw.kitszel@intel.com>
References: <20240926134347.19371-1-przemyslaw.kitszel@intel.com>
TO: Przemek Kitszel <przemyslaw.kitszel@intel.com>

Hi Przemek,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build warnings:

[auto build test WARNING on 151ac45348afc5b56baa584c7cd4876addf461ff]

url:    https://github.com/intel-lab-lkp/linux/commits/Przemek-Kitszel/cleanup-make-scoped_guard-to-be-return-friendly/20240926-214521
base:   151ac45348afc5b56baa584c7cd4876addf461ff
patch link:    https://lore.kernel.org/r/20240926134347.19371-1-przemyslaw.kitszel%40intel.com
patch subject: [RFC PATCH] cleanup: make scoped_guard() to be return-friendly
:::::: branch date: 20 hours ago
:::::: commit date: 20 hours ago
config: x86_64-randconfig-161-20240927 (https://download.01.org/0day-ci/archive/20240927/202409271754.FsBPrJQM-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0

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>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Closes: https://lore.kernel.org/r/202409271754.FsBPrJQM-lkp@intel.com/

smatch warnings:
drivers/gpio/gpio-graniterapids.c:260 gnr_gpio_irq() warn: iterator 'i' not incremented

vim +/i +260 drivers/gpio/gpio-graniterapids.c

ecc4b1418e2399 Aapo Vienamo 2024-04-23  254  
ecc4b1418e2399 Aapo Vienamo 2024-04-23  255  static irqreturn_t gnr_gpio_irq(int irq, void *data)
ecc4b1418e2399 Aapo Vienamo 2024-04-23  256  {
ecc4b1418e2399 Aapo Vienamo 2024-04-23  257  	struct gnr_gpio *priv = data;
ecc4b1418e2399 Aapo Vienamo 2024-04-23  258  	unsigned int handled = 0;
ecc4b1418e2399 Aapo Vienamo 2024-04-23  259  
ecc4b1418e2399 Aapo Vienamo 2024-04-23 @260  	for (unsigned int i = 0; i < GNR_NUM_REGS; i++) {
ecc4b1418e2399 Aapo Vienamo 2024-04-23  261  		const void __iomem *reg = priv->reg_base + i * sizeof(u32);
ecc4b1418e2399 Aapo Vienamo 2024-04-23  262  		unsigned long pending;
ecc4b1418e2399 Aapo Vienamo 2024-04-23  263  		unsigned long enabled;
ecc4b1418e2399 Aapo Vienamo 2024-04-23  264  		unsigned int bit_idx;
ecc4b1418e2399 Aapo Vienamo 2024-04-23  265  
ecc4b1418e2399 Aapo Vienamo 2024-04-23  266  		scoped_guard(raw_spinlock, &priv->lock) {
ecc4b1418e2399 Aapo Vienamo 2024-04-23  267  			pending = readl(reg + GNR_GPI_STATUS_OFFSET);
ecc4b1418e2399 Aapo Vienamo 2024-04-23  268  			enabled = readl(reg + GNR_GPI_ENABLE_OFFSET);
ecc4b1418e2399 Aapo Vienamo 2024-04-23  269  		}
ecc4b1418e2399 Aapo Vienamo 2024-04-23  270  
ecc4b1418e2399 Aapo Vienamo 2024-04-23  271  		/* Only enabled interrupts */
ecc4b1418e2399 Aapo Vienamo 2024-04-23  272  		pending &= enabled;
ecc4b1418e2399 Aapo Vienamo 2024-04-23  273  
ecc4b1418e2399 Aapo Vienamo 2024-04-23  274  		for_each_set_bit(bit_idx, &pending, GNR_PINS_PER_REG) {
ecc4b1418e2399 Aapo Vienamo 2024-04-23  275  			unsigned int hwirq = i * GNR_PINS_PER_REG + bit_idx;
ecc4b1418e2399 Aapo Vienamo 2024-04-23  276  
ecc4b1418e2399 Aapo Vienamo 2024-04-23  277  			generic_handle_domain_irq(priv->gc.irq.domain, hwirq);
ecc4b1418e2399 Aapo Vienamo 2024-04-23  278  		}
ecc4b1418e2399 Aapo Vienamo 2024-04-23  279  
ecc4b1418e2399 Aapo Vienamo 2024-04-23  280  		handled += pending ? 1 : 0;
ecc4b1418e2399 Aapo Vienamo 2024-04-23  281  
ecc4b1418e2399 Aapo Vienamo 2024-04-23  282  	}
ecc4b1418e2399 Aapo Vienamo 2024-04-23  283  	return IRQ_RETVAL(handled);
ecc4b1418e2399 Aapo Vienamo 2024-04-23  284  }
ecc4b1418e2399 Aapo Vienamo 2024-04-23  285  

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

Re: [RFC PATCH] cleanup: make scoped_guard() to be return-friendly

[Przemek Kitszel]

On 9/27/24 09:31, Dan Carpenter wrote:
> On Thu, Sep 26, 2024 at 03:41:38PM +0200, Przemek Kitszel wrote:
>> diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
>> index d9e613803df1..6b568a8a7f9c 100644
>> --- a/include/linux/cleanup.h
>> +++ b/include/linux/cleanup.h
>> @@ -168,9 +168,16 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
>>   
>>   #define __guard_ptr(_name) class_##_name##_lock_ptr
>>   
>> -#define scoped_guard(_name, args...)					\
>> -	for (CLASS(_name, scope)(args),					\
>> -	     *done = NULL; __guard_ptr(_name)(&scope) && !done; done = (void *)1)
>> +#define scoped_guard(_name, args...)	\
>> +	__scoped_guard_labeled(__UNIQUE_ID(label), _name, args)
>> +
>> +#define __scoped_guard_labeled(_label, _name, args...)	\
>> +	if (0)						\
>> +		_label: ;				\
>> +	else						\
>> +		for (CLASS(_name, scope)(args);		\
>> +		     __guard_ptr(_name)(&scope), 1;	\
>                                                 ^^^
>> +		     ({ goto _label; }))
>>   
> 
> Remove the ", 1".  The point of the __guard_ptr() condition is for try_locks
> but the ", 1" means they always succeed.  The only try lock I can find in

You are right that the __guard_ptr() is conditional for the benefit of
try_locks. But here we have unconditional lock. And removing ", 1" part
makes compiler complaining with the very same message:
error: control reaches end of non-void function [-Werror=return-type]

so ", 1" part is on purpose and must stay there to aid compiler.

> the current tree is tsc200x_esd_work().
> 
> regards,
> dan carpenter
> 
> 

Re: [RFC PATCH] cleanup: make scoped_guard() to be return-friendly

[Dan Carpenter]

On Fri, Sep 27, 2024 at 04:08:30PM +0200, Przemek Kitszel wrote:
> On 9/27/24 09:31, Dan Carpenter wrote:
> > On Thu, Sep 26, 2024 at 03:41:38PM +0200, Przemek Kitszel wrote:
> > > diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
> > > index d9e613803df1..6b568a8a7f9c 100644
> > > --- a/include/linux/cleanup.h
> > > +++ b/include/linux/cleanup.h
> > > @@ -168,9 +168,16 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
> > >   #define __guard_ptr(_name) class_##_name##_lock_ptr
> > > -#define scoped_guard(_name, args...)					\
> > > -	for (CLASS(_name, scope)(args),					\
> > > -	     *done = NULL; __guard_ptr(_name)(&scope) && !done; done = (void *)1)
> > > +#define scoped_guard(_name, args...)	\
> > > +	__scoped_guard_labeled(__UNIQUE_ID(label), _name, args)
> > > +
> > > +#define __scoped_guard_labeled(_label, _name, args...)	\
> > > +	if (0)						\
> > > +		_label: ;				\
> > > +	else						\
> > > +		for (CLASS(_name, scope)(args);		\
> > > +		     __guard_ptr(_name)(&scope), 1;	\
> >                                                 ^^^
> > > +		     ({ goto _label; }))
> > 
> > Remove the ", 1".  The point of the __guard_ptr() condition is for try_locks
> > but the ", 1" means they always succeed.  The only try lock I can find in
> 
> You are right that the __guard_ptr() is conditional for the benefit of
> try_locks. But here we have unconditional lock. And removing ", 1" part
> makes compiler complaining with the very same message:
> error: control reaches end of non-void function [-Werror=return-type]
> 
> so ", 1" part is on purpose and must stay there to aid compiler.
> 
> > the current tree is tsc200x_esd_work().

Obviously, we can't break stuff and also checking __guard_ptr(_name)(&scope) is
pointless if we're going to ignore the return value.

But, sure, I get that we want to the compiler to know that regular spin_lock()
is going to succeed and spin_trylock() might not.  As a static checker
developer, I want that as well.  Currently, whenever someone creates a new class
of locks, I have to add a couple lines to Smatch to add this information.  It's
not a huge deal, but it would be nice to avoid this.

I did a `git grep scoped_guard | grep try` and I think tsc200x_esd_work() is the
only place which actually uses try locks with scoped_guard().  If it's just the
one, then why don't we create a scoped_guard_trylock() macro?

regards,
dan carpenter

Re: [RFC PATCH] cleanup: make scoped_guard() to be return-friendly

[Przemek Kitszel]

On 9/27/24 17:04, Dan Carpenter wrote:
> On Fri, Sep 27, 2024 at 04:08:30PM +0200, Przemek Kitszel wrote:
>> On 9/27/24 09:31, Dan Carpenter wrote:
>>> On Thu, Sep 26, 2024 at 03:41:38PM +0200, Przemek Kitszel wrote:
>>>> diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
>>>> index d9e613803df1..6b568a8a7f9c 100644
>>>> --- a/include/linux/cleanup.h
>>>> +++ b/include/linux/cleanup.h
>>>> @@ -168,9 +168,16 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
>>>>    #define __guard_ptr(_name) class_##_name##_lock_ptr
>>>> -#define scoped_guard(_name, args...)					\
>>>> -	for (CLASS(_name, scope)(args),					\
>>>> -	     *done = NULL; __guard_ptr(_name)(&scope) && !done; done = (void *)1)
>>>> +#define scoped_guard(_name, args...)	\
>>>> +	__scoped_guard_labeled(__UNIQUE_ID(label), _name, args)
>>>> +
>>>> +#define __scoped_guard_labeled(_label, _name, args...)	\
>>>> +	if (0)						\
>>>> +		_label: ;				\
>>>> +	else						\
>>>> +		for (CLASS(_name, scope)(args);		\
>>>> +		     __guard_ptr(_name)(&scope), 1;	\
>>>                                                  ^^^
>>>> +		     ({ goto _label; }))
>>>
>>> Remove the ", 1".  The point of the __guard_ptr() condition is for try_locks
>>> but the ", 1" means they always succeed.  The only try lock I can find in
>>
>> You are right that the __guard_ptr() is conditional for the benefit of
>> try_locks. But here we have unconditional lock. And removing ", 1" part
>> makes compiler complaining with the very same message:
>> error: control reaches end of non-void function [-Werror=return-type]
>>
>> so ", 1" part is on purpose and must stay there to aid compiler.
>>
>>> the current tree is tsc200x_esd_work().
> 
> Obviously, we can't break stuff and also checking __guard_ptr(_name)(&scope) is
> pointless if we're going to ignore the return value.
> 
> But, sure, I get that we want to the compiler to know that regular spin_lock()
> is going to succeed and spin_trylock() might not.  As a static checker
> developer, I want that as well.  Currently, whenever someone creates a new class
> of locks, I have to add a couple lines to Smatch to add this information.  It's
> not a huge deal, but it would be nice to avoid this.
> 
> I did a `git grep scoped_guard | grep try` and I think tsc200x_esd_work() is the
> only place which actually uses try locks with scoped_guard().  If it's just the
> one, then why don't we create a scoped_guard_trylock() macro?

Most of the time it is just easier to bend your driver than change or
extend the core of the kernel.

There is actually scoped_cond_guard() which is a trylock variant.

scoped_guard(mutex_try, &ts->mutex) you have found is semantically
wrong and must be fixed.

---
I have received also a bot message about "if (x) scoped_guard(y, z)"
usage (without braces), so will need to adjust it too.

Re: [RFC PATCH] cleanup: make scoped_guard() to be return-friendly

[Dan Carpenter]

On Mon, Sep 30, 2024 at 12:21:44PM +0200, Przemek Kitszel wrote:
> 
> Most of the time it is just easier to bend your driver than change or
> extend the core of the kernel.
> 
> There is actually scoped_cond_guard() which is a trylock variant.
> 
> scoped_guard(mutex_try, &ts->mutex) you have found is semantically
> wrong and must be fixed.

What?  I'm so puzzled by this conversation.

Anyway, I don't have a problem with your goal, but your macro is wrong and will
need to be re-written.  You will need to update any drivers which use the
scoped_guard() for try locks.  I don't care how you do that.  Use
scoped_cond_guard() if you want or invent a new macro.  But that work always
falls on the person changing the API.  Plus, it's only the one tsc200x-core.c
driver so I don't understand why you're making a big deal about it.

regards,
dan carpenter

Re: [RFC PATCH] cleanup: make scoped_guard() to be return-friendly

[Markus Elfring]

…
> Current scoped_guard() implementation does not support that,
> due to compiler complaining:
…
> +++ b/include/linux/cleanup.h
> @@ -168,9 +168,16 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
>
>  #define __guard_ptr(_name) class_##_name##_lock_ptr
>
> -#define scoped_guard(_name, args...)					\
> -	for (CLASS(_name, scope)(args),					\
> -	     *done = NULL; __guard_ptr(_name)(&scope) && !done; done = (void *)1)
> +#define scoped_guard(_name, args...)	\
> +	__scoped_guard_labeled(__UNIQUE_ID(label), _name, args)
> +
> +#define __scoped_guard_labeled(_label, _name, args...)	\
> +	if (0)						\
> +		_label: ;				\
> +	else						\
> +		for (CLASS(_name, scope)(args);		\
> +		     __guard_ptr(_name)(&scope), 1;	\
> +		     ({ goto _label; }))
>
>  #define scoped_cond_guard(_name, _fail, args...) \
>  	for (CLASS(_name, scope)(args), \

* How do you think about to define such macros before their use?

* Would you ever like to avoid reserved identifiers in such source code?
  https://wiki.sei.cmu.edu/confluence/display/c/DCL37-C.+Do+not+declare+or+define+a+reserved+identifier


Regards,
Markus

Re: [RFC PATCH] cleanup: make scoped_guard() to be return-friendly

[Przemek Kitszel]

On 9/30/24 13:08, Dan Carpenter wrote:
> On Mon, Sep 30, 2024 at 12:21:44PM +0200, Przemek Kitszel wrote:
>>
>> Most of the time it is just easier to bend your driver than change or
>> extend the core of the kernel.
>>
>> There is actually scoped_cond_guard() which is a trylock variant.
>>
>> scoped_guard(mutex_try, &ts->mutex) you have found is semantically
>> wrong and must be fixed.
> 
> What?  I'm so puzzled by this conversation.

there are two variants of scoped_guard() and you have found a place
where the wrong one is used

> 
> Anyway, I don't have a problem with your goal, but your macro is wrong and will
> need to be re-written.  You will need to update any drivers which use the
> scoped_guard() for try locks.  I don't care how you do that.  Use
> scoped_cond_guard() if you want or invent a new macro.  But that work always
> falls on the person changing the API.  Plus, it's only the one tsc200x-core.c
> driver so I don't understand why you're making a big deal about it.

apologies for upsetting you
I will send next iteration of this series with additional patches fixing
current code (thanks you for finding it for me in this case!)

I didn't said so in prev mail to leave you an option to send the fix for
the usage bug you have reported, just confirmed it. But by all means I'm
happy to fix current code myself.

> but your macro is wrong and will need to be re-written

could you please elaborate here?

Re: [RFC PATCH] cleanup: make scoped_guard() to be return-friendly

[Przemek Kitszel]

On 9/30/24 13:30, Markus Elfring wrote:
> …
>> Current scoped_guard() implementation does not support that,
>> due to compiler complaining:
> …
>> +++ b/include/linux/cleanup.h
>> @@ -168,9 +168,16 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
>>
>>   #define __guard_ptr(_name) class_##_name##_lock_ptr
>>
>> -#define scoped_guard(_name, args...)					\
>> -	for (CLASS(_name, scope)(args),					\
>> -	     *done = NULL; __guard_ptr(_name)(&scope) && !done; done = (void *)1)
>> +#define scoped_guard(_name, args...)	\
>> +	__scoped_guard_labeled(__UNIQUE_ID(label), _name, args)
>> +
>> +#define __scoped_guard_labeled(_label, _name, args...)	\
>> +	if (0)						\
>> +		_label: ;				\
>> +	else						\
>> +		for (CLASS(_name, scope)(args);		\
>> +		     __guard_ptr(_name)(&scope), 1;	\
>> +		     ({ goto _label; }))
>>
>>   #define scoped_cond_guard(_name, _fail, args...) \
>>   	for (CLASS(_name, scope)(args), \
> 
> * How do you think about to define such macros before their use?

will do, no problem

> 
> * Would you ever like to avoid reserved identifiers in such source code?
>    https://wiki.sei.cmu.edu/confluence/display/c/DCL37-C.+Do+not+declare+or+define+a+reserved+identifier

we already don't care about this guideline (see __guard_ptr())

OTOH there is DCL37-C-EX3 that says "does not apply for std lib",
and here (in the kernel) we don' need stdlib, (or for the purpose of
bureaucracy we ARE the stdlib for ourselves)

> 
> 
> Regards,
> Markus

Re: [RFC] cleanup: make scoped_guard() to be return-friendly

[Markus Elfring]

>> * Would you ever like to avoid reserved identifiers in such source code?
>>   https://wiki.sei.cmu.edu/confluence/display/c/DCL37-C.+Do+not+declare+or+define+a+reserved+identifier
>
> we already don't care about this guideline (see __guard_ptr())

Please take another look at name conventions.
How do you think about to avoid that this software depends on undefined behaviour?

Regards,
Markus

Re: [RFC PATCH] cleanup: make scoped_guard() to be return-friendly

[Dan Carpenter]

On Mon, Sep 30, 2024 at 01:30:58PM +0200, Przemek Kitszel wrote:
> > but your macro is wrong and will need to be re-written
> 
> could you please elaborate here?

- 		__guard_ptr(_name)(&scope) && !done;
+		__guard_ptr(_name)(&scope), 1;     \

The __guard_ptr(_name)(&scope) check is checking whether lock function
succeeded.  With the new macro we only use scoped_guard() for locks which can't
fail.  We can (basically must) remove the __guard_ptr(_name)(&scope) check since
we're ignoring the result.

There are only four drivers which rely on conditional scoped_guard() locks.

$ git grep scoped_guard | egrep '(try|_intr)'
drivers/input/keyboard/atkbd.c: scoped_guard(mutex_intr, &atkbd->mutex) {
drivers/input/touchscreen/tsc200x-core.c:       scoped_guard(mutex_try, &ts->mutex) {
drivers/input/touchscreen/wacom_w8001.c:        scoped_guard(mutex_intr, &w8001->mutex) {
drivers/platform/x86/ideapad-laptop.c:  scoped_guard(mutex_intr, &dytc->mutex) {

This change breaks the drivers at runtime, but you need to ensure that drivers
using the old API will break at compile time so that people don't introduce new
bugs during the transition.  In other words, you will need to update the
DEFINE_GUARD_COND() stuff as well.

$ git grep DEFINE_GUARD_COND
include/linux/cleanup.h: * DEFINE_GUARD_COND(name, ext, condlock)
include/linux/cleanup.h:#define DEFINE_GUARD_COND(_name, _ext, _condlock) \
include/linux/iio/iio.h:DEFINE_GUARD_COND(iio_claim_direct, _try, ({
include/linux/mutex.h:DEFINE_GUARD_COND(mutex, _try, mutex_trylock(_T))
include/linux/mutex.h:DEFINE_GUARD_COND(mutex, _intr, mutex_lock_interruptible(_T) == 0)
include/linux/rwsem.h:DEFINE_GUARD_COND(rwsem_read, _try, down_read_trylock(_T))
include/linux/rwsem.h:DEFINE_GUARD_COND(rwsem_read, _intr, down_read_interruptible(_T) == 0)
include/linux/rwsem.h:DEFINE_GUARD_COND(rwsem_write, _try, down_write_trylock(_T))

I propose that you use scoped_try_guard() and scoped_guard_interruptible() to
support conditional locking.  Creating different macros for conditional locks is
the only way you can silence your GCC warnings and it makes life easier for me
as a static checker developer as well.  It's probably more complicated than I
have described so I'll leave that up to you, but this first draft doesn't work.

regards,
dan carpenter

Re: [RFC PATCH] cleanup: make scoped_guard() to be return-friendly

[Dmitry Torokhov]

On Mon, Sep 30, 2024 at 01:30:58PM +0200, Przemek Kitszel wrote:
> On 9/30/24 13:08, Dan Carpenter wrote:
> > On Mon, Sep 30, 2024 at 12:21:44PM +0200, Przemek Kitszel wrote:
> > > 
> > > Most of the time it is just easier to bend your driver than change or
> > > extend the core of the kernel.
> > > 
> > > There is actually scoped_cond_guard() which is a trylock variant.
> > > 
> > > scoped_guard(mutex_try, &ts->mutex) you have found is semantically
> > > wrong and must be fixed.
> > 
> > What?  I'm so puzzled by this conversation.
> 
> there are two variants of scoped_guard() and you have found a place
> where the wrong one is used

"Yeah? Well, you know, that's just like uh, your opinion, man."

From include/linux/cleanup.h:

 * 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
 *	bound to the next (compound) statement.
 *
 *	for conditional locks the loop body is skipped when the lock is not
 *	acquired.

Please note the 2nd paragraph that explains this particular usage and
that it was done this way on purpose.

> 
> > 
> > Anyway, I don't have a problem with your goal, but your macro is wrong and will
> > need to be re-written.  You will need to update any drivers which use the
> > scoped_guard() for try locks.  I don't care how you do that.  Use
> > scoped_cond_guard() if you want or invent a new macro.  But that work always
> > falls on the person changing the API.  Plus, it's only the one tsc200x-core.c
> > driver so I don't understand why you're making a big deal about it.

I think if you also count uses of "scoped_guard(mutex_intr, ...)" you
will find more of such examples.

> 
> apologies for upsetting you
> I will send next iteration of this series with additional patches fixing
> current code (thanks you for finding it for me in this case!)

No, please do not. Your "fix" it looks like will prevent writing
code like:

	scoped_guard(mutex_intr, &some_mutex) {
		do_stuff();

		return 0;
	}

	return -EINTR;

You might not like it, but it is a valid pattern.

> 
> I didn't said so in prev mail to leave you an option to send the fix for
> the usage bug you have reported, just confirmed it. But by all means I'm
> happy to fix current code myself.
> 
> > but your macro is wrong and will need to be re-written
> 
> could you please elaborate here?
i
Dan explained that you are changing the behavior of the guards, in an
undesirable way, breaking users. Please re-read what was written before.

Thanks.

-- 
Dmitry

Re: [RFC PATCH] cleanup: make scoped_guard() to be return-friendly

[Dmitry Torokhov]

On Mon, Sep 30, 2024 at 03:57:08PM +0300, Dan Carpenter wrote:
> On Mon, Sep 30, 2024 at 01:30:58PM +0200, Przemek Kitszel wrote:
> > > but your macro is wrong and will need to be re-written
> > 
> > could you please elaborate here?
> 
> - 		__guard_ptr(_name)(&scope) && !done;
> +		__guard_ptr(_name)(&scope), 1;     \
> 
> The __guard_ptr(_name)(&scope) check is checking whether lock function
> succeeded.  With the new macro we only use scoped_guard() for locks which can't
> fail.  We can (basically must) remove the __guard_ptr(_name)(&scope) check since
> we're ignoring the result.
> 
> There are only four drivers which rely on conditional scoped_guard() locks.
> 
> $ git grep scoped_guard | egrep '(try|_intr)'
> drivers/input/keyboard/atkbd.c: scoped_guard(mutex_intr, &atkbd->mutex) {
> drivers/input/touchscreen/tsc200x-core.c:       scoped_guard(mutex_try, &ts->mutex) {
> drivers/input/touchscreen/wacom_w8001.c:        scoped_guard(mutex_intr, &w8001->mutex) {
> drivers/platform/x86/ideapad-laptop.c:  scoped_guard(mutex_intr, &dytc->mutex) {

FTR I have many more pending changes using scoped_guard() this way.

> 
> This change breaks the drivers at runtime, but you need to ensure that drivers
> using the old API will break at compile time so that people don't introduce new
> bugs during the transition.  In other words, you will need to update the
> DEFINE_GUARD_COND() stuff as well.
> 
> $ git grep DEFINE_GUARD_COND
> include/linux/cleanup.h: * DEFINE_GUARD_COND(name, ext, condlock)
> include/linux/cleanup.h:#define DEFINE_GUARD_COND(_name, _ext, _condlock) \
> include/linux/iio/iio.h:DEFINE_GUARD_COND(iio_claim_direct, _try, ({
> include/linux/mutex.h:DEFINE_GUARD_COND(mutex, _try, mutex_trylock(_T))
> include/linux/mutex.h:DEFINE_GUARD_COND(mutex, _intr, mutex_lock_interruptible(_T) == 0)
> include/linux/rwsem.h:DEFINE_GUARD_COND(rwsem_read, _try, down_read_trylock(_T))
> include/linux/rwsem.h:DEFINE_GUARD_COND(rwsem_read, _intr, down_read_interruptible(_T) == 0)
> include/linux/rwsem.h:DEFINE_GUARD_COND(rwsem_write, _try, down_write_trylock(_T))
> 
> I propose that you use scoped_try_guard() and scoped_guard_interruptible() to
> support conditional locking.  Creating different macros for conditional locks is
> the only way you can silence your GCC warnings and it makes life easier for me
> as a static checker developer as well.  It's probably more complicated than I
> have described so I'll leave that up to you, but this first draft doesn't work.

No, please do not. Right now the whether resource acquisition can fail
or not is decided by a particular class (which in turn can be used in
various guard macros). You are proposing to bubble this knowledge up and
make specialized "try" and "interruptible" and "killable" and "fallible"
version of the previously generic macros.

Now, the whole issue is because GCC is unable to figure out the flow
control of a complex macro. Please either fix GCC or rework that one
call site to shut GCC up. Do not wreck nice generic guard
implementation, that is flexible and supports several styles of use.

Again, the fact that scoped_guard() body can be skipped if the
constructor fails is explicitly documented as a desirable property.

Thanks.

-- 
Dmitry