Re: [PATCH 3/7] cxl/region: Hold memdev lock during region poison injection/clear
From: Li Ming
Date: Wed Mar 11 2026 - 07:15:48 EST
在 2026/3/11 05:57, Alison Schofield 写道:
On Tue, Mar 10, 2026 at 11:57:55PM +0800, Li Ming wrote:
cxl_dpa_to_region() will require callers holding the given CXL memdevThis seems complicated, that this path needs to do needless work to
lock for endpoint validity checking in the following patch. To prepare
it, region poison injection/clearing debugfs interfaces need to ensure
the correct CXL memdev lock is held at the beginning.
To keep lock sequence(cxlmd.dev -> cxl_rwsem.region -> cxl_rwsem.dpa)
for avoiding deadlock. the interfaces have to find out the correct CXL
memdev at first, holding lock in the sequence then checking if the DPA
data has been changed before holding locks.
comply with a new locking scheme that this path doesn't really need.
One thought, if in Patch 2 we lock at the debugfs callsite for
by memdev poison, then here, we don't need to think about following
unnecessary locking protocol. I say that because I think locking
down w dpa and region rwsem is enough here. Then there is nothing
new to do in this path. Might that work?
Along w that can cxl_dpa_to_region() go back to safety checking as
opposed to the new device_lock_assert?
I haven't gone thru every case in this set, so might be way off ;)
Actually, my understanding is same as yours.
I think we don't need this patch either if we remove the device_lock_assert() in cxl_dpa_to_region().(Just keep the cxlmd->dev.driver checking)
Because region is created only when endpoint is ready. Detaching an endpoint from a region always holds cxl_rwsem.region.
But we always hold device lock for the device driver bingding checking in CXL subsystem.
I'm OK to remove this patch, Let see any inputs from other reviewers.
Ming
Suggested-by: Dan Williams <dan.j.williams@xxxxxxxxx>
Signed-off-by: Li Ming <ming.li@xxxxxxxxxxxx>
---
drivers/cxl/core/region.c | 101 +++++++++++++++++++++++++++++++++++-----------
1 file changed, 77 insertions(+), 24 deletions(-)
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 42874948b589..806512dfab07 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -4074,12 +4074,60 @@ static int validate_region_offset(struct cxl_region *cxlr, u64 offset)
return 0;
}
+static int cxl_region_poison_lookup_locked(struct cxl_region *cxlr, u64 offset,
+ struct dpa_result *res)
+{
+ int rc;
+
+ *res = (struct dpa_result){ .dpa = ULLONG_MAX, .cxlmd = NULL };
+
+ if (validate_region_offset(cxlr, offset))
+ return -EINVAL;
+
+ offset -= cxlr->params.cache_size;
+ rc = region_offset_to_dpa_result(cxlr, offset, res);
+ if (rc || !res->cxlmd || res->dpa == ULLONG_MAX) {
+ dev_dbg(&cxlr->dev,
+ "Failed to resolve DPA for region offset %#llx rc %d\n",
+ offset, rc);
+
+ return rc ? rc : -EINVAL;
+ }
+
+ return 0;
+}
+
+static int cxl_region_poison_lookup(struct cxl_region *cxlr, u64 offset,
+ struct dpa_result *res)
+{
+ int rc;
+
+ ACQUIRE(rwsem_read_intr, region_rwsem)(&cxl_rwsem.region);
+ if ((rc = ACQUIRE_ERR(rwsem_read_intr, ®ion_rwsem)))
+ return rc;
+
+ ACQUIRE(rwsem_read_intr, dpa_rwsem)(&cxl_rwsem.dpa);
+ if ((rc = ACQUIRE_ERR(rwsem_read_intr, &dpa_rwsem)))
+ return rc;
+
+ return cxl_region_poison_lookup_locked(cxlr, offset, res);
+}
+
static int cxl_region_debugfs_poison_inject(void *data, u64 offset)
{
- struct dpa_result result = { .dpa = ULLONG_MAX, .cxlmd = NULL };
struct cxl_region *cxlr = data;
+ struct dpa_result res1, res2;
int rc;
+ /* To retrieve the correct memdev */
+ rc = cxl_region_poison_lookup(cxlr, offset, &res1);
+ if (rc)
+ return rc;
+
+ ACQUIRE(device_intr, devlock)(&res1.cxlmd->dev);
+ if ((rc = ACQUIRE_ERR(device_intr, &devlock)))
+ return rc;
+
ACQUIRE(rwsem_read_intr, region_rwsem)(&cxl_rwsem.region);
if ((rc = ACQUIRE_ERR(rwsem_read_intr, ®ion_rwsem)))
return rc;
@@ -4088,20 +4136,18 @@ static int cxl_region_debugfs_poison_inject(void *data, u64 offset)
if ((rc = ACQUIRE_ERR(rwsem_read_intr, &dpa_rwsem)))
return rc;
- if (validate_region_offset(cxlr, offset))
- return -EINVAL;
-
- offset -= cxlr->params.cache_size;
- rc = region_offset_to_dpa_result(cxlr, offset, &result);
- if (rc || !result.cxlmd || result.dpa == ULLONG_MAX) {
+ /*
+ * Retrieve memdev and DPA data again in case that the data
+ * has been changed before holding locks.
+ */
+ rc = cxl_region_poison_lookup_locked(cxlr, offset, &res2);
+ if (rc || res2.cxlmd != res1.cxlmd || res2.dpa != res1.dpa) {
dev_dbg(&cxlr->dev,
- "Failed to resolve DPA for region offset %#llx rc %d\n",
- offset, rc);
-
- return rc ? rc : -EINVAL;
+ "Error injection raced region reconfiguration: %d", rc);
+ return -ENXIO;
}
- return cxl_inject_poison_locked(result.cxlmd, result.dpa);
+ return cxl_inject_poison_locked(res2.cxlmd, res2.dpa);
}
DEFINE_DEBUGFS_ATTRIBUTE(cxl_poison_inject_fops, NULL,
@@ -4109,10 +4155,19 @@ DEFINE_DEBUGFS_ATTRIBUTE(cxl_poison_inject_fops, NULL,
static int cxl_region_debugfs_poison_clear(void *data, u64 offset)
{
- struct dpa_result result = { .dpa = ULLONG_MAX, .cxlmd = NULL };
struct cxl_region *cxlr = data;
+ struct dpa_result res1, res2;
int rc;
+ /* To retrieve the correct memdev */
+ rc = cxl_region_poison_lookup(cxlr, offset, &res1);
+ if (rc)
+ return rc;
+
+ ACQUIRE(device_intr, devlock)(&res1.cxlmd->dev);
+ if ((rc = ACQUIRE_ERR(device_intr, &devlock)))
+ return rc;
+
ACQUIRE(rwsem_read_intr, region_rwsem)(&cxl_rwsem.region);
if ((rc = ACQUIRE_ERR(rwsem_read_intr, ®ion_rwsem)))
return rc;
@@ -4121,20 +4176,18 @@ static int cxl_region_debugfs_poison_clear(void *data, u64 offset)
if ((rc = ACQUIRE_ERR(rwsem_read_intr, &dpa_rwsem)))
return rc;
- if (validate_region_offset(cxlr, offset))
- return -EINVAL;
-
- offset -= cxlr->params.cache_size;
- rc = region_offset_to_dpa_result(cxlr, offset, &result);
- if (rc || !result.cxlmd || result.dpa == ULLONG_MAX) {
+ /*
+ * Retrieve memdev and DPA data again in case that the data
+ * has been changed before holding locks.
+ */
+ rc = cxl_region_poison_lookup_locked(cxlr, offset, &res2);
+ if (rc || res2.cxlmd != res1.cxlmd || res2.dpa != res1.dpa) {
dev_dbg(&cxlr->dev,
- "Failed to resolve DPA for region offset %#llx rc %d\n",
- offset, rc);
-
- return rc ? rc : -EINVAL;
+ "Error clearing raced region reconfiguration: %d", rc);
+ return -ENXIO;
}
- return cxl_clear_poison_locked(result.cxlmd, result.dpa);
+ return cxl_clear_poison_locked(res2.cxlmd, res2.dpa);
}
DEFINE_DEBUGFS_ATTRIBUTE(cxl_poison_clear_fops, NULL,
--
2.43.0