1. Patchew
  2. linux
  3. [PATCH v3 0/3] Fix wrong dpa checking in PPR operation
  4. View patch

[PATCH v3 2/3] cxl/edac: Fix wrong dpa checking for PPR operation

Li Ming posted 3 patches 3 months ago
There is a newer version of this series
[PATCH v3 2/3] cxl/edac: Fix wrong dpa checking for PPR operation
Posted by Li Ming 3 months ago 
DPA 0 is considered invalid in cxl_do_ppr(), but per Table 8-143. "Get
Partition Info Output Payload" in CXL r3.2 section 8.2.10.9.2.1 "Get
Partition Info(Opcode 4100h)", it mentions that DPA 0 is a valid address
of a CXL device. So the correct implementation should be checking if the
DPA is in the DPA range of the CXL device rather than checking if the
DPA is equal to 0.

Fixes: be9b359e056a ("cxl/edac: Add CXL memory device soft PPR control feature")
Signed-off-by: Li Ming <ming.li@zohomail.com>
Tested-by: Shiju Jose <shiju.jose@huawei.com>
Reviewed-by: Shiju Jose <shiju.jose@huawei.com>
---
 drivers/cxl/core/edac.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/cxl/core/edac.c b/drivers/cxl/core/edac.c
index 623aaa4439c4..1cf65b1538b9 100644
--- a/drivers/cxl/core/edac.c
+++ b/drivers/cxl/core/edac.c
@@ -1923,8 +1923,11 @@ static int cxl_ppr_set_nibble_mask(struct device *dev, void *drv_data,
 static int cxl_do_ppr(struct device *dev, void *drv_data, u32 val)
 {
 	struct cxl_ppr_context *cxl_ppr_ctx = drv_data;
+	struct cxl_memdev *cxlmd = cxl_ppr_ctx->cxlmd;
+	struct cxl_dev_state *cxlds = cxlmd->cxlds;
 
-	if (!cxl_ppr_ctx->dpa || val != EDAC_DO_MEM_REPAIR)
+	if (!resource_contains_addr(&cxlds->dpa_res, cxl_ppr_ctx->dpa) ||
+	    val != EDAC_DO_MEM_REPAIR)
 		return -EINVAL;
 
 	return cxl_mem_perform_ppr(cxl_ppr_ctx);
-- 
2.34.1
Re: [PATCH v3 2/3] cxl/edac: Fix wrong dpa checking for PPR operation
Posted by Alison Schofield 3 months ago 
On Tue, Jul 08, 2025 at 01:15:35PM +0800, Li Ming wrote:
> DPA 0 is considered invalid in cxl_do_ppr(), but per Table 8-143. "Get
> Partition Info Output Payload" in CXL r3.2 section 8.2.10.9.2.1 "Get
> Partition Info(Opcode 4100h)", it mentions that DPA 0 is a valid address
> of a CXL device. So the correct implementation should be checking if the
> DPA is in the DPA range of the CXL device rather than checking if the
> DPA is equal to 0.
> 

If it needs a fixes tag, doesn't it also need a user visible impact
statement? I get that the PPR won't happen. What does that look like
to the user? Is there a user level error message we can add to the
commit log?

With that, you can add:
Reviewed-by: Alison Schofield <alison.schofield@intel.com>



> Fixes: be9b359e056a ("cxl/edac: Add CXL memory device soft PPR control feature")
> Signed-off-by: Li Ming <ming.li@zohomail.com>
> Tested-by: Shiju Jose <shiju.jose@huawei.com>
> Reviewed-by: Shiju Jose <shiju.jose@huawei.com>
> ---
>  drivers/cxl/core/edac.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/edac.c b/drivers/cxl/core/edac.c
> index 623aaa4439c4..1cf65b1538b9 100644
> --- a/drivers/cxl/core/edac.c
> +++ b/drivers/cxl/core/edac.c
> @@ -1923,8 +1923,11 @@ static int cxl_ppr_set_nibble_mask(struct device *dev, void *drv_data,
>  static int cxl_do_ppr(struct device *dev, void *drv_data, u32 val)
>  {
>  	struct cxl_ppr_context *cxl_ppr_ctx = drv_data;
> +	struct cxl_memdev *cxlmd = cxl_ppr_ctx->cxlmd;
> +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>  
> -	if (!cxl_ppr_ctx->dpa || val != EDAC_DO_MEM_REPAIR)
> +	if (!resource_contains_addr(&cxlds->dpa_res, cxl_ppr_ctx->dpa) ||
> +	    val != EDAC_DO_MEM_REPAIR)
>  		return -EINVAL;
>  
>  	return cxl_mem_perform_ppr(cxl_ppr_ctx);
> -- 
> 2.34.1
>
Re: [PATCH v3 2/3] cxl/edac: Fix wrong dpa checking for PPR operation
Posted by Li Ming 3 months ago 
On 7/9/2025 9:42 AM, Alison Schofield wrote:
> On Tue, Jul 08, 2025 at 01:15:35PM +0800, Li Ming wrote:
>> DPA 0 is considered invalid in cxl_do_ppr(), but per Table 8-143. "Get
>> Partition Info Output Payload" in CXL r3.2 section 8.2.10.9.2.1 "Get
>> Partition Info(Opcode 4100h)", it mentions that DPA 0 is a valid address
>> of a CXL device. So the correct implementation should be checking if the
>> DPA is in the DPA range of the CXL device rather than checking if the
>> DPA is equal to 0.
>>
> If it needs a fixes tag, doesn't it also need a user visible impact
> statement? I get that the PPR won't happen. What does that look like
> to the user? Is there a user level error message we can add to the
> commit log?
>
> With that, you can add:
> Reviewed-by: Alison Schofield <alison.schofield@intel.com>
>
Will do. Thanks.


Ming

>
>> Fixes: be9b359e056a ("cxl/edac: Add CXL memory device soft PPR control feature")
>> Signed-off-by: Li Ming <ming.li@zohomail.com>
>> Tested-by: Shiju Jose <shiju.jose@huawei.com>
>> Reviewed-by: Shiju Jose <shiju.jose@huawei.com>
>> ---
>>  drivers/cxl/core/edac.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/cxl/core/edac.c b/drivers/cxl/core/edac.c
>> index 623aaa4439c4..1cf65b1538b9 100644
>> --- a/drivers/cxl/core/edac.c
>> +++ b/drivers/cxl/core/edac.c
>> @@ -1923,8 +1923,11 @@ static int cxl_ppr_set_nibble_mask(struct device *dev, void *drv_data,
>>  static int cxl_do_ppr(struct device *dev, void *drv_data, u32 val)
>>  {
>>  	struct cxl_ppr_context *cxl_ppr_ctx = drv_data;
>> +	struct cxl_memdev *cxlmd = cxl_ppr_ctx->cxlmd;
>> +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>>  
>> -	if (!cxl_ppr_ctx->dpa || val != EDAC_DO_MEM_REPAIR)
>> +	if (!resource_contains_addr(&cxlds->dpa_res, cxl_ppr_ctx->dpa) ||
>> +	    val != EDAC_DO_MEM_REPAIR)
>>  		return -EINVAL;
>>  
>>  	return cxl_mem_perform_ppr(cxl_ppr_ctx);
>> -- 
>> 2.34.1
>>
Re: [PATCH v3 2/3] cxl/edac: Fix wrong dpa checking for PPR operation
Posted by Dave Jiang 3 months ago 

On 7/7/25 10:15 PM, Li Ming wrote:
> DPA 0 is considered invalid in cxl_do_ppr(), but per Table 8-143. "Get
> Partition Info Output Payload" in CXL r3.2 section 8.2.10.9.2.1 "Get
> Partition Info(Opcode 4100h)", it mentions that DPA 0 is a valid address
> of a CXL device. So the correct implementation should be checking if the
> DPA is in the DPA range of the CXL device rather than checking if the
> DPA is equal to 0.
> 
> Fixes: be9b359e056a ("cxl/edac: Add CXL memory device soft PPR control feature")
> Signed-off-by: Li Ming <ming.li@zohomail.com>
> Tested-by: Shiju Jose <shiju.jose@huawei.com>
> Reviewed-by: Shiju Jose <shiju.jose@huawei.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/core/edac.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/edac.c b/drivers/cxl/core/edac.c
> index 623aaa4439c4..1cf65b1538b9 100644
> --- a/drivers/cxl/core/edac.c
> +++ b/drivers/cxl/core/edac.c
> @@ -1923,8 +1923,11 @@ static int cxl_ppr_set_nibble_mask(struct device *dev, void *drv_data,
>  static int cxl_do_ppr(struct device *dev, void *drv_data, u32 val)
>  {
>  	struct cxl_ppr_context *cxl_ppr_ctx = drv_data;
> +	struct cxl_memdev *cxlmd = cxl_ppr_ctx->cxlmd;
> +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>  
> -	if (!cxl_ppr_ctx->dpa || val != EDAC_DO_MEM_REPAIR)
> +	if (!resource_contains_addr(&cxlds->dpa_res, cxl_ppr_ctx->dpa) ||
> +	    val != EDAC_DO_MEM_REPAIR)
>  		return -EINVAL;
>  
>  	return cxl_mem_perform_ppr(cxl_ppr_ctx);

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.