Re: [PATCH v8 16/16] CXL/PCI: Disable CXL protocol errors during CXL Port cleanup

["Bowman, Terry" <terry.bowman@amd.com>]

On 4/4/2025 12:04 PM, Jonathan Cameron wrote:
> On Wed, 26 Mar 2025 20:47:17 -0500
> Terry Bowman <terry.bowman@amd.com> wrote:
>
>> During CXL device cleanup the CXL PCIe Port device interrupts may remain
>> enabled. This can potentialy allow unnecessary interrupt processing on
>> behalf of the CXL errors while the device is destroyed.
>>
>> Disable CXL protocol errors by setting the CXL devices' AER mask register.
>>
>> Introduce pci_aer_mask_internal_errors() similar to pci_aer_unmask_internal_errors().
>>
>> Next, introduce cxl_disable_prot_errors() to call pci_aer_mask_internal_errors().
>> Register cxl_disable_prot_errors() to run at CXL device cleanup.
>> Register for CXL Root Ports, CXL Downstream Ports, CXL Upstream Ports, and
>> CXL Endpoints.
>>
>> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> A few small comments in here.  I haven't looked through all the rest of the series
> as out of time today but this one caught my eye.
>>  
>> @@ -223,7 +238,7 @@ static void cxl_endpoint_port_init_ras(struct cxl_port *port)
>>  	struct device *cxlmd_dev __free(put_device) = &cxlmd->dev;
>>  	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>>  
>> -	if (!dport || !dev_is_pci(dport->dport_dev)) {
>> +	if (!dport || !dev_is_pci(dport->dport_dev) || !dev_is_pci(cxlds->dev)) {
>>  		dev_err(&port->dev, "CXL port topology not found\n");
>>  		return;
>>  	}
>> @@ -232,6 +247,7 @@ static void cxl_endpoint_port_init_ras(struct cxl_port *port)
>>  
>>  	cxl_assign_error_handlers(cxlmd_dev, &cxl_ep_error_handlers);
>>  	cxl_enable_prot_errors(cxlds->dev);
>> +	devm_add_action_or_reset(cxlds->dev, cxl_disable_prot_errors, cxlds->dev);
> This can fail (at least in theory).  Should at least scream that oddly we've
> disabled error handling interrupts if it is hard to return anything cleanly.
>
> Same for all the other cases.

Ok. I will add a dev_err() for errors returned by devm_add_action_or_reset().
>>  }
>>  
>>  #else
>> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
>> index d3068f5cc767..d1ef0c676ff8 100644
>> --- a/drivers/pci/pcie/aer.c
>> +++ b/drivers/pci/pcie/aer.c
>> @@ -977,6 +977,31 @@ void pci_aer_unmask_internal_errors(struct pci_dev *dev)
>>  }
>>  EXPORT_SYMBOL_NS_GPL(pci_aer_unmask_internal_errors, "CXL");
>>  
>> +/**
>> + * pci_aer_mask_internal_errors - mask internal errors
>> + * @dev: pointer to the pcie_dev data structure
>> + *
>> + * Masks internal errors in the Uncorrectable and Correctable Error
>> + * Mask registers.
>> + *
>> + * Note: AER must be enabled and supported by the device which must be
>> + * checked in advance, e.g. with pcie_aer_is_native().
>> + */
>> +void pci_aer_mask_internal_errors(struct pci_dev *dev)
>> +{
>> +	int aer = dev->aer_cap;
>> +	u32 mask;
>> +
>> +	pci_read_config_dword(dev, aer + PCI_ERR_UNCOR_MASK, &mask);
>> +	mask |= PCI_ERR_UNC_INTN;
>> +	pci_write_config_dword(dev, aer + PCI_ERR_UNCOR_MASK, mask);
>> +
> It does an extra clear we don't need, but....
> 	pci_clear_and_set_config_dword(dev, aer + PCI_ERR_UNCOR_MASK,
> 				       0, PCI_ERR_UNC_INTN);
>
> 	is at very least shorter than the above 3 lines.
Doing so will overwrite the existing mask. CXL normally only uses AER UIE/CIE but if the device
happens to lose alternate training and no longer identifies as a CXL device than this mask
value would be critical for reporting PCI AER errors and would need UCE/CE enabled (other
than UIE/CIE).

-Terry

>> +	pci_read_config_dword(dev, aer + PCI_ERR_COR_MASK, &mask);
>> +	mask |= PCI_ERR_COR_INTERNAL;
>> +	pci_write_config_dword(dev, aer + PCI_ERR_COR_MASK, mask);
>> +}
>> +EXPORT_SYMBOL_NS_GPL(pci_aer_mask_internal_errors, "CXL");
>> +
>>  static bool is_cxl_mem_dev(struct pci_dev *dev)
>>  {
>>  	/*
>> diff --git a/include/linux/aer.h b/include/linux/aer.h
>> index a65fe324fad2..f0c84db466e5 100644
>> --- a/include/linux/aer.h
>> +++ b/include/linux/aer.h
>> @@ -101,5 +101,6 @@ int cper_severity_to_aer(int cper_severity);
>>  void aer_recover_queue(int domain, unsigned int bus, unsigned int devfn,
>>  		       int severity, struct aer_capability_regs *aer_regs);
>>  void pci_aer_unmask_internal_errors(struct pci_dev *dev);
>> +void pci_aer_mask_internal_errors(struct pci_dev *dev);
>>  #endif //_AER_H_
>>