cxl_dpa_to_region() will require callers holding the given CXL memdev
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.

Suggested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Li Ming <ming.li@zohomail.com>
---
 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, &region_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, &region_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, &region_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

On Tue, Mar 10, 2026 at 11:57:55PM +0800, Li Ming wrote:
> cxl_dpa_to_region() will require callers holding the given CXL memdev
> 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.

This seems complicated, that this path needs to do needless work to
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 ;)


> 
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Li Ming <ming.li@zohomail.com>
> ---
>  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, &region_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, &region_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, &region_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
>

在 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 memdev
>> 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.
> This seems complicated, that this path needs to do needless work to
> 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@intel.com>
>> Signed-off-by: Li Ming <ming.li@zohomail.com>
>> ---
>>   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, &region_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, &region_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, &region_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
>>

Li Ming wrote:
> 
> 在 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 memdev
> >> 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.
> > This seems complicated, that this path needs to do needless work to
> > 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.

The question is can cxlmd->dev.driver be invalidated while userspace is
pending inside of a debugfs callback?

Yes, it can. Debugfs, unlike sysfs which uses kernfs_drain(), does not
flush active threads inside debugfs handlers. At least not as far as I
can see.

The next question is, are we protected by the fact that a region needs
to disconnect a decoder and it only does that under the rwsem held for
write?

I think the answer is "no".

If the protocol was to have a region in hand and then validate its
locked association to an endpoint decoder then, yes. In this case though
we have no idea if the cxlmd is associated with any region at all.  So I
think you can theoretically race injecting an error to an idle cxlmd
that is in the process of destroying its ->endpoint.

Now, maybe some other detail saves us, but I do not think it is worth
the mental gymnastics of just requiring that no driver detach
shenanigans are running concurrent with poison injection shenanigans.

Li Ming wrote:
> cxl_dpa_to_region() will require callers holding the given CXL memdev
> lock for endpoint validity checking in the following patch.

So, the justification for this is not that cxl_dpa_to_region() is going
to start enforcing lock holding, it is that cxl_dpa_to_region() has
expectations that ->endpoint remains valid for the duration of the call.

> To prepare
> it, region poison injection/clearing debugfs interfaces need to ensure
> the correct CXL memdev lock is held at the beginning.

My RFC [1] suggestion was "if there is a problem, here is locking
changes to fix it". That "if there is a problem" is what needed more thought
and time. Before we act on that RFC I want to see some plausible
analysis of how not holding this lock can cause a failure in theory
(better in practice), or how it makes the code hard to reason about.

In other words the justification is not "future patch adds a lockdep
assert so all code paths need to get more complicated". The
justification is identifying a theoretical race window and how the
locking solves it, and how the previous fix [2] did not handle all the
potential races.

[1]: http://lore.kernel.org/69813ac070f79_55fa1005c@dwillia2-mobl4.notmuch
[2]: 0066688dbcdc cxl/port: Hold port host lock during dport adding.