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

[Shiju Jose <shiju.jose@huawei.com>]

>-----Original Message-----
>From: Dan Williams <dan.j.williams@intel.com>
>Sent: 12 July 2025 00:50
>To: linux-cxl@vger.kernel.org
>Cc: linux-kernel@vger.kernel.org; David Lechner <dlechner@baylibre.com>;
>Peter Zijlstra <peterz@infradead.org>; Linus Torvalds <torvalds@linux-
>foundation.org>; Ingo Molnar <mingo@kernel.org>; Fabio M. De Francesco
><fabio.m.de.francesco@linux.intel.com>; Davidlohr Bueso <dave@stgolabs.net>;
>Jonathan Cameron <jonathan.cameron@huawei.com>; Dave Jiang
><dave.jiang@intel.com>; Alison Schofield <alison.schofield@intel.com>; Vishal
>Verma <vishal.l.verma@intel.com>; Ira Weiny <ira.weiny@intel.com>; Shiju Jose
><shiju.jose@huawei.com>
>Subject: [PATCH v3 8/8] cxl: Convert to ACQUIRE() for conditional rwsem locking
>
>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.m.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>
>Acked-by: "Peter Zijlstra (Intel)" <peterz@infradead.org>
>Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Hi Dan,

For changes in CXL EDAC (drivers/cxl/core/edac.c),
Tested-by: Shiju Jose <shiju.jose@huawei.com>

>---
> drivers/cxl/core/cdat.c   |   6 +-
> drivers/cxl/core/core.h   |  17 ++-
> 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 | 295 ++++++++++++++++----------------------
> drivers/cxl/cxl.h         |  13 +-
> include/linux/rwsem.h     |   1 +
> 10 files changed, 212 insertions(+), 279 deletions(-)
>
[...]
>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)))
Checkpatch is giving error here and in other places with similar coding style,  
"ERROR: do not use assignment in if condition"

>+		return ret;
>
[...]
> 		p = &cxlr->params;
>@@ -2215,18 +2173,18 @@ int cxl_decoder_detach(struct cxl_region *cxlr,
> 	struct cxl_region *detach;
>
> 	/* when the decoder is being destroyed lock unconditionally */
>-	if (mode == DETACH_INVALIDATE)
>-		down_write(&cxl_region_rwsem);
>-	else {
>-		int rc = down_write_killable(&cxl_region_rwsem);
>+	if (mode == DETACH_INVALIDATE) {
>+		guard(rwsem_write)(&cxl_rwsem.region);
>+		detach = __cxl_decoder_detach(cxlr, cxled, pos, mode);
>+	} else {
>+		int rc;
>
>-		if (rc)
>+		ACQUIRE(rwsem_write_kill, rwsem)(&cxl_rwsem.region);
>+		if ((rc = ACQUIRE_ERR(rwsem_write_kill, &rwsem)))
> 			return rc;
>+		detach = __cxl_decoder_detach(cxlr, cxled, pos, mode);
> 	}
May be detach = __cxl_decoder_detach(cxlr, cxled, pos, mode);
add outside if ... else as before?
>
>-	detach = __cxl_decoder_detach(cxlr, cxled, pos, mode);
>-	up_write(&cxl_region_rwsem);
>-
> 	if (detach) {
> 		device_release_driver(&detach->dev);
> 		put_device(&detach->dev);
>@@ -2234,29 +2192,35 @@ int cxl_decoder_detach(struct cxl_region *cxlr,
> 	return 0;
> }
[...]
> /*
>  * downgrade write lock to read lock
>--
>2.50.0
>

Thanks,
Shiju