Re: [PATCH v3 0/8] cleanup: Introduce ACQUIRE(), a guard() for conditional locks

[Nathan Chancellor <nathan@kernel.org>]

On Fri, Aug 01, 2025 at 11:49:22AM -0700, dan.j.williams@intel.com wrote:
> Andy Shevchenko wrote:
> > This series broke `make W=1` build vor clang. +Cc Nathan.
> > 
> > Par exemple:
> > 
> > /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);
> >       | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > /include/linux/cleanup.h:376:2: note: expanded from macro 'DEFINE_CLASS_IS_COND_GUARD'
> >   376 |         __DEFINE_GUARD_LOCK_PTR(_name, _T)
> >       |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > /include/linux/cleanup.h:358:20: note: expanded from macro '__DEFINE_GUARD_LOCK_PTR'
> >   358 |         static inline int class_##_name##_lock_err(class_##_name##_t *_T)   \
> >       |                           ^~~~~~~~~~~~~~~~~~~~~~~~
> > <scratch space>:24:1: note: expanded from here
> >    24 | class_lock_timer_lock_err
> >       | ^~~~~~~~~~~~~~~~~~~~~~~~~
> > 1 error generated.
> 
> A few observations:
> 
> - This is odd, the inline should have been compiled away if not used.
> - __always_inline does not help
> - Seems to go away with __maybe_unused, but that seems more like a
>   compiler band-aid than a fix

See commit 6863f5643dd7 ("kbuild: allow Clang to find unused static
inline functions for W=1 build") for more information on the difference
between GCC and Clang when it comes to how 'static inline' functions
behave with -Wunused-function, namely that Clang will warn for functions
defined in .c files (but not .h files), whereas GCC will not warn for
either.

> - This locking pattern is not immediately amenable to the ACQUIRE_ERR()
>   approach because the unlock object is the return code from the
>   constructor.
> 
> Given all that, and that an ACQUIRE_ERR() would end up being messier
> than the scoped_timer_get_or_fail() approach, I think the best fix is to
> quiet the warning, but maybe Peter and Nathan have other ideas?

Yes, this is what I would recommend, as we never care if this function
is unused, right? You could probably outright substitute
'__maybe_unused' for 'inline' in this case, since the compiler is
already free to ignore it and the attribute takes care of any potential
unused warnings, which I think 'inline' is primarily used for nowadays.

Cheers,
Nathan

> -- 8< --
> diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
> index 4eb83dd71cfe..0dc7148d1b88 100644
> --- a/include/linux/cleanup.h
> +++ b/include/linux/cleanup.h
> @@ -348,7 +348,8 @@ static __maybe_unused const bool class_##_name##_is_conditional = _is_cond
>  		}                                                           \
>  		return _ptr;                                                \
>  	}                                                                   \
> -	static inline int class_##_name##_lock_err(class_##_name##_t *_T)   \
> +	static __maybe_unused inline int class_##_name##_lock_err(          \
> +		class_##_name##_t *_T)                                      \
>  	{                                                                   \
>  		long _rc = (__force unsigned long)*(_exp);                  \
>  		if (!_rc) {                                                 \