Re: [PATCH v9 13/16] cxl/pci: Introduce CXL Port protocol error handlers

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

On 6/12/2025 12:14 PM, Jonathan Cameron wrote:
> On Tue, 3 Jun 2025 12:22:36 -0500
> Terry Bowman <terry.bowman@amd.com> wrote:
> 
>> Introduce CXL error handlers for CXL Port devices.
>>
>> Add functions cxl_port_cor_error_detected() and cxl_port_error_detected().
>> These will serve as the handlers for all CXL Port devices. Introduce
>> cxl_get_ras_base() to provide the RAS base address needed by the handlers.
>>
>> Update cxl_handle_prot_error() to call the CXL Port or CXL Endpoint handler
>> depending on which CXL device reports the error.
>>
>> Implement pci_get_ras_base() to return the cached RAS register address of a
>> CXL Root Port, CXL Downstream Port, or CXL Upstream Port.
>>
>> Update the AER driver's is_cxl_error() to remove the filter PCI type check
>> because CXL Port devices are now supported.
>>
>> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> A few minor things on a fresh read.
> 
>>  size_t cxl_get_feature(struct cxl_mailbox *cxl_mbox, const uuid_t *feat_uuid,
>> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
>> index e094ef518e0a..b6836825e8df 100644
>> --- a/drivers/cxl/core/pci.c
>> +++ b/drivers/cxl/core/pci.c
>> @@ -753,6 +753,67 @@ static bool cxl_handle_endpoint_ras(struct cxl_dev_state *cxlds)
>>  
>>  #ifdef CONFIG_PCIEAER_CXL
>>  
>> +static void __iomem *cxl_get_ras_base(struct device *dev)
>> +{
>> +	struct pci_dev *pdev = to_pci_dev(dev);
>> +	void __iomem *ras_base;
>> +
>> +	switch (pci_pcie_type(pdev)) {
>> +	case PCI_EXP_TYPE_ROOT_PORT:
>> +	case PCI_EXP_TYPE_DOWNSTREAM:
>> +	{
>> +		struct cxl_dport *dport = NULL;
>> +		struct cxl_port *port __free(put_cxl_port) = find_cxl_port(&pdev->dev, &dport);
>> +
>> +		if (!dport || !dport->dport_dev) {
>> +			pci_err(pdev, "Failed to find the CXL device");
>> +			return NULL;
>> +		}
>> +
>> +		ras_base = dport ? dport->regs.ras : NULL;
> As below - perhaps a sanity check for error and early return makes sense here.
> 

Good idea.

>> +		break;
>> +	}
>> +	case PCI_EXP_TYPE_UPSTREAM:
>> +	{
>> +		struct cxl_port *port;
>> +		struct device *dev __free(put_device) = bus_find_device(&cxl_bus_type, NULL,
>> +									&pdev->dev, match_uport);
>> +
>> +		if (!dev || !is_cxl_port(dev)) {
>> +			pci_err(pdev, "Failed to find the CXL device");
>> +			return NULL;
>> +		}
>> +
>> +		port = to_cxl_port(dev);
>> +		ras_base = port ? port->uport_regs.ras : NULL;
> 
> I'd be tempted to return here to keep the flows simple.  Maybe avoiding the ternary
> 		if (!port)
> 			return NULL;
> 
> 		return port->uport_regs.ras;
> 
> 

Ok.

>> +		break;
>> +	}
>> +	default:
>> +	{
>> +		pci_warn_once(pdev, "Error: Unsupported device type (%X)", pci_pcie_type(pdev));
>> +		return NULL;
> 
> Better not to introduce scope {} when not needed.
> 

Ok.

>> +	}
>> +	}
>> +
>> +	return ras_base;
>> +}
> 
>> diff --git a/drivers/cxl/core/ras.c b/drivers/cxl/core/ras.c
>> index 664f532cc838..6093e70ece37 100644
>> --- a/drivers/cxl/core/ras.c
>> +++ b/drivers/cxl/core/ras.c
> 
>> +
>> +/* Return 'struct device*' responsible for freeing pdev's CXL resources */
>> +static struct device *get_pci_cxl_host_dev(struct pci_dev *pdev)
>> +{
>> +	struct device *host_dev;
>> +
>> +	switch (pci_pcie_type(pdev)) {
>> +	case PCI_EXP_TYPE_ROOT_PORT:
>> +	case PCI_EXP_TYPE_DOWNSTREAM:
>> +	{
>> +		struct cxl_dport *dport = NULL;
>> +		struct cxl_port *port = find_cxl_port(&pdev->dev, &dport);
>> +
>> +		if (!dport || !dport->dport_dev)
> 
> What does !dport->dprot_dev mean?  I.e. how does that happen?
> I can only find places where we set it just after allocating a dport.
> Perhaps a comment?
> 
> 
I'll remove the check for !dport->dport_dev.

> 
>> +			return NULL;
>> +
>> +		host_dev = &port->dev;
>> +		break;
>> +	}
>> +	case PCI_EXP_TYPE_UPSTREAM:
>> +	{
>> +		struct cxl_port *port;
>> +		struct device *cxl_dev = bus_find_device(&cxl_bus_type, NULL,
>> +							 &pdev->dev, match_uport);
> 
> Doesn't his leave you holding a reference to a device different form
> the one you return? Hence wrong one gets put in caller.
> 



>> +
>> +		if (!cxl_dev || !is_cxl_port(cxl_dev))
>> +			return NULL;
>> +
>> +		port = to_cxl_port(cxl_dev);
>> +		host_dev = &port->dev;
> Actually no. Isn't this a circle that lands you on cxl_dev again?
> 
> 		container_of(dev, struct cxl_port, dev)->dev
> 

You're right, it is circular. I'll simplify it. Thanks.

>> +		break;
>> +	}
>> +	case PCI_EXP_TYPE_ENDPOINT:
>> +	case PCI_EXP_TYPE_RC_END:
>> +	{
>> +		struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
>> +
>> +		if (!cxlds)
>> +			return NULL;
>> +
>> +		host_dev = get_device(&cxlds->cxlmd->dev);
>> +		break;
> 
> Maybe just return it here? Similar for other cases.
> Saves a reader keeping track of references if we get them roughly where
> we return them.
> 

Ok.

>> +	}
>> +	default:
>> +	{
> No need for scope on this one (or at least some of the others) so drop the {}
> 

Ok.

-Terry

>> +		pci_warn_once(pdev, "Error: Unsupported device type (%X)", pci_pcie_type(pdev));
>> +		return NULL;
>> +	}
>> +	}
>> +
>> +	return host_dev;
>> +}
>> +
>> +static int cxl_report_error_detected(struct pci_dev *pdev, void *data)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct device *host_dev __free(put_device) = get_pci_cxl_host_dev(pdev);
>> +	pci_ers_result_t vote, *result = data;
>>  
>>  	device_lock(&pdev->dev);
>> -	vote = cxl_error_detected(&pdev->dev);
>> +	if ((pci_pcie_type(pdev) == PCI_EXP_TYPE_ENDPOINT) ||
>> +	    (pci_pcie_type(pdev) == PCI_EXP_TYPE_RC_END)) {
>> +		vote = cxl_error_detected(dev);
>> +	} else {
>> +		vote = cxl_port_error_detected(dev);
>> +	}
>>  	*result = cxl_merge_result(*result, vote);
>>  	device_unlock(&pdev->dev);
>>  
>> @@ -244,14 +309,18 @@ static struct pci_dev *sbdf_to_pci(struct cxl_prot_error_info *err_info)
>>  static void cxl_handle_prot_error(struct cxl_prot_error_info *err_info)
>>  {
>>  	struct pci_dev *pdev __free(pci_dev_put) = pci_dev_get(sbdf_to_pci(err_info));
>> -	struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
>> -	struct device *cxlmd_dev __free(put_device) = get_device(&cxlds->cxlmd->dev);
>>  
>>  	if (!pdev) {
>>  		pr_err("Failed to find the CXL device\n");
>>  		return;
>>  	}
>>  
>> +	struct device *host_dev __free(put_device) = get_pci_cxl_host_dev(pdev);
>> +	if (!host_dev) {
>> +		pr_err("Failed to find the CXL device's host\n");
>> +		return;
>> +	}
>> +
>