Message-Id: <20241003113906.750116-1-przemyslaw.kitszel@intel.com>
Date: Thu,  3 Oct 2024 13:39:06 +0200
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: linux-kernel@...r.kernel.org
Cc: amadeuszx.slawinski@...ux.intel.com,
	Tony Nguyen <anthony.l.nguyen@...el.com>,
	nex.sw.ncis.osdt.itp.upstreaming@...el.com,
	netdev@...r.kernel.org,
	Markus Elfring <Markus.Elfring@....de>,
	Kees Cook <kees@...nel.org>,
	Przemek Kitszel <przemyslaw.kitszel@...el.com>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Dan Carpenter <dan.carpenter@...aro.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Andy Shevchenko <andriy.shevchenko@...el.com>
Subject: [PATCH v1] cleanup: adjust scoped_guard() to avoid potential warning

Change scoped_guard() to make reasoning about it easier for static
analysis tools (smatch, compiler diagnostics), especially to enable them
to tell if the given scoped_guard() is conditional (interruptible-locks,
try-locks) or not (like simple mutex_lock()).

Add compile-time error if scoped_cond_guard() is used for non-conditional
lock class.

Beyond easier tooling and a little shrink reported by bloat-o-meter:
add/remove: 3/2 grow/shrink: 45/55 up/down: 1573/-2069 (-496)
this patch enables developer 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]

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 non-const 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 the
"for (...; goto label) if (0) label: break;" idiom, so it's not packed
for reuse, what makes actual macros code cleaner.

There was also a need to introduce const true/false variable per lock
class, it is used to aid compiler diagnostics reasoning about "exactly
1 step" loops (note that converting that to function would undo the whole
benefit).

CC: Dmitry Torokhov <dmitry.torokhov@...il.com>
CC: Dan Carpenter <dan.carpenter@...aro.org>
CC: Peter Zijlstra <peterz@...radead.org>
NAKed-by: Andy Shevchenko <andriy.shevchenko@...el.com>
Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>
---
Andy believes that this change enables completely wrong C, the reasons
(that I disagree with of course, are in v1, below the commit message).

PATCH v1:
changes thanks to Dmitry Torokhov:
 better writeup in commit msg;
 "__" prefix added to internal macros;
 reorder "if (0)-else" and "for" to avoid goto jumping back;
 compile-time check for scoped_cond_guard()

RFC v2:
https://lore.kernel.org/netdev/35b3305f-d2c6-4d2b-9ac5-8bbb93873b92@stanley.mountain
 remove ", 1" condition, as scoped_guard() could be used also for
 conditional locks (try-lock, irq-lock, etc) - this was pointed out by
 Dmitry Torokhov and Dan Carpenter;
 reorder macros to have them defined prior to use - Markus Elfring.

RFC v1:
https://lore.kernel.org/netdev/20240926134347.19371-1-przemyslaw.kitszel@intel.com
---
 include/linux/cleanup.h | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index a3d3e888cf1f..eb97d0520202 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -151,12 +151,17 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
  *
  */
 
+#define __DEFINE_CLASS_IS_CONDITIONAL(_name, _is_cond)	\
+static __maybe_unused const bool class_##_name##_is_conditional = _is_cond
+
 #define DEFINE_GUARD(_name, _type, _lock, _unlock) \
+	__DEFINE_CLASS_IS_CONDITIONAL(_name, false); \
 	DEFINE_CLASS(_name, _type, if (_T) { _unlock; }, ({ _lock; _T; }), _type _T); \
 	static inline void * class_##_name##_lock_ptr(class_##_name##_t *_T) \
 	{ return *_T; }
 
 #define DEFINE_GUARD_COND(_name, _ext, _condlock) \
+	__DEFINE_CLASS_IS_CONDITIONAL(_name##_ext, true); \
 	EXTEND_CLASS(_name, _ext, \
 		     ({ void *_t = _T; if (_T && !(_condlock)) _t = NULL; _t; }), \
 		     class_##_name##_t _T) \
@@ -167,14 +172,25 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
 	CLASS(_name, __UNIQUE_ID(guard))
 
 #define __guard_ptr(_name) class_##_name##_lock_ptr
+#define __is_cond_ptr(_name) class_##_name##_is_conditional
+
+#define __scoped_guard_labeled(_label, _name, args...)			\
+	for (CLASS(_name, scope)(args);					\
+	     __guard_ptr(_name)(&scope) || !__is_cond_ptr(_name);	\
+		     ({ goto _label; }))				\
+		if (0)							\
+		_label:							\
+			break;						\
+		else
+
+#define scoped_guard(_name, args...)	\
+	__scoped_guard_labeled(__UNIQUE_ID(label), _name, args)
 
-#define scoped_guard(_name, args...)					\
-	for (CLASS(_name, scope)(args),					\
-	     *done = NULL; __guard_ptr(_name)(&scope) && !done; done = (void *)1)
 
 #define scoped_cond_guard(_name, _fail, args...) \
 	for (CLASS(_name, scope)(args), \
-	     *done = NULL; !done; done = (void *)1) \
+	     *done = NULL; !done; done = (void *)1 +  	\
+	     BUILD_BUG_ON_ZERO(!__is_cond_ptr(_name)))	\
 		if (!__guard_ptr(_name)(&scope)) _fail; \
 		else
 
@@ -233,14 +249,17 @@ static inline class_##_name##_t class_##_name##_constructor(void)	\
 }
 
 #define DEFINE_LOCK_GUARD_1(_name, _type, _lock, _unlock, ...)		\
+__DEFINE_CLASS_IS_CONDITIONAL(_name, false);				\
 __DEFINE_UNLOCK_GUARD(_name, _type, _unlock, __VA_ARGS__)		\
 __DEFINE_LOCK_GUARD_1(_name, _type, _lock)
 
 #define DEFINE_LOCK_GUARD_0(_name, _lock, _unlock, ...)			\
+__DEFINE_CLASS_IS_CONDITIONAL(_name, false);				\
 __DEFINE_UNLOCK_GUARD(_name, void, _unlock, __VA_ARGS__)		\
 __DEFINE_LOCK_GUARD_0(_name, _lock)
 
 #define DEFINE_LOCK_GUARD_1_COND(_name, _ext, _condlock)		\
+	__DEFINE_CLASS_IS_CONDITIONAL(_name##_ext, true);		\
 	EXTEND_CLASS(_name, _ext,					\
 		     ({ class_##_name##_t _t = { .lock = l }, *_T = &_t;\
 		        if (_T->lock && !(_condlock)) _T->lock = NULL;	\

base-commit: 62a0e2fa40c5c06742b8b4997ba5095a3ec28503
-- 
2.39.3

Powered by blists - more mailing lists