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

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

[Dan Williams]

Changes since v1: [1]
* Peter took one look at v1 and rewrote it into something significantly
  better. Unlike my attempt that required suffering a new parallel
  universe of lock guards, the rewrite reuses existing lock guards.
  ACQUIRE() can be used any place guard() can be used, and adds
  ACQUIRE_ERR() to pass the result of conditional locks.

[1]: http://lore.kernel.org/20250507072145.3614298-1-dan.j.williams@intel.com

Note, all the code in patch1 is Peter's I just wrapped it in a changelog
and added some commentary. Peter, forgive me if you were still in the
process of circling back to this topic. I marked the patch attributed to
you as: "Not-yet-signed-off-by". Otherwise, my motivation for going
ahead with a formal submission are the multiple patchsets in my review /
development queue where I would like to use ACQUIRE().

The orginal motivation of v1 for this work is that the CXL subsystem
adopted scope-based helpers and achieved some decent cleanups. However,
that work stalled with conditional locks. It stalled due to the pain
points of scoped_cond_guard() detailed in patch1.

This work also allows for replacing open-coded equivalents like
rwsem_read_intr_acquire() that went upstream in v6.16:

    0c6e6f1357cb cxl/edac: Add CXL memory device patrol scrub control feature

The open question from the discussion [2] was whether it was worth
defining a __GUARD_IS_ERR() asm helper. I left that alone.

Lastly, this version of ACQUIRE_ERR() matches Peter's original proposal
to have the caller pass the lock scope variable by reference [3]. My
change of heart came from looking at the conversion and wanting
ACQUIRE_ERR() to be more visually distinct from ACQUIRE() especially
because it is accessing lock condition metadata, not the lock itself.

E.g.

       ACQUIRE(rwsem_read_intr, rwsem)(&cxl_rwsem.region);
       if ((ret = ACQUIRE_ERR(rwsem_read_intr, &rwsem)))
               return ret;

Yes, checkpatch disagrees with assignment in if(), but cleanup.h already
demands other expections for historical style, and a compact / limited
idiom for ACQUIRE_ERR() feels reasonable.

[2]: http://lore.kernel.org/20250514064624.GA24938@noisy.programming.kicks-ass.net
[3]: http://lore.kernel.org/20250512105026.GP4439@noisy.programming.kicks-ass.net

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: Introduce CLASS(cxl_decoder_detach...) consolidate
    multiple paths
  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   |  60 +++++-
 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   |  24 +--
 drivers/cxl/core/region.c | 443 +++++++++++++++++++-------------------
 drivers/cxl/cxl.h         |  13 +-
 drivers/cxl/cxlmem.h      |   4 +-
 include/linux/cleanup.h   |  77 +++++--
 include/linux/mutex.h     |   2 +-
 include/linux/rwsem.h     |   3 +-
 13 files changed, 460 insertions(+), 397 deletions(-)


base-commit: e04c78d86a9699d136910cfc0bdcf01087e3267e
-- 
2.49.0

[PATCH v2 1/8] cleanup: Introduce ACQUIRE() and ACQUIRE_ERR() for conditional locks

[Dan Williams]

From: Peter Zijlstra <peterz@infradead.org>

scoped_cond_guard(), automatic cleanup for conditional locks, has a couple
pain points:

* It causes existing straight-line code to be re-indented into a new
  bracketed scope. While this can be mitigated by a new helper function
  to contain the scope, that is not always a comfortable conversion.

* The return code from the conditional lock is tossed in favor of a scheme
  to pass a 'return err;' statement to the macro.

Other attempts to clean this up, to behave more like guard() [1], got hung
up trying to both establish and evaluate the conditional lock in one
statement.

ACQUIRE() solves this by reflecting the result of the condition in the
automatic variable established by the lock CLASS(). The result is
separately retrieved with the ACQUIRE_ERR() helper, effectively a PTR_ERR()
operation.

Link: http://lore.kernel.org/all/Z1LBnX9TpZLR5Dkf@gmail.com [1]
Link: http://patch.msgid.link/20250512105026.GP4439@noisy.programming.kicks-ass.net
Link: http://patch.msgid.link/20250512185817.GA1808@noisy.programming.kicks-ass.net
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: David Lechner <dlechner@baylibre.com>
Cc: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>
Not-yet-signed-off-by: Peter Zijlstra <peterz@infradead.org>
[djbw: wrap Peter's proposal with changelog and comments]
Co-developed-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 include/linux/cleanup.h | 77 ++++++++++++++++++++++++++++++++++-------
 include/linux/mutex.h   |  2 +-
 include/linux/rwsem.h   |  2 +-
 3 files changed, 67 insertions(+), 14 deletions(-)

diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index 7093e1d08af0..1e1eb35cc225 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -3,6 +3,8 @@
 #define _LINUX_CLEANUP_H
 
 #include <linux/compiler.h>
+#include <linux/err.h>
+#include <linux/args.h>
 
 /**
  * DOC: scope-based cleanup helpers
@@ -61,9 +63,21 @@
  * Observe the lock is held for the remainder of the "if ()" block not
  * the remainder of "func()".
  *
- * Now, when a function uses both __free() and guard(), or multiple
- * instances of __free(), the LIFO order of variable definition order
- * matters. GCC documentation says:
+ * The ACQUIRE() macro can be used in all places that guard() can be
+ * used and additionally support conditional locks
+ *
+ *
+ *	DEFINE_GUARD_COND(pci_dev, _try, pci_dev_trylock(_T))
+ *	...
+ *	ACQUIRE(pci_dev_try, lock)(dev);
+ *	rc = ACQUIRE_ERR(pci_dev_try, &lock);
+ *	if (rc)
+ *		return rc;
+ *	// @lock is held
+ *
+ * Now, when a function uses both __free() and guard()/ACQUIRE(), or
+ * multiple instances of __free(), the LIFO order of variable definition
+ * order matters. GCC documentation says:
  *
  * "When multiple variables in the same scope have cleanup attributes,
  * at exit from the scope their associated cleanup functions are run in
@@ -305,14 +319,32 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
  *      acquire fails.
  *
  *      Only for conditional locks.
+ *
+ * ACQUIRE(name, var):
+ *	a named instance of the (guard) class, suitable for conditional
+ *	locks when paired with ACQUIRE_ERR().
+ *
+ * ACQUIRE_ERR(name, &var):
+ *	a helper that is effectively a PTR_ERR() conversion of the guard
+ *	pointer. Returns 0 when the lock was acquired and a negative
+ *	error code otherwise.
  */
 
 #define __DEFINE_CLASS_IS_CONDITIONAL(_name, _is_cond)	\
 static __maybe_unused const bool class_##_name##_is_conditional = _is_cond
 
+#define __GUARD_IS_ERR(_ptr) \
+	({ unsigned long _rc = (__force unsigned long)(_ptr); \
+	   unlikely((_rc-1) >= -MAX_ERRNO-1); })
+
 #define __DEFINE_GUARD_LOCK_PTR(_name, _exp) \
 	static inline void * class_##_name##_lock_ptr(class_##_name##_t *_T) \
-	{ return (void *)(__force unsigned long)*(_exp); }
+	{ void *_ptr = (void *)(__force unsigned long)*(_exp); \
+	  if (IS_ERR(_ptr)) { _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_CLASS_IS_GUARD(_name) \
 	__DEFINE_CLASS_IS_CONDITIONAL(_name, false); \
@@ -323,23 +355,37 @@ static __maybe_unused const bool class_##_name##_is_conditional = _is_cond
 	__DEFINE_GUARD_LOCK_PTR(_name, _T)
 
 #define DEFINE_GUARD(_name, _type, _lock, _unlock) \
-	DEFINE_CLASS(_name, _type, if (_T) { _unlock; }, ({ _lock; _T; }), _type _T); \
+	DEFINE_CLASS(_name, _type, if (!__GUARD_IS_ERR(_T)) { _unlock; }, ({ _lock; _T; }), _type _T); \
 	DEFINE_CLASS_IS_GUARD(_name)
 
-#define DEFINE_GUARD_COND(_name, _ext, _condlock) \
+#define DEFINE_GUARD_COND_4(_name, _ext, _lock, _cond) \
 	__DEFINE_CLASS_IS_CONDITIONAL(_name##_ext, true); \
 	EXTEND_CLASS(_name, _ext, \
-		     ({ void *_t = _T; if (_T && !(_condlock)) _t = NULL; _t; }), \
+		     ({ void *_t = _T; int _RET = (_lock); if (_T && !(_cond)) _t = ERR_PTR(_RET); _t; }), \
 		     class_##_name##_t _T) \
 	static inline void * class_##_name##_ext##_lock_ptr(class_##_name##_t *_T) \
-	{ return class_##_name##_lock_ptr(_T); }
+	{ return class_##_name##_lock_ptr(_T); } \
+	static inline int class_##_name##_ext##_lock_err(class_##_name##_t *_T) \
+	{ return class_##_name##_lock_err(_T); }
+
+/*
+ * Default binary condition; success on 'true'.
+ */
+#define DEFINE_GUARD_COND_3(_name, _ext, _lock) \
+	DEFINE_GUARD_COND_4(_name, _ext, _lock, _RET)
+
+#define DEFINE_GUARD_COND(X...) CONCATENATE(DEFINE_GUARD_COND_, COUNT_ARGS(X))(X)
 
 #define guard(_name) \
 	CLASS(_name, __UNIQUE_ID(guard))
 
 #define __guard_ptr(_name) class_##_name##_lock_ptr
+#define __guard_err(_name) class_##_name##_lock_err
 #define __is_cond_ptr(_name) class_##_name##_is_conditional
 
+#define ACQUIRE(_name, _var)     CLASS(_name, _var)
+#define ACQUIRE_ERR(_name, _var) __guard_err(_name)(_var)
+
 /*
  * Helper macro for scoped_guard().
  *
@@ -401,7 +447,7 @@ typedef struct {							\
 									\
 static inline void class_##_name##_destructor(class_##_name##_t *_T)	\
 {									\
-	if (_T->lock) { _unlock; }					\
+	if (!__GUARD_IS_ERR(_T->lock)) { _unlock; }			\
 }									\
 									\
 __DEFINE_GUARD_LOCK_PTR(_name, &_T->lock)
@@ -433,15 +479,22 @@ __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 DEFINE_LOCK_GUARD_1_COND_4(_name, _ext, _lock, _cond)		\
 	__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;	\
+		        int _RET = (_lock);                             \
+		        if (_T->lock && !(_cond)) _T->lock = ERR_PTR(_RET);\
 			_t; }),						\
 		     typeof_member(class_##_name##_t, lock) l)		\
 	static inline void * class_##_name##_ext##_lock_ptr(class_##_name##_t *_T) \
-	{ return class_##_name##_lock_ptr(_T); }
+	{ return class_##_name##_lock_ptr(_T); } \
+	static inline int class_##_name##_ext##_lock_err(class_##_name##_t *_T) \
+	{ return class_##_name##_lock_err(_T); }
+
+#define DEFINE_LOCK_GUARD_1_COND_3(_name, _ext, _lock) \
+	DEFINE_LOCK_GUARD_1_COND_4(_name, _ext, _lock, _RET)
 
+#define DEFINE_LOCK_GUARD_1_COND(X...) CONCATENATE(DEFINE_LOCK_GUARD_1_COND_, COUNT_ARGS(X))(X)
 
 #endif /* _LINUX_CLEANUP_H */
diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index a039fa8c1780..9d5d7ed5c101 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -224,7 +224,7 @@ extern int atomic_dec_and_mutex_lock(atomic_t *cnt, struct mutex *lock);
 
 DEFINE_GUARD(mutex, struct mutex *, mutex_lock(_T), mutex_unlock(_T))
 DEFINE_GUARD_COND(mutex, _try, mutex_trylock(_T))
-DEFINE_GUARD_COND(mutex, _intr, mutex_lock_interruptible(_T) == 0)
+DEFINE_GUARD_COND(mutex, _intr, mutex_lock_interruptible(_T), _RET == 0)
 
 extern unsigned long mutex_get_owner(struct mutex *lock);
 
diff --git a/include/linux/rwsem.h b/include/linux/rwsem.h
index c8b543d428b0..c810deb88d13 100644
--- a/include/linux/rwsem.h
+++ b/include/linux/rwsem.h
@@ -240,7 +240,7 @@ extern void up_write(struct rw_semaphore *sem);
 
 DEFINE_GUARD(rwsem_read, struct rw_semaphore *, down_read(_T), up_read(_T))
 DEFINE_GUARD_COND(rwsem_read, _try, down_read_trylock(_T))
-DEFINE_GUARD_COND(rwsem_read, _intr, down_read_interruptible(_T) == 0)
+DEFINE_GUARD_COND(rwsem_read, _intr, down_read_interruptible(_T), _RET == 0)
 
 DEFINE_GUARD(rwsem_write, struct rw_semaphore *, down_write(_T), up_write(_T))
 DEFINE_GUARD_COND(rwsem_write, _try, down_write_trylock(_T))
-- 
2.49.0

Re: [PATCH v2 1/8] cleanup: Introduce ACQUIRE() and ACQUIRE_ERR() for conditional locks

[Dan Williams]

Dan Williams wrote:
> From: Peter Zijlstra <peterz@infradead.org>
> 
> scoped_cond_guard(), automatic cleanup for conditional locks, has a couple
> pain points:
> 
> * It causes existing straight-line code to be re-indented into a new
>   bracketed scope. While this can be mitigated by a new helper function
>   to contain the scope, that is not always a comfortable conversion.
> 
> * The return code from the conditional lock is tossed in favor of a scheme
>   to pass a 'return err;' statement to the macro.
> 
> Other attempts to clean this up, to behave more like guard() [1], got hung
> up trying to both establish and evaluate the conditional lock in one
> statement.
> 
> ACQUIRE() solves this by reflecting the result of the condition in the
> automatic variable established by the lock CLASS(). The result is
> separately retrieved with the ACQUIRE_ERR() helper, effectively a PTR_ERR()
> operation.
> 
> Link: http://lore.kernel.org/all/Z1LBnX9TpZLR5Dkf@gmail.com [1]
> Link: http://patch.msgid.link/20250512105026.GP4439@noisy.programming.kicks-ass.net
> Link: http://patch.msgid.link/20250512185817.GA1808@noisy.programming.kicks-ass.net
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: David Lechner <dlechner@baylibre.com>
> Cc: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>
> Not-yet-signed-off-by: Peter Zijlstra <peterz@infradead.org>
> [djbw: wrap Peter's proposal with changelog and comments]
> Co-developed-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

With Peter's sign-off, pushed to for-6.17/cleanup-acquire as a stable
commit that others can reference:

https://git.kernel.org/pub/scm/linux/kernel/git/cxl/cxl.git/log/?h=for-6.17/cleanup-acquire

Re: [PATCH v2 1/8] cleanup: Introduce ACQUIRE() and ACQUIRE_ERR() for conditional locks

[Jonathan Cameron]

On Wed, 18 Jun 2025 22:04:09 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> From: Peter Zijlstra <peterz@infradead.org>
> 
> scoped_cond_guard(), automatic cleanup for conditional locks, has a couple
> pain points:
> 
> * It causes existing straight-line code to be re-indented into a new
>   bracketed scope. While this can be mitigated by a new helper function
>   to contain the scope, that is not always a comfortable conversion.
> 
> * The return code from the conditional lock is tossed in favor of a scheme
>   to pass a 'return err;' statement to the macro.
> 
> Other attempts to clean this up, to behave more like guard() [1], got hung
> up trying to both establish and evaluate the conditional lock in one
> statement.
> 
> ACQUIRE() solves this by reflecting the result of the condition in the
> automatic variable established by the lock CLASS(). The result is
> separately retrieved with the ACQUIRE_ERR() helper, effectively a PTR_ERR()
> operation.
> 
> Link: http://lore.kernel.org/all/Z1LBnX9TpZLR5Dkf@gmail.com [1]
> Link: http://patch.msgid.link/20250512105026.GP4439@noisy.programming.kicks-ass.net
> Link: http://patch.msgid.link/20250512185817.GA1808@noisy.programming.kicks-ass.net
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: David Lechner <dlechner@baylibre.com>
> Cc: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>
> Not-yet-signed-off-by: Peter Zijlstra <peterz@infradead.org>
> [djbw: wrap Peter's proposal with changelog and comments]
> Co-developed-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
This looks like a nice solution.  One trivial style thing inline.

> ---
>  include/linux/cleanup.h | 77 ++++++++++++++++++++++++++++++++++-------
>  include/linux/mutex.h   |  2 +-
>  include/linux/rwsem.h   |  2 +-
>  3 files changed, 67 insertions(+), 14 deletions(-)
> 
> diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
> index 7093e1d08af0..1e1eb35cc225 100644
> --- a/include/linux/cleanup.h
> +++ b/include/linux/cleanup.h
> +#define __GUARD_IS_ERR(_ptr) \
> +	({ unsigned long _rc = (__force unsigned long)(_ptr); \
> +	   unlikely((_rc-1) >= -MAX_ERRNO-1); })

Trivial but I'd have added spaces to make this
	   unlikely((_rc - 1) >= -MAX_ERRNO - 1); })

> +
>  #define __DEFINE_GUARD_LOCK_PTR(_name, _exp) \
>  	static inline void * class_##_name##_lock_ptr(class_##_name##_t *_T) \
> -	{ return (void *)(__force unsigned long)*(_exp); }
> +	{ void *_ptr = (void *)(__force unsigned long)*(_exp); \
> +	  if (IS_ERR(_ptr)) { _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; }
>  

Re: [PATCH v2 1/8] cleanup: Introduce ACQUIRE() and ACQUIRE_ERR() for conditional locks

[dan.j.williams]

Jonathan Cameron wrote:
> On Wed, 18 Jun 2025 22:04:09 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > From: Peter Zijlstra <peterz@infradead.org>
> > 
> > scoped_cond_guard(), automatic cleanup for conditional locks, has a couple
> > pain points:
> > 
> > * It causes existing straight-line code to be re-indented into a new
> >   bracketed scope. While this can be mitigated by a new helper function
> >   to contain the scope, that is not always a comfortable conversion.
> > 
> > * The return code from the conditional lock is tossed in favor of a scheme
> >   to pass a 'return err;' statement to the macro.
> > 
> > Other attempts to clean this up, to behave more like guard() [1], got hung
> > up trying to both establish and evaluate the conditional lock in one
> > statement.
> > 
> > ACQUIRE() solves this by reflecting the result of the condition in the
> > automatic variable established by the lock CLASS(). The result is
> > separately retrieved with the ACQUIRE_ERR() helper, effectively a PTR_ERR()
> > operation.
> > 
> > Link: http://lore.kernel.org/all/Z1LBnX9TpZLR5Dkf@gmail.com [1]
> > Link: http://patch.msgid.link/20250512105026.GP4439@noisy.programming.kicks-ass.net
> > Link: http://patch.msgid.link/20250512185817.GA1808@noisy.programming.kicks-ass.net
> > Cc: Ingo Molnar <mingo@kernel.org>
> > Cc: Linus Torvalds <torvalds@linux-foundation.org>
> > Cc: David Lechner <dlechner@baylibre.com>
> > Cc: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>
> > Not-yet-signed-off-by: Peter Zijlstra <peterz@infradead.org>
> > [djbw: wrap Peter's proposal with changelog and comments]
> > Co-developed-by: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> This looks like a nice solution.  One trivial style thing inline.
> 
> > ---
> >  include/linux/cleanup.h | 77 ++++++++++++++++++++++++++++++++++-------
> >  include/linux/mutex.h   |  2 +-
> >  include/linux/rwsem.h   |  2 +-
> >  3 files changed, 67 insertions(+), 14 deletions(-)
> > 
> > diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
> > index 7093e1d08af0..1e1eb35cc225 100644
> > --- a/include/linux/cleanup.h
> > +++ b/include/linux/cleanup.h
> > +#define __GUARD_IS_ERR(_ptr) \
> > +	({ unsigned long _rc = (__force unsigned long)(_ptr); \
> > +	   unlikely((_rc-1) >= -MAX_ERRNO-1); })
> 
> Trivial but I'd have added spaces to make this
> 	   unlikely((_rc - 1) >= -MAX_ERRNO - 1); })

Looks like even though I pushed out a stable commit with this nobody has
pulled this into linux-next so I can fold in this fixup and send out a
new version.

> 
> > +
> >  #define __DEFINE_GUARD_LOCK_PTR(_name, _exp) \
> >  	static inline void * class_##_name##_lock_ptr(class_##_name##_t *_T) \
> > -	{ return (void *)(__force unsigned long)*(_exp); }
> > +	{ void *_ptr = (void *)(__force unsigned long)*(_exp); \
> > +	  if (IS_ERR(_ptr)) { _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; }

I will also take the opportunity to let clang-format add more whitespace
to this to make it more readable:

-#define __DEFINE_GUARD_LOCK_PTR(_name, _exp) \
-       static inline void * class_##_name##_lock_ptr(class_##_name##_t *_T) \
-       { return (void *)(__force unsigned long)*(_exp); }
+#define __GUARD_IS_ERR(_ptr)                                       \
+       ({                                                         \
+               unsigned long _rc = (__force unsigned long)(_ptr); \
+               unlikely((_rc - 1) >= -MAX_ERRNO - 1);             \
+       })
+
+#define __DEFINE_GUARD_LOCK_PTR(_name, _exp)                                \
+       static inline void *class_##_name##_lock_ptr(class_##_name##_t *_T) \
+       {                                                                   \
+               void *_ptr = (void *)(__force unsigned long)*(_exp);        \
+               if (IS_ERR(_ptr)) {                                         \
+                       _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;                                                 \
+       }

[PATCH v2 2/8] cxl/mbox: Convert poison list mutex to ACQUIRE()

[Dan Williams]

Towards removing all explicit unlock calls in the CXL subsystem, convert
the conditional poison list mutex to use a conditional lock guard.

Rename the lock to have the compiler validate that all existing call sites
are converted.

Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
Cc: Dave Jiang <dave.jiang@intel.com>
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Vishal Verma <vishal.l.verma@intel.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/mbox.c | 7 +++----
 drivers/cxl/cxlmem.h    | 4 ++--
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 2689e6453c5a..81b21effe8cf 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -1401,8 +1401,8 @@ int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
 	int nr_records = 0;
 	int rc;
 
-	rc = mutex_lock_interruptible(&mds->poison.lock);
-	if (rc)
+	ACQUIRE(mutex_intr, lock)(&mds->poison.mutex);
+	if ((rc = ACQUIRE_ERR(mutex_intr, &lock)))
 		return rc;
 
 	po = mds->poison.list_out;
@@ -1437,7 +1437,6 @@ int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
 		}
 	} while (po->flags & CXL_POISON_FLAG_MORE);
 
-	mutex_unlock(&mds->poison.lock);
 	return rc;
 }
 EXPORT_SYMBOL_NS_GPL(cxl_mem_get_poison, "CXL");
@@ -1473,7 +1472,7 @@ int cxl_poison_state_init(struct cxl_memdev_state *mds)
 		return rc;
 	}
 
-	mutex_init(&mds->poison.lock);
+	mutex_init(&mds->poison.mutex);
 	return 0;
 }
 EXPORT_SYMBOL_NS_GPL(cxl_poison_state_init, "CXL");
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 551b0ba2caa1..f5b20641e57c 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -254,7 +254,7 @@ enum security_cmd_enabled_bits {
  * @max_errors: Maximum media error records held in device cache
  * @enabled_cmds: All poison commands enabled in the CEL
  * @list_out: The poison list payload returned by device
- * @lock: Protect reads of the poison list
+ * @mutex: Protect reads of the poison list
  *
  * Reads of the poison list are synchronized to ensure that a reader
  * does not get an incomplete list because their request overlapped
@@ -265,7 +265,7 @@ struct cxl_poison_state {
 	u32 max_errors;
 	DECLARE_BITMAP(enabled_cmds, CXL_POISON_ENABLED_MAX);
 	struct cxl_mbox_poison_out *list_out;
-	struct mutex lock;  /* Protect reads of poison list */
+	struct mutex mutex;  /* Protect reads of poison list */
 };
 
 /*
-- 
2.49.0

Re: [PATCH v2 2/8] cxl/mbox: Convert poison list mutex to ACQUIRE()

[Alison Schofield]

On Wed, Jun 18, 2025 at 10:04:10PM -0700, Dan Williams wrote:
> Towards removing all explicit unlock calls in the CXL subsystem, convert
> the conditional poison list mutex to use a conditional lock guard.
> 
> Rename the lock to have the compiler validate that all existing call sites
> are converted.

Reviewed-by: Alison Schofield <alison.schofield@intel.com>

Re: [PATCH v2 2/8] cxl/mbox: Convert poison list mutex to ACQUIRE()

[Jonathan Cameron]

On Wed, 18 Jun 2025 22:04:10 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Towards removing all explicit unlock calls in the CXL subsystem, convert
> the conditional poison list mutex to use a conditional lock guard.
> 
> Rename the lock to have the compiler validate that all existing call sites
> are converted.
> 
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Davidlohr Bueso <dave@stgolabs.net>
> Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Alison Schofield <alison.schofield@intel.com>
> Cc: Vishal Verma <vishal.l.verma@intel.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

One trivial inline. Either way

Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

> ---
>  drivers/cxl/core/mbox.c | 7 +++----
>  drivers/cxl/cxlmem.h    | 4 ++--
>  2 files changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 2689e6453c5a..81b21effe8cf 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -1401,8 +1401,8 @@ int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
>  	int nr_records = 0;
>  	int rc;
>  
> -	rc = mutex_lock_interruptible(&mds->poison.lock);
> -	if (rc)
> +	ACQUIRE(mutex_intr, lock)(&mds->poison.mutex);

I'd slightly prefer the 'canonical' style from the cleanup.h docs in previous patch.

	rc = ACQUIRE_ERR(mutex_intr, &lock);
	if (rc)
		return rc;

> +	if ((rc = ACQUIRE_ERR(mutex_intr, &lock)))
>  		return rc;
>  
>  	po = mds->poison.list_out;
> @@ -1437,7 +1437,6 @@ int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
>  		}
>  	} while (po->flags & CXL_POISON_FLAG_MORE);
>  
> -	mutex_unlock(&mds->poison.lock);
>  	return rc;
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_mem_get_poison, "CXL");
> @@ -1473,7 +1472,7 @@ int cxl_poison_state_init(struct cxl_memdev_state *mds)
>  		return rc;
>  	}
>  
> -	mutex_init(&mds->poison.lock);
> +	mutex_init(&mds->poison.mutex);
>  	return 0;
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_poison_state_init, "CXL");
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 551b0ba2caa1..f5b20641e57c 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -254,7 +254,7 @@ enum security_cmd_enabled_bits {
>   * @max_errors: Maximum media error records held in device cache
>   * @enabled_cmds: All poison commands enabled in the CEL
>   * @list_out: The poison list payload returned by device
> - * @lock: Protect reads of the poison list
> + * @mutex: Protect reads of the poison list
>   *
>   * Reads of the poison list are synchronized to ensure that a reader
>   * does not get an incomplete list because their request overlapped
> @@ -265,7 +265,7 @@ struct cxl_poison_state {
>  	u32 max_errors;
>  	DECLARE_BITMAP(enabled_cmds, CXL_POISON_ENABLED_MAX);
>  	struct cxl_mbox_poison_out *list_out;
> -	struct mutex lock;  /* Protect reads of poison list */
> +	struct mutex mutex;  /* Protect reads of poison list */
>  };
>  
>  /*

Re: [PATCH v2 2/8] cxl/mbox: Convert poison list mutex to ACQUIRE()

[dan.j.williams]

Jonathan Cameron wrote:
> On Wed, 18 Jun 2025 22:04:10 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > Towards removing all explicit unlock calls in the CXL subsystem, convert
> > the conditional poison list mutex to use a conditional lock guard.
> > 
> > Rename the lock to have the compiler validate that all existing call sites
> > are converted.
> > 
> > Cc: Linus Torvalds <torvalds@linux-foundation.org>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Davidlohr Bueso <dave@stgolabs.net>
> > Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
> > Cc: Dave Jiang <dave.jiang@intel.com>
> > Cc: Alison Schofield <alison.schofield@intel.com>
> > Cc: Vishal Verma <vishal.l.verma@intel.com>
> > Cc: Ira Weiny <ira.weiny@intel.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> 
> One trivial inline. Either way
> 
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> 
> > ---
> >  drivers/cxl/core/mbox.c | 7 +++----
> >  drivers/cxl/cxlmem.h    | 4 ++--
> >  2 files changed, 5 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> > index 2689e6453c5a..81b21effe8cf 100644
> > --- a/drivers/cxl/core/mbox.c
> > +++ b/drivers/cxl/core/mbox.c
> > @@ -1401,8 +1401,8 @@ int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
> >  	int nr_records = 0;
> >  	int rc;
> >  
> > -	rc = mutex_lock_interruptible(&mds->poison.lock);
> > -	if (rc)
> > +	ACQUIRE(mutex_intr, lock)(&mds->poison.mutex);
> 
> I'd slightly prefer the 'canonical' style from the cleanup.h docs in previous patch.
> 
> 	rc = ACQUIRE_ERR(mutex_intr, &lock);
> 	if (rc)
> 		return rc;

I took a pass at that conversion before sending this out, but came back
to the inline assignment approach because the compactness matched the
familiar pattern with other error pointer handling.

The comfortable pattern with ERR_PTR() is:

	if (IS_ERR())
		return PTR_ERR();

The same could be done here with something like:

	if (ACQUIRE_ERR(@class, @lock))
		return ACQUIRE_ERR(@class, @lock))

...but that also feels like a mouthful especially if there is already a
local return code variable that can be used. So,

	if ((ret = ACQUIRE_ERR(@class, @lock))
		return ret;

...feel like a reasonable compromise for compactness on a common
pattern. Especially if cleanup helpers are also generally accepted as a
reason to bend the "variables declared at the top of scope" convention.

Re: [PATCH v2 2/8] cxl/mbox: Convert poison list mutex to ACQUIRE()

[Dave Jiang]

On 6/18/25 10:04 PM, Dan Williams wrote:
> Towards removing all explicit unlock calls in the CXL subsystem, convert
> the conditional poison list mutex to use a conditional lock guard.
> 
> Rename the lock to have the compiler validate that all existing call sites
> are converted.
> 
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Davidlohr Bueso <dave@stgolabs.net>
> Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Alison Schofield <alison.schofield@intel.com>
> Cc: Vishal Verma <vishal.l.verma@intel.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/core/mbox.c | 7 +++----
>  drivers/cxl/cxlmem.h    | 4 ++--
>  2 files changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 2689e6453c5a..81b21effe8cf 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -1401,8 +1401,8 @@ int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
>  	int nr_records = 0;
>  	int rc;
>  
> -	rc = mutex_lock_interruptible(&mds->poison.lock);
> -	if (rc)
> +	ACQUIRE(mutex_intr, lock)(&mds->poison.mutex);
> +	if ((rc = ACQUIRE_ERR(mutex_intr, &lock)))
>  		return rc;
>  
>  	po = mds->poison.list_out;
> @@ -1437,7 +1437,6 @@ int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
>  		}
>  	} while (po->flags & CXL_POISON_FLAG_MORE);
>  
> -	mutex_unlock(&mds->poison.lock);
>  	return rc;
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_mem_get_poison, "CXL");
> @@ -1473,7 +1472,7 @@ int cxl_poison_state_init(struct cxl_memdev_state *mds)
>  		return rc;
>  	}
>  
> -	mutex_init(&mds->poison.lock);
> +	mutex_init(&mds->poison.mutex);
>  	return 0;
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_poison_state_init, "CXL");
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 551b0ba2caa1..f5b20641e57c 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -254,7 +254,7 @@ enum security_cmd_enabled_bits {
>   * @max_errors: Maximum media error records held in device cache
>   * @enabled_cmds: All poison commands enabled in the CEL
>   * @list_out: The poison list payload returned by device
> - * @lock: Protect reads of the poison list
> + * @mutex: Protect reads of the poison list
>   *
>   * Reads of the poison list are synchronized to ensure that a reader
>   * does not get an incomplete list because their request overlapped
> @@ -265,7 +265,7 @@ struct cxl_poison_state {
>  	u32 max_errors;
>  	DECLARE_BITMAP(enabled_cmds, CXL_POISON_ENABLED_MAX);
>  	struct cxl_mbox_poison_out *list_out;
> -	struct mutex lock;  /* Protect reads of poison list */
> +	struct mutex mutex;  /* Protect reads of poison list */
>  };
>  
>  /*

[PATCH v2 3/8] cxl/decoder: Move decoder register programming to a helper

[Dan Williams]

In preparation for converting to rw_semaphore_acquire semantics move the
contents of an open-coded {down,up}_read(&cxl_dpa_rwsem) section to a
helper function.

Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
Cc: Dave Jiang <dave.jiang@intel.com>
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Vishal Verma <vishal.l.verma@intel.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/hdm.c | 77 +++++++++++++++++++++++-------------------
 1 file changed, 42 insertions(+), 35 deletions(-)

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index ab1007495f6b..81556d12e9b8 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -764,14 +764,53 @@ static int cxld_await_commit(void __iomem *hdm, int id)
 	return -ETIMEDOUT;
 }
 
+static void setup_hw_decoder(struct cxl_decoder *cxld, void __iomem *hdm)
+{
+	int id = cxld->id;
+	u64 base, size;
+	u32 ctrl;
+
+	/* common decoder settings */
+	ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
+	cxld_set_interleave(cxld, &ctrl);
+	cxld_set_type(cxld, &ctrl);
+	base = cxld->hpa_range.start;
+	size = range_len(&cxld->hpa_range);
+
+	writel(upper_32_bits(base), hdm + CXL_HDM_DECODER0_BASE_HIGH_OFFSET(id));
+	writel(lower_32_bits(base), hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(id));
+	writel(upper_32_bits(size), hdm + CXL_HDM_DECODER0_SIZE_HIGH_OFFSET(id));
+	writel(lower_32_bits(size), hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(id));
+
+	if (is_switch_decoder(&cxld->dev)) {
+		struct cxl_switch_decoder *cxlsd =
+			to_cxl_switch_decoder(&cxld->dev);
+		void __iomem *tl_hi = hdm + CXL_HDM_DECODER0_TL_HIGH(id);
+		void __iomem *tl_lo = hdm + CXL_HDM_DECODER0_TL_LOW(id);
+		u64 targets;
+
+		cxlsd_set_targets(cxlsd, &targets);
+		writel(upper_32_bits(targets), tl_hi);
+		writel(lower_32_bits(targets), tl_lo);
+	} else {
+		struct cxl_endpoint_decoder *cxled =
+			to_cxl_endpoint_decoder(&cxld->dev);
+		void __iomem *sk_hi = hdm + CXL_HDM_DECODER0_SKIP_HIGH(id);
+		void __iomem *sk_lo = hdm + CXL_HDM_DECODER0_SKIP_LOW(id);
+
+		writel(upper_32_bits(cxled->skip), sk_hi);
+		writel(lower_32_bits(cxled->skip), sk_lo);
+	}
+
+	writel(ctrl, hdm + CXL_HDM_DECODER0_CTRL_OFFSET(id));
+}
+
 static int cxl_decoder_commit(struct cxl_decoder *cxld)
 {
 	struct cxl_port *port = to_cxl_port(cxld->dev.parent);
 	struct cxl_hdm *cxlhdm = dev_get_drvdata(&port->dev);
 	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
 	int id = cxld->id, rc;
-	u64 base, size;
-	u32 ctrl;
 
 	if (cxld->flags & CXL_DECODER_F_ENABLE)
 		return 0;
@@ -804,39 +843,7 @@ static int cxl_decoder_commit(struct cxl_decoder *cxld)
 	}
 
 	down_read(&cxl_dpa_rwsem);
-	/* common decoder settings */
-	ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
-	cxld_set_interleave(cxld, &ctrl);
-	cxld_set_type(cxld, &ctrl);
-	base = cxld->hpa_range.start;
-	size = range_len(&cxld->hpa_range);
-
-	writel(upper_32_bits(base), hdm + CXL_HDM_DECODER0_BASE_HIGH_OFFSET(id));
-	writel(lower_32_bits(base), hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(id));
-	writel(upper_32_bits(size), hdm + CXL_HDM_DECODER0_SIZE_HIGH_OFFSET(id));
-	writel(lower_32_bits(size), hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(id));
-
-	if (is_switch_decoder(&cxld->dev)) {
-		struct cxl_switch_decoder *cxlsd =
-			to_cxl_switch_decoder(&cxld->dev);
-		void __iomem *tl_hi = hdm + CXL_HDM_DECODER0_TL_HIGH(id);
-		void __iomem *tl_lo = hdm + CXL_HDM_DECODER0_TL_LOW(id);
-		u64 targets;
-
-		cxlsd_set_targets(cxlsd, &targets);
-		writel(upper_32_bits(targets), tl_hi);
-		writel(lower_32_bits(targets), tl_lo);
-	} else {
-		struct cxl_endpoint_decoder *cxled =
-			to_cxl_endpoint_decoder(&cxld->dev);
-		void __iomem *sk_hi = hdm + CXL_HDM_DECODER0_SKIP_HIGH(id);
-		void __iomem *sk_lo = hdm + CXL_HDM_DECODER0_SKIP_LOW(id);
-
-		writel(upper_32_bits(cxled->skip), sk_hi);
-		writel(lower_32_bits(cxled->skip), sk_lo);
-	}
-
-	writel(ctrl, hdm + CXL_HDM_DECODER0_CTRL_OFFSET(id));
+	setup_hw_decoder(cxld, hdm);
 	up_read(&cxl_dpa_rwsem);
 
 	port->commit_end++;
-- 
2.49.0

Re: [PATCH v2 3/8] cxl/decoder: Move decoder register programming to a helper

[Alison Schofield]

On Wed, Jun 18, 2025 at 10:04:11PM -0700, Dan Williams wrote:
> In preparation for converting to rw_semaphore_acquire semantics move the
> contents of an open-coded {down,up}_read(&cxl_dpa_rwsem) section to a
> helper function.
> 

Reviewed-by: Alison Schofield <alison.schofield@intel.com>

Re: [PATCH v2 3/8] cxl/decoder: Move decoder register programming to a helper

[Jonathan Cameron]

On Wed, 18 Jun 2025 22:04:11 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> In preparation for converting to rw_semaphore_acquire semantics move the
> contents of an open-coded {down,up}_read(&cxl_dpa_rwsem) section to a
> helper function.
> 
> Cc: Davidlohr Bueso <dave@stgolabs.net>
> Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Alison Schofield <alison.schofield@intel.com>
> Cc: Vishal Verma <vishal.l.verma@intel.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Seems reasonable.

Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

> ---
>  drivers/cxl/core/hdm.c | 77 +++++++++++++++++++++++-------------------
>  1 file changed, 42 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index ab1007495f6b..81556d12e9b8 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -764,14 +764,53 @@ static int cxld_await_commit(void __iomem *hdm, int id)
>  	return -ETIMEDOUT;
>  }
>  
> +static void setup_hw_decoder(struct cxl_decoder *cxld, void __iomem *hdm)
> +{
> +	int id = cxld->id;
> +	u64 base, size;
> +	u32 ctrl;
> +
> +	/* common decoder settings */
> +	ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
> +	cxld_set_interleave(cxld, &ctrl);
> +	cxld_set_type(cxld, &ctrl);
> +	base = cxld->hpa_range.start;
> +	size = range_len(&cxld->hpa_range);
> +
> +	writel(upper_32_bits(base), hdm + CXL_HDM_DECODER0_BASE_HIGH_OFFSET(id));
> +	writel(lower_32_bits(base), hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(id));
> +	writel(upper_32_bits(size), hdm + CXL_HDM_DECODER0_SIZE_HIGH_OFFSET(id));
> +	writel(lower_32_bits(size), hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(id));
> +
> +	if (is_switch_decoder(&cxld->dev)) {
> +		struct cxl_switch_decoder *cxlsd =
> +			to_cxl_switch_decoder(&cxld->dev);
> +		void __iomem *tl_hi = hdm + CXL_HDM_DECODER0_TL_HIGH(id);
> +		void __iomem *tl_lo = hdm + CXL_HDM_DECODER0_TL_LOW(id);
> +		u64 targets;
> +
> +		cxlsd_set_targets(cxlsd, &targets);
> +		writel(upper_32_bits(targets), tl_hi);
> +		writel(lower_32_bits(targets), tl_lo);
> +	} else {
> +		struct cxl_endpoint_decoder *cxled =
> +			to_cxl_endpoint_decoder(&cxld->dev);
> +		void __iomem *sk_hi = hdm + CXL_HDM_DECODER0_SKIP_HIGH(id);
> +		void __iomem *sk_lo = hdm + CXL_HDM_DECODER0_SKIP_LOW(id);
> +
> +		writel(upper_32_bits(cxled->skip), sk_hi);
> +		writel(lower_32_bits(cxled->skip), sk_lo);
> +	}
> +
> +	writel(ctrl, hdm + CXL_HDM_DECODER0_CTRL_OFFSET(id));
> +}
> +
>  static int cxl_decoder_commit(struct cxl_decoder *cxld)
>  {
>  	struct cxl_port *port = to_cxl_port(cxld->dev.parent);
>  	struct cxl_hdm *cxlhdm = dev_get_drvdata(&port->dev);
>  	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
>  	int id = cxld->id, rc;
> -	u64 base, size;
> -	u32 ctrl;
>  
>  	if (cxld->flags & CXL_DECODER_F_ENABLE)
>  		return 0;
> @@ -804,39 +843,7 @@ static int cxl_decoder_commit(struct cxl_decoder *cxld)
>  	}
>  
>  	down_read(&cxl_dpa_rwsem);
> -	/* common decoder settings */
> -	ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
> -	cxld_set_interleave(cxld, &ctrl);
> -	cxld_set_type(cxld, &ctrl);
> -	base = cxld->hpa_range.start;
> -	size = range_len(&cxld->hpa_range);
> -
> -	writel(upper_32_bits(base), hdm + CXL_HDM_DECODER0_BASE_HIGH_OFFSET(id));
> -	writel(lower_32_bits(base), hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(id));
> -	writel(upper_32_bits(size), hdm + CXL_HDM_DECODER0_SIZE_HIGH_OFFSET(id));
> -	writel(lower_32_bits(size), hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(id));
> -
> -	if (is_switch_decoder(&cxld->dev)) {
> -		struct cxl_switch_decoder *cxlsd =
> -			to_cxl_switch_decoder(&cxld->dev);
> -		void __iomem *tl_hi = hdm + CXL_HDM_DECODER0_TL_HIGH(id);
> -		void __iomem *tl_lo = hdm + CXL_HDM_DECODER0_TL_LOW(id);
> -		u64 targets;
> -
> -		cxlsd_set_targets(cxlsd, &targets);
> -		writel(upper_32_bits(targets), tl_hi);
> -		writel(lower_32_bits(targets), tl_lo);
> -	} else {
> -		struct cxl_endpoint_decoder *cxled =
> -			to_cxl_endpoint_decoder(&cxld->dev);
> -		void __iomem *sk_hi = hdm + CXL_HDM_DECODER0_SKIP_HIGH(id);
> -		void __iomem *sk_lo = hdm + CXL_HDM_DECODER0_SKIP_LOW(id);
> -
> -		writel(upper_32_bits(cxled->skip), sk_hi);
> -		writel(lower_32_bits(cxled->skip), sk_lo);
> -	}
> -
> -	writel(ctrl, hdm + CXL_HDM_DECODER0_CTRL_OFFSET(id));
> +	setup_hw_decoder(cxld, hdm);
>  	up_read(&cxl_dpa_rwsem);
>  
>  	port->commit_end++;

Re: [PATCH v2 3/8] cxl/decoder: Move decoder register programming to a helper

[Dave Jiang]

On 6/18/25 10:04 PM, Dan Williams wrote:
> In preparation for converting to rw_semaphore_acquire semantics move the
> contents of an open-coded {down,up}_read(&cxl_dpa_rwsem) section to a
> helper function.
> 
> Cc: Davidlohr Bueso <dave@stgolabs.net>
> Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Alison Schofield <alison.schofield@intel.com>
> Cc: Vishal Verma <vishal.l.verma@intel.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/core/hdm.c | 77 +++++++++++++++++++++++-------------------
>  1 file changed, 42 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index ab1007495f6b..81556d12e9b8 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -764,14 +764,53 @@ static int cxld_await_commit(void __iomem *hdm, int id)
>  	return -ETIMEDOUT;
>  }
>  
> +static void setup_hw_decoder(struct cxl_decoder *cxld, void __iomem *hdm)
> +{
> +	int id = cxld->id;
> +	u64 base, size;
> +	u32 ctrl;
> +
> +	/* common decoder settings */
> +	ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
> +	cxld_set_interleave(cxld, &ctrl);
> +	cxld_set_type(cxld, &ctrl);
> +	base = cxld->hpa_range.start;
> +	size = range_len(&cxld->hpa_range);
> +
> +	writel(upper_32_bits(base), hdm + CXL_HDM_DECODER0_BASE_HIGH_OFFSET(id));
> +	writel(lower_32_bits(base), hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(id));
> +	writel(upper_32_bits(size), hdm + CXL_HDM_DECODER0_SIZE_HIGH_OFFSET(id));
> +	writel(lower_32_bits(size), hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(id));
> +
> +	if (is_switch_decoder(&cxld->dev)) {
> +		struct cxl_switch_decoder *cxlsd =
> +			to_cxl_switch_decoder(&cxld->dev);
> +		void __iomem *tl_hi = hdm + CXL_HDM_DECODER0_TL_HIGH(id);
> +		void __iomem *tl_lo = hdm + CXL_HDM_DECODER0_TL_LOW(id);
> +		u64 targets;
> +
> +		cxlsd_set_targets(cxlsd, &targets);
> +		writel(upper_32_bits(targets), tl_hi);
> +		writel(lower_32_bits(targets), tl_lo);
> +	} else {
> +		struct cxl_endpoint_decoder *cxled =
> +			to_cxl_endpoint_decoder(&cxld->dev);
> +		void __iomem *sk_hi = hdm + CXL_HDM_DECODER0_SKIP_HIGH(id);
> +		void __iomem *sk_lo = hdm + CXL_HDM_DECODER0_SKIP_LOW(id);
> +
> +		writel(upper_32_bits(cxled->skip), sk_hi);
> +		writel(lower_32_bits(cxled->skip), sk_lo);
> +	}
> +
> +	writel(ctrl, hdm + CXL_HDM_DECODER0_CTRL_OFFSET(id));
> +}
> +
>  static int cxl_decoder_commit(struct cxl_decoder *cxld)
>  {
>  	struct cxl_port *port = to_cxl_port(cxld->dev.parent);
>  	struct cxl_hdm *cxlhdm = dev_get_drvdata(&port->dev);
>  	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
>  	int id = cxld->id, rc;
> -	u64 base, size;
> -	u32 ctrl;
>  
>  	if (cxld->flags & CXL_DECODER_F_ENABLE)
>  		return 0;
> @@ -804,39 +843,7 @@ static int cxl_decoder_commit(struct cxl_decoder *cxld)
>  	}
>  
>  	down_read(&cxl_dpa_rwsem);
> -	/* common decoder settings */
> -	ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(cxld->id));
> -	cxld_set_interleave(cxld, &ctrl);
> -	cxld_set_type(cxld, &ctrl);
> -	base = cxld->hpa_range.start;
> -	size = range_len(&cxld->hpa_range);
> -
> -	writel(upper_32_bits(base), hdm + CXL_HDM_DECODER0_BASE_HIGH_OFFSET(id));
> -	writel(lower_32_bits(base), hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(id));
> -	writel(upper_32_bits(size), hdm + CXL_HDM_DECODER0_SIZE_HIGH_OFFSET(id));
> -	writel(lower_32_bits(size), hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(id));
> -
> -	if (is_switch_decoder(&cxld->dev)) {
> -		struct cxl_switch_decoder *cxlsd =
> -			to_cxl_switch_decoder(&cxld->dev);
> -		void __iomem *tl_hi = hdm + CXL_HDM_DECODER0_TL_HIGH(id);
> -		void __iomem *tl_lo = hdm + CXL_HDM_DECODER0_TL_LOW(id);
> -		u64 targets;
> -
> -		cxlsd_set_targets(cxlsd, &targets);
> -		writel(upper_32_bits(targets), tl_hi);
> -		writel(lower_32_bits(targets), tl_lo);
> -	} else {
> -		struct cxl_endpoint_decoder *cxled =
> -			to_cxl_endpoint_decoder(&cxld->dev);
> -		void __iomem *sk_hi = hdm + CXL_HDM_DECODER0_SKIP_HIGH(id);
> -		void __iomem *sk_lo = hdm + CXL_HDM_DECODER0_SKIP_LOW(id);
> -
> -		writel(upper_32_bits(cxled->skip), sk_hi);
> -		writel(lower_32_bits(cxled->skip), sk_lo);
> -	}
> -
> -	writel(ctrl, hdm + CXL_HDM_DECODER0_CTRL_OFFSET(id));
> +	setup_hw_decoder(cxld, hdm);
>  	up_read(&cxl_dpa_rwsem);
>  
>  	port->commit_end++;

[PATCH v2 4/8] cxl/decoder: Drop pointless locking

[Dan Williams]

cxl_dpa_rwsem coordinates changes to dpa allocation settings for a given
decoder. cxl_decoder_reset() has no need for a consistent snapshot of the
dpa settings since it is merely clearing out whatever was there previously.

Otherwise, cxl_region_rwsem protects against 'reset' racing 'setup'.

In preparationg for converting to rw_semaphore_acquire semantics, drop this
locking.

Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
Cc: Dave Jiang <dave.jiang@intel.com>
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Vishal Verma <vishal.l.verma@intel.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/hdm.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 81556d12e9b8..e9cb34e30248 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -914,7 +914,6 @@ static void cxl_decoder_reset(struct cxl_decoder *cxld)
 			"%s: out of order reset, expected decoder%d.%d\n",
 			dev_name(&cxld->dev), port->id, port->commit_end);
 
-	down_read(&cxl_dpa_rwsem);
 	ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(id));
 	ctrl &= ~CXL_HDM_DECODER0_CTRL_COMMIT;
 	writel(ctrl, hdm + CXL_HDM_DECODER0_CTRL_OFFSET(id));
@@ -923,7 +922,6 @@ static void cxl_decoder_reset(struct cxl_decoder *cxld)
 	writel(0, hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(id));
 	writel(0, hdm + CXL_HDM_DECODER0_BASE_HIGH_OFFSET(id));
 	writel(0, hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(id));
-	up_read(&cxl_dpa_rwsem);
 
 	cxld->flags &= ~CXL_DECODER_F_ENABLE;
 
-- 
2.49.0

Re: [PATCH v2 4/8] cxl/decoder: Drop pointless locking

[Davidlohr Bueso]

On Wed, 18 Jun 2025, Dan Williams wrote:

>cxl_dpa_rwsem coordinates changes to dpa allocation settings for a given
>decoder. cxl_decoder_reset() has no need for a consistent snapshot of the
>dpa settings since it is merely clearing out whatever was there previously.
>
>Otherwise, cxl_region_rwsem protects against 'reset' racing 'setup'.
>
>In preparationg for converting to rw_semaphore_acquire semantics, drop this
>locking.
>
>Cc: Davidlohr Bueso <dave@stgolabs.net>
>Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
>Cc: Dave Jiang <dave.jiang@intel.com>
>Cc: Alison Schofield <alison.schofield@intel.com>
>Cc: Vishal Verma <vishal.l.verma@intel.com>
>Cc: Ira Weiny <ira.weiny@intel.com>
>Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>

Re: [PATCH v2 4/8] cxl/decoder: Drop pointless locking

[Alison Schofield]

On Wed, Jun 18, 2025 at 10:04:12PM -0700, Dan Williams wrote:
> cxl_dpa_rwsem coordinates changes to dpa allocation settings for a given
> decoder. cxl_decoder_reset() has no need for a consistent snapshot of the
> dpa settings since it is merely clearing out whatever was there previously.
> 
> Otherwise, cxl_region_rwsem protects against 'reset' racing 'setup'.
> 
> In preparationg for converting to rw_semaphore_acquire semantics, drop this
> locking.
> 

Reviewed-by: Alison Schofield <alison.schofield@intel.com>

Re: [PATCH v2 4/8] cxl/decoder: Drop pointless locking

[Jonathan Cameron]

On Wed, 18 Jun 2025 22:04:12 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> cxl_dpa_rwsem coordinates changes to dpa allocation settings for a given
> decoder. cxl_decoder_reset() has no need for a consistent snapshot of the
> dpa settings since it is merely clearing out whatever was there previously.
> 
> Otherwise, cxl_region_rwsem protects against 'reset' racing 'setup'.
> 
> In preparationg for converting to rw_semaphore_acquire semantics, drop this

typo

> locking.
> 
> Cc: Davidlohr Bueso <dave@stgolabs.net>
> Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Alison Schofield <alison.schofield@intel.com>
> Cc: Vishal Verma <vishal.l.verma@intel.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

> ---
>  drivers/cxl/core/hdm.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 81556d12e9b8..e9cb34e30248 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -914,7 +914,6 @@ static void cxl_decoder_reset(struct cxl_decoder *cxld)
>  			"%s: out of order reset, expected decoder%d.%d\n",
>  			dev_name(&cxld->dev), port->id, port->commit_end);
>  
> -	down_read(&cxl_dpa_rwsem);
>  	ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(id));
>  	ctrl &= ~CXL_HDM_DECODER0_CTRL_COMMIT;
>  	writel(ctrl, hdm + CXL_HDM_DECODER0_CTRL_OFFSET(id));
> @@ -923,7 +922,6 @@ static void cxl_decoder_reset(struct cxl_decoder *cxld)
>  	writel(0, hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(id));
>  	writel(0, hdm + CXL_HDM_DECODER0_BASE_HIGH_OFFSET(id));
>  	writel(0, hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(id));
> -	up_read(&cxl_dpa_rwsem);
>  
>  	cxld->flags &= ~CXL_DECODER_F_ENABLE;
>  

Re: [PATCH v2 4/8] cxl/decoder: Drop pointless locking

[Dave Jiang]

On 6/18/25 10:04 PM, Dan Williams wrote:
> cxl_dpa_rwsem coordinates changes to dpa allocation settings for a given
> decoder. cxl_decoder_reset() has no need for a consistent snapshot of the
> dpa settings since it is merely clearing out whatever was there previously.
> 
> Otherwise, cxl_region_rwsem protects against 'reset' racing 'setup'.
> 
> In preparationg for converting to rw_semaphore_acquire semantics, drop this
> locking.
> 
> Cc: Davidlohr Bueso <dave@stgolabs.net>
> Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Alison Schofield <alison.schofield@intel.com>
> Cc: Vishal Verma <vishal.l.verma@intel.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/core/hdm.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 81556d12e9b8..e9cb34e30248 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -914,7 +914,6 @@ static void cxl_decoder_reset(struct cxl_decoder *cxld)
>  			"%s: out of order reset, expected decoder%d.%d\n",
>  			dev_name(&cxld->dev), port->id, port->commit_end);
>  
> -	down_read(&cxl_dpa_rwsem);
>  	ctrl = readl(hdm + CXL_HDM_DECODER0_CTRL_OFFSET(id));
>  	ctrl &= ~CXL_HDM_DECODER0_CTRL_COMMIT;
>  	writel(ctrl, hdm + CXL_HDM_DECODER0_CTRL_OFFSET(id));
> @@ -923,7 +922,6 @@ static void cxl_decoder_reset(struct cxl_decoder *cxld)
>  	writel(0, hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(id));
>  	writel(0, hdm + CXL_HDM_DECODER0_BASE_HIGH_OFFSET(id));
>  	writel(0, hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(id));
> -	up_read(&cxl_dpa_rwsem);
>  
>  	cxld->flags &= ~CXL_DECODER_F_ENABLE;
>  

[PATCH v2 5/8] cxl/region: Split commit_store() into __commit() and queue_reset() helpers

[Dan Williams]

The complexity of dropping the lock is removed in favor of splitting commit
operations to a helper, and leaving all the complexities of "decommit" for
commit_store() to coordinate the different locking contexts.

The CPU cache-invalidation in the decommit path is solely handled now by
cxl_region_decode_reset(). Previously the CPU caches were being needlessly
flushed twice in the decommit path where the first flush had no guarantee
that the memory would not be immediately re-dirtied.

Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
Cc: Dave Jiang <dave.jiang@intel.com>
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Vishal Verma <vishal.l.verma@intel.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/region.c | 99 +++++++++++++++++++++++++++------------
 1 file changed, 70 insertions(+), 29 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 6e5e1460068d..3a77aec2c447 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -349,30 +349,42 @@ static int cxl_region_decode_commit(struct cxl_region *cxlr)
 	return rc;
 }
 
-static ssize_t commit_store(struct device *dev, struct device_attribute *attr,
-			    const char *buf, size_t len)
+static int queue_reset(struct cxl_region *cxlr)
 {
-	struct cxl_region *cxlr = to_cxl_region(dev);
 	struct cxl_region_params *p = &cxlr->params;
-	bool commit;
-	ssize_t rc;
+	int rc;
 
-	rc = kstrtobool(buf, &commit);
+	rc = down_write_killable(&cxl_region_rwsem);
 	if (rc)
 		return rc;
 
+	/* Already in the requested state? */
+	if (p->state < CXL_CONFIG_COMMIT)
+		goto out;
+
+	p->state = CXL_CONFIG_RESET_PENDING;
+
+out:
+	up_write(&cxl_region_rwsem);
+
+	return rc;
+}
+
+static int __commit(struct cxl_region *cxlr)
+{
+	struct cxl_region_params *p = &cxlr->params;
+	int rc;
+
 	rc = down_write_killable(&cxl_region_rwsem);
 	if (rc)
 		return rc;
 
 	/* Already in the requested state? */
-	if (commit && p->state >= CXL_CONFIG_COMMIT)
-		goto out;
-	if (!commit && p->state < CXL_CONFIG_COMMIT)
+	if (p->state >= CXL_CONFIG_COMMIT)
 		goto out;
 
 	/* Not ready to commit? */
-	if (commit && p->state < CXL_CONFIG_ACTIVE) {
+	if (p->state < CXL_CONFIG_ACTIVE) {
 		rc = -ENXIO;
 		goto out;
 	}
@@ -385,31 +397,60 @@ static ssize_t commit_store(struct device *dev, struct device_attribute *attr,
 	if (rc)
 		goto out;
 
-	if (commit) {
-		rc = cxl_region_decode_commit(cxlr);
-		if (rc == 0)
-			p->state = CXL_CONFIG_COMMIT;
-	} else {
-		p->state = CXL_CONFIG_RESET_PENDING;
-		up_write(&cxl_region_rwsem);
-		device_release_driver(&cxlr->dev);
-		down_write(&cxl_region_rwsem);
-
-		/*
-		 * The lock was dropped, so need to revalidate that the reset is
-		 * still pending.
-		 */
-		if (p->state == CXL_CONFIG_RESET_PENDING) {
-			cxl_region_decode_reset(cxlr, p->interleave_ways);
-			p->state = CXL_CONFIG_ACTIVE;
-		}
-	}
+	rc = cxl_region_decode_commit(cxlr);
+	if (rc == 0)
+		p->state = CXL_CONFIG_COMMIT;
 
 out:
 	up_write(&cxl_region_rwsem);
 
+	return rc;
+}
+
+static ssize_t commit_store(struct device *dev, struct device_attribute *attr,
+			    const char *buf, size_t len)
+{
+	struct cxl_region *cxlr = to_cxl_region(dev);
+	struct cxl_region_params *p = &cxlr->params;
+	bool commit;
+	ssize_t rc;
+
+	rc = kstrtobool(buf, &commit);
 	if (rc)
 		return rc;
+
+	if (commit) {
+		rc = __commit(cxlr);
+		if (rc)
+			return rc;
+		return len;
+	}
+
+	rc = queue_reset(cxlr);
+	if (rc)
+		return rc;
+
+	/*
+	 * Unmap the region and depend the reset-pending state to ensure
+	 * it does not go active again until post reset
+	 */
+	device_release_driver(&cxlr->dev);
+
+	/*
+	 * With the reset pending take cxl_region_rwsem unconditionally
+	 * to ensure the reset gets handled before returning.
+	 */
+	guard(rwsem_write)(&cxl_region_rwsem);
+
+	/*
+	 * Revalidate that the reset is still pending in case another
+	 * thread already handled this reset.
+	 */
+	if (p->state == CXL_CONFIG_RESET_PENDING) {
+		cxl_region_decode_reset(cxlr, p->interleave_ways);
+		p->state = CXL_CONFIG_ACTIVE;
+	}
+
 	return len;
 }
 
-- 
2.49.0

Re: [PATCH v2 5/8] cxl/region: Split commit_store() into __commit() and queue_reset() helpers

[Alison Schofield]

On Wed, Jun 18, 2025 at 10:04:13PM -0700, Dan Williams wrote:
> The complexity of dropping the lock is removed in favor of splitting commit
> operations to a helper, and leaving all the complexities of "decommit" for
> commit_store() to coordinate the different locking contexts.
> 
> The CPU cache-invalidation in the decommit path is solely handled now by
> cxl_region_decode_reset(). Previously the CPU caches were being needlessly
> flushed twice in the decommit path where the first flush had no guarantee
> that the memory would not be immediately re-dirtied.
> 
> Cc: Davidlohr Bueso <dave@stgolabs.net>
> Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Alison Schofield <alison.schofield@intel.com>
> Cc: Vishal Verma <vishal.l.verma@intel.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/cxl/core/region.c | 99 +++++++++++++++++++++++++++------------
>  1 file changed, 70 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 6e5e1460068d..3a77aec2c447 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -349,30 +349,42 @@ static int cxl_region_decode_commit(struct cxl_region *cxlr)
>  	return rc;
>  }
>  
> -static ssize_t commit_store(struct device *dev, struct device_attribute *attr,
> -			    const char *buf, size_t len)
> +static int queue_reset(struct cxl_region *cxlr)

How about defining both new helpers to return ssize_t type for consistency
with sysfs functions. The local 'int rc' can remain as long as the function
return type is ssize_t.

There no possible breakage here based on the actual limited return values.
Just a suggestion to follow sysfs convention.

>  {
> -	struct cxl_region *cxlr = to_cxl_region(dev);
>  	struct cxl_region_params *p = &cxlr->params;
> -	bool commit;
> -	ssize_t rc;
> +	int rc;
>  
> -	rc = kstrtobool(buf, &commit);
> +	rc = down_write_killable(&cxl_region_rwsem);
>  	if (rc)
>  		return rc;
>  
> +	/* Already in the requested state? */
> +	if (p->state < CXL_CONFIG_COMMIT)
> +		goto out;
> +
> +	p->state = CXL_CONFIG_RESET_PENDING;
> +
> +out:
> +	up_write(&cxl_region_rwsem);
> +
> +	return rc;
> +}

snip

Re: [PATCH v2 5/8] cxl/region: Split commit_store() into __commit() and queue_reset() helpers

[dan.j.williams]

Alison Schofield wrote:
> On Wed, Jun 18, 2025 at 10:04:13PM -0700, Dan Williams wrote:
> > The complexity of dropping the lock is removed in favor of splitting commit
> > operations to a helper, and leaving all the complexities of "decommit" for
> > commit_store() to coordinate the different locking contexts.
> > 
> > The CPU cache-invalidation in the decommit path is solely handled now by
> > cxl_region_decode_reset(). Previously the CPU caches were being needlessly
> > flushed twice in the decommit path where the first flush had no guarantee
> > that the memory would not be immediately re-dirtied.
> > 
> > Cc: Davidlohr Bueso <dave@stgolabs.net>
> > Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
> > Cc: Dave Jiang <dave.jiang@intel.com>
> > Cc: Alison Schofield <alison.schofield@intel.com>
> > Cc: Vishal Verma <vishal.l.verma@intel.com>
> > Cc: Ira Weiny <ira.weiny@intel.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> >  drivers/cxl/core/region.c | 99 +++++++++++++++++++++++++++------------
> >  1 file changed, 70 insertions(+), 29 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index 6e5e1460068d..3a77aec2c447 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -349,30 +349,42 @@ static int cxl_region_decode_commit(struct cxl_region *cxlr)
> >  	return rc;
> >  }
> >  
> > -static ssize_t commit_store(struct device *dev, struct device_attribute *attr,
> > -			    const char *buf, size_t len)
> > +static int queue_reset(struct cxl_region *cxlr)
> 
> How about defining both new helpers to return ssize_t type for consistency
> with sysfs functions. The local 'int rc' can remain as long as the function
> return type is ssize_t.

The type change to 'int' for the helpers that are not dealing with
input/ouput is deliberate. queue_reset() is never returning a "size", it
is always returning 0 or a negative error code. An int fits in ssize_t.

Re: [PATCH v2 5/8] cxl/region: Split commit_store() into __commit() and queue_reset() helpers

[Jonathan Cameron]

On Wed, 18 Jun 2025 22:04:13 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> The complexity of dropping the lock is removed in favor of splitting commit
> operations to a helper, and leaving all the complexities of "decommit" for
> commit_store() to coordinate the different locking contexts.
> 
> The CPU cache-invalidation in the decommit path is solely handled now by
> cxl_region_decode_reset(). Previously the CPU caches were being needlessly
> flushed twice in the decommit path where the first flush had no guarantee
> that the memory would not be immediately re-dirtied.
> 
> Cc: Davidlohr Bueso <dave@stgolabs.net>
> Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Alison Schofield <alison.schofield@intel.com>
> Cc: Vishal Verma <vishal.l.verma@intel.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Seems fine
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

Re: [PATCH v2 5/8] cxl/region: Split commit_store() into __commit() and queue_reset() helpers

[Dave Jiang]

On 6/18/25 10:04 PM, Dan Williams wrote:
> The complexity of dropping the lock is removed in favor of splitting commit
> operations to a helper, and leaving all the complexities of "decommit" for
> commit_store() to coordinate the different locking contexts.
> 
> The CPU cache-invalidation in the decommit path is solely handled now by
> cxl_region_decode_reset(). Previously the CPU caches were being needlessly
> flushed twice in the decommit path where the first flush had no guarantee
> that the memory would not be immediately re-dirtied.
> 
> Cc: Davidlohr Bueso <dave@stgolabs.net>
> Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Alison Schofield <alison.schofield@intel.com>
> Cc: Vishal Verma <vishal.l.verma@intel.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/core/region.c | 99 +++++++++++++++++++++++++++------------
>  1 file changed, 70 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 6e5e1460068d..3a77aec2c447 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -349,30 +349,42 @@ static int cxl_region_decode_commit(struct cxl_region *cxlr)
>  	return rc;
>  }
>  
> -static ssize_t commit_store(struct device *dev, struct device_attribute *attr,
> -			    const char *buf, size_t len)
> +static int queue_reset(struct cxl_region *cxlr)
>  {
> -	struct cxl_region *cxlr = to_cxl_region(dev);
>  	struct cxl_region_params *p = &cxlr->params;
> -	bool commit;
> -	ssize_t rc;
> +	int rc;
>  
> -	rc = kstrtobool(buf, &commit);
> +	rc = down_write_killable(&cxl_region_rwsem);
>  	if (rc)
>  		return rc;
>  
> +	/* Already in the requested state? */
> +	if (p->state < CXL_CONFIG_COMMIT)
> +		goto out;
> +
> +	p->state = CXL_CONFIG_RESET_PENDING;
> +
> +out:
> +	up_write(&cxl_region_rwsem);
> +
> +	return rc;
> +}
> +
> +static int __commit(struct cxl_region *cxlr)
> +{
> +	struct cxl_region_params *p = &cxlr->params;
> +	int rc;
> +
>  	rc = down_write_killable(&cxl_region_rwsem);
>  	if (rc)
>  		return rc;
>  
>  	/* Already in the requested state? */
> -	if (commit && p->state >= CXL_CONFIG_COMMIT)
> -		goto out;
> -	if (!commit && p->state < CXL_CONFIG_COMMIT)
> +	if (p->state >= CXL_CONFIG_COMMIT)
>  		goto out;
>  
>  	/* Not ready to commit? */
> -	if (commit && p->state < CXL_CONFIG_ACTIVE) {
> +	if (p->state < CXL_CONFIG_ACTIVE) {
>  		rc = -ENXIO;
>  		goto out;
>  	}
> @@ -385,31 +397,60 @@ static ssize_t commit_store(struct device *dev, struct device_attribute *attr,
>  	if (rc)
>  		goto out;
>  
> -	if (commit) {
> -		rc = cxl_region_decode_commit(cxlr);
> -		if (rc == 0)
> -			p->state = CXL_CONFIG_COMMIT;
> -	} else {
> -		p->state = CXL_CONFIG_RESET_PENDING;
> -		up_write(&cxl_region_rwsem);
> -		device_release_driver(&cxlr->dev);
> -		down_write(&cxl_region_rwsem);
> -
> -		/*
> -		 * The lock was dropped, so need to revalidate that the reset is
> -		 * still pending.
> -		 */
> -		if (p->state == CXL_CONFIG_RESET_PENDING) {
> -			cxl_region_decode_reset(cxlr, p->interleave_ways);
> -			p->state = CXL_CONFIG_ACTIVE;
> -		}
> -	}
> +	rc = cxl_region_decode_commit(cxlr);
> +	if (rc == 0)
> +		p->state = CXL_CONFIG_COMMIT;
>  
>  out:
>  	up_write(&cxl_region_rwsem);
>  
> +	return rc;
> +}
> +
> +static ssize_t commit_store(struct device *dev, struct device_attribute *attr,
> +			    const char *buf, size_t len)
> +{
> +	struct cxl_region *cxlr = to_cxl_region(dev);
> +	struct cxl_region_params *p = &cxlr->params;
> +	bool commit;
> +	ssize_t rc;
> +
> +	rc = kstrtobool(buf, &commit);
>  	if (rc)
>  		return rc;
> +
> +	if (commit) {
> +		rc = __commit(cxlr);
> +		if (rc)
> +			return rc;
> +		return len;
> +	}
> +
> +	rc = queue_reset(cxlr);
> +	if (rc)
> +		return rc;
> +
> +	/*
> +	 * Unmap the region and depend the reset-pending state to ensure
> +	 * it does not go active again until post reset
> +	 */
> +	device_release_driver(&cxlr->dev);
> +
> +	/*
> +	 * With the reset pending take cxl_region_rwsem unconditionally
> +	 * to ensure the reset gets handled before returning.
> +	 */
> +	guard(rwsem_write)(&cxl_region_rwsem);
> +
> +	/*
> +	 * Revalidate that the reset is still pending in case another
> +	 * thread already handled this reset.
> +	 */
> +	if (p->state == CXL_CONFIG_RESET_PENDING) {
> +		cxl_region_decode_reset(cxlr, p->interleave_ways);
> +		p->state = CXL_CONFIG_ACTIVE;
> +	}
> +
>  	return len;
>  }
>  

[PATCH v2 6/8] cxl/region: Move ready-to-probe state check to a helper

[Dan Williams]

Rather than unlocking the region rwsem in the middle of cxl_region_probe()
create a helper for determining when the region is ready-to-probe.

Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
Cc: Dave Jiang <dave.jiang@intel.com>
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Vishal Verma <vishal.l.verma@intel.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/region.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 3a77aec2c447..2a97fa9a394f 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -3572,9 +3572,8 @@ static void shutdown_notifiers(void *_cxlr)
 	unregister_mt_adistance_algorithm(&cxlr->adist_notifier);
 }
 
-static int cxl_region_probe(struct device *dev)
+static int cxl_region_can_probe(struct cxl_region *cxlr)
 {
-	struct cxl_region *cxlr = to_cxl_region(dev);
 	struct cxl_region_params *p = &cxlr->params;
 	int rc;
 
@@ -3597,15 +3596,28 @@ static int cxl_region_probe(struct device *dev)
 		goto out;
 	}
 
-	/*
-	 * From this point on any path that changes the region's state away from
-	 * CXL_CONFIG_COMMIT is also responsible for releasing the driver.
-	 */
 out:
 	up_read(&cxl_region_rwsem);
 
 	if (rc)
 		return rc;
+	return 0;
+}
+
+static int cxl_region_probe(struct device *dev)
+{
+	struct cxl_region *cxlr = to_cxl_region(dev);
+	struct cxl_region_params *p = &cxlr->params;
+	int rc;
+
+	rc = cxl_region_can_probe(cxlr);
+	if (rc)
+		return rc;
+
+	/*
+	 * From this point on any path that changes the region's state away from
+	 * CXL_CONFIG_COMMIT is also responsible for releasing the driver.
+	 */
 
 	cxlr->memory_notifier.notifier_call = cxl_region_perf_attrs_callback;
 	cxlr->memory_notifier.priority = CXL_CALLBACK_PRI;
-- 
2.49.0

Re: [PATCH v2 6/8] cxl/region: Move ready-to-probe state check to a helper

[Dave Jiang]

On 6/18/25 10:04 PM, Dan Williams wrote:
> Rather than unlocking the region rwsem in the middle of cxl_region_probe()
> create a helper for determining when the region is ready-to-probe.
> 
> Cc: Davidlohr Bueso <dave@stgolabs.net>
> Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Alison Schofield <alison.schofield@intel.com>
> Cc: Vishal Verma <vishal.l.verma@intel.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/core/region.c | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 3a77aec2c447..2a97fa9a394f 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -3572,9 +3572,8 @@ static void shutdown_notifiers(void *_cxlr)
>  	unregister_mt_adistance_algorithm(&cxlr->adist_notifier);
>  }
>  
> -static int cxl_region_probe(struct device *dev)
> +static int cxl_region_can_probe(struct cxl_region *cxlr)
>  {
> -	struct cxl_region *cxlr = to_cxl_region(dev);
>  	struct cxl_region_params *p = &cxlr->params;
>  	int rc;
>  
> @@ -3597,15 +3596,28 @@ static int cxl_region_probe(struct device *dev)
>  		goto out;
>  	}
>  
> -	/*
> -	 * From this point on any path that changes the region's state away from
> -	 * CXL_CONFIG_COMMIT is also responsible for releasing the driver.
> -	 */
>  out:
>  	up_read(&cxl_region_rwsem);
>  
>  	if (rc)
>  		return rc;
> +	return 0;
> +}
> +
> +static int cxl_region_probe(struct device *dev)
> +{
> +	struct cxl_region *cxlr = to_cxl_region(dev);
> +	struct cxl_region_params *p = &cxlr->params;
> +	int rc;
> +
> +	rc = cxl_region_can_probe(cxlr);
> +	if (rc)
> +		return rc;
> +
> +	/*
> +	 * From this point on any path that changes the region's state away from
> +	 * CXL_CONFIG_COMMIT is also responsible for releasing the driver.
> +	 */
>  
>  	cxlr->memory_notifier.notifier_call = cxl_region_perf_attrs_callback;
>  	cxlr->memory_notifier.priority = CXL_CALLBACK_PRI;

[PATCH v2 7/8] cxl/region: Introduce CLASS(cxl_decoder_detach...) consolidate multiple paths

[Dan Williams]

Both detach_target() and cxld_unregister() want to tear down a cxl_region
when an endpoint decoder is either detached or destroyed.

When a region is to be destroyed cxl_decoder_detach() releases
cxl_region_rwsem unbinds the cxl_region driver and re-acquires the rwsem.

This "reverse" locking pattern is difficult to reason about, not amenable
to scope-based cleanup, and the minor differences in the calling convention
of cxl_decoder_detach() currently results in the cxl_decoder_kill_region()
wrapper.

Introduce CLASS(cxl_decoder_detach...) which creates an object that moves
the post-detach cleanup work to a destructor, and consolidates minor
preamble differences in the constructor.

Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
Cc: Dave Jiang <dave.jiang@intel.com>
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Vishal Verma <vishal.l.verma@intel.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/core.h   | 43 ++++++++++++++++++-
 drivers/cxl/core/port.c   |  6 +--
 drivers/cxl/core/region.c | 88 ++++++++++++++++++---------------------
 3 files changed, 83 insertions(+), 54 deletions(-)

diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
index 29b61828a847..8a65777ef3d3 100644
--- a/drivers/cxl/core/core.h
+++ b/drivers/cxl/core/core.h
@@ -12,6 +12,11 @@ extern const struct device_type cxl_pmu_type;
 
 extern struct attribute_group cxl_base_attribute_group;
 
+enum cxl_detach_mode {
+	DETACH_ONLY,
+	DETACH_INVALIDATE,
+};
+
 #ifdef CONFIG_CXL_REGION
 extern struct device_attribute dev_attr_create_pmem_region;
 extern struct device_attribute dev_attr_create_ram_region;
@@ -20,7 +25,11 @@ extern struct device_attribute dev_attr_region;
 extern const struct device_type cxl_pmem_region_type;
 extern const struct device_type cxl_dax_region_type;
 extern const struct device_type cxl_region_type;
-void cxl_decoder_kill_region(struct cxl_endpoint_decoder *cxled);
+
+struct cxl_region *cxl_decoder_detach(struct cxl_region *cxlr,
+				      struct cxl_endpoint_decoder *cxled,
+				      int pos, enum cxl_detach_mode mode);
+
 #define CXL_REGION_ATTR(x) (&dev_attr_##x.attr)
 #define CXL_REGION_TYPE(x) (&cxl_region_type)
 #define SET_CXL_REGION_ATTR(x) (&dev_attr_##x.attr),
@@ -48,7 +57,9 @@ static inline int cxl_get_poison_by_endpoint(struct cxl_port *port)
 {
 	return 0;
 }
-static inline void cxl_decoder_kill_region(struct cxl_endpoint_decoder *cxled)
+static inline struct cxl_region *
+cxl_decoder_detach(struct cxl_region *cxlr, struct cxl_endpoint_decoder *cxled,
+		   int pos, enum cxl_detach_mode mode)
 {
 }
 static inline int cxl_region_init(void)
@@ -99,6 +110,34 @@ u16 cxl_rcrb_to_aer(struct device *dev, resource_size_t rcrb);
 extern struct rw_semaphore cxl_dpa_rwsem;
 extern struct rw_semaphore cxl_region_rwsem;
 
+DEFINE_CLASS(
+	cxl_decoder_detach, struct cxl_region *,
+	if (!IS_ERR_OR_NULL(_T)) {
+		device_release_driver(&_T->dev);
+		put_device(&_T->dev);
+	},
+	({
+		int rc = 0;
+
+		/* when the decoder is being destroyed lock unconditionally */
+		if (mode == DETACH_INVALIDATE)
+			down_write(&cxl_region_rwsem);
+		else
+			rc = down_write_killable(&cxl_region_rwsem);
+
+		if (rc)
+			cxlr = ERR_PTR(rc);
+		else {
+			cxlr = cxl_decoder_detach(cxlr, cxled, pos, mode);
+			get_device(&cxlr->dev);
+		}
+		up_write(&cxl_region_rwsem);
+
+		cxlr;
+	}),
+	struct cxl_region *cxlr, struct cxl_endpoint_decoder *cxled, int pos,
+	enum cxl_detach_mode mode)
+
 int cxl_memdev_init(void);
 void cxl_memdev_exit(void);
 void cxl_mbox_init(void);
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index eb46c6764d20..0f1629856380 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -2001,11 +2001,9 @@ EXPORT_SYMBOL_NS_GPL(cxl_decoder_add, "CXL");
 
 static void cxld_unregister(void *dev)
 {
-	struct cxl_endpoint_decoder *cxled;
-
 	if (is_endpoint_decoder(dev)) {
-		cxled = to_cxl_endpoint_decoder(dev);
-		cxl_decoder_kill_region(cxled);
+		CLASS(cxl_decoder_detach, cxlr)
+		(NULL, to_cxl_endpoint_decoder(dev), -1, DETACH_INVALIDATE);
 	}
 
 	device_unregister(dev);
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 2a97fa9a394f..010964aa5489 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -2135,27 +2135,52 @@ static int cxl_region_attach(struct cxl_region *cxlr,
 	return 0;
 }
 
-static int cxl_region_detach(struct cxl_endpoint_decoder *cxled)
+/*
+ * Cleanup a decoder's interest in a region. There are 2 cases to
+ * handle, removing an unknown @cxled from a known position in a region
+ * (detach_target()) or removing a known @cxled from an unknown @cxlr
+ * (cxld_unregister())
+ *
+ * When the detachment finds a region, the caller is responsible for
+ * releasing the region driver.
+ */
+struct cxl_region *cxl_decoder_detach(struct cxl_region *cxlr,
+				      struct cxl_endpoint_decoder *cxled,
+				      int pos, enum cxl_detach_mode mode)
 {
-	struct cxl_port *iter, *ep_port = cxled_to_port(cxled);
-	struct cxl_region *cxlr = cxled->cxld.region;
 	struct cxl_region_params *p;
-	int rc = 0;
 
 	lockdep_assert_held_write(&cxl_region_rwsem);
 
-	if (!cxlr)
-		return 0;
+	if (!cxled) {
+		p = &cxlr->params;
 
-	p = &cxlr->params;
-	get_device(&cxlr->dev);
+		if (pos >= p->interleave_ways) {
+			dev_dbg(&cxlr->dev, "position %d out of range %d\n",
+				pos, p->interleave_ways);
+			return ERR_PTR(-ENXIO);
+		}
+
+		if (!p->targets[pos])
+			return NULL;
+		cxled = p->targets[pos];
+	} else {
+		cxlr = cxled->cxld.region;
+		if (!cxlr)
+			return NULL;
+		p = &cxlr->params;
+	}
+
+
+	if (mode == DETACH_INVALIDATE)
+		cxled->part = -1;
 
 	if (p->state > CXL_CONFIG_ACTIVE) {
 		cxl_region_decode_reset(cxlr, p->interleave_ways);
 		p->state = CXL_CONFIG_ACTIVE;
 	}
 
-	for (iter = ep_port; !is_cxl_root(iter);
+	for (struct cxl_port *iter = cxled_to_port(cxled); !is_cxl_root(iter);
 	     iter = to_cxl_port(iter->dev.parent))
 		cxl_port_detach_region(iter, cxlr, cxled);
 
@@ -2166,7 +2191,7 @@ static int cxl_region_detach(struct cxl_endpoint_decoder *cxled)
 		dev_WARN_ONCE(&cxlr->dev, 1, "expected %s:%s at position %d\n",
 			      dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
 			      cxled->pos);
-		goto out;
+		return NULL;
 	}
 
 	if (p->state == CXL_CONFIG_ACTIVE) {
@@ -2180,21 +2205,7 @@ static int cxl_region_detach(struct cxl_endpoint_decoder *cxled)
 		.end = -1,
 	};
 
-	/* notify the region driver that one of its targets has departed */
-	up_write(&cxl_region_rwsem);
-	device_release_driver(&cxlr->dev);
-	down_write(&cxl_region_rwsem);
-out:
-	put_device(&cxlr->dev);
-	return rc;
-}
-
-void cxl_decoder_kill_region(struct cxl_endpoint_decoder *cxled)
-{
-	down_write(&cxl_region_rwsem);
-	cxled->part = -1;
-	cxl_region_detach(cxled);
-	up_write(&cxl_region_rwsem);
+	return cxlr;
 }
 
 static int attach_target(struct cxl_region *cxlr,
@@ -2225,29 +2236,10 @@ static int attach_target(struct cxl_region *cxlr,
 
 static int detach_target(struct cxl_region *cxlr, int pos)
 {
-	struct cxl_region_params *p = &cxlr->params;
-	int rc;
-
-	rc = down_write_killable(&cxl_region_rwsem);
-	if (rc)
-		return rc;
-
-	if (pos >= p->interleave_ways) {
-		dev_dbg(&cxlr->dev, "position %d out of range %d\n", pos,
-			p->interleave_ways);
-		rc = -ENXIO;
-		goto out;
-	}
-
-	if (!p->targets[pos]) {
-		rc = 0;
-		goto out;
-	}
-
-	rc = cxl_region_detach(p->targets[pos]);
-out:
-	up_write(&cxl_region_rwsem);
-	return rc;
+	CLASS(cxl_decoder_detach, ret)(cxlr, NULL, pos, DETACH_ONLY);
+	if (IS_ERR(ret))
+		return PTR_ERR(ret);
+	return 0;
 }
 
 static size_t store_targetN(struct cxl_region *cxlr, const char *buf, int pos,
-- 
2.49.0

Re: [PATCH v2 7/8] cxl/region: Introduce CLASS(cxl_decoder_detach...) consolidate multiple paths

[Jonathan Cameron]

On Wed, 18 Jun 2025 22:04:15 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Both detach_target() and cxld_unregister() want to tear down a cxl_region
> when an endpoint decoder is either detached or destroyed.
> 
> When a region is to be destroyed cxl_decoder_detach() releases
> cxl_region_rwsem unbinds the cxl_region driver and re-acquires the rwsem.
> 
> This "reverse" locking pattern is difficult to reason about, not amenable
> to scope-based cleanup, and the minor differences in the calling convention
> of cxl_decoder_detach() currently results in the cxl_decoder_kill_region()
> wrapper.
> 
> Introduce CLASS(cxl_decoder_detach...) which creates an object that moves
> the post-detach cleanup work to a destructor, and consolidates minor
> preamble differences in the constructor.

I'm struggling somewhat with grasping how the destructor pattern is useful
here. In the two cases the scope is tightly defined around the
declaration of the class instance. Doesn't that just boil down to
automatically calling the destuctor function immediately?  If so what
is the use of wrapping it up in a destructor?

> 
> Cc: Davidlohr Bueso <dave@stgolabs.net>
> Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Alison Schofield <alison.schofield@intel.com>
> Cc: Vishal Verma <vishal.l.verma@intel.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/cxl/core/core.h   | 43 ++++++++++++++++++-
>  drivers/cxl/core/port.c   |  6 +--
>  drivers/cxl/core/region.c | 88 ++++++++++++++++++---------------------
>  3 files changed, 83 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> index 29b61828a847..8a65777ef3d3 100644
> --- a/drivers/cxl/core/core.h
> +++ b/drivers/cxl/core/core.h
> @@ -12,6 +12,11 @@ extern const struct device_type cxl_pmu_type;
>  
>  extern struct attribute_group cxl_base_attribute_group;
>  
> +enum cxl_detach_mode {
> +	DETACH_ONLY,
> +	DETACH_INVALIDATE,
> +};
> +
>  #ifdef CONFIG_CXL_REGION
>  extern struct device_attribute dev_attr_create_pmem_region;
>  extern struct device_attribute dev_attr_create_ram_region;
> @@ -20,7 +25,11 @@ extern struct device_attribute dev_attr_region;
>  extern const struct device_type cxl_pmem_region_type;
>  extern const struct device_type cxl_dax_region_type;
>  extern const struct device_type cxl_region_type;
> -void cxl_decoder_kill_region(struct cxl_endpoint_decoder *cxled);
> +
> +struct cxl_region *cxl_decoder_detach(struct cxl_region *cxlr,
> +				      struct cxl_endpoint_decoder *cxled,
> +				      int pos, enum cxl_detach_mode mode);
> +
>  #define CXL_REGION_ATTR(x) (&dev_attr_##x.attr)
>  #define CXL_REGION_TYPE(x) (&cxl_region_type)
>  #define SET_CXL_REGION_ATTR(x) (&dev_attr_##x.attr),
> @@ -48,7 +57,9 @@ static inline int cxl_get_poison_by_endpoint(struct cxl_port *port)
>  {
>  	return 0;
>  }
> -static inline void cxl_decoder_kill_region(struct cxl_endpoint_decoder *cxled)
> +static inline struct cxl_region *
> +cxl_decoder_detach(struct cxl_region *cxlr, struct cxl_endpoint_decoder *cxled,
> +		   int pos, enum cxl_detach_mode mode)
>  {
>  }
>  static inline int cxl_region_init(void)
> @@ -99,6 +110,34 @@ u16 cxl_rcrb_to_aer(struct device *dev, resource_size_t rcrb);
>  extern struct rw_semaphore cxl_dpa_rwsem;
>  extern struct rw_semaphore cxl_region_rwsem;
>  
> +DEFINE_CLASS(
> +	cxl_decoder_detach, struct cxl_region *,
> +	if (!IS_ERR_OR_NULL(_T)) {
> +		device_release_driver(&_T->dev);
> +		put_device(&_T->dev);
> +	},
> +	({
> +		int rc = 0;
> +
> +		/* when the decoder is being destroyed lock unconditionally */
> +		if (mode == DETACH_INVALIDATE)
> +			down_write(&cxl_region_rwsem);
> +		else
> +			rc = down_write_killable(&cxl_region_rwsem);
> +
> +		if (rc)
> +			cxlr = ERR_PTR(rc);
> +		else {
> +			cxlr = cxl_decoder_detach(cxlr, cxled, pos, mode);
> +			get_device(&cxlr->dev);
> +		}
> +		up_write(&cxl_region_rwsem);
> +
> +		cxlr;
> +	}),
> +	struct cxl_region *cxlr, struct cxl_endpoint_decoder *cxled, int pos,
> +	enum cxl_detach_mode mode)
> +
>  int cxl_memdev_init(void);
>  void cxl_memdev_exit(void);
>  void cxl_mbox_init(void);
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index eb46c6764d20..0f1629856380 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -2001,11 +2001,9 @@ EXPORT_SYMBOL_NS_GPL(cxl_decoder_add, "CXL");
>  
>  static void cxld_unregister(void *dev)
>  {
> -	struct cxl_endpoint_decoder *cxled;
> -
>  	if (is_endpoint_decoder(dev)) {
> -		cxled = to_cxl_endpoint_decoder(dev);
> -		cxl_decoder_kill_region(cxled);
> +		CLASS(cxl_decoder_detach, cxlr)
> +		(NULL, to_cxl_endpoint_decoder(dev), -1, DETACH_INVALIDATE);
>  	}
>  
>  	device_unregister(dev);
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 2a97fa9a394f..010964aa5489 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2135,27 +2135,52 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>  	return 0;
>  }
>  
> -static int cxl_region_detach(struct cxl_endpoint_decoder *cxled)
> +/*
> + * Cleanup a decoder's interest in a region. There are 2 cases to
> + * handle, removing an unknown @cxled from a known position in a region
> + * (detach_target()) or removing a known @cxled from an unknown @cxlr
> + * (cxld_unregister())
> + *
> + * When the detachment finds a region, the caller is responsible for
> + * releasing the region driver.
> + */
> +struct cxl_region *cxl_decoder_detach(struct cxl_region *cxlr,
> +				      struct cxl_endpoint_decoder *cxled,
> +				      int pos, enum cxl_detach_mode mode)
>  {
> -	struct cxl_port *iter, *ep_port = cxled_to_port(cxled);
> -	struct cxl_region *cxlr = cxled->cxld.region;
>  	struct cxl_region_params *p;
> -	int rc = 0;
>  
>  	lockdep_assert_held_write(&cxl_region_rwsem);
>  
> -	if (!cxlr)
> -		return 0;
> +	if (!cxled) {
> +		p = &cxlr->params;
>  
> -	p = &cxlr->params;
> -	get_device(&cxlr->dev);
> +		if (pos >= p->interleave_ways) {
> +			dev_dbg(&cxlr->dev, "position %d out of range %d\n",
> +				pos, p->interleave_ways);
> +			return ERR_PTR(-ENXIO);
> +		}
> +
> +		if (!p->targets[pos])
> +			return NULL;
> +		cxled = p->targets[pos];
> +	} else {
> +		cxlr = cxled->cxld.region;
> +		if (!cxlr)
> +			return NULL;
> +		p = &cxlr->params;
> +	}
> +
> +
> +	if (mode == DETACH_INVALIDATE)
> +		cxled->part = -1;
>  
>  	if (p->state > CXL_CONFIG_ACTIVE) {
>  		cxl_region_decode_reset(cxlr, p->interleave_ways);
>  		p->state = CXL_CONFIG_ACTIVE;
>  	}
>  
> -	for (iter = ep_port; !is_cxl_root(iter);
> +	for (struct cxl_port *iter = cxled_to_port(cxled); !is_cxl_root(iter);
>  	     iter = to_cxl_port(iter->dev.parent))
>  		cxl_port_detach_region(iter, cxlr, cxled);
>  
> @@ -2166,7 +2191,7 @@ static int cxl_region_detach(struct cxl_endpoint_decoder *cxled)
>  		dev_WARN_ONCE(&cxlr->dev, 1, "expected %s:%s at position %d\n",
>  			      dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
>  			      cxled->pos);
> -		goto out;
> +		return NULL;
>  	}
>  
>  	if (p->state == CXL_CONFIG_ACTIVE) {
> @@ -2180,21 +2205,7 @@ static int cxl_region_detach(struct cxl_endpoint_decoder *cxled)
>  		.end = -1,
>  	};
>  
> -	/* notify the region driver that one of its targets has departed */
> -	up_write(&cxl_region_rwsem);
> -	device_release_driver(&cxlr->dev);
> -	down_write(&cxl_region_rwsem);
> -out:
> -	put_device(&cxlr->dev);
> -	return rc;
> -}
> -
> -void cxl_decoder_kill_region(struct cxl_endpoint_decoder *cxled)
> -{
> -	down_write(&cxl_region_rwsem);
> -	cxled->part = -1;
> -	cxl_region_detach(cxled);
> -	up_write(&cxl_region_rwsem);
> +	return cxlr;
>  }
>  
>  static int attach_target(struct cxl_region *cxlr,
> @@ -2225,29 +2236,10 @@ static int attach_target(struct cxl_region *cxlr,
>  
>  static int detach_target(struct cxl_region *cxlr, int pos)
>  {
> -	struct cxl_region_params *p = &cxlr->params;
> -	int rc;
> -
> -	rc = down_write_killable(&cxl_region_rwsem);
> -	if (rc)
> -		return rc;
> -
> -	if (pos >= p->interleave_ways) {
> -		dev_dbg(&cxlr->dev, "position %d out of range %d\n", pos,
> -			p->interleave_ways);
> -		rc = -ENXIO;
> -		goto out;
> -	}
> -
> -	if (!p->targets[pos]) {
> -		rc = 0;
> -		goto out;
> -	}
> -
> -	rc = cxl_region_detach(p->targets[pos]);
> -out:
> -	up_write(&cxl_region_rwsem);
> -	return rc;
> +	CLASS(cxl_decoder_detach, ret)(cxlr, NULL, pos, DETACH_ONLY);
> +	if (IS_ERR(ret))
> +		return PTR_ERR(ret);
> +	return 0;
>  }
>  
>  static size_t store_targetN(struct cxl_region *cxlr, const char *buf, int pos,

Re: [PATCH v2 7/8] cxl/region: Introduce CLASS(cxl_decoder_detach...) consolidate multiple paths

[dan.j.williams]

Jonathan Cameron wrote:
> On Wed, 18 Jun 2025 22:04:15 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > Both detach_target() and cxld_unregister() want to tear down a cxl_region
> > when an endpoint decoder is either detached or destroyed.
> > 
> > When a region is to be destroyed cxl_decoder_detach() releases
> > cxl_region_rwsem unbinds the cxl_region driver and re-acquires the rwsem.
> > 
> > This "reverse" locking pattern is difficult to reason about, not amenable
> > to scope-based cleanup, and the minor differences in the calling convention
> > of cxl_decoder_detach() currently results in the cxl_decoder_kill_region()
> > wrapper.
> > 
> > Introduce CLASS(cxl_decoder_detach...) which creates an object that moves
> > the post-detach cleanup work to a destructor, and consolidates minor
> > preamble differences in the constructor.
> 
> I'm struggling somewhat with grasping how the destructor pattern is useful
> here. In the two cases the scope is tightly defined around the
> declaration of the class instance. Doesn't that just boil down to
> automatically calling the destuctor function immediately?  If so what
> is the use of wrapping it up in a destructor?

tl;dr: yes, the CLASS() is not needed.

I came at this from the perspective of removing the mid-function unlock
pattern, and it was useful for me to decompose that into a
constructor/destructor pattern. However, you're right, with the
refactoring in place this further simplifies into a straightforward
helper function scheme.

I have __cxl_decoder_detach() which does the conditional cxl_region
lookup, and cxl_decoder_detach() which handles the conditional locking
and device_release_driver() follow-up.

[PATCH v2 8/8] cxl: Convert to ACQUIRE() for conditional rwsem locking

[Dan Williams]

Use ACQUIRE() to cleanup conditional locking paths in the CXL driver
The ACQUIRE() macro and its associated ACQUIRE_ERR() helpers, like
scoped_cond_guard(), arrange for scoped-based conditional locking. Unlike
scoped_cond_guard(), these macros arrange for an ERR_PTR() to be retrieved
representing the state of the conditional lock.

The goal of this conversion is to complete the removal of all explicit
unlock calls in the subsystem. I.e. the methods to acquire a lock are
solely via guard(), scoped_guard() (for limited cases), or ACQUIRE(). All
unlock is implicit / scope-based. In order to make sure all lock sites are
converted, the existing rwsem's are consolidated and renamed in 'struct
cxl_rwsem'. While that makes the patch noisier it gives a clean cut-off
between old-world (explicit unlock allowed), and new world (explicit unlock
deleted).

Cc: David Lechner <dlechner@baylibre.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: "Fabio M. De Francesco" <fabio.maria.de.francesco@linux.intel.com>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
Cc: Dave Jiang <dave.jiang@intel.com>
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Vishal Verma <vishal.l.verma@intel.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Shiju Jose <shiju.jose@huawei.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/cdat.c   |   6 +-
 drivers/cxl/core/core.h   |  43 +++---
 drivers/cxl/core/edac.c   |  44 +++----
 drivers/cxl/core/hdm.c    |  41 +++---
 drivers/cxl/core/mbox.c   |   6 +-
 drivers/cxl/core/memdev.c |  50 +++----
 drivers/cxl/core/port.c   |  18 +--
 drivers/cxl/core/region.c | 266 +++++++++++++++-----------------------
 drivers/cxl/cxl.h         |  13 +-
 include/linux/rwsem.h     |   1 +
 10 files changed, 206 insertions(+), 282 deletions(-)

diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
index 0ccef2f2a26a..c0af645425f4 100644
--- a/drivers/cxl/core/cdat.c
+++ b/drivers/cxl/core/cdat.c
@@ -336,7 +336,7 @@ static int match_cxlrd_hb(struct device *dev, void *data)
 	cxlrd = to_cxl_root_decoder(dev);
 	cxlsd = &cxlrd->cxlsd;
 
-	guard(rwsem_read)(&cxl_region_rwsem);
+	guard(rwsem_read)(&cxl_rwsem.region);
 	for (int i = 0; i < cxlsd->nr_targets; i++) {
 		if (host_bridge == cxlsd->target[i]->dport_dev)
 			return 1;
@@ -987,7 +987,7 @@ void cxl_region_shared_upstream_bandwidth_update(struct cxl_region *cxlr)
 	bool is_root;
 	int rc;
 
-	lockdep_assert_held(&cxl_dpa_rwsem);
+	lockdep_assert_held(&cxl_rwsem.dpa);
 
 	struct xarray *usp_xa __free(free_perf_xa) =
 		kzalloc(sizeof(*usp_xa), GFP_KERNEL);
@@ -1057,7 +1057,7 @@ void cxl_region_perf_data_calculate(struct cxl_region *cxlr,
 {
 	struct cxl_dpa_perf *perf;
 
-	lockdep_assert_held(&cxl_dpa_rwsem);
+	lockdep_assert_held(&cxl_rwsem.dpa);
 
 	perf = cxled_get_dpa_perf(cxled);
 	if (IS_ERR(perf))
diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
index 8a65777ef3d3..ed7d08244542 100644
--- a/drivers/cxl/core/core.h
+++ b/drivers/cxl/core/core.h
@@ -5,6 +5,7 @@
 #define __CXL_CORE_H__
 
 #include <cxl/mailbox.h>
+#include <linux/rwsem.h>
 
 extern const struct device_type cxl_nvdimm_bridge_type;
 extern const struct device_type cxl_nvdimm_type;
@@ -107,8 +108,20 @@ u16 cxl_rcrb_to_aer(struct device *dev, resource_size_t rcrb);
 #define PCI_RCRB_CAP_HDR_NEXT_MASK	GENMASK(15, 8)
 #define PCI_CAP_EXP_SIZEOF		0x3c
 
-extern struct rw_semaphore cxl_dpa_rwsem;
-extern struct rw_semaphore cxl_region_rwsem;
+struct cxl_rwsem {
+	/*
+	 * All changes to HPA (interleave configuration) occur with this
+	 * lock held for write.
+	 */
+	struct rw_semaphore region;
+	/*
+	 * All changes to a device DPA space occur with this lock held
+	 * for write.
+	 */
+	struct rw_semaphore dpa;
+};
+
+extern struct cxl_rwsem cxl_rwsem;
 
 DEFINE_CLASS(
 	cxl_decoder_detach, struct cxl_region *,
@@ -117,22 +130,22 @@ DEFINE_CLASS(
 		put_device(&_T->dev);
 	},
 	({
-		int rc = 0;
-
 		/* when the decoder is being destroyed lock unconditionally */
-		if (mode == DETACH_INVALIDATE)
-			down_write(&cxl_region_rwsem);
-		else
-			rc = down_write_killable(&cxl_region_rwsem);
-
-		if (rc)
-			cxlr = ERR_PTR(rc);
-		else {
+		if (mode == DETACH_INVALIDATE) {
+			guard(rwsem_write)(&cxl_rwsem.region);
 			cxlr = cxl_decoder_detach(cxlr, cxled, pos, mode);
-			get_device(&cxlr->dev);
+		} else {
+			int rc;
+
+			ACQUIRE(rwsem_write_kill, rwsem)(&cxl_rwsem.region);
+			if ((rc = ACQUIRE_ERR(rwsem_write_kill, &rwsem)))
+				cxlr = ERR_PTR(rc);
+			else
+				cxlr = cxl_decoder_detach(cxlr, cxled, pos,
+							  mode);
 		}
-		up_write(&cxl_region_rwsem);
-
+		if (!IS_ERR_OR_NULL(cxlr))
+			get_device(&cxlr->dev);
 		cxlr;
 	}),
 	struct cxl_region *cxlr, struct cxl_endpoint_decoder *cxled, int pos,
diff --git a/drivers/cxl/core/edac.c b/drivers/cxl/core/edac.c
index 2cbc664e5d62..f1ebdbe222c8 100644
--- a/drivers/cxl/core/edac.c
+++ b/drivers/cxl/core/edac.c
@@ -115,10 +115,9 @@ static int cxl_scrub_get_attrbs(struct cxl_patrol_scrub_context *cxl_ps_ctx,
 						flags, min_cycle);
 	}
 
-	struct rw_semaphore *region_lock __free(rwsem_read_release) =
-		rwsem_read_intr_acquire(&cxl_region_rwsem);
-	if (!region_lock)
-		return -EINTR;
+	ACQUIRE(rwsem_read_intr, rwsem)(&cxl_rwsem.region);
+	if ((ret = ACQUIRE_ERR(rwsem_read_intr, &rwsem)))
+		return ret;
 
 	cxlr = cxl_ps_ctx->cxlr;
 	p = &cxlr->params;
@@ -154,10 +153,9 @@ static int cxl_scrub_set_attrbs_region(struct device *dev,
 	struct cxl_region *cxlr;
 	int ret, i;
 
-	struct rw_semaphore *region_lock __free(rwsem_read_release) =
-		rwsem_read_intr_acquire(&cxl_region_rwsem);
-	if (!region_lock)
-		return -EINTR;
+	ACQUIRE(rwsem_read_intr, rwsem)(&cxl_rwsem.region);
+	if ((ret = ACQUIRE_ERR(rwsem_read_intr, &rwsem)))
+		return ret;
 
 	cxlr = cxl_ps_ctx->cxlr;
 	p = &cxlr->params;
@@ -1332,16 +1330,15 @@ cxl_mem_perform_sparing(struct device *dev,
 	struct cxl_memdev_sparing_in_payload sparing_pi;
 	struct cxl_event_dram *rec = NULL;
 	u16 validity_flags = 0;
+	int ret;
 
-	struct rw_semaphore *region_lock __free(rwsem_read_release) =
-		rwsem_read_intr_acquire(&cxl_region_rwsem);
-	if (!region_lock)
-		return -EINTR;
+	ACQUIRE(rwsem_read_intr, region_rwsem)(&cxl_rwsem.region);
+	if ((ret = ACQUIRE_ERR(rwsem_read_intr, &region_rwsem)))
+		return ret;
 
-	struct rw_semaphore *dpa_lock __free(rwsem_read_release) =
-		rwsem_read_intr_acquire(&cxl_dpa_rwsem);
-	if (!dpa_lock)
-		return -EINTR;
+	ACQUIRE(rwsem_read_intr, dpa_rwsem)(&cxl_rwsem.dpa);
+	if ((ret = ACQUIRE_ERR(rwsem_read_intr, &dpa_rwsem)))
+		return ret;
 
 	if (!cxl_sparing_ctx->cap_safe_when_in_use) {
 		/* Memory to repair must be offline */
@@ -1779,16 +1776,15 @@ static int cxl_mem_perform_ppr(struct cxl_ppr_context *cxl_ppr_ctx)
 	struct cxl_memdev_ppr_maintenance_attrbs maintenance_attrbs;
 	struct cxl_memdev *cxlmd = cxl_ppr_ctx->cxlmd;
 	struct cxl_mem_repair_attrbs attrbs = { 0 };
+	int ret;
 
-	struct rw_semaphore *region_lock __free(rwsem_read_release) =
-		rwsem_read_intr_acquire(&cxl_region_rwsem);
-	if (!region_lock)
-		return -EINTR;
+	ACQUIRE(rwsem_read_intr, region_rwsem)(&cxl_rwsem.region);
+	if ((ret = ACQUIRE_ERR(rwsem_read_intr, &region_rwsem)))
+		return ret;
 
-	struct rw_semaphore *dpa_lock __free(rwsem_read_release) =
-		rwsem_read_intr_acquire(&cxl_dpa_rwsem);
-	if (!dpa_lock)
-		return -EINTR;
+	ACQUIRE(rwsem_read_intr, dpa_rwsem)(&cxl_rwsem.dpa);
+	if ((ret = ACQUIRE_ERR(rwsem_read_intr, &dpa_rwsem)))
+		return ret;
 
 	if (!cxl_ppr_ctx->media_accessible || !cxl_ppr_ctx->data_retained) {
 		/* Memory to repair must be offline */
diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index e9cb34e30248..865a71bce251 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -16,7 +16,10 @@
  * for enumerating these registers and capabilities.
  */
 
-DECLARE_RWSEM(cxl_dpa_rwsem);
+struct cxl_rwsem cxl_rwsem = {
+	.region = __RWSEM_INITIALIZER(cxl_rwsem.region),
+	.dpa = __RWSEM_INITIALIZER(cxl_rwsem.dpa),
+};
 
 static int add_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
 			   int *target_map)
@@ -214,7 +217,7 @@ void cxl_dpa_debug(struct seq_file *file, struct cxl_dev_state *cxlds)
 {
 	struct resource *p1, *p2;
 
-	guard(rwsem_read)(&cxl_dpa_rwsem);
+	guard(rwsem_read)(&cxl_rwsem.dpa);
 	for (p1 = cxlds->dpa_res.child; p1; p1 = p1->sibling) {
 		__cxl_dpa_debug(file, p1, 0);
 		for (p2 = p1->child; p2; p2 = p2->sibling)
@@ -266,7 +269,7 @@ static void __cxl_dpa_release(struct cxl_endpoint_decoder *cxled)
 	struct resource *res = cxled->dpa_res;
 	resource_size_t skip_start;
 
-	lockdep_assert_held_write(&cxl_dpa_rwsem);
+	lockdep_assert_held_write(&cxl_rwsem.dpa);
 
 	/* save @skip_start, before @res is released */
 	skip_start = res->start - cxled->skip;
@@ -281,7 +284,7 @@ static void __cxl_dpa_release(struct cxl_endpoint_decoder *cxled)
 
 static void cxl_dpa_release(void *cxled)
 {
-	guard(rwsem_write)(&cxl_dpa_rwsem);
+	guard(rwsem_write)(&cxl_rwsem.dpa);
 	__cxl_dpa_release(cxled);
 }
 
@@ -293,7 +296,7 @@ static void devm_cxl_dpa_release(struct cxl_endpoint_decoder *cxled)
 {
 	struct cxl_port *port = cxled_to_port(cxled);
 
-	lockdep_assert_held_write(&cxl_dpa_rwsem);
+	lockdep_assert_held_write(&cxl_rwsem.dpa);
 	devm_remove_action(&port->dev, cxl_dpa_release, cxled);
 	__cxl_dpa_release(cxled);
 }
@@ -361,7 +364,7 @@ static int __cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled,
 	struct resource *res;
 	int rc;
 
-	lockdep_assert_held_write(&cxl_dpa_rwsem);
+	lockdep_assert_held_write(&cxl_rwsem.dpa);
 
 	if (!len) {
 		dev_warn(dev, "decoder%d.%d: empty reservation attempted\n",
@@ -470,7 +473,7 @@ int cxl_dpa_setup(struct cxl_dev_state *cxlds, const struct cxl_dpa_info *info)
 {
 	struct device *dev = cxlds->dev;
 
-	guard(rwsem_write)(&cxl_dpa_rwsem);
+	guard(rwsem_write)(&cxl_rwsem.dpa);
 
 	if (cxlds->nr_partitions)
 		return -EBUSY;
@@ -516,9 +519,8 @@ int devm_cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled,
 	struct cxl_port *port = cxled_to_port(cxled);
 	int rc;
 
-	down_write(&cxl_dpa_rwsem);
-	rc = __cxl_dpa_reserve(cxled, base, len, skipped);
-	up_write(&cxl_dpa_rwsem);
+	scoped_guard(rwsem_write, &cxl_rwsem.dpa)
+		rc = __cxl_dpa_reserve(cxled, base, len, skipped);
 
 	if (rc)
 		return rc;
@@ -529,7 +531,7 @@ EXPORT_SYMBOL_NS_GPL(devm_cxl_dpa_reserve, "CXL");
 
 resource_size_t cxl_dpa_size(struct cxl_endpoint_decoder *cxled)
 {
-	guard(rwsem_read)(&cxl_dpa_rwsem);
+	guard(rwsem_read)(&cxl_rwsem.dpa);
 	if (cxled->dpa_res)
 		return resource_size(cxled->dpa_res);
 
@@ -540,7 +542,7 @@ resource_size_t cxl_dpa_resource_start(struct cxl_endpoint_decoder *cxled)
 {
 	resource_size_t base = -1;
 
-	lockdep_assert_held(&cxl_dpa_rwsem);
+	lockdep_assert_held(&cxl_rwsem.dpa);
 	if (cxled->dpa_res)
 		base = cxled->dpa_res->start;
 
@@ -552,7 +554,7 @@ int cxl_dpa_free(struct cxl_endpoint_decoder *cxled)
 	struct cxl_port *port = cxled_to_port(cxled);
 	struct device *dev = &cxled->cxld.dev;
 
-	guard(rwsem_write)(&cxl_dpa_rwsem);
+	guard(rwsem_write)(&cxl_rwsem.dpa);
 	if (!cxled->dpa_res)
 		return 0;
 	if (cxled->cxld.region) {
@@ -582,7 +584,7 @@ int cxl_dpa_set_part(struct cxl_endpoint_decoder *cxled,
 	struct device *dev = &cxled->cxld.dev;
 	int part;
 
-	guard(rwsem_write)(&cxl_dpa_rwsem);
+	guard(rwsem_write)(&cxl_rwsem.dpa);
 	if (cxled->cxld.flags & CXL_DECODER_F_ENABLE)
 		return -EBUSY;
 
@@ -614,7 +616,7 @@ static int __cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, u64 size)
 	struct resource *p, *last;
 	int part;
 
-	guard(rwsem_write)(&cxl_dpa_rwsem);
+	guard(rwsem_write)(&cxl_rwsem.dpa);
 	if (cxled->cxld.region) {
 		dev_dbg(dev, "decoder attached to %s\n",
 			dev_name(&cxled->cxld.region->dev));
@@ -842,9 +844,8 @@ static int cxl_decoder_commit(struct cxl_decoder *cxld)
 		}
 	}
 
-	down_read(&cxl_dpa_rwsem);
-	setup_hw_decoder(cxld, hdm);
-	up_read(&cxl_dpa_rwsem);
+	scoped_guard(rwsem_read, &cxl_rwsem.dpa)
+		setup_hw_decoder(cxld, hdm);
 
 	port->commit_end++;
 	rc = cxld_await_commit(hdm, cxld->id);
@@ -882,7 +883,7 @@ void cxl_port_commit_reap(struct cxl_decoder *cxld)
 {
 	struct cxl_port *port = to_cxl_port(cxld->dev.parent);
 
-	lockdep_assert_held_write(&cxl_region_rwsem);
+	lockdep_assert_held_write(&cxl_rwsem.region);
 
 	/*
 	 * Once the highest committed decoder is disabled, free any other
@@ -1030,7 +1031,7 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
 		else
 			cxld->target_type = CXL_DECODER_DEVMEM;
 
-		guard(rwsem_write)(&cxl_region_rwsem);
+		guard(rwsem_write)(&cxl_rwsem.region);
 		if (cxld->id != cxl_num_decoders_committed(port)) {
 			dev_warn(&port->dev,
 				 "decoder%d.%d: Committed out of order\n",
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 81b21effe8cf..92cd3cbdd8ec 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -909,8 +909,8 @@ void cxl_event_trace_record(const struct cxl_memdev *cxlmd,
 		 * translations. Take topology mutation locks and lookup
 		 * { HPA, REGION } from { DPA, MEMDEV } in the event record.
 		 */
-		guard(rwsem_read)(&cxl_region_rwsem);
-		guard(rwsem_read)(&cxl_dpa_rwsem);
+		guard(rwsem_read)(&cxl_rwsem.region);
+		guard(rwsem_read)(&cxl_rwsem.dpa);
 
 		dpa = le64_to_cpu(evt->media_hdr.phys_addr) & CXL_DPA_MASK;
 		cxlr = cxl_dpa_to_region(cxlmd, dpa);
@@ -1265,7 +1265,7 @@ int cxl_mem_sanitize(struct cxl_memdev *cxlmd, u16 cmd)
 	/* synchronize with cxl_mem_probe() and decoder write operations */
 	guard(device)(&cxlmd->dev);
 	endpoint = cxlmd->endpoint;
-	guard(rwsem_read)(&cxl_region_rwsem);
+	guard(rwsem_read)(&cxl_rwsem.region);
 	/*
 	 * Require an endpoint to be safe otherwise the driver can not
 	 * be sure that the device is unmapped.
diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
index f88a13adf7fa..f5fbd34310fd 100644
--- a/drivers/cxl/core/memdev.c
+++ b/drivers/cxl/core/memdev.c
@@ -232,15 +232,13 @@ int cxl_trigger_poison_list(struct cxl_memdev *cxlmd)
 	if (!port || !is_cxl_endpoint(port))
 		return -EINVAL;
 
-	rc = down_read_interruptible(&cxl_region_rwsem);
-	if (rc)
+	ACQUIRE(rwsem_read_intr, region_rwsem)(&cxl_rwsem.region);
+	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &region_rwsem)))
 		return rc;
 
-	rc = down_read_interruptible(&cxl_dpa_rwsem);
-	if (rc) {
-		up_read(&cxl_region_rwsem);
+	ACQUIRE(rwsem_read_intr, dpa_rwsem)(&cxl_rwsem.dpa);
+	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &dpa_rwsem)))
 		return rc;
-	}
 
 	if (cxl_num_decoders_committed(port) == 0) {
 		/* No regions mapped to this memdev */
@@ -249,8 +247,6 @@ int cxl_trigger_poison_list(struct cxl_memdev *cxlmd)
 		/* Regions mapped, collect poison by endpoint */
 		rc =  cxl_get_poison_by_endpoint(port);
 	}
-	up_read(&cxl_dpa_rwsem);
-	up_read(&cxl_region_rwsem);
 
 	return rc;
 }
@@ -292,19 +288,17 @@ int cxl_inject_poison(struct cxl_memdev *cxlmd, u64 dpa)
 	if (!IS_ENABLED(CONFIG_DEBUG_FS))
 		return 0;
 
-	rc = down_read_interruptible(&cxl_region_rwsem);
-	if (rc)
+	ACQUIRE(rwsem_read_intr, region_rwsem)(&cxl_rwsem.region);
+	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &region_rwsem)))
 		return rc;
 
-	rc = down_read_interruptible(&cxl_dpa_rwsem);
-	if (rc) {
-		up_read(&cxl_region_rwsem);
+	ACQUIRE(rwsem_read_intr, dpa_rwsem)(&cxl_rwsem.dpa);
+	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &dpa_rwsem)))
 		return rc;
-	}
 
 	rc = cxl_validate_poison_dpa(cxlmd, dpa);
 	if (rc)
-		goto out;
+		return rc;
 
 	inject.address = cpu_to_le64(dpa);
 	mbox_cmd = (struct cxl_mbox_cmd) {
@@ -314,7 +308,7 @@ int cxl_inject_poison(struct cxl_memdev *cxlmd, u64 dpa)
 	};
 	rc = cxl_internal_send_cmd(cxl_mbox, &mbox_cmd);
 	if (rc)
-		goto out;
+		return rc;
 
 	cxlr = cxl_dpa_to_region(cxlmd, dpa);
 	if (cxlr)
@@ -327,11 +321,8 @@ int cxl_inject_poison(struct cxl_memdev *cxlmd, u64 dpa)
 		.length = cpu_to_le32(1),
 	};
 	trace_cxl_poison(cxlmd, cxlr, &record, 0, 0, CXL_POISON_TRACE_INJECT);
-out:
-	up_read(&cxl_dpa_rwsem);
-	up_read(&cxl_region_rwsem);
 
-	return rc;
+	return 0;
 }
 EXPORT_SYMBOL_NS_GPL(cxl_inject_poison, "CXL");
 
@@ -347,19 +338,17 @@ int cxl_clear_poison(struct cxl_memdev *cxlmd, u64 dpa)
 	if (!IS_ENABLED(CONFIG_DEBUG_FS))
 		return 0;
 
-	rc = down_read_interruptible(&cxl_region_rwsem);
-	if (rc)
+	ACQUIRE(rwsem_read_intr, region_rwsem)(&cxl_rwsem.region);
+	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &region_rwsem)))
 		return rc;
 
-	rc = down_read_interruptible(&cxl_dpa_rwsem);
-	if (rc) {
-		up_read(&cxl_region_rwsem);
+	ACQUIRE(rwsem_read_intr, dpa_rwsem)(&cxl_rwsem.dpa);
+	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &dpa_rwsem)))
 		return rc;
-	}
 
 	rc = cxl_validate_poison_dpa(cxlmd, dpa);
 	if (rc)
-		goto out;
+		return rc;
 
 	/*
 	 * In CXL 3.0 Spec 8.2.9.8.4.3, the Clear Poison mailbox command
@@ -378,7 +367,7 @@ int cxl_clear_poison(struct cxl_memdev *cxlmd, u64 dpa)
 
 	rc = cxl_internal_send_cmd(cxl_mbox, &mbox_cmd);
 	if (rc)
-		goto out;
+		return rc;
 
 	cxlr = cxl_dpa_to_region(cxlmd, dpa);
 	if (cxlr)
@@ -391,11 +380,8 @@ int cxl_clear_poison(struct cxl_memdev *cxlmd, u64 dpa)
 		.length = cpu_to_le32(1),
 	};
 	trace_cxl_poison(cxlmd, cxlr, &record, 0, 0, CXL_POISON_TRACE_CLEAR);
-out:
-	up_read(&cxl_dpa_rwsem);
-	up_read(&cxl_region_rwsem);
 
-	return rc;
+	return 0;
 }
 EXPORT_SYMBOL_NS_GPL(cxl_clear_poison, "CXL");
 
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 0f1629856380..58764d8a935b 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -30,18 +30,12 @@
  * instantiated by the core.
  */
 
-/*
- * All changes to the interleave configuration occur with this lock held
- * for write.
- */
-DECLARE_RWSEM(cxl_region_rwsem);
-
 static DEFINE_IDA(cxl_port_ida);
 static DEFINE_XARRAY(cxl_root_buses);
 
 int cxl_num_decoders_committed(struct cxl_port *port)
 {
-	lockdep_assert_held(&cxl_region_rwsem);
+	lockdep_assert_held(&cxl_rwsem.region);
 
 	return port->commit_end + 1;
 }
@@ -176,7 +170,7 @@ static ssize_t target_list_show(struct device *dev,
 	ssize_t offset;
 	int rc;
 
-	guard(rwsem_read)(&cxl_region_rwsem);
+	guard(rwsem_read)(&cxl_rwsem.region);
 	rc = emit_target_list(cxlsd, buf);
 	if (rc < 0)
 		return rc;
@@ -196,7 +190,7 @@ static ssize_t mode_show(struct device *dev, struct device_attribute *attr,
 	struct cxl_endpoint_decoder *cxled = to_cxl_endpoint_decoder(dev);
 	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
 	struct cxl_dev_state *cxlds = cxlmd->cxlds;
-	/* without @cxl_dpa_rwsem, make sure @part is not reloaded */
+	/* without @cxl_rwsem.dpa, make sure @part is not reloaded */
 	int part = READ_ONCE(cxled->part);
 	const char *desc;
 
@@ -235,7 +229,7 @@ static ssize_t dpa_resource_show(struct device *dev, struct device_attribute *at
 {
 	struct cxl_endpoint_decoder *cxled = to_cxl_endpoint_decoder(dev);
 
-	guard(rwsem_read)(&cxl_dpa_rwsem);
+	guard(rwsem_read)(&cxl_rwsem.dpa);
 	return sysfs_emit(buf, "%#llx\n", (u64)cxl_dpa_resource_start(cxled));
 }
 static DEVICE_ATTR_RO(dpa_resource);
@@ -560,7 +554,7 @@ static ssize_t decoders_committed_show(struct device *dev,
 {
 	struct cxl_port *port = to_cxl_port(dev);
 
-	guard(rwsem_read)(&cxl_region_rwsem);
+	guard(rwsem_read)(&cxl_rwsem.region);
 	return sysfs_emit(buf, "%d\n", cxl_num_decoders_committed(port));
 }
 
@@ -1722,7 +1716,7 @@ static int decoder_populate_targets(struct cxl_switch_decoder *cxlsd,
 	if (xa_empty(&port->dports))
 		return -EINVAL;
 
-	guard(rwsem_write)(&cxl_region_rwsem);
+	guard(rwsem_write)(&cxl_rwsem.region);
 	for (i = 0; i < cxlsd->cxld.interleave_ways; i++) {
 		struct cxl_dport *dport = find_dport(port, target_map[i]);
 
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 010964aa5489..a2ba19151d4f 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -141,16 +141,12 @@ static ssize_t uuid_show(struct device *dev, struct device_attribute *attr,
 	struct cxl_region_params *p = &cxlr->params;
 	ssize_t rc;
 
-	rc = down_read_interruptible(&cxl_region_rwsem);
-	if (rc)
+	ACQUIRE(rwsem_read_intr, region_rwsem)(&cxl_rwsem.region);
+	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &region_rwsem)))
 		return rc;
 	if (cxlr->mode != CXL_PARTMODE_PMEM)
-		rc = sysfs_emit(buf, "\n");
-	else
-		rc = sysfs_emit(buf, "%pUb\n", &p->uuid);
-	up_read(&cxl_region_rwsem);
-
-	return rc;
+		return sysfs_emit(buf, "\n");
+	return sysfs_emit(buf, "%pUb\n", &p->uuid);
 }
 
 static int is_dup(struct device *match, void *data)
@@ -162,7 +158,7 @@ static int is_dup(struct device *match, void *data)
 	if (!is_cxl_region(match))
 		return 0;
 
-	lockdep_assert_held(&cxl_region_rwsem);
+	lockdep_assert_held(&cxl_rwsem.region);
 	cxlr = to_cxl_region(match);
 	p = &cxlr->params;
 
@@ -192,27 +188,22 @@ static ssize_t uuid_store(struct device *dev, struct device_attribute *attr,
 	if (uuid_is_null(&temp))
 		return -EINVAL;
 
-	rc = down_write_killable(&cxl_region_rwsem);
-	if (rc)
+	ACQUIRE(rwsem_write_kill, region_rwsem)(&cxl_rwsem.region);
+	if ((rc = ACQUIRE_ERR(rwsem_write_kill, &region_rwsem)))
 		return rc;
 
 	if (uuid_equal(&p->uuid, &temp))
-		goto out;
+		return len;
 
-	rc = -EBUSY;
 	if (p->state >= CXL_CONFIG_ACTIVE)
-		goto out;
+		return -EBUSY;
 
 	rc = bus_for_each_dev(&cxl_bus_type, NULL, &temp, is_dup);
 	if (rc < 0)
-		goto out;
+		return rc;
 
 	uuid_copy(&p->uuid, &temp);
-out:
-	up_write(&cxl_region_rwsem);
 
-	if (rc)
-		return rc;
 	return len;
 }
 static DEVICE_ATTR_RW(uuid);
@@ -354,20 +345,17 @@ static int queue_reset(struct cxl_region *cxlr)
 	struct cxl_region_params *p = &cxlr->params;
 	int rc;
 
-	rc = down_write_killable(&cxl_region_rwsem);
-	if (rc)
+	ACQUIRE(rwsem_write_kill, rwsem)(&cxl_rwsem.region);
+	if ((rc = ACQUIRE_ERR(rwsem_write_kill, &rwsem)))
 		return rc;
 
 	/* Already in the requested state? */
 	if (p->state < CXL_CONFIG_COMMIT)
-		goto out;
+		return 0;
 
 	p->state = CXL_CONFIG_RESET_PENDING;
 
-out:
-	up_write(&cxl_region_rwsem);
-
-	return rc;
+	return 0;
 }
 
 static int __commit(struct cxl_region *cxlr)
@@ -375,19 +363,17 @@ static int __commit(struct cxl_region *cxlr)
 	struct cxl_region_params *p = &cxlr->params;
 	int rc;
 
-	rc = down_write_killable(&cxl_region_rwsem);
-	if (rc)
+	ACQUIRE(rwsem_write_kill, rwsem)(&cxl_rwsem.region);
+	if ((rc = ACQUIRE_ERR(rwsem_write_kill, &rwsem)))
 		return rc;
 
 	/* Already in the requested state? */
 	if (p->state >= CXL_CONFIG_COMMIT)
-		goto out;
+		return 0;
 
 	/* Not ready to commit? */
-	if (p->state < CXL_CONFIG_ACTIVE) {
-		rc = -ENXIO;
-		goto out;
-	}
+	if (p->state < CXL_CONFIG_ACTIVE)
+		return -ENXIO;
 
 	/*
 	 * Invalidate caches before region setup to drop any speculative
@@ -395,16 +381,15 @@ static int __commit(struct cxl_region *cxlr)
 	 */
 	rc = cxl_region_invalidate_memregion(cxlr);
 	if (rc)
-		goto out;
+		return rc;
 
 	rc = cxl_region_decode_commit(cxlr);
-	if (rc == 0)
-		p->state = CXL_CONFIG_COMMIT;
+	if (rc)
+		return rc;
 
-out:
-	up_write(&cxl_region_rwsem);
+	p->state = CXL_CONFIG_COMMIT;
 
-	return rc;
+	return 0;
 }
 
 static ssize_t commit_store(struct device *dev, struct device_attribute *attr,
@@ -437,10 +422,10 @@ static ssize_t commit_store(struct device *dev, struct device_attribute *attr,
 	device_release_driver(&cxlr->dev);
 
 	/*
-	 * With the reset pending take cxl_region_rwsem unconditionally
+	 * With the reset pending take cxl_rwsem.region unconditionally
 	 * to ensure the reset gets handled before returning.
 	 */
-	guard(rwsem_write)(&cxl_region_rwsem);
+	guard(rwsem_write)(&cxl_rwsem.region);
 
 	/*
 	 * Revalidate that the reset is still pending in case another
@@ -461,13 +446,10 @@ static ssize_t commit_show(struct device *dev, struct device_attribute *attr,
 	struct cxl_region_params *p = &cxlr->params;
 	ssize_t rc;
 
-	rc = down_read_interruptible(&cxl_region_rwsem);
-	if (rc)
+	ACQUIRE(rwsem_read_intr, rwsem)(&cxl_rwsem.region);
+	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &rwsem)))
 		return rc;
-	rc = sysfs_emit(buf, "%d\n", p->state >= CXL_CONFIG_COMMIT);
-	up_read(&cxl_region_rwsem);
-
-	return rc;
+	return sysfs_emit(buf, "%d\n", p->state >= CXL_CONFIG_COMMIT);
 }
 static DEVICE_ATTR_RW(commit);
 
@@ -491,15 +473,11 @@ static ssize_t interleave_ways_show(struct device *dev,
 {
 	struct cxl_region *cxlr = to_cxl_region(dev);
 	struct cxl_region_params *p = &cxlr->params;
-	ssize_t rc;
 
-	rc = down_read_interruptible(&cxl_region_rwsem);
-	if (rc)
-		return rc;
-	rc = sysfs_emit(buf, "%d\n", p->interleave_ways);
-	up_read(&cxl_region_rwsem);
-
-	return rc;
+	ACQUIRE(rwsem_read_intr, rwsem)(&cxl_rwsem.region);
+	if (ACQUIRE_ERR(rwsem_read_intr, &rwsem))
+		return ACQUIRE_ERR(rwsem_read_intr, &rwsem);
+	return sysfs_emit(buf, "%d\n", p->interleave_ways);
 }
 
 static const struct attribute_group *get_cxl_region_target_group(void);
@@ -534,23 +512,21 @@ static ssize_t interleave_ways_store(struct device *dev,
 		return -EINVAL;
 	}
 
-	rc = down_write_killable(&cxl_region_rwsem);
-	if (rc)
+	ACQUIRE(rwsem_write_kill, rwsem)(&cxl_rwsem.region);
+	if ((rc = ACQUIRE_ERR(rwsem_write_kill, &rwsem)))
 		return rc;
-	if (p->state >= CXL_CONFIG_INTERLEAVE_ACTIVE) {
-		rc = -EBUSY;
-		goto out;
-	}
+
+	if (p->state >= CXL_CONFIG_INTERLEAVE_ACTIVE)
+		return -EBUSY;
 
 	save = p->interleave_ways;
 	p->interleave_ways = val;
 	rc = sysfs_update_group(&cxlr->dev.kobj, get_cxl_region_target_group());
-	if (rc)
+	if (rc) {
 		p->interleave_ways = save;
-out:
-	up_write(&cxl_region_rwsem);
-	if (rc)
 		return rc;
+	}
+
 	return len;
 }
 static DEVICE_ATTR_RW(interleave_ways);
@@ -561,15 +537,11 @@ static ssize_t interleave_granularity_show(struct device *dev,
 {
 	struct cxl_region *cxlr = to_cxl_region(dev);
 	struct cxl_region_params *p = &cxlr->params;
-	ssize_t rc;
-
-	rc = down_read_interruptible(&cxl_region_rwsem);
-	if (rc)
-		return rc;
-	rc = sysfs_emit(buf, "%d\n", p->interleave_granularity);
-	up_read(&cxl_region_rwsem);
 
-	return rc;
+	ACQUIRE(rwsem_read_intr, rwsem)(&cxl_rwsem.region);
+	if (ACQUIRE_ERR(rwsem_read_intr, &rwsem))
+		return ACQUIRE_ERR(rwsem_read_intr, &rwsem);
+	return sysfs_emit(buf, "%d\n", p->interleave_granularity);
 }
 
 static ssize_t interleave_granularity_store(struct device *dev,
@@ -602,19 +574,15 @@ static ssize_t interleave_granularity_store(struct device *dev,
 	if (cxld->interleave_ways > 1 && val != cxld->interleave_granularity)
 		return -EINVAL;
 
-	rc = down_write_killable(&cxl_region_rwsem);
-	if (rc)
+	ACQUIRE(rwsem_write_kill, rwsem)(&cxl_rwsem.region);
+	if ((rc = ACQUIRE_ERR(rwsem_write_kill, &rwsem)))
 		return rc;
-	if (p->state >= CXL_CONFIG_INTERLEAVE_ACTIVE) {
-		rc = -EBUSY;
-		goto out;
-	}
+
+	if (p->state >= CXL_CONFIG_INTERLEAVE_ACTIVE)
+		return -EBUSY;
 
 	p->interleave_granularity = val;
-out:
-	up_write(&cxl_region_rwsem);
-	if (rc)
-		return rc;
+
 	return len;
 }
 static DEVICE_ATTR_RW(interleave_granularity);
@@ -625,17 +593,14 @@ static ssize_t resource_show(struct device *dev, struct device_attribute *attr,
 	struct cxl_region *cxlr = to_cxl_region(dev);
 	struct cxl_region_params *p = &cxlr->params;
 	u64 resource = -1ULL;
-	ssize_t rc;
 
-	rc = down_read_interruptible(&cxl_region_rwsem);
-	if (rc)
-		return rc;
+	ACQUIRE(rwsem_read_intr, rwsem)(&cxl_rwsem.region);
+	if (ACQUIRE_ERR(rwsem_read_intr, &rwsem))
+		return ACQUIRE_ERR(rwsem_read_intr, &rwsem);
+
 	if (p->res)
 		resource = p->res->start;
-	rc = sysfs_emit(buf, "%#llx\n", resource);
-	up_read(&cxl_region_rwsem);
-
-	return rc;
+	return sysfs_emit(buf, "%#llx\n", resource);
 }
 static DEVICE_ATTR_RO(resource);
 
@@ -663,7 +628,7 @@ static int alloc_hpa(struct cxl_region *cxlr, resource_size_t size)
 	struct resource *res;
 	u64 remainder = 0;
 
-	lockdep_assert_held_write(&cxl_region_rwsem);
+	lockdep_assert_held_write(&cxl_rwsem.region);
 
 	/* Nothing to do... */
 	if (p->res && resource_size(p->res) == size)
@@ -705,7 +670,7 @@ static void cxl_region_iomem_release(struct cxl_region *cxlr)
 	struct cxl_region_params *p = &cxlr->params;
 
 	if (device_is_registered(&cxlr->dev))
-		lockdep_assert_held_write(&cxl_region_rwsem);
+		lockdep_assert_held_write(&cxl_rwsem.region);
 	if (p->res) {
 		/*
 		 * Autodiscovered regions may not have been able to insert their
@@ -722,7 +687,7 @@ static int free_hpa(struct cxl_region *cxlr)
 {
 	struct cxl_region_params *p = &cxlr->params;
 
-	lockdep_assert_held_write(&cxl_region_rwsem);
+	lockdep_assert_held_write(&cxl_rwsem.region);
 
 	if (!p->res)
 		return 0;
@@ -746,15 +711,14 @@ static ssize_t size_store(struct device *dev, struct device_attribute *attr,
 	if (rc)
 		return rc;
 
-	rc = down_write_killable(&cxl_region_rwsem);
-	if (rc)
+	ACQUIRE(rwsem_write_kill, rwsem)(&cxl_rwsem.region);
+	if ((rc = ACQUIRE_ERR(rwsem_write_kill, &rwsem)))
 		return rc;
 
 	if (val)
 		rc = alloc_hpa(cxlr, val);
 	else
 		rc = free_hpa(cxlr);
-	up_write(&cxl_region_rwsem);
 
 	if (rc)
 		return rc;
@@ -770,15 +734,12 @@ static ssize_t size_show(struct device *dev, struct device_attribute *attr,
 	u64 size = 0;
 	ssize_t rc;
 
-	rc = down_read_interruptible(&cxl_region_rwsem);
-	if (rc)
+	ACQUIRE(rwsem_read_intr, rwsem)(&cxl_rwsem.region);
+	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &rwsem)))
 		return rc;
 	if (p->res)
 		size = resource_size(p->res);
-	rc = sysfs_emit(buf, "%#llx\n", size);
-	up_read(&cxl_region_rwsem);
-
-	return rc;
+	return sysfs_emit(buf, "%#llx\n", size);
 }
 static DEVICE_ATTR_RW(size);
 
@@ -804,26 +765,20 @@ static size_t show_targetN(struct cxl_region *cxlr, char *buf, int pos)
 	struct cxl_endpoint_decoder *cxled;
 	int rc;
 
-	rc = down_read_interruptible(&cxl_region_rwsem);
-	if (rc)
+	ACQUIRE(rwsem_read_intr, rwsem)(&cxl_rwsem.region);
+	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &rwsem)))
 		return rc;
 
 	if (pos >= p->interleave_ways) {
 		dev_dbg(&cxlr->dev, "position %d out of range %d\n", pos,
 			p->interleave_ways);
-		rc = -ENXIO;
-		goto out;
+		return -ENXIO;
 	}
 
 	cxled = p->targets[pos];
 	if (!cxled)
-		rc = sysfs_emit(buf, "\n");
-	else
-		rc = sysfs_emit(buf, "%s\n", dev_name(&cxled->cxld.dev));
-out:
-	up_read(&cxl_region_rwsem);
-
-	return rc;
+		return sysfs_emit(buf, "\n");
+	return sysfs_emit(buf, "%s\n", dev_name(&cxled->cxld.dev));
 }
 
 static int check_commit_order(struct device *dev, void *data)
@@ -938,7 +893,7 @@ cxl_port_pick_region_decoder(struct cxl_port *port,
 	/*
 	 * This decoder is pinned registered as long as the endpoint decoder is
 	 * registered, and endpoint decoder unregistration holds the
-	 * cxl_region_rwsem over unregister events, so no need to hold on to
+	 * cxl_rwsem.region over unregister events, so no need to hold on to
 	 * this extra reference.
 	 */
 	put_device(dev);
@@ -1129,7 +1084,7 @@ static int cxl_port_attach_region(struct cxl_port *port,
 	unsigned long index;
 	int rc = -EBUSY;
 
-	lockdep_assert_held_write(&cxl_region_rwsem);
+	lockdep_assert_held_write(&cxl_rwsem.region);
 
 	cxl_rr = cxl_rr_load(port, cxlr);
 	if (cxl_rr) {
@@ -1239,7 +1194,7 @@ static void cxl_port_detach_region(struct cxl_port *port,
 	struct cxl_region_ref *cxl_rr;
 	struct cxl_ep *ep = NULL;
 
-	lockdep_assert_held_write(&cxl_region_rwsem);
+	lockdep_assert_held_write(&cxl_rwsem.region);
 
 	cxl_rr = cxl_rr_load(port, cxlr);
 	if (!cxl_rr)
@@ -2150,7 +2105,7 @@ struct cxl_region *cxl_decoder_detach(struct cxl_region *cxlr,
 {
 	struct cxl_region_params *p;
 
-	lockdep_assert_held_write(&cxl_region_rwsem);
+	lockdep_assert_held_write(&cxl_rwsem.region);
 
 	if (!cxled) {
 		p = &cxlr->params;
@@ -2212,19 +2167,19 @@ static int attach_target(struct cxl_region *cxlr,
 			 struct cxl_endpoint_decoder *cxled, int pos,
 			 unsigned int state)
 {
-	int rc = 0;
-
-	if (state == TASK_INTERRUPTIBLE)
-		rc = down_write_killable(&cxl_region_rwsem);
-	else
-		down_write(&cxl_region_rwsem);
-	if (rc)
-		return rc;
+	int rc;
 
-	down_read(&cxl_dpa_rwsem);
-	rc = cxl_region_attach(cxlr, cxled, pos);
-	up_read(&cxl_dpa_rwsem);
-	up_write(&cxl_region_rwsem);
+	if (state == TASK_INTERRUPTIBLE) {
+		ACQUIRE(rwsem_write_kill, rwsem)(&cxl_rwsem.region);
+		if ((rc = ACQUIRE_ERR(rwsem_write_kill, &rwsem)) == 0) {
+			guard(rwsem_read)(&cxl_rwsem.dpa);
+			rc = cxl_region_attach(cxlr, cxled, pos);
+		}
+	} else {
+		guard(rwsem_write)(&cxl_rwsem.region);
+		guard(rwsem_read)(&cxl_rwsem.dpa);
+		rc = cxl_region_attach(cxlr, cxled, pos);
+	}
 
 	if (rc)
 		dev_warn(cxled->cxld.dev.parent,
@@ -2493,7 +2448,7 @@ static int cxl_region_perf_attrs_callback(struct notifier_block *nb,
 		return NOTIFY_DONE;
 
 	/*
-	 * No need to hold cxl_region_rwsem; region parameters are stable
+	 * No need to hold cxl_rwsem.region; region parameters are stable
 	 * within the cxl_region driver.
 	 */
 	region_nid = phys_to_target_node(cxlr->params.res->start);
@@ -2516,7 +2471,7 @@ static int cxl_region_calculate_adistance(struct notifier_block *nb,
 	int region_nid;
 
 	/*
-	 * No need to hold cxl_region_rwsem; region parameters are stable
+	 * No need to hold cxl_rwsem.region; region parameters are stable
 	 * within the cxl_region driver.
 	 */
 	region_nid = phys_to_target_node(cxlr->params.res->start);
@@ -2665,17 +2620,13 @@ static ssize_t region_show(struct device *dev, struct device_attribute *attr,
 	struct cxl_decoder *cxld = to_cxl_decoder(dev);
 	ssize_t rc;
 
-	rc = down_read_interruptible(&cxl_region_rwsem);
-	if (rc)
+	ACQUIRE(rwsem_read_intr, rwsem)(&cxl_rwsem.region);
+	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &rwsem)))
 		return rc;
 
 	if (cxld->region)
-		rc = sysfs_emit(buf, "%s\n", dev_name(&cxld->region->dev));
-	else
-		rc = sysfs_emit(buf, "\n");
-	up_read(&cxl_region_rwsem);
-
-	return rc;
+		return sysfs_emit(buf, "%s\n", dev_name(&cxld->region->dev));
+	return sysfs_emit(buf, "\n");
 }
 DEVICE_ATTR_RO(region);
 
@@ -3014,7 +2965,7 @@ static int cxl_pmem_region_alloc(struct cxl_region *cxlr)
 	struct device *dev;
 	int i;
 
-	guard(rwsem_read)(&cxl_region_rwsem);
+	guard(rwsem_read)(&cxl_rwsem.region);
 	if (p->state != CXL_CONFIG_COMMIT)
 		return -ENXIO;
 
@@ -3026,7 +2977,7 @@ static int cxl_pmem_region_alloc(struct cxl_region *cxlr)
 	cxlr_pmem->hpa_range.start = p->res->start;
 	cxlr_pmem->hpa_range.end = p->res->end;
 
-	/* Snapshot the region configuration underneath the cxl_region_rwsem */
+	/* Snapshot the region configuration underneath the cxl_rwsem.region */
 	cxlr_pmem->nr_mappings = p->nr_targets;
 	for (i = 0; i < p->nr_targets; i++) {
 		struct cxl_endpoint_decoder *cxled = p->targets[i];
@@ -3103,7 +3054,7 @@ static struct cxl_dax_region *cxl_dax_region_alloc(struct cxl_region *cxlr)
 	struct cxl_dax_region *cxlr_dax;
 	struct device *dev;
 
-	guard(rwsem_read)(&cxl_region_rwsem);
+	guard(rwsem_read)(&cxl_rwsem.region);
 	if (p->state != CXL_CONFIG_COMMIT)
 		return ERR_PTR(-ENXIO);
 
@@ -3303,7 +3254,7 @@ static int match_region_by_range(struct device *dev, const void *data)
 	cxlr = to_cxl_region(dev);
 	p = &cxlr->params;
 
-	guard(rwsem_read)(&cxl_region_rwsem);
+	guard(rwsem_read)(&cxl_rwsem.region);
 	if (p->res && p->res->start == r->start && p->res->end == r->end)
 		return 1;
 
@@ -3363,7 +3314,7 @@ static int __construct_region(struct cxl_region *cxlr,
 	struct resource *res;
 	int rc;
 
-	guard(rwsem_write)(&cxl_region_rwsem);
+	guard(rwsem_write)(&cxl_rwsem.region);
 	p = &cxlr->params;
 	if (p->state >= CXL_CONFIG_INTERLEAVE_ACTIVE) {
 		dev_err(cxlmd->dev.parent,
@@ -3499,10 +3450,10 @@ int cxl_add_to_region(struct cxl_endpoint_decoder *cxled)
 
 	attach_target(cxlr, cxled, -1, TASK_UNINTERRUPTIBLE);
 
-	down_read(&cxl_region_rwsem);
-	p = &cxlr->params;
-	attach = p->state == CXL_CONFIG_COMMIT;
-	up_read(&cxl_region_rwsem);
+	scoped_guard(rwsem_read, &cxl_rwsem.region) {
+		p = &cxlr->params;
+		attach = p->state == CXL_CONFIG_COMMIT;
+	}
 
 	if (attach) {
 		/*
@@ -3527,7 +3478,7 @@ u64 cxl_port_get_spa_cache_alias(struct cxl_port *endpoint, u64 spa)
 	if (!endpoint)
 		return ~0ULL;
 
-	guard(rwsem_write)(&cxl_region_rwsem);
+	guard(rwsem_write)(&cxl_rwsem.region);
 
 	xa_for_each(&endpoint->regions, index, iter) {
 		struct cxl_region_params *p = &iter->region->params;
@@ -3569,30 +3520,23 @@ static int cxl_region_can_probe(struct cxl_region *cxlr)
 	struct cxl_region_params *p = &cxlr->params;
 	int rc;
 
-	rc = down_read_interruptible(&cxl_region_rwsem);
-	if (rc) {
+	ACQUIRE(rwsem_read_intr, rwsem)(&cxl_rwsem.region);
+	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &rwsem))) {
 		dev_dbg(&cxlr->dev, "probe interrupted\n");
 		return rc;
 	}
 
 	if (p->state < CXL_CONFIG_COMMIT) {
 		dev_dbg(&cxlr->dev, "config state: %d\n", p->state);
-		rc = -ENXIO;
-		goto out;
+		return -ENXIO;
 	}
 
 	if (test_bit(CXL_REGION_F_NEEDS_RESET, &cxlr->flags)) {
 		dev_err(&cxlr->dev,
 			"failed to activate, re-commit region and retry\n");
-		rc = -ENXIO;
-		goto out;
+		return -ENXIO;
 	}
 
-out:
-	up_read(&cxl_region_rwsem);
-
-	if (rc)
-		return rc;
 	return 0;
 }
 
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 3f1695c96abc..50799a681231 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -469,7 +469,7 @@ enum cxl_config_state {
  * @nr_targets: number of targets
  * @cache_size: extended linear cache size if exists, otherwise zero.
  *
- * State transitions are protected by the cxl_region_rwsem
+ * State transitions are protected by cxl_rwsem.region
  */
 struct cxl_region_params {
 	enum cxl_config_state state;
@@ -912,15 +912,4 @@ bool cxl_endpoint_decoder_reset_detected(struct cxl_port *port);
 #endif
 
 u16 cxl_gpf_get_dvsec(struct device *dev);
-
-static inline struct rw_semaphore *rwsem_read_intr_acquire(struct rw_semaphore *rwsem)
-{
-	if (down_read_interruptible(rwsem))
-		return NULL;
-
-	return rwsem;
-}
-
-DEFINE_FREE(rwsem_read_release, struct rw_semaphore *, if (_T) up_read(_T))
-
 #endif /* __CXL_H__ */
diff --git a/include/linux/rwsem.h b/include/linux/rwsem.h
index c810deb88d13..cbafdc12e743 100644
--- a/include/linux/rwsem.h
+++ b/include/linux/rwsem.h
@@ -244,6 +244,7 @@ DEFINE_GUARD_COND(rwsem_read, _intr, down_read_interruptible(_T), _RET == 0)
 
 DEFINE_GUARD(rwsem_write, struct rw_semaphore *, down_write(_T), up_write(_T))
 DEFINE_GUARD_COND(rwsem_write, _try, down_write_trylock(_T))
+DEFINE_GUARD_COND(rwsem_write, _kill, down_write_killable(_T), _RET == 0)
 
 /*
  * downgrade write lock to read lock
-- 
2.49.0

Re: [PATCH v2 8/8] cxl: Convert to ACQUIRE() for conditional rwsem locking

[Jonathan Cameron]

On Wed, 18 Jun 2025 22:04:16 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Use ACQUIRE() to cleanup conditional locking paths in the CXL driver
> The ACQUIRE() macro and its associated ACQUIRE_ERR() helpers, like
> scoped_cond_guard(), arrange for scoped-based conditional locking. Unlike
> scoped_cond_guard(), these macros arrange for an ERR_PTR() to be retrieved
> representing the state of the conditional lock.
> 
> The goal of this conversion is to complete the removal of all explicit
> unlock calls in the subsystem. I.e. the methods to acquire a lock are
> solely via guard(), scoped_guard() (for limited cases), or ACQUIRE(). All
> unlock is implicit / scope-based. In order to make sure all lock sites are
> converted, the existing rwsem's are consolidated and renamed in 'struct
> cxl_rwsem'. While that makes the patch noisier it gives a clean cut-off
> between old-world (explicit unlock allowed), and new world (explicit unlock
> deleted).
> 
> Cc: David Lechner <dlechner@baylibre.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: "Fabio M. De Francesco" <fabio.maria.de.francesco@linux.intel.com>
> Cc: Davidlohr Bueso <dave@stgolabs.net>
> Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Alison Schofield <alison.schofield@intel.com>
> Cc: Vishal Verma <vishal.l.verma@intel.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Cc: Shiju Jose <shiju.jose@huawei.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
A few comments inline.




> index 010964aa5489..a2ba19151d4f 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c


>  static DEVICE_ATTR_RW(interleave_ways);
> @@ -561,15 +537,11 @@ static ssize_t interleave_granularity_show(struct device *dev,
>  {
>  	struct cxl_region *cxlr = to_cxl_region(dev);
>  	struct cxl_region_params *p = &cxlr->params;
> -	ssize_t rc;
> -
> -	rc = down_read_interruptible(&cxl_region_rwsem);
> -	if (rc)
> -		return rc;
> -	rc = sysfs_emit(buf, "%d\n", p->interleave_granularity);
> -	up_read(&cxl_region_rwsem);
>  
> -	return rc;
> +	ACQUIRE(rwsem_read_intr, rwsem)(&cxl_rwsem.region);
> +	if (ACQUIRE_ERR(rwsem_read_intr, &rwsem))
> +		return ACQUIRE_ERR(rwsem_read_intr, &rwsem);

Local variable?

> +	return sysfs_emit(buf, "%d\n", p->interleave_granularity);
>  }

> @@ -2212,19 +2167,19 @@ static int attach_target(struct cxl_region *cxlr,
>  			 struct cxl_endpoint_decoder *cxled, int pos,
>  			 unsigned int state)
>  {
> -	int rc = 0;
> -
> -	if (state == TASK_INTERRUPTIBLE)
> -		rc = down_write_killable(&cxl_region_rwsem);
> -	else
> -		down_write(&cxl_region_rwsem);
> -	if (rc)
> -		return rc;
> +	int rc;
>  
> -	down_read(&cxl_dpa_rwsem);
> -	rc = cxl_region_attach(cxlr, cxled, pos);
> -	up_read(&cxl_dpa_rwsem);
> -	up_write(&cxl_region_rwsem);
> +	if (state == TASK_INTERRUPTIBLE) {
> +		ACQUIRE(rwsem_write_kill, rwsem)(&cxl_rwsem.region);
> +		if ((rc = ACQUIRE_ERR(rwsem_write_kill, &rwsem)) == 0) {

I'd lift the warning print to a wrapper function so that you can
return early in error case and avoid this rather messy line.
e.g.

static int do_attach_target(struct cxl_region *cxlr,
			    struct cxl_endpoint_decoder *cxled, int pos,
			    unsigned int state)

	if (state == TASK_INTERRUPTIBLE) {
		ACQUIRE(rwsem_write_kill, rwsem)(&cxl_rwsem.region);
		rc = ACQUIRE_ERR(rwsem_write_kill, &rwsem);
		if (rc)
			return rc;

		guard(rwsem_read)(&cxl_rwsem.dpa);
		return cxl_region_attach(cxlr, cxled, pos);
	}
	
	guard(rwsem_write)(&cxl_rwsem.region);
	guard(rwsem_read)(&cxl_rwsem.dpa);
	return cxl_region_attach(cxlr, cxled, pos);		
}

static int attach_target(struct cxl_region *cxlr,
			 struct cxl_endpoint_decoder *cxled, int pos,
			 unsigned int state)
{
	int rc = do_attach_target(cxlr, cxled, pos, state);

	if (rc)
		dev_warn();
	
	return rc;
}

> +			guard(rwsem_read)(&cxl_rwsem.dpa);
> +			rc = cxl_region_attach(cxlr, cxled, pos);
> +		}
> +	} else {
> +		guard(rwsem_write)(&cxl_rwsem.region);
> +		guard(rwsem_read)(&cxl_rwsem.dpa);
> +		rc = cxl_region_attach(cxlr, cxled, pos);
> +	}
>  
>  	if (rc)
>  		dev_warn(cxled->cxld.dev.parent,


> @@ -3569,30 +3520,23 @@ static int cxl_region_can_probe(struct cxl_region *cxlr)
>  	struct cxl_region_params *p = &cxlr->params;
>  	int rc;
>  
> -	rc = down_read_interruptible(&cxl_region_rwsem);
> -	if (rc) {
> +	ACQUIRE(rwsem_read_intr, rwsem)(&cxl_rwsem.region);

Similar to earlier - I'd do this next bit in two lines for slightly
better readability.  Same for the other cases. I don't care that strongly
though.

> +	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &rwsem))) {
>  		dev_dbg(&cxlr->dev, "probe interrupted\n");
>  		return rc;
>  	}

Re: [PATCH v2 8/8] cxl: Convert to ACQUIRE() for conditional rwsem locking

[dan.j.williams]

Jonathan Cameron wrote:
> On Wed, 18 Jun 2025 22:04:16 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > Use ACQUIRE() to cleanup conditional locking paths in the CXL driver
> > The ACQUIRE() macro and its associated ACQUIRE_ERR() helpers, like
> > scoped_cond_guard(), arrange for scoped-based conditional locking. Unlike
> > scoped_cond_guard(), these macros arrange for an ERR_PTR() to be retrieved
> > representing the state of the conditional lock.
> > 
> > The goal of this conversion is to complete the removal of all explicit
> > unlock calls in the subsystem. I.e. the methods to acquire a lock are
> > solely via guard(), scoped_guard() (for limited cases), or ACQUIRE(). All
> > unlock is implicit / scope-based. In order to make sure all lock sites are
> > converted, the existing rwsem's are consolidated and renamed in 'struct
> > cxl_rwsem'. While that makes the patch noisier it gives a clean cut-off
> > between old-world (explicit unlock allowed), and new world (explicit unlock
> > deleted).
> > 
> > Cc: David Lechner <dlechner@baylibre.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Linus Torvalds <torvalds@linux-foundation.org>
> > Cc: Ingo Molnar <mingo@kernel.org>
> > Cc: "Fabio M. De Francesco" <fabio.maria.de.francesco@linux.intel.com>
> > Cc: Davidlohr Bueso <dave@stgolabs.net>
> > Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
> > Cc: Dave Jiang <dave.jiang@intel.com>
> > Cc: Alison Schofield <alison.schofield@intel.com>
> > Cc: Vishal Verma <vishal.l.verma@intel.com>
> > Cc: Ira Weiny <ira.weiny@intel.com>
> > Cc: Shiju Jose <shiju.jose@huawei.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> A few comments inline.
> 
> 
> 
> 
> > index 010964aa5489..a2ba19151d4f 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> 
> 
> >  static DEVICE_ATTR_RW(interleave_ways);
> > @@ -561,15 +537,11 @@ static ssize_t interleave_granularity_show(struct device *dev,
> >  {
> >  	struct cxl_region *cxlr = to_cxl_region(dev);
> >  	struct cxl_region_params *p = &cxlr->params;
> > -	ssize_t rc;
> > -
> > -	rc = down_read_interruptible(&cxl_region_rwsem);
> > -	if (rc)
> > -		return rc;
> > -	rc = sysfs_emit(buf, "%d\n", p->interleave_granularity);
> > -	up_read(&cxl_region_rwsem);
> >  
> > -	return rc;
> > +	ACQUIRE(rwsem_read_intr, rwsem)(&cxl_rwsem.region);
> > +	if (ACQUIRE_ERR(rwsem_read_intr, &rwsem))
> > +		return ACQUIRE_ERR(rwsem_read_intr, &rwsem);
> 
> Local variable?

Yeah, I think this was leftover from playing with ways to make this
more compact, will switch it to:

	if ((rc = ACQUIRE_ERR(@class, @lock))
		return rc;

> 
> > +	return sysfs_emit(buf, "%d\n", p->interleave_granularity);
> >  }
> 
> > @@ -2212,19 +2167,19 @@ static int attach_target(struct cxl_region *cxlr,
> >  			 struct cxl_endpoint_decoder *cxled, int pos,
> >  			 unsigned int state)
> >  {
> > -	int rc = 0;
> > -
> > -	if (state == TASK_INTERRUPTIBLE)
> > -		rc = down_write_killable(&cxl_region_rwsem);
> > -	else
> > -		down_write(&cxl_region_rwsem);
> > -	if (rc)
> > -		return rc;
> > +	int rc;
> >  
> > -	down_read(&cxl_dpa_rwsem);
> > -	rc = cxl_region_attach(cxlr, cxled, pos);
> > -	up_read(&cxl_dpa_rwsem);
> > -	up_write(&cxl_region_rwsem);
> > +	if (state == TASK_INTERRUPTIBLE) {
> > +		ACQUIRE(rwsem_write_kill, rwsem)(&cxl_rwsem.region);
> > +		if ((rc = ACQUIRE_ERR(rwsem_write_kill, &rwsem)) == 0) {
> 
> I'd lift the warning print to a wrapper function so that you can
> return early in error case and avoid this rather messy line.
> e.g.
> 
> static int do_attach_target(struct cxl_region *cxlr,
> 			    struct cxl_endpoint_decoder *cxled, int pos,
> 			    unsigned int state)
> 
> 	if (state == TASK_INTERRUPTIBLE) {
> 		ACQUIRE(rwsem_write_kill, rwsem)(&cxl_rwsem.region);
> 		rc = ACQUIRE_ERR(rwsem_write_kill, &rwsem);
> 		if (rc)
> 			return rc;
> 
> 		guard(rwsem_read)(&cxl_rwsem.dpa);
> 		return cxl_region_attach(cxlr, cxled, pos);
> 	}
> 	
> 	guard(rwsem_write)(&cxl_rwsem.region);
> 	guard(rwsem_read)(&cxl_rwsem.dpa);
> 	return cxl_region_attach(cxlr, cxled, pos);		
> }
> 
> static int attach_target(struct cxl_region *cxlr,
> 			 struct cxl_endpoint_decoder *cxled, int pos,
> 			 unsigned int state)
> {
> 	int rc = do_attach_target(cxlr, cxled, pos, state);
> 
> 	if (rc)
> 		dev_warn();
> 	
> 	return rc;
> }

Yes, I like that better.

> 
> > +			guard(rwsem_read)(&cxl_rwsem.dpa);
> > +			rc = cxl_region_attach(cxlr, cxled, pos);
> > +		}
> > +	} else {
> > +		guard(rwsem_write)(&cxl_rwsem.region);
> > +		guard(rwsem_read)(&cxl_rwsem.dpa);
> > +		rc = cxl_region_attach(cxlr, cxled, pos);
> > +	}
> >  
> >  	if (rc)
> >  		dev_warn(cxled->cxld.dev.parent,
> 
> 
> > @@ -3569,30 +3520,23 @@ static int cxl_region_can_probe(struct cxl_region *cxlr)
> >  	struct cxl_region_params *p = &cxlr->params;
> >  	int rc;
> >  
> > -	rc = down_read_interruptible(&cxl_region_rwsem);
> > -	if (rc) {
> > +	ACQUIRE(rwsem_read_intr, rwsem)(&cxl_rwsem.region);
> 
> Similar to earlier - I'd do this next bit in two lines for slightly
> better readability.  Same for the other cases. I don't care that strongly
> though.

I want to keep the compactness, if only in the CXL subsystem.

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

[Peter Zijlstra]

On Wed, Jun 18, 2025 at 10:04:08PM -0700, Dan Williams wrote:

> Note, all the code in patch1 is Peter's I just wrapped it in a changelog
> and added some commentary. Peter, forgive me if you were still in the
> process of circling back to this topic. I marked the patch attributed to
> you as: "Not-yet-signed-off-by". Otherwise, my motivation for going
> ahead with a formal submission are the multiple patchsets in my review /
> development queue where I would like to use ACQUIRE().

Definitely a case of too many balls in the air. Feel free to make that
Signed-off-by, and also:

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

for the patches that were sent my way. Thanks for the changelogs and
pushing this ahead.

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

[Alison Schofield]

On Wed, Jun 18, 2025 at 10:04:08PM -0700, Dan Williams wrote:
> Changes since v1: [1]
> * Peter took one look at v1 and rewrote it into something significantly
>   better. Unlike my attempt that required suffering a new parallel
>   universe of lock guards, the rewrite reuses existing lock guards.
>   ACQUIRE() can be used any place guard() can be used, and adds
>   ACQUIRE_ERR() to pass the result of conditional locks.
> 
> [1]: http://lore.kernel.org/20250507072145.3614298-1-dan.j.williams@intel.com
> 
> Note, all the code in patch1 is Peter's I just wrapped it in a changelog
> and added some commentary. Peter, forgive me if you were still in the
> process of circling back to this topic. I marked the patch attributed to
> you as: "Not-yet-signed-off-by". Otherwise, my motivation for going
> ahead with a formal submission are the multiple patchsets in my review /
> development queue where I would like to use ACQUIRE().
> 
> The orginal motivation of v1 for this work is that the CXL subsystem
> adopted scope-based helpers and achieved some decent cleanups. However,
> that work stalled with conditional locks. It stalled due to the pain
> points of scoped_cond_guard() detailed in patch1.
> 
> This work also allows for replacing open-coded equivalents like
> rwsem_read_intr_acquire() that went upstream in v6.16:
> 
>     0c6e6f1357cb cxl/edac: Add CXL memory device patrol scrub control feature
> 
> The open question from the discussion [2] was whether it was worth
> defining a __GUARD_IS_ERR() asm helper. I left that alone.
> 
> Lastly, this version of ACQUIRE_ERR() matches Peter's original proposal
> to have the caller pass the lock scope variable by reference [3]. My
> change of heart came from looking at the conversion and wanting
> ACQUIRE_ERR() to be more visually distinct from ACQUIRE() especially
> because it is accessing lock condition metadata, not the lock itself.
> 
> E.g.
> 
>        ACQUIRE(rwsem_read_intr, rwsem)(&cxl_rwsem.region);
>        if ((ret = ACQUIRE_ERR(rwsem_read_intr, &rwsem)))
>                return ret;
> 
> Yes, checkpatch disagrees with assignment in if(), but cleanup.h already
> demands other expections for historical style, and a compact / limited
> idiom for ACQUIRE_ERR() feels reasonable.

Hi Dan,

I've been building upon this set and applying this diff to squelch
those checkpatch ERRORs. Please take a look and consider adding for
review in next version.

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 664f7b7a622c..193a03fa7114 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5682,7 +5682,14 @@ sub process {
 			my ($s, $c) = ($stat, $cond);
 			my $fixed_assign_in_if = 0;
 
-			if ($c =~ /\bif\s*\(.*[^<>!=]=[^=].*/s) {
+			if ($c =~ /\bif\s*\((.*[^<>!=]=[^=].*)\)/s) {
+				my $expr = $1;
+
+				# Allow ACQUIRE_ERR() special case
+				if ($expr =~ /\w+\s*=\s*ACQUIRE_ERR\s*\(/) {
+					next;
+				}
+
 				if (ERROR("ASSIGN_IN_IF",
 					  "do not use assignment in if condition\n" . $herecurr) &&
 				    $fix && $perl_version_ok) {



snip

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

[dan.j.williams]

Alison Schofield wrote:
> On Wed, Jun 18, 2025 at 10:04:08PM -0700, Dan Williams wrote:
> > Changes since v1: [1]
> > * Peter took one look at v1 and rewrote it into something significantly
> >   better. Unlike my attempt that required suffering a new parallel
> >   universe of lock guards, the rewrite reuses existing lock guards.
> >   ACQUIRE() can be used any place guard() can be used, and adds
> >   ACQUIRE_ERR() to pass the result of conditional locks.
> > 
> > [1]: http://lore.kernel.org/20250507072145.3614298-1-dan.j.williams@intel.com
> > 
> > Note, all the code in patch1 is Peter's I just wrapped it in a changelog
> > and added some commentary. Peter, forgive me if you were still in the
> > process of circling back to this topic. I marked the patch attributed to
> > you as: "Not-yet-signed-off-by". Otherwise, my motivation for going
> > ahead with a formal submission are the multiple patchsets in my review /
> > development queue where I would like to use ACQUIRE().
> > 
> > The orginal motivation of v1 for this work is that the CXL subsystem
> > adopted scope-based helpers and achieved some decent cleanups. However,
> > that work stalled with conditional locks. It stalled due to the pain
> > points of scoped_cond_guard() detailed in patch1.
> > 
> > This work also allows for replacing open-coded equivalents like
> > rwsem_read_intr_acquire() that went upstream in v6.16:
> > 
> >     0c6e6f1357cb cxl/edac: Add CXL memory device patrol scrub control feature
> > 
> > The open question from the discussion [2] was whether it was worth
> > defining a __GUARD_IS_ERR() asm helper. I left that alone.
> > 
> > Lastly, this version of ACQUIRE_ERR() matches Peter's original proposal
> > to have the caller pass the lock scope variable by reference [3]. My
> > change of heart came from looking at the conversion and wanting
> > ACQUIRE_ERR() to be more visually distinct from ACQUIRE() especially
> > because it is accessing lock condition metadata, not the lock itself.
> > 
> > E.g.
> > 
> >        ACQUIRE(rwsem_read_intr, rwsem)(&cxl_rwsem.region);
> >        if ((ret = ACQUIRE_ERR(rwsem_read_intr, &rwsem)))
> >                return ret;
> > 
> > Yes, checkpatch disagrees with assignment in if(), but cleanup.h already
> > demands other expections for historical style, and a compact / limited
> > idiom for ACQUIRE_ERR() feels reasonable.
> 
> Hi Dan,
> 
> I've been building upon this set and applying this diff to squelch
> those checkpatch ERRORs. Please take a look and consider adding for
> review in next version.
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 664f7b7a622c..193a03fa7114 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -5682,7 +5682,14 @@ sub process {
>  			my ($s, $c) = ($stat, $cond);
>  			my $fixed_assign_in_if = 0;
>  
> -			if ($c =~ /\bif\s*\(.*[^<>!=]=[^=].*/s) {
> +			if ($c =~ /\bif\s*\((.*[^<>!=]=[^=].*)\)/s) {
> +				my $expr = $1;
> +
> +				# Allow ACQUIRE_ERR() special case
> +				if ($expr =~ /\w+\s*=\s*ACQUIRE_ERR\s*\(/) {
> +					next;
> +				}
> +

Thanks! This lookls like a good fixup to send after ACQUIRE_ERR() moves
upstream.  Should probably go with a wider set to update checkpatch's
understanding of other scoped-based-macros like DEFINE_{FREE,GUARD}().