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

[Li Ming <ming.li@zohomail.com>]

On 7/3/2025 3:38 AM, Alison Schofield wrote:
> On Wed, Jul 02, 2025 at 03:20:07PM +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.
>>
>> Fixes: be9b359e056a ("cxl/edac: Add CXL memory device soft PPR control feature")
>> Signed-off-by: Li Ming <ming.li@zohomail.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;
> Hi Ming,
>
> I think this one needs a user visible impact statement.
>
> I'm hoping the broader helper gets accepted. That may be the ioport.h
> addition, or maybe we end up with a CXL special helper.
>
> However, if this patch is aiming to go upstream as a FIX in a 6.16 rc,
> then we are probably better off fixing the check inline right here, and
> then you follow on with the other 2 patches to be considered for the
> next merge window.
>
> Please share that impact and suggest whether it can wait for next merge
> window.
>
> -- Alison
>
Hi Alison,


I think the impact of this issue is limited, because the issue is inside CXL edac part, just causes that user cannot issue PPR maintenance operation for the DPA 0 of a CXL device. (A PPR maintenance operation requests the CXL device to perform a repair operation on its media.)

I am not sure if cxl subsystem will have another fixes PR for 6.16 rc, my understanding is that it is not worth to submit an additional PR only for this issue. So  I think merging it in next merge window is also good.


Ming

>>  
>>  	return cxl_mem_perform_ppr(cxl_ppr_ctx);
>> -- 
>> 2.34.1
>>