On 6/12/2025 12:14 PM, Jonathan Cameron wrote:
> 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.
>
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;
>> + }
>> +
>
