Re: [RFC PATCH 7/9] cxl/pci: Add atomic notifier callback for CXL PCIe port AER internal errors

Jonathan Cameron
2024-07-02T15:58:40Z

> >> +
> >> +static void __iomem *cxl_pci_port_ras(struct pci_dev *pdev)
> >> +{
> >> + void __iomem *ras_base = NULL;
> > Don't initialize and...
>
> There is possibility the incorrect PCI type is passed and this is intended to
> return NULL for these cases. Should ras_base not be preinitialized in
> that for the scenario?

From a code point of view at least, nope - just return NULL directly
give it's an error case.

>
> >> +
> >> + if ((pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT) ||
> >> + (pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM)) {
> >> + struct cxl_dport *dport;
> >> +
> >> + find_cxl_port(&pdev->dev, &dport);
> >> + ras_base = dport ? dport->regs.ras : NULL;
> > if (dport)
> > return dport->regs.ras;
> >> + } else if (pci_pcie_type(pdev) == PCI_EXP_TYPE_UPSTREAM) {
> >> + struct cxl_port *port = pci_to_cxl_uport(pdev);
> >> +
> >> + ras_base = port ? port->regs.ras : NULL;
> > if (port)
> > return port->regs.ras;
> >> + }
> > return NULL;

This is why you don't need to set ras_base.
If you get here it's always NULL.

> >> +
> >> + return ras_base;
> >> +}
> >> +
> >> +int port_internal_err_cb(struct notifier_block *unused,
> >> + unsigned long event, void *ptr)
> >> +{
> >> + struct pci_dev *pdev = (struct pci_dev *)ptr;
> >
> > I think you can use this notifier for other types of device in future?
> > If it's going to be global definitely want to check here that we
> > actually have a CXL port of some type.
> >
> > It may be that via the various calls any non CXL device
> > will result in a safe error. However that's not obvious, so an
> > upfront check makes sense (or a per device notifier registration!)
> >
>
> cxl_pci_port_ras() performs PCIe type check and sets RAS base to NULL if