include/linux/cleanup.h | 36 +++++++++++++++++++++--------------- 1 file changed, 21 insertions(+), 15 deletions(-)
While the warning could simply be moved to W=2 [1], there is some small
value, and not much cost to fixing it.
The issue, Andy reports that the "lock_timer" scheme in
kernel/time/posix-timers.c, with its custom usage of
DEFINE_CLASS_IS_COND_GUARD(), results in:
kernel/time/posix-timers.c:89:1: error: unused function 'class_lock_timer_lock_err' [-Werror,-Wunused-function]
89 | DEFINE_CLASS_IS_COND_GUARD(lock_timer);
...with a clang W=1 build. This warning has some value because it can catch
when a conditional guard is defined, but not evaluated by a conditional
acquisition helper like scoped_cond_guard() or ACQUIRE().
Andy also reports that plain DEFINE_GUARD() also encounters this warning:
drivers/pwm/core.c:54:1: error: unused function 'class_pwmchip_lock_err' [-Werror,-Wunused-function]
54 | DEFINE_GUARD(pwmchip, struct pwm_chip *, pwmchip_lock(_T), pwmchip_unlock(_T))
...which *is* a false positive.
Fix those 2 issues by teaching scoped_cond_guard() to check for error
values, and otherwise teach the DEFINE_GUARD() path to mark the conditional
helpers as __maybe_unused.
The compiler does change the polarity of some tests, but the size of the
binary is identical:
Before:
scoped_timer_get_or_fail(timer_id)
1246: 44 89 ff mov %r15d,%edi
1249: e8 d2 5a 00 00 call 6d20 <class_lock_timer_constructor>
124e: 49 89 c7 mov %rax,%r15
1251: 48 3d 01 f0 ff ff cmp $0xfffffffffffff001,%rax
1257: 0f 92 c0 setb %al
125a: 4d 85 ff test %r15,%r15
125d: 0f 95 c1 setne %cl
1260: 84 c8 test %cl,%al
1262: 0f 84 85 00 00 00 je 12ed <__ia32_sys_timer_gettime+0x15d>
...
12ed: 48 c7 c3 ea ff ff ff mov $0xffffffffffffffea,%rbx
if (likely((timr)))
12f4: 4d 85 ff test %r15,%r15
12f7: 74 0c je 1305 <__ia32_sys_timer_gettime+0x175>
After:
scoped_timer_get_or_fail(timer_id)
1246: 44 89 ff mov %r15d,%edi
1249: e8 d2 5a 00 00 call 6d20 <class_lock_timer_constructor>
124e: 49 89 c7 mov %rax,%r15
1251: 48 3d 01 f0 ff ff cmp $0xfffffffffffff001,%rax
1257: 0f 92 c0 setb %al
125a: 4d 85 ff test %r15,%r15
125d: 0f 95 c1 setne %cl
1260: 84 c8 test %cl,%al
1262: 75 21 jne 1285 <__ia32_sys_timer_gettime+0xf5>
1264: 48 c7 c3 ea ff ff ff mov $0xffffffffffffffea,%rbx
126b: 4d 85 ff test %r15,%r15
if (likely((timr)))
126e: 0f 84 94 00 00 00 je 1308 <__ia32_sys_timer_gettime+0x178>
Alternatively just merge the suggestion in [1], and call it a day.
Link: http://lore.kernel.org/20250813152142.GP4067720@noisy.programming.kicks-ass.net [1]
Cc: Nathan Chancellor <nathan@kernel.org>
Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Dave Jiang <dave.jiang@intel.com>
Cc: David Lechner <dlechner@baylibre.com>
Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
Reported-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Closes: http://lore.kernel.org/aIo18KZpmKuR4hVZ@black.igk.intel.com
Tested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Fixes: 857d18f23ab1 ("cleanup: Introduce ACQUIRE() and ACQUIRE_ERR() for conditional locks")
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
v1: http://lore.kernel.org/20250804220955.1453135-1-dan.j.williams@intel.com
include/linux/cleanup.h | 36 +++++++++++++++++++++---------------
1 file changed, 21 insertions(+), 15 deletions(-)
diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index 2573585b7f06..b3a7f6b2142d 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -354,26 +354,30 @@ static __maybe_unused const bool class_##_name##_is_conditional = _is_cond
_ptr = NULL; \
} \
return _ptr; \
- } \
- static inline int class_##_name##_lock_err(class_##_name##_t *_T) \
- { \
- long _rc = (__force unsigned long)*(_exp); \
- if (!_rc) { \
- _rc = -EBUSY; \
- } \
- if (!IS_ERR_VALUE(_rc)) { \
- _rc = 0; \
- } \
- return _rc; \
+ }
+
+#define __DEFINE_GUARD_LOCK_ERR(_name, _exp, _attr) \
+ static _attr int class_##_name##_lock_err(class_##_name##_t *_T) \
+ { \
+ long _rc = (__force unsigned long)*(_exp); \
+ if (!_rc) { \
+ _rc = -EBUSY; \
+ } \
+ if (!IS_ERR_VALUE(_rc)) { \
+ _rc = 0; \
+ } \
+ return _rc; \
}
#define DEFINE_CLASS_IS_GUARD(_name) \
__DEFINE_CLASS_IS_CONDITIONAL(_name, false); \
- __DEFINE_GUARD_LOCK_PTR(_name, _T)
+ __DEFINE_GUARD_LOCK_PTR(_name, _T) \
+ __DEFINE_GUARD_LOCK_ERR(_name, _T, __maybe_unused)
#define DEFINE_CLASS_IS_COND_GUARD(_name) \
__DEFINE_CLASS_IS_CONDITIONAL(_name, true); \
- __DEFINE_GUARD_LOCK_PTR(_name, _T)
+ __DEFINE_GUARD_LOCK_PTR(_name, _T) \
+ __DEFINE_GUARD_LOCK_ERR(_name, _T, inline)
#define DEFINE_GUARD(_name, _type, _lock, _unlock) \
DEFINE_CLASS(_name, _type, if (!__GUARD_IS_ERR(_T)) { _unlock; }, ({ _lock; _T; }), _type _T); \
@@ -430,7 +434,8 @@ _label: \
#define __scoped_cond_guard(_name, _fail, _label, args...) \
for (CLASS(_name, scope)(args); true; ({ goto _label; })) \
- if (!__guard_ptr(_name)(&scope)) { \
+ if (!__guard_ptr(_name)(&scope) || \
+ __guard_err(_name)(&scope)) { \
BUILD_BUG_ON(!__is_cond_ptr(_name)); \
_fail; \
_label: \
@@ -471,7 +476,8 @@ static inline void class_##_name##_destructor(class_##_name##_t *_T) \
if (!__GUARD_IS_ERR(_T->lock)) { _unlock; } \
} \
\
-__DEFINE_GUARD_LOCK_PTR(_name, &_T->lock)
+__DEFINE_GUARD_LOCK_PTR(_name, &_T->lock) \
+__DEFINE_GUARD_LOCK_ERR(_name, &_T->lock, __maybe_unused)
#define __DEFINE_LOCK_GUARD_1(_name, _type, _lock) \
static inline class_##_name##_t class_##_name##_constructor(_type *l) \
base-commit: b320789d6883cc00ac78ce83bccbfe7ed58afcf0
--
2.51.0On Thu, 4 Sep 2025 15:50:10 -0700 Dan Williams <dan.j.williams@intel.com> wrote: > While the warning could simply be moved to W=2 [1], there is some small > value, and not much cost to fixing it. > > The issue, Andy reports that the "lock_timer" scheme in > kernel/time/posix-timers.c, with its custom usage of > DEFINE_CLASS_IS_COND_GUARD(), results in: > > kernel/time/posix-timers.c:89:1: error: unused function 'class_lock_timer_lock_err' [-Werror,-Wunused-function] > 89 | DEFINE_CLASS_IS_COND_GUARD(lock_timer); > > ...with a clang W=1 build. This warning has some value because it can catch > when a conditional guard is defined, but not evaluated by a conditional > acquisition helper like scoped_cond_guard() or ACQUIRE(). > > Andy also reports that plain DEFINE_GUARD() also encounters this warning: > > drivers/pwm/core.c:54:1: error: unused function 'class_pwmchip_lock_err' [-Werror,-Wunused-function] > 54 | DEFINE_GUARD(pwmchip, struct pwm_chip *, pwmchip_lock(_T), pwmchip_unlock(_T)) > > ...which *is* a false positive. > > Fix those 2 issues by teaching scoped_cond_guard() to check for error > values, and otherwise teach the DEFINE_GUARD() path to mark the conditional > helpers as __maybe_unused. Warning about unused static inlines in .c files is just annoying. If the function is unused in all possible configs (man GREP(1)) then OK. Otherwise, let it be. > Alternatively just merge the suggestion in [1], and call it a day. > > Link: http://lore.kernel.org/20250813152142.GP4067720@noisy.programming.kicks-ass.net [1] lgtm, unless we think this (your) patch improves the code for other reasons?
Andrew Morton wrote: [..] > > Alternatively just merge the suggestion in [1], and call it a day. > > > > Link: http://lore.kernel.org/20250813152142.GP4067720@noisy.programming.kicks-ass.net [1] > > lgtm, unless we think this (your) patch improves the code for other reasons? The tl;dr above is that the warning could have small value, but probably not greater than the overall benefit to Linux to stop bothering folks with this low-value warning by default at W=1. So I am over the sunk costs, and moving this warning to W=2 is the way to go.
On Wed, Sep 17, 2025 at 04:21:07PM -0700, dan.j.williams@intel.com wrote: > Andrew Morton wrote: [..] > > > Alternatively just merge the suggestion in [1], and call it a day. > > > > > > Link: http://lore.kernel.org/20250813152142.GP4067720@noisy.programming.kicks-ass.net [1] > > > > lgtm, unless we think this (your) patch improves the code for other reasons? > > The tl;dr above is that the warning could have small value, but probably > not greater than the overall benefit to Linux to stop bothering folks > with this low-value warning by default at W=1. > > So I am over the sunk costs, and moving this warning to W=2 is the way > to go. Can somebody add a fix so, we have v6.17 able to be built with `make W=1`, please? -- With Best Regards, Andy Shevchenko
Andy Shevchenko wrote: > On Wed, Sep 17, 2025 at 04:21:07PM -0700, dan.j.williams@intel.com wrote: > > Andrew Morton wrote: > > [..] > > > > > Alternatively just merge the suggestion in [1], and call it a day. > > > > > > > > Link: http://lore.kernel.org/20250813152142.GP4067720@noisy.programming.kicks-ass.net [1] > > > > > > lgtm, unless we think this (your) patch improves the code for other reasons? > > > > The tl;dr above is that the warning could have small value, but probably > > not greater than the overall benefit to Linux to stop bothering folks > > with this low-value warning by default at W=1. > > > > So I am over the sunk costs, and moving this warning to W=2 is the way > > to go. > > Can somebody add a fix so, we have v6.17 able to be built with `make W=1`, please? Andy, might you have time to take Peter's proposed diff and wrap it with a changelog for Andrew to pull?
On Thu, Sep 18, 2025 at 12:26:28PM -0700, dan.j.williams@intel.com wrote: > Andy Shevchenko wrote: > > On Wed, Sep 17, 2025 at 04:21:07PM -0700, dan.j.williams@intel.com wrote: > > > Andrew Morton wrote: [..] > > > > > Alternatively just merge the suggestion in [1], and call it a day. > > > > > > > > > > Link: http://lore.kernel.org/20250813152142.GP4067720@noisy.programming.kicks-ass.net [1] > > > > > > > > lgtm, unless we think this (your) patch improves the code for other reasons? > > > > > > The tl;dr above is that the warning could have small value, but probably > > > not greater than the overall benefit to Linux to stop bothering folks > > > with this low-value warning by default at W=1. > > > > > > So I am over the sunk costs, and moving this warning to W=2 is the way > > > to go. > > > > Can somebody add a fix so, we have v6.17 able to be built with `make W=1`, please? > > Andy, might you have time to take Peter's proposed diff and wrap it with > a changelog for Andrew to pull? I will try my best, thanks! -- With Best Regards, Andy Shevchenko
© 2016 - 2025 Red Hat, Inc.