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

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

Shiju Jose wrote:
> >-----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"

Yes, the proposal is that the rule be relaxed for ACQUIRE_ERR()
compactness similar to how the C99 variable declaration expectations
were relaxed for __free().

Prior discussion with Jonathan [1] and Alison [2] about this. 

[1]: http://lore.kernel.org/6870833aa1344_588c100dd@dwillia2-xfh.jf.intel.com.notmuch
[2]: http://lore.kernel.org/aGXDMZB6omShJpoj@aschofie-mobl2.lan

In general, checkpatch also needs to catch up with cleanup macros like
DEFINE_FREE. So, I think that "checkpatch updates for cleanup.h" is a
worthwhile discussion as these helpers get more use.

> [...]
> > 		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?

Recall that the lock is only held within its bracketed scope. So this...

	if (mode == DETACH_INVALIDATE) {
		guard(rwsem_write)(&cxl_rwsem.region);
	} else {
		int 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);

...would be equivalent to:

	if (mode == DETACH_INVALIDATE) {
		down_write(&cxl_rwsem.region);
		up_write(&cxl_rwsem.region);
	} else {
		int rc = down_write_killable(&cxl_rwsem.region);

		if (rc)
			return rc;
		up_write(&cxl_rwsem.region);
	}
	detach = __cxl_decoder_detach(cxlr, cxled, pos, mode);