Re: [PATCH v16 10/10] cxl: Enable CXL protocol error reporting

[Bowman, Terry]

On 3/29/2026 8:41 PM, Dan Williams wrote:
> Terry Bowman wrote:
>> CXL protocol errors are not enabled for all CXL devices after boot. These
>> must be enabled inorder to process CXL protocol errors.
>>
>> Introduce cxl_unmask_proto_interrupts() to call pci_aer_unmask_internal_errors().
>> pci_aer_unmask_internal_errors() expects the pdev->aer_cap is initialized.
>> But, dev->aer_cap is not initialized for CXL Upstream Switch Ports and CXL
>> Downstream Switch Ports. Initialize the dev->aer_cap if necessary. Enable AER
>> correctable internal errors and uncorrectable internal errors for all CXL
>> devices.
>>
>> Signed-off-by: Terry Bowman <terry.bowman@xxxxxxx>
>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
>> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
>> Reviewed-by: Dave Jiang <dave.jiang@xxxxxxxxx>
>> Reviewed-by: Ben Cheatham <benjamin.cheatham@xxxxxxx>
>>
>> ---
>>  drivers/cxl/core/port.c |  2 ++
>>  drivers/cxl/core/ras.c  | 22 ++++++++++++++++++++++
>>  drivers/cxl/cxlpci.h    |  4 ++++
>>  3 files changed, 28 insertions(+)
>>
>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>> index 27271402915f..c33d58fb7264 100644
>> --- a/drivers/cxl/core/port.c
>> +++ b/drivers/cxl/core/port.c
>> @@ -1852,6 +1852,8 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
>>  
>>  			rc = cxl_add_ep(dport, &cxlmd->dev);
>>  
>> +			cxl_unmask_proto_interrupts(cxlmd->cxlds->dev);
>> +
> 
> Why here? devm_cxl_port_ras_setup() will just redo it, right?
> 

No, I found this change is needed otherwise injection fails.

>>  			/*
>>  			 * If the endpoint already exists in the port's list,
>>  			 * that's ok, it was added on a previous pass.
>> diff --git a/drivers/cxl/core/ras.c b/drivers/cxl/core/ras.c
>> index d6112b812c82..bfe6cb35154e 100644
>> --- a/drivers/cxl/core/ras.c
>> +++ b/drivers/cxl/core/ras.c
>> @@ -119,6 +119,24 @@ static void cxl_cper_prot_err_work_fn(struct work_struct *work)
>>  }
>>  static DECLARE_WORK(cxl_cper_prot_err_work, cxl_cper_prot_err_work_fn);
>>  
>> +void cxl_unmask_proto_interrupts(struct device *dev)
>> +{
>> +	if (!dev || !dev_is_pci(dev))
>> +		return;
>> +
>> +	struct pci_dev *pdev __free(pci_dev_put) = pci_dev_get(to_pci_dev(dev));
> 
> Why does this need to hold a reference for a simple to_pci_dev() conversion?
> 
It is unnecessary and can be removed. 

>> +
>> +	if (!pdev->aer_cap) {
>> +		pdev->aer_cap = pci_find_ext_capability(pdev,
>> +							PCI_EXT_CAP_ID_ERR);
>> +		if (!pdev->aer_cap)
>> +			return;
>> +	}
>> +
>> +	pci_aer_unmask_internal_errors(pdev);
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_unmask_proto_interrupts, "CXL");
> 
> Why export? The only callers are internal to cxl_core.ko?
> 

Export can be taken off. This was changed to export when this was called from 
another CXL module in previous iterations.

- Terry

> ---
>  drivers/cxl/cxlpci.h    | 4 ----
>  drivers/cxl/core/port.c | 2 --
>  drivers/cxl/core/ras.c  | 3 +--
>  3 files changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h
> index b7cf9d6137b3..184a95e96ea9 100644
> --- a/drivers/cxl/cxlpci.h
> +++ b/drivers/cxl/cxlpci.h
> @@ -82,7 +82,6 @@ void devm_cxl_dport_rch_ras_setup(struct cxl_dport *dport);
>  pci_ers_result_t cxl_pci_error_detected(struct pci_dev *pdev,
>  					pci_channel_state_t error);
>  void devm_cxl_port_ras_setup(struct cxl_port *port);
> -void cxl_unmask_proto_interrupts(struct device *dev);
>  #else
>  static inline pci_ers_result_t cxl_pci_error_detected(struct pci_dev *pdev,
>  						      pci_channel_state_t state)
> @@ -97,9 +96,6 @@ static inline void devm_cxl_dport_rch_ras_setup(struct cxl_dport *dport)
>  static inline void devm_cxl_port_ras_setup(struct cxl_port *port)
>  {
>  }
> -static inline void cxl_unmask_proto_interrupts(struct device *dev)
> -{
> -}
>  #endif
>  
>  #endif /* __CXL_PCI_H__ */
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index c33d58fb7264..27271402915f 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -1852,8 +1852,6 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd)
>  
>  			rc = cxl_add_ep(dport, &cxlmd->dev);
>  
> -			cxl_unmask_proto_interrupts(cxlmd->cxlds->dev);
> -
>  			/*
>  			 * If the endpoint already exists in the port's list,
>  			 * that's ok, it was added on a previous pass.
> diff --git a/drivers/cxl/core/ras.c b/drivers/cxl/core/ras.c
> index bfe6cb35154e..1f688110f0f4 100644
> --- a/drivers/cxl/core/ras.c
> +++ b/drivers/cxl/core/ras.c
> @@ -119,7 +119,7 @@ static void cxl_cper_prot_err_work_fn(struct work_struct *work)
>  }
>  static DECLARE_WORK(cxl_cper_prot_err_work, cxl_cper_prot_err_work_fn);
>  
> -void cxl_unmask_proto_interrupts(struct device *dev)
> +static void cxl_unmask_proto_interrupts(struct device *dev)
>  {
>  	if (!dev || !dev_is_pci(dev))
>  		return;
> @@ -135,7 +135,6 @@ void cxl_unmask_proto_interrupts(struct device *dev)
>  
>  	pci_aer_unmask_internal_errors(pdev);
>  }
> -EXPORT_SYMBOL_NS_GPL(cxl_unmask_proto_interrupts, "CXL");
>  
>  static void cxl_dport_map_ras(struct cxl_dport *dport)
>  {