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