Re: [PATCH v4 07/23] cxl/acpi: Directly bind the CEDT detected CHBCR to the Host Bridge's port

[Robert Richter <rrichter@amd.com>]

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);
> >  
>