| Message ID | 20230523232214.55282-8-terry.bowman@amd.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cxl/pci: Add support for RCH RAS error handling | expand |
On Tue, 23 May 2023 18:21:58 -0500 Terry Bowman <terry.bowman@amd.com> wrote: > From: Robert Richter <rrichter@amd.com> > > During a Host Bridge's downstream port enumeration the CHBS entries in > the CEDT table are parsed, its Component Register base address > extracted and then stored in struct cxl_dport. The CHBS may contain > either the RCRB (RCH mode) or the Host Bridge's Component Registers > (CHBCR, VH mode). The RCRB further contains the CXL downstream port > register base address, while in VH mode the CXL Downstream Switch > Ports are visible in the PCI hierarchy and the DP's component regs are > disovered using the CXL DVSEC register locator capability. The > Component Registers derived from the CHBS for both modes are different > and thus also must be treated differently. That is, in RCH mode, the > component regs base should be bound to the dport, but in VH mode to > the CXL host bridge's port object. > > The current implementation stores the CHBCR in addition in struct > cxl_dport and copies it later from there to struct cxl_port. As a > result, the dport contains the wrong Component Registers base address > and, e.g. the RAS capability of a CXL Root Port cannot be detected. > > To fix the CHBCR binding, attach it directly to the Host Bridge's > @cxl_port structure. Do this during port creation of the Host Bridge > in add_host_bridge_uport(). Factor out CHBS parsing code in > add_host_bridge_dport() and use it in both functions. > > Signed-off-by: Robert Richter <rrichter@amd.com> > Signed-off-by: Terry Bowman <terry.bowman@amd.com> A few trivial formatting things. With those tidied up or reason given for why not, Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/cxl/acpi.c | 65 +++++++++++++++++++++++++++++++++++----------- > 1 file changed, 50 insertions(+), 15 deletions(-) > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c > index 4fd9fe32f830..78a24b2ca923 100644 > --- a/drivers/cxl/acpi.c > +++ b/drivers/cxl/acpi.c > @@ -333,8 +333,8 @@ struct cxl_chbs_context { > u32 cxl_version; > }; > > -static int cxl_get_chbs(union acpi_subtable_headers *header, void *arg, > - const unsigned long end) > +static int cxl_get_chbs_iter(union acpi_subtable_headers *header, void *arg, > + const unsigned long end) > { > struct cxl_chbs_context *ctx = arg; > struct acpi_cedt_chbs *chbs; > @@ -362,6 +362,22 @@ static int cxl_get_chbs(union acpi_subtable_headers *header, void *arg, > return 0; > } > > +static int cxl_get_chbs(struct acpi_device *hb, struct cxl_chbs_context *ctx) > +{ > + unsigned long long uid; > + int rc; > + > + rc = acpi_evaluate_integer(hb->handle, METHOD_NAME__UID, NULL, &uid); > + if (rc != AE_OK) > + return -ENOENT; > + > + memset(ctx, 0, sizeof(*ctx)); > + ctx->uid = uid; For consistency with original code better to use *ctx = (struct cxl_chbs_context) { .uid = uid, }; > + acpi_table_parse_cedt(ACPI_CEDT_TYPE_CHBS, cxl_get_chbs_iter, ctx); > + > + return 0; > +} > + > static int add_host_bridge_dport(struct device *match, void *arg) > { > acpi_status rc; > @@ -377,19 +393,15 @@ static int add_host_bridge_dport(struct device *match, void *arg) > if (!hb) > return 0; > > - rc = acpi_evaluate_integer(hb->handle, METHOD_NAME__UID, NULL, &uid); > - if (rc != AE_OK) { > + rc = cxl_get_chbs(hb, &ctx); > + if (rc == -ENOENT) > dev_err(match, "unable to retrieve _UID\n"); Why not push that down into the cxl_get_chbs() where no special handling of error code is needed? > - return -ENODEV; > - } > + if (rc) > + return rc; > > + uid = ctx.uid; > dev_dbg(match, "UID found: %lld\n", uid); > > - ctx = (struct cxl_chbs_context) { > - .uid = uid, > - }; > - acpi_table_parse_cedt(ACPI_CEDT_TYPE_CHBS, cxl_get_chbs, &ctx); > - > if (!ctx.base) { > dev_warn(match, "No CHBS found for Host Bridge (UID %lld)\n", > uid); > @@ -405,12 +417,17 @@ static int add_host_bridge_dport(struct device *match, void *arg) > pci_root = acpi_pci_find_root(hb->handle); > bridge = pci_root->bus->bridge; > > + /* > + * In RCH mode, bind the component regs base to the dport. In > + * VH mode it will be bound to the CXL host bridge's port > + * object later in add_host_bridge_uport(). > + */ > if (ctx.cxl_version == ACPI_CEDT_CHBS_VERSION_CXL11) { > dev_dbg(match, "RCRB found for UID %lld: %pa\n", uid, &ctx.base); > dport = devm_cxl_add_rch_dport(root_port, bridge, uid, ctx.base); > } else { > - dev_dbg(match, "CHBCR found for UID %lld: %pa\n", uid, &ctx.base); > - dport = devm_cxl_add_dport(root_port, bridge, uid, ctx.base); > + dport = devm_cxl_add_dport(root_port, bridge, uid, > + CXL_RESOURCE_NONE); > } > > if (IS_ERR(dport)) > @@ -432,6 +449,8 @@ static int add_host_bridge_uport(struct device *match, void *arg) > struct cxl_dport *dport; > struct cxl_port *port; > struct device *bridge; > + struct cxl_chbs_context ctx; > + resource_size_t component_reg_phys; > int rc; > > if (!hb) > @@ -450,12 +469,28 @@ static int add_host_bridge_uport(struct device *match, void *arg) > return 0; > } > > + rc = cxl_get_chbs(hb, &ctx); > + if (rc) > + return rc; > + > + if (ctx.cxl_version == ACPI_CEDT_CHBS_VERSION_CXL11) > + /* RCH mode, should never happen */ > + return 0; > + > + if (ctx.base) > + component_reg_phys = ctx.base; > + else > + component_reg_phys = CXL_RESOURCE_NONE; > + > + if (component_reg_phys != CXL_RESOURCE_NONE) > + dev_dbg(match, "CHBCR found for UID %lld: %pa\n", > + ctx.uid, &component_reg_phys); Why not put that in the block above? Fine leaving it here if this makes sense after further refactoring. > + > rc = devm_cxl_register_pci_bus(host, bridge, pci_root->bus); > if (rc) > return rc; > > - port = devm_cxl_add_port(host, bridge, dport->component_reg_phys, > - dport); > + port = devm_cxl_add_port(host, bridge, component_reg_phys, dport); > if (IS_ERR(port)) > return PTR_ERR(port); >
On 01.06.23 13:45:57, Jonathan Cameron wrote: > On Tue, 23 May 2023 18:21:58 -0500 > Terry Bowman <terry.bowman@amd.com> wrote: > > > From: Robert Richter <rrichter@amd.com> > > > > During a Host Bridge's downstream port enumeration the CHBS entries in > > the CEDT table are parsed, its Component Register base address > > extracted and then stored in struct cxl_dport. The CHBS may contain > > either the RCRB (RCH mode) or the Host Bridge's Component Registers > > (CHBCR, VH mode). The RCRB further contains the CXL downstream port > > register base address, while in VH mode the CXL Downstream Switch > > Ports are visible in the PCI hierarchy and the DP's component regs are > > disovered using the CXL DVSEC register locator capability. The > > Component Registers derived from the CHBS for both modes are different > > and thus also must be treated differently. That is, in RCH mode, the > > component regs base should be bound to the dport, but in VH mode to > > the CXL host bridge's port object. > > > > The current implementation stores the CHBCR in addition in struct > > cxl_dport and copies it later from there to struct cxl_port. As a > > result, the dport contains the wrong Component Registers base address > > and, e.g. the RAS capability of a CXL Root Port cannot be detected. > > > > To fix the CHBCR binding, attach it directly to the Host Bridge's > > @cxl_port structure. Do this during port creation of the Host Bridge > > in add_host_bridge_uport(). Factor out CHBS parsing code in > > add_host_bridge_dport() and use it in both functions. > > > > Signed-off-by: Robert Richter <rrichter@amd.com> > > Signed-off-by: Terry Bowman <terry.bowman@amd.com> > A few trivial formatting things. With those tidied up or > reason given for why not, > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > --- > > drivers/cxl/acpi.c | 65 +++++++++++++++++++++++++++++++++++----------- > > 1 file changed, 50 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c > > index 4fd9fe32f830..78a24b2ca923 100644 > > --- a/drivers/cxl/acpi.c > > +++ b/drivers/cxl/acpi.c > > @@ -333,8 +333,8 @@ struct cxl_chbs_context { > > u32 cxl_version; > > }; > > > > -static int cxl_get_chbs(union acpi_subtable_headers *header, void *arg, > > - const unsigned long end) > > +static int cxl_get_chbs_iter(union acpi_subtable_headers *header, void *arg, > > + const unsigned long end) > > { > > struct cxl_chbs_context *ctx = arg; > > struct acpi_cedt_chbs *chbs; > > @@ -362,6 +362,22 @@ static int cxl_get_chbs(union acpi_subtable_headers *header, void *arg, > > return 0; > > } > > > > +static int cxl_get_chbs(struct acpi_device *hb, struct cxl_chbs_context *ctx) > > +{ > > + unsigned long long uid; > > + int rc; > > + > > + rc = acpi_evaluate_integer(hb->handle, METHOD_NAME__UID, NULL, &uid); > > + if (rc != AE_OK) > > + return -ENOENT; > > + > > + memset(ctx, 0, sizeof(*ctx)); > > + ctx->uid = uid; > > For consistency with original code better to use > > *ctx = (struct cxl_chbs_context) { > .uid = uid, > }; The memset() pattern is much more common in the kernel, better readable (and writable :-)) and shorter. I have started using it for all the changes. There are not too many left: drivers/cxl/core/mbox.c: *mbox = (struct cxl_mbox_cmd) { drivers/cxl/core/mbox.c: *mem_cmd = (struct cxl_mem_command) { drivers/cxl/core/mbox.c: *mem_cmd = (struct cxl_mem_command) { drivers/cxl/core/mbox.c: *payload = (struct cxl_mbox_clear_event_payload) { drivers/cxl/core/regs.c: *map = (struct cxl_component_reg_map) { 0 }; drivers/cxl/core/regs.c: *map = (struct cxl_device_reg_map){ 0 }; drivers/cxl/pci.c: *policy = (struct cxl_event_interrupt_policy) { drivers/cxl/pmem.c: *cmd = (struct nd_cmd_get_config_size) { drivers/cxl/pmem.c: *set_lsa = (struct cxl_mbox_set_lsa) { > > > + acpi_table_parse_cedt(ACPI_CEDT_TYPE_CHBS, cxl_get_chbs_iter, ctx); > > + > > + return 0; > > +} > > + > > static int add_host_bridge_dport(struct device *match, void *arg) > > { > > acpi_status rc; > > @@ -377,19 +393,15 @@ static int add_host_bridge_dport(struct device *match, void *arg) > > if (!hb) > > return 0; > > > > - rc = acpi_evaluate_integer(hb->handle, METHOD_NAME__UID, NULL, &uid); > > - if (rc != AE_OK) { > > + rc = cxl_get_chbs(hb, &ctx); > > + if (rc == -ENOENT) > > dev_err(match, "unable to retrieve _UID\n"); > > Why not push that down into the cxl_get_chbs() where no special handling > of error code is needed? All messages are generated here in this function and not in cxl_get_chbs() at a lower level. That allows to reuse it later in add_host_bridge_uport() there messages are different or unnecessary. You would also need to pass @dev down to cxl_get_chbs() which polutes the function i/f. > > > - return -ENODEV; > > - } > > + if (rc) > > + return rc; > > > > + uid = ctx.uid; > > dev_dbg(match, "UID found: %lld\n", uid); > > > > - ctx = (struct cxl_chbs_context) { > > - .uid = uid, > > - }; > > - acpi_table_parse_cedt(ACPI_CEDT_TYPE_CHBS, cxl_get_chbs, &ctx); > > - > > if (!ctx.base) { > > dev_warn(match, "No CHBS found for Host Bridge (UID %lld)\n", > > uid); > > @@ -405,12 +417,17 @@ static int add_host_bridge_dport(struct device *match, void *arg) > > pci_root = acpi_pci_find_root(hb->handle); > > bridge = pci_root->bus->bridge; > > > > + /* > > + * In RCH mode, bind the component regs base to the dport. In > > + * VH mode it will be bound to the CXL host bridge's port > > + * object later in add_host_bridge_uport(). > > + */ > > if (ctx.cxl_version == ACPI_CEDT_CHBS_VERSION_CXL11) { > > dev_dbg(match, "RCRB found for UID %lld: %pa\n", uid, &ctx.base); > > dport = devm_cxl_add_rch_dport(root_port, bridge, uid, ctx.base); > > } else { > > - dev_dbg(match, "CHBCR found for UID %lld: %pa\n", uid, &ctx.base); > > - dport = devm_cxl_add_dport(root_port, bridge, uid, ctx.base); > > + dport = devm_cxl_add_dport(root_port, bridge, uid, > > + CXL_RESOURCE_NONE); > > } > > > > if (IS_ERR(dport)) > > @@ -432,6 +449,8 @@ static int add_host_bridge_uport(struct device *match, void *arg) > > struct cxl_dport *dport; > > struct cxl_port *port; > > struct device *bridge; > > + struct cxl_chbs_context ctx; > > + resource_size_t component_reg_phys; > > int rc; > > > > if (!hb) > > @@ -450,12 +469,28 @@ static int add_host_bridge_uport(struct device *match, void *arg) > > return 0; > > } > > > > + rc = cxl_get_chbs(hb, &ctx); > > + if (rc) > > + return rc; > > + > > + if (ctx.cxl_version == ACPI_CEDT_CHBS_VERSION_CXL11) > > + /* RCH mode, should never happen */ > > + return 0; > > + > > + if (ctx.base) > > + component_reg_phys = ctx.base; > > + else > > + component_reg_phys = CXL_RESOURCE_NONE; > > + > > + if (component_reg_phys != CXL_RESOURCE_NONE) > > + dev_dbg(match, "CHBCR found for UID %lld: %pa\n", > > + ctx.uid, &component_reg_phys); > > Why not put that in the block above? Fine leaving it here if this > makes sense after further refactoring. ctx.base could be CXL_RESOURCE_NONE which we want to catch too. I stumbled over that too then reviewing my own code, but I don't see how this could be made more obvious. Maybe I will add a comment here. Thanks, -Robert > > > + > > rc = devm_cxl_register_pci_bus(host, bridge, pci_root->bus); > > if (rc) > > return rc; > > > > - port = devm_cxl_add_port(host, bridge, dport->component_reg_phys, > > - dport); > > + port = devm_cxl_add_port(host, bridge, component_reg_phys, dport); > > if (IS_ERR(port)) > > return PTR_ERR(port); > > >
diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c index 4fd9fe32f830..78a24b2ca923 100644 --- a/drivers/cxl/acpi.c +++ b/drivers/cxl/acpi.c @@ -333,8 +333,8 @@ struct cxl_chbs_context { u32 cxl_version; }; -static int cxl_get_chbs(union acpi_subtable_headers *header, void *arg, - const unsigned long end) +static int cxl_get_chbs_iter(union acpi_subtable_headers *header, void *arg, + const unsigned long end) { struct cxl_chbs_context *ctx = arg; struct acpi_cedt_chbs *chbs; @@ -362,6 +362,22 @@ static int cxl_get_chbs(union acpi_subtable_headers *header, void *arg, return 0; } +static int cxl_get_chbs(struct acpi_device *hb, struct cxl_chbs_context *ctx) +{ + unsigned long long uid; + int rc; + + rc = acpi_evaluate_integer(hb->handle, METHOD_NAME__UID, NULL, &uid); + if (rc != AE_OK) + return -ENOENT; + + memset(ctx, 0, sizeof(*ctx)); + ctx->uid = uid; + acpi_table_parse_cedt(ACPI_CEDT_TYPE_CHBS, cxl_get_chbs_iter, ctx); + + return 0; +} + static int add_host_bridge_dport(struct device *match, void *arg) { acpi_status rc; @@ -377,19 +393,15 @@ static int add_host_bridge_dport(struct device *match, void *arg) if (!hb) return 0; - rc = acpi_evaluate_integer(hb->handle, METHOD_NAME__UID, NULL, &uid); - if (rc != AE_OK) { + rc = cxl_get_chbs(hb, &ctx); + if (rc == -ENOENT) dev_err(match, "unable to retrieve _UID\n"); - return -ENODEV; - } + if (rc) + return rc; + uid = ctx.uid; dev_dbg(match, "UID found: %lld\n", uid); - ctx = (struct cxl_chbs_context) { - .uid = uid, - }; - acpi_table_parse_cedt(ACPI_CEDT_TYPE_CHBS, cxl_get_chbs, &ctx); - if (!ctx.base) { dev_warn(match, "No CHBS found for Host Bridge (UID %lld)\n", uid); @@ -405,12 +417,17 @@ static int add_host_bridge_dport(struct device *match, void *arg) pci_root = acpi_pci_find_root(hb->handle); bridge = pci_root->bus->bridge; + /* + * In RCH mode, bind the component regs base to the dport. In + * VH mode it will be bound to the CXL host bridge's port + * object later in add_host_bridge_uport(). + */ if (ctx.cxl_version == ACPI_CEDT_CHBS_VERSION_CXL11) { dev_dbg(match, "RCRB found for UID %lld: %pa\n", uid, &ctx.base); dport = devm_cxl_add_rch_dport(root_port, bridge, uid, ctx.base); } else { - dev_dbg(match, "CHBCR found for UID %lld: %pa\n", uid, &ctx.base); - dport = devm_cxl_add_dport(root_port, bridge, uid, ctx.base); + dport = devm_cxl_add_dport(root_port, bridge, uid, + CXL_RESOURCE_NONE); } if (IS_ERR(dport)) @@ -432,6 +449,8 @@ static int add_host_bridge_uport(struct device *match, void *arg) struct cxl_dport *dport; struct cxl_port *port; struct device *bridge; + struct cxl_chbs_context ctx; + resource_size_t component_reg_phys; int rc; if (!hb) @@ -450,12 +469,28 @@ static int add_host_bridge_uport(struct device *match, void *arg) return 0; } + rc = cxl_get_chbs(hb, &ctx); + if (rc) + return rc; + + if (ctx.cxl_version == ACPI_CEDT_CHBS_VERSION_CXL11) + /* RCH mode, should never happen */ + return 0; + + if (ctx.base) + component_reg_phys = ctx.base; + else + component_reg_phys = CXL_RESOURCE_NONE; + + if (component_reg_phys != CXL_RESOURCE_NONE) + dev_dbg(match, "CHBCR found for UID %lld: %pa\n", + ctx.uid, &component_reg_phys); + rc = devm_cxl_register_pci_bus(host, bridge, pci_root->bus); if (rc) return rc; - port = devm_cxl_add_port(host, bridge, dport->component_reg_phys, - dport); + port = devm_cxl_add_port(host, bridge, component_reg_phys, dport); if (IS_ERR(port)) return PTR_ERR(port);