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

[<dan.j.williams@intel.com>]

Andy Shevchenko wrote:
> On Fri, Jul 11, 2025 at 04:49:24PM -0700, Dan Williams wrote:
> > Changes since v2 [1]:
> > - Pick up Acks and Reviews
> > - Whitespace fixups for cleanup.h changes (Jonathan)
> > - Use consistent local variable style for ACQUIRE_ERR() (Jonathan)
> >   - Not addressed: switch to less compact style ACQUIRE_ERR()
> > - Not addressed: pickup checkpatch change for ACQUIRE_ERR() style in
> >   this series (Alison)
> > - Drop the cxl_decoder_detach() CLASS() and convert to a helper function (Jonathan)
> > - Refactor attach_target() to make it easier to read (Jonathan)
> > 
> > [1]: http://lore.kernel.org/20250619050416.782871-1-dan.j.williams@intel.com
> > 
> > For those new to this set, the motivation for this work is that the CXL
> > subsystem adopted scope-based-cleanup helpers and achieved some decent
> > cleanups. However, that work stalled with conditional locks. It stalled
> > due to the pain points of scoped_cond_guard(). See patch1.
> > 
> > In the interim, approaches like rwsem_read_intr_acquire() attempted to
> > workaround the pain points, but started a "parallel universe" of helpers
> > that is not sustainable.
> > 
> >     0c6e6f1357cb cxl/edac: Add CXL memory device patrol scrub control feature
> > 
> > Peter fixed all of this up in a manner consistent with existing guards.
> > Take that proposal and run with it to unblock further cleanups of "goto"
> > in unwind paths in the CXL subsystem.
> > 
> > Potential follow-on work identified by this effort:
> > 
> > - __GUARD_IS_ERR() asm helper [2]
> > - Checkpatch fixups for proposed ACQUIRE_ERR() style [3]
> > 
> > [2]: http://lore.kernel.org/20250514064624.GA24938@noisy.programming.kicks-ass.net
> > [3]: http://lore.kernel.org/aGXDMZB6omShJpoj@aschofie-mobl2.lan
> 
> 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
- 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?

-- 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) {                                                 \