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

[Jonathan Cameron]

On Tue, 3 Jun 2025 12:22:36 -0500
Terry Bowman <terry.bowman@xxxxxxx> 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@xxxxxxx>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>

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.

> +		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;


> +		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.

> +	}
> +	}
> +
> +	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?



> +			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

> +		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.

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

> +		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;
> +	}
> +