Messages in this thread Patch in this message | | | Date | Fri, 2 Jun 2023 17:42:34 +0200 | From | Robert Richter <> | Subject | Re: [PATCH v4 07/23] cxl/acpi: Directly bind the CEDT detected CHBCR to the Host Bridge's 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); > > >
| |