cleanup: make scoped_guard() to be return-friendly

RFC PATCH In review Przemek Kitszel 5 participants 15 messages Sep 26, 2024 – Sep 30, 2024

Subsystems (3)

Networking Network drivers Kernel janitors

lore

Share & tools

lore

Przemek Kitszel

author

2024-09-26 13:44 UTC #0 lore raw

RFC PATCH 1/1 cleanup: make scoped_guard() to be return-friendly

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 <[email protected]>

Signed-off-by Przemek Kitszel <[email protected]>

---
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(-)

include/linux/cleanup.h +10 -3

index d9e613803df1..6b568a8a7f9c 100644
@@ -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

Signature · 1 line

2.46.0

kernel test robot

build bot

2024-09-27 00:48 UTC #1 lore raw

kernel test robot noticed the following build warnings:

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/[email protected]/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/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

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

Signature · 2 lines

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

kernel test robot

build bot

2024-09-27 00:48 UTC #2 lore raw

In reply to Przemek Kitszel

kernel test robot noticed the following build warnings:

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/[email protected]/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/[email protected]/reproduce)

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

Signature · 2 lines

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

Dan Carpenter

2024-09-27 07:31 UTC #3 lore raw

In reply to Przemek Kitszel

Quoted 10 lines · Przemek Kitszel

include/linux/cleanup.h +0 -2

index d9e613803df1..6b568a8a7f9c 100644
@@ -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),					\

include/linux/cleanup.h static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \ · line 179

Quoted 10 lines

include/linux/cleanup.h +9 -1

-	     *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;	\

^^^

include/linux/cleanup.h static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \ · line 181

Quoted 2 lines

include/linux/cleanup.h +1 -0
+		     ({ 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

Przemek Kitszel

author

2024-09-27 14:08 UTC #4 lore raw

Quoted 22 lines · Dan Carpenter

Quoted 10 lines · Przemek Kitszel

include/linux/cleanup.h +0 -2

index d9e613803df1..6b568a8a7f9c 100644
@@ -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),					\

include/linux/cleanup.h static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \ · line 179

Quoted 10 lines

include/linux/cleanup.h +9 -1

-	     *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;	\

^^^

Quoted 5 lines

include/linux/cleanup.h static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \ · line 181
Quoted 2 lines
include/linux/cleanup.h +1 -0
+		     ({ 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.

Quoted 6 lines

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

Dan Carpenter

2024-09-27 15:04 UTC #5 lore raw

Quoted 26 lines · Przemek Kitszel

Quoted 20 lines · Dan Carpenter
Quoted 8 lines · Przemek Kitszel
include/linux/cleanup.h +0 -2
index d9e613803df1..6b568a8a7f9c 100644
@@ -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),					\
include/linux/cleanup.h static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \ · line 177
Quoted 10 lines
include/linux/cleanup.h +9 -1
-	     *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;	\
                                                ^^^
Quoted 4 lines
include/linux/cleanup.h static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \ · line 180
Quoted 1 line
include/linux/cleanup.h +1 -0
+		     ({ 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

Quoted 8 lines

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.
Quoted 1 line
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

Przemek Kitszel

author

2024-09-30 10:21 UTC #6 lore raw

Quoted 45 lines · Dan Carpenter

Quoted 26 lines · Przemek Kitszel
Quoted 20 lines · Dan Carpenter
Quoted 8 lines · Przemek Kitszel
include/linux/cleanup.h +0 -2
index d9e613803df1..6b568a8a7f9c 100644
@@ -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),					\
include/linux/cleanup.h static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \ · line 177
Quoted 10 lines
include/linux/cleanup.h +9 -1
-	     *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;	\
                                                 ^^^
Quoted 4 lines
include/linux/cleanup.h static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \ · line 180
Quoted 1 line
include/linux/cleanup.h +1 -0
+		     ({ 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
Quoted 8 lines
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.
Quoted 1 line
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.

Quoted 3 lines

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.

Dan Carpenter

2024-09-30 11:08 UTC #7 lore raw

Quoted 8 lines · Przemek Kitszel

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

Przemek Kitszel

author

2024-09-30 11:31 UTC #8 lore raw

Quoted 11 lines · Dan Carpenter

Quoted 8 lines · Przemek Kitszel
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

Quoted 7 lines

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.

Quoted 1 line

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

could you please elaborate here?

Dan Carpenter

2024-09-30 12:57 UTC #9 lore raw

Quoted 3 lines · Przemek Kitszel

Quoted 1 line
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

Dmitry Torokhov

2024-09-30 13:07 UTC #10 lore raw

Quoted 13 lines · Dan Carpenter

Quoted 3 lines · Przemek Kitszel
Quoted 1 line
but your macro is wrong and will need to be re-written
could you please elaborate here?
(quoted diff) +1 -1
- 		__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.

Quoted 7 lines

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.

Quoted 16 lines

$ 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))

Quoted 5 lines

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.

Signature · 1 line

Dmitry

Dmitry Torokhov

2024-09-30 12:58 UTC #11 lore raw

In reply to Przemek Kitszel

Quoted 8 lines · Przemek Kitszel

Quoted 7 lines · Dan Carpenter
Quoted 6 lines · Przemek Kitszel
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.

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
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.

Quoted 8 lines

Quoted 8 lines
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.

Quoted 4 lines

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.

Quoted 8 lines

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.
Quoted 1 line
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.

Signature · 1 line

Dmitry

Markus Elfring

2024-09-30 11:30 UTC #12 lore raw

In reply to Przemek Kitszel

…

Quoted 2 lines

Current scoped_guard() implementation does not support that, due to compiler complaining:

…

Quoted 11 lines

+++ b/include/linux/cleanup.h

include/linux/cleanup.h +3 -3

@@ -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)
+

include/linux/cleanup.h static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \ · line 183

Quoted 10 lines

include/linux/cleanup.h +7 -0

+#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

Przemek Kitszel

author

2024-09-30 12:33 UTC #13 lore raw

Quoted 23 lines · Markus Elfring

…

Quoted 2 lines

Current scoped_guard() implementation does not support that, due to compiler complaining:

…

Quoted 19 lines

+++ b/include/linux/cleanup.h

include/linux/cleanup.h +10 -3

@@ -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; }))

Quoted 4 lines

include/linux/cleanup.h static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \ · line 183
Quoted 2 lines
include/linux/cleanup.h +0 -0
  #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

Quoted 3 lines

* 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)

Quoted 4 lines

Regards,
Markus

Markus Elfring

2024-09-30 12:52 UTC #14 lore raw

Quoted 4 lines

Quoted 2 lines
* 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