drivers/cxl/core/cdat.c | 6 +- drivers/cxl/core/core.h | 32 ++- drivers/cxl/core/edac.c | 44 ++-- drivers/cxl/core/hdm.c | 118 +++++----- drivers/cxl/core/mbox.c | 13 +- drivers/cxl/core/memdev.c | 50 ++-- drivers/cxl/core/port.c | 27 +-- drivers/cxl/core/region.c | 473 ++++++++++++++++++++------------------ drivers/cxl/cxl.h | 13 +- drivers/cxl/cxlmem.h | 4 +- include/linux/cleanup.h | 95 ++++++-- include/linux/mutex.h | 2 +- include/linux/rwsem.h | 3 +- 13 files changed, 480 insertions(+), 400 deletions(-)
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
Dan Williams (7):
cxl/mbox: Convert poison list mutex to ACQUIRE()
cxl/decoder: Move decoder register programming to a helper
cxl/decoder: Drop pointless locking
cxl/region: Split commit_store() into __commit() and queue_reset()
helpers
cxl/region: Move ready-to-probe state check to a helper
cxl/region: Consolidate cxl_decoder_kill_region() and
cxl_region_detach()
cxl: Convert to ACQUIRE() for conditional rwsem locking
Peter Zijlstra (1):
cleanup: Introduce ACQUIRE() and ACQUIRE_ERR() for conditional locks
drivers/cxl/core/cdat.c | 6 +-
drivers/cxl/core/core.h | 32 ++-
drivers/cxl/core/edac.c | 44 ++--
drivers/cxl/core/hdm.c | 118 +++++-----
drivers/cxl/core/mbox.c | 13 +-
drivers/cxl/core/memdev.c | 50 ++--
drivers/cxl/core/port.c | 27 +--
drivers/cxl/core/region.c | 473 ++++++++++++++++++++------------------
drivers/cxl/cxl.h | 13 +-
drivers/cxl/cxlmem.h | 4 +-
include/linux/cleanup.h | 95 ++++++--
include/linux/mutex.h | 2 +-
include/linux/rwsem.h | 3 +-
13 files changed, 480 insertions(+), 400 deletions(-)
base-commit: e04c78d86a9699d136910cfc0bdcf01087e3267e
--
2.50.0
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.
--
With Best Regards,
Andy Shevchenko
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) { \
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) { \
Nathan Chancellor wrote:
> 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.
Ah, thanks!
> > - 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?
Hmm, it would be nice to care. For example, if someone did something
like use ACQUIRE() without a corresponding ACQUIRE_ERR(). It would be
nice to catch that, but that warning would only fire for clang builds
where the lock guard is defined in a C file.
> 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.
Oh, true, good point.
Another option is make scoped_cond_guard() use the lock error helper.
The compiler should always elide that __guard_err() call, and in the
meantime maybe catch a true positive unused symbol for
class_##_name##_lock_err.
-- 8< --
diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index 4eb83dd71cfe..534723b14d92 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -421,13 +421,14 @@ _label: \
#define scoped_guard(_name, args...) \
__scoped_guard(_name, __UNIQUE_ID(label), args)
-#define __scoped_cond_guard(_name, _fail, _label, args...) \
- for (CLASS(_name, scope)(args); true; ({ goto _label; })) \
- if (!__guard_ptr(_name)(&scope)) { \
- BUILD_BUG_ON(!__is_cond_ptr(_name)); \
- _fail; \
-_label: \
- break; \
+#define __scoped_cond_guard(_name, _fail, _label, args...) \
+ for (CLASS(_name, scope)(args); true; ({ goto _label; })) \
+ if (!__guard_ptr(_name)(&scope) || \
+ __guard_err(_name)(&scope)) { \
+ BUILD_BUG_ON(!__is_cond_ptr(_name)); \
+ _fail; \
+_label: \
+ break; \
} else
#define scoped_cond_guard(_name, _fail, args...) \
On 7/11/25 4:49 PM, 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) Applied to cxl/next b873adfddeeb337fa8e9f381fd35eb94f7887f2f > > [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 > > Dan Williams (7): > cxl/mbox: Convert poison list mutex to ACQUIRE() > cxl/decoder: Move decoder register programming to a helper > cxl/decoder: Drop pointless locking > cxl/region: Split commit_store() into __commit() and queue_reset() > helpers > cxl/region: Move ready-to-probe state check to a helper > cxl/region: Consolidate cxl_decoder_kill_region() and > cxl_region_detach() > cxl: Convert to ACQUIRE() for conditional rwsem locking > > Peter Zijlstra (1): > cleanup: Introduce ACQUIRE() and ACQUIRE_ERR() for conditional locks > > drivers/cxl/core/cdat.c | 6 +- > drivers/cxl/core/core.h | 32 ++- > drivers/cxl/core/edac.c | 44 ++-- > drivers/cxl/core/hdm.c | 118 +++++----- > drivers/cxl/core/mbox.c | 13 +- > drivers/cxl/core/memdev.c | 50 ++-- > drivers/cxl/core/port.c | 27 +-- > drivers/cxl/core/region.c | 473 ++++++++++++++++++++------------------ > drivers/cxl/cxl.h | 13 +- > drivers/cxl/cxlmem.h | 4 +- > include/linux/cleanup.h | 95 ++++++-- > include/linux/mutex.h | 2 +- > include/linux/rwsem.h | 3 +- > 13 files changed, 480 insertions(+), 400 deletions(-) > > > base-commit: e04c78d86a9699d136910cfc0bdcf01087e3267e
© 2016 - 2025 Red Hat, Inc.