| Message ID | 20230607221651.2454764-12-terry.bowman@amd.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cxl/pci: Add support for RCH RAS error handling | expand |
Terry Bowman wrote: > From: Robert Richter <rrichter@amd.com> > > CXL RAS capabilities must be enabled and accessible as soon as the CXL > endpoint is detected in the PCI hierarchy and bound to the cxl_pci > driver. This needs to be independent of other modules such as cxl_port > or cxl_mem. > > CXL RAS capabilities reside in the Component Registers. For an RCH > this is determined by probing RCRB which is implemented very late once > the CXL Memory Device is created. > > Change this by moving the RCRB probe to the cxl_pci driver. Do this by > using a new introduced function cxl_pci_find_port() similar to > cxl_mem_find_port() to determine the involved dport by the endpoint's > PCI handle. Plug this into the existing cxl_pci_setup_regs() function > to setup Component Registers. Probe the RCRB in case the Component > Registers cannot be located through the CXL Register Locator > capability. > > This unifies code and early sets up the Component Registers at the > same time for both, VH and RCH mode. Only the cxl_pci driver is > involved for this. This allows an early mapping of the CXL RAS > capability registers. This is problematic because it creates an implicit dependency between cxl_acpi and cxl_pci. More comments below: > > Signed-off-by: Robert Richter <rrichter@amd.com> > Signed-off-by: Terry Bowman <terry.bowman@amd.com> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/cxl/core/port.c | 7 +++++++ > drivers/cxl/cxl.h | 2 ++ > drivers/cxl/mem.c | 10 ---------- > drivers/cxl/pci.c | 37 ++++++++++++++++++++++++++++++++++++- > 4 files changed, 45 insertions(+), 11 deletions(-) > > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c > index 82de858506c7..8b688ac506ca 100644 > --- a/drivers/cxl/core/port.c > +++ b/drivers/cxl/core/port.c > @@ -1476,6 +1476,13 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd) > } > EXPORT_SYMBOL_NS_GPL(devm_cxl_enumerate_ports, CXL); > > +struct cxl_port *cxl_pci_find_port(struct pci_dev *pdev, > + struct cxl_dport **dport) > +{ > + return find_cxl_port(pdev->dev.parent, dport); > +} > +EXPORT_SYMBOL_NS_GPL(cxl_pci_find_port, CXL); > + > struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd, > struct cxl_dport **dport) > { > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > index 1c6fe53e9dc7..e5ae5f4e6669 100644 > --- a/drivers/cxl/cxl.h > +++ b/drivers/cxl/cxl.h > @@ -670,6 +670,8 @@ struct cxl_port *find_cxl_root(struct cxl_port *port); > int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd); > void cxl_bus_rescan(void); > void cxl_bus_drain(void); > +struct cxl_port *cxl_pci_find_port(struct pci_dev *pdev, > + struct cxl_dport **dport); > struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd, > struct cxl_dport **dport); > bool schedule_cxl_memdev_detach(struct cxl_memdev *cxlmd); > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > index a34d6560c25c..0643852444f3 100644 > --- a/drivers/cxl/mem.c > +++ b/drivers/cxl/mem.c > @@ -65,16 +65,6 @@ static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd, > ep->next = down; > } > > - /* > - * The component registers for an RCD might come from the > - * host-bridge RCRB if they are not already mapped via the > - * typical register locator mechanism. > - */ > - if (parent_dport->rch && cxlds->component_reg_phys == CXL_RESOURCE_NONE) > - cxlds->component_reg_phys = > - cxl_probe_rcrb(&cxlmd->dev, parent_dport->rcrb.base, > - NULL, CXL_RCRB_UPSTREAM); > - > endpoint = devm_cxl_add_port(host, &cxlmd->dev, > cxlds->component_reg_phys, > parent_dport); > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > index 945ca0304d68..2975b232fcd1 100644 > --- a/drivers/cxl/pci.c > +++ b/drivers/cxl/pci.c > @@ -274,13 +274,48 @@ static int cxl_pci_setup_mailbox(struct cxl_dev_state *cxlds) > return 0; > } > > +/* Extract RCRB, use same function interface as cxl_find_regblock(). */ > +static int cxl_rcrb_get_comp_regs(struct pci_dev *pdev, > + enum cxl_regloc_type type, > + struct cxl_register_map *map) > +{ > + struct cxl_dport *dport; > + resource_size_t component_reg_phys; > + > + memset(map, 0, sizeof(*map)); > + map->dev = &pdev->dev; > + map->resource = CXL_RESOURCE_NONE; > + > + if (type != CXL_REGLOC_RBI_COMPONENT) > + return -ENODEV; Why would a function called cxl_rcrb_get_comp_regs() need a type parameter that only takes one acceptable value. I would drop the parameter and move this distinction to the caller. > + > + if (!cxl_pci_find_port(pdev, &dport) || !dport->rch) > + return -ENXIO; This should return -EPROBE_DEFER in the !cxl_pci_find_port() case to try to await cxl_acpi initialization. > + > + component_reg_phys = cxl_probe_rcrb(&pdev->dev, dport->rcrb.base, > + NULL, CXL_RCRB_UPSTREAM); > + if (component_reg_phys == CXL_RESOURCE_NONE) > + return -ENXIO; > + > + map->resource = component_reg_phys; > + map->reg_type = type; > + map->max_size = CXL_COMPONENT_REG_BLOCK_SIZE; > + > + return 0; > +} > + > static int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type, > struct cxl_register_map *map) > { > int rc; > > + /* > + * If the Register Locator DVSEC does not contain the > + * Component Registers, assume it is an RCH and try to extract > + * them from an RCRB. > + */ > rc = cxl_find_regblock(pdev, type, map); > - if (rc) > + if (rc && cxl_rcrb_get_comp_regs(pdev, type, map)) > return rc; I am not a big fan of just assuming this is an RCD, especially when there is the is_cxl_restricted() helper just beneath this.
Terry Bowman wrote: > From: Robert Richter <rrichter@amd.com> > > CXL RAS capabilities must be enabled and accessible as soon as the CXL > endpoint is detected in the PCI hierarchy and bound to the cxl_pci > driver. This needs to be independent of other modules such as cxl_port > or cxl_mem. > > CXL RAS capabilities reside in the Component Registers. For an RCH > this is determined by probing RCRB which is implemented very late once > the CXL Memory Device is created. > > Change this by moving the RCRB probe to the cxl_pci driver. Do this by > using a new introduced function cxl_pci_find_port() similar to > cxl_mem_find_port() to determine the involved dport by the endpoint's > PCI handle. Plug this into the existing cxl_pci_setup_regs() function > to setup Component Registers. Probe the RCRB in case the Component > Registers cannot be located through the CXL Register Locator > capability. > > This unifies code and early sets up the Component Registers at the > same time for both, VH and RCH mode. Only the cxl_pci driver is > involved for this. This allows an early mapping of the CXL RAS > capability registers. > > Signed-off-by: Robert Richter <rrichter@amd.com> > Signed-off-by: Terry Bowman <terry.bowman@amd.com> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/cxl/core/port.c | 7 +++++++ > drivers/cxl/cxl.h | 2 ++ > drivers/cxl/mem.c | 10 ---------- > drivers/cxl/pci.c | 37 ++++++++++++++++++++++++++++++++++++- > 4 files changed, 45 insertions(+), 11 deletions(-) > [..] > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > index 945ca0304d68..2975b232fcd1 100644 > --- a/drivers/cxl/pci.c > +++ b/drivers/cxl/pci.c > @@ -274,13 +274,48 @@ static int cxl_pci_setup_mailbox(struct cxl_dev_state *cxlds) > return 0; > } > > +/* Extract RCRB, use same function interface as cxl_find_regblock(). */ > +static int cxl_rcrb_get_comp_regs(struct pci_dev *pdev, > + enum cxl_regloc_type type, > + struct cxl_register_map *map) > +{ > + struct cxl_dport *dport; > + resource_size_t component_reg_phys; > + > + memset(map, 0, sizeof(*map)); > + map->dev = &pdev->dev; > + map->resource = CXL_RESOURCE_NONE; > + > + if (type != CXL_REGLOC_RBI_COMPONENT) > + return -ENODEV; > + > + if (!cxl_pci_find_port(pdev, &dport) || !dport->rch) > + return -ENXIO; > + > + component_reg_phys = cxl_probe_rcrb(&pdev->dev, dport->rcrb.base, > + NULL, CXL_RCRB_UPSTREAM); > + if (component_reg_phys == CXL_RESOURCE_NONE) > + return -ENXIO; > + > + map->resource = component_reg_phys; > + map->reg_type = type; > + map->max_size = CXL_COMPONENT_REG_BLOCK_SIZE; One more note, I would prefer a designated initializer for this.
Terry Bowman wrote: > From: Robert Richter <rrichter@amd.com> > > CXL RAS capabilities must be enabled and accessible as soon as the CXL > endpoint is detected in the PCI hierarchy and bound to the cxl_pci > driver. This needs to be independent of other modules such as cxl_port > or cxl_mem. > > CXL RAS capabilities reside in the Component Registers. For an RCH > this is determined by probing RCRB which is implemented very late once > the CXL Memory Device is created. > > Change this by moving the RCRB probe to the cxl_pci driver. Do this by > using a new introduced function cxl_pci_find_port() similar to > cxl_mem_find_port() to determine the involved dport by the endpoint's > PCI handle. Plug this into the existing cxl_pci_setup_regs() function > to setup Component Registers. Probe the RCRB in case the Component > Registers cannot be located through the CXL Register Locator > capability. > > This unifies code and early sets up the Component Registers at the > same time for both, VH and RCH mode. Only the cxl_pci driver is > involved for this. This allows an early mapping of the CXL RAS > capability registers. > > Signed-off-by: Robert Richter <rrichter@amd.com> > Signed-off-by: Terry Bowman <terry.bowman@amd.com> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> [..] > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > index 945ca0304d68..2975b232fcd1 100644 > --- a/drivers/cxl/pci.c > +++ b/drivers/cxl/pci.c > @@ -274,13 +274,48 @@ static int cxl_pci_setup_mailbox(struct cxl_dev_state *cxlds) > return 0; > } > > +/* Extract RCRB, use same function interface as cxl_find_regblock(). */ > +static int cxl_rcrb_get_comp_regs(struct pci_dev *pdev, > + enum cxl_regloc_type type, > + struct cxl_register_map *map) > +{ > + struct cxl_dport *dport; > + resource_size_t component_reg_phys; > + > + memset(map, 0, sizeof(*map)); > + map->dev = &pdev->dev; > + map->resource = CXL_RESOURCE_NONE; > + > + if (type != CXL_REGLOC_RBI_COMPONENT) > + return -ENODEV; > + > + if (!cxl_pci_find_port(pdev, &dport) || !dport->rch) > + return -ENXIO; I noticed while reviewing a later patch that this pattern leaks the port reference from cxl_pci_find_port(). I.e. this needs to do: port = cxl_pci_find_port(...); if (!port) return -ENXIO; if (!dport->rch) goto out; ... out: put_device(&port->dev);
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c index 82de858506c7..8b688ac506ca 100644 --- a/drivers/cxl/core/port.c +++ b/drivers/cxl/core/port.c @@ -1476,6 +1476,13 @@ int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd) } EXPORT_SYMBOL_NS_GPL(devm_cxl_enumerate_ports, CXL); +struct cxl_port *cxl_pci_find_port(struct pci_dev *pdev, + struct cxl_dport **dport) +{ + return find_cxl_port(pdev->dev.parent, dport); +} +EXPORT_SYMBOL_NS_GPL(cxl_pci_find_port, CXL); + struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd, struct cxl_dport **dport) { diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index 1c6fe53e9dc7..e5ae5f4e6669 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -670,6 +670,8 @@ struct cxl_port *find_cxl_root(struct cxl_port *port); int devm_cxl_enumerate_ports(struct cxl_memdev *cxlmd); void cxl_bus_rescan(void); void cxl_bus_drain(void); +struct cxl_port *cxl_pci_find_port(struct pci_dev *pdev, + struct cxl_dport **dport); struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd, struct cxl_dport **dport); bool schedule_cxl_memdev_detach(struct cxl_memdev *cxlmd); diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c index a34d6560c25c..0643852444f3 100644 --- a/drivers/cxl/mem.c +++ b/drivers/cxl/mem.c @@ -65,16 +65,6 @@ static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd, ep->next = down; } - /* - * The component registers for an RCD might come from the - * host-bridge RCRB if they are not already mapped via the - * typical register locator mechanism. - */ - if (parent_dport->rch && cxlds->component_reg_phys == CXL_RESOURCE_NONE) - cxlds->component_reg_phys = - cxl_probe_rcrb(&cxlmd->dev, parent_dport->rcrb.base, - NULL, CXL_RCRB_UPSTREAM); - endpoint = devm_cxl_add_port(host, &cxlmd->dev, cxlds->component_reg_phys, parent_dport); diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c index 945ca0304d68..2975b232fcd1 100644 --- a/drivers/cxl/pci.c +++ b/drivers/cxl/pci.c @@ -274,13 +274,48 @@ static int cxl_pci_setup_mailbox(struct cxl_dev_state *cxlds) return 0; } +/* Extract RCRB, use same function interface as cxl_find_regblock(). */ +static int cxl_rcrb_get_comp_regs(struct pci_dev *pdev, + enum cxl_regloc_type type, + struct cxl_register_map *map) +{ + struct cxl_dport *dport; + resource_size_t component_reg_phys; + + memset(map, 0, sizeof(*map)); + map->dev = &pdev->dev; + map->resource = CXL_RESOURCE_NONE; + + if (type != CXL_REGLOC_RBI_COMPONENT) + return -ENODEV; + + if (!cxl_pci_find_port(pdev, &dport) || !dport->rch) + return -ENXIO; + + component_reg_phys = cxl_probe_rcrb(&pdev->dev, dport->rcrb.base, + NULL, CXL_RCRB_UPSTREAM); + if (component_reg_phys == CXL_RESOURCE_NONE) + return -ENXIO; + + map->resource = component_reg_phys; + map->reg_type = type; + map->max_size = CXL_COMPONENT_REG_BLOCK_SIZE; + + return 0; +} + static int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type, struct cxl_register_map *map) { int rc; + /* + * If the Register Locator DVSEC does not contain the + * Component Registers, assume it is an RCH and try to extract + * them from an RCRB. + */ rc = cxl_find_regblock(pdev, type, map); - if (rc) + if (rc && cxl_rcrb_get_comp_regs(pdev, type, map)) return rc; return cxl_setup_regs(map);