Re: [PATCH 21/23] cxl: Unify port enumeration for decoders

[Ben Widawsky <ben.widawsky@intel.com>]

On 21-11-22 17:48:16, Jonathan Cameron wrote:
> On Fri, 19 Nov 2021 16:02:48 -0800
> Ben Widawsky <ben.widawsky@intel.com> wrote:
> 
> > The port driver exists to do proper enumeration of the component
> > registers for ports, including HDM decoder resources. Any port which
> > follows the CXL specification to implement HDM decoder registers should
> > be handled by the port driver. This includes host bridge registers that
> > are currently handled within the cxl_acpi driver.
> > 
> > In moving the responsibility from cxl_acpi to cxl_port, three primary
> > things are accomplished here:
> > 1. Multi-port host bridges are now handled by the port driver
> > 2. Single port host bridges are handled by the port driver
> > 3. Single port switches without component registers will be handled by
> >    the port driver.
> > 
> > While it's tempting to remove decoder APIs from cxl_core entirely, it is
> > still required that platform specific drivers are able to add decoders
> > which aren't specified in CXL 2.0+. An example of this is the CFMWS
> > parsing which is implementing in cxl_acpi.
> > 
> > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> One trivial suggestion inline, but looks fine to me otherwise.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> > 
> > ---
> > Changes since RFCv2:
> > - Renamed subject
> > - Reworded commit message
> > - Handle *all* host bridge port enumeration in cxl_port (Dan)
> >   - Handle passthrough decoding in cxl_port
> > ---
> >  drivers/cxl/acpi.c     | 41 +++-----------------------------
> >  drivers/cxl/core/bus.c |  6 +++--
> >  drivers/cxl/cxl.h      |  2 ++
> >  drivers/cxl/port.c     | 54 +++++++++++++++++++++++++++++++++++++++++-
> >  4 files changed, 62 insertions(+), 41 deletions(-)
> > 
> > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > index c12e4fed7941..c85a04ecbf7f 100644
> > --- a/drivers/cxl/acpi.c
> > +++ b/drivers/cxl/acpi.c
> > @@ -210,8 +210,6 @@ static int add_host_bridge_uport(struct device *match, void *arg)
> >  	struct acpi_device *bridge = to_cxl_host_bridge(host, match);
> >  	struct acpi_pci_root *pci_root;
> >  	struct cxl_walk_context ctx;
> > -	int single_port_map[1], rc;
> > -	struct cxl_decoder *cxld;
> >  	struct cxl_dport *dport;
> >  	struct cxl_port *port;
> >  
> > @@ -245,43 +243,9 @@ static int add_host_bridge_uport(struct device *match, void *arg)
> >  		return -ENODEV;
> >  	if (ctx.error)
> >  		return ctx.error;
> > -	if (ctx.count > 1)
> > -		return 0;
> >  
> > -	/* TODO: Scan CHBCR for HDM Decoder resources */
> > -
> > -	/*
> > -	 * Per the CXL specification (8.2.5.12 CXL HDM Decoder Capability
> > -	 * Structure) single ported host-bridges need not publish a decoder
> > -	 * capability when a passthrough decode can be assumed, i.e. all
> > -	 * transactions that the uport sees are claimed and passed to the single
> > -	 * dport. Disable the range until the first CXL region is enumerated /
> > -	 * activated.
> > -	 */
> > -	cxld = cxl_decoder_alloc(port, 1);
> > -	if (IS_ERR(cxld))
> > -		return PTR_ERR(cxld);
> > -
> > -	cxld->interleave_ways = 1;
> > -	cxld->interleave_granularity = PAGE_SIZE;
> > -	cxld->target_type = CXL_DECODER_EXPANDER;
> > -	cxld->platform_res = (struct resource)DEFINE_RES_MEM(0, 0);
> > -
> > -	device_lock(&port->dev);
> > -	dport = list_first_entry(&port->dports, typeof(*dport), list);
> > -	device_unlock(&port->dev);
> > -
> > -	single_port_map[0] = dport->port_id;
> > -
> > -	rc = cxl_decoder_add(cxld, single_port_map);
> > -	if (rc)
> > -		put_device(&cxld->dev);
> > -	else
> > -		rc = cxl_decoder_autoremove(host, cxld);
> > -
> > -	if (rc == 0)
> > -		dev_dbg(host, "add: %s\n", dev_name(&cxld->dev));
> > -	return rc;
> > +	/* Host bridge ports are enumerated by the port driver. */
> > +	return 0;
> >  }
> >  
> >  struct cxl_chbs_context {
> > @@ -448,3 +412,4 @@ module_platform_driver(cxl_acpi_driver);
> >  MODULE_LICENSE("GPL v2");
> >  MODULE_IMPORT_NS(CXL);
> >  MODULE_IMPORT_NS(ACPI);
> > +MODULE_SOFTDEP("pre: cxl_port");
> > diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c
> > index 46a06cfe79bd..acfa212eea21 100644
> > --- a/drivers/cxl/core/bus.c
> > +++ b/drivers/cxl/core/bus.c
> > @@ -62,7 +62,7 @@ void cxl_unregister_topology_host(struct device *host)
> >  }
> >  EXPORT_SYMBOL_NS_GPL(cxl_unregister_topology_host, CXL);
> >  
> > -static struct device *get_cxl_topology_host(void)
> > +struct device *get_cxl_topology_host(void)
> >  {
> >  	down_read(&topology_host_sem);
> >  	if (cxl_topology_host)
> > @@ -70,12 +70,14 @@ static struct device *get_cxl_topology_host(void)
> >  	up_read(&topology_host_sem);
> >  	return NULL;
> >  }
> > +EXPORT_SYMBOL_NS_GPL(get_cxl_topology_host, CXL);
> >  
> > -static void put_cxl_topology_host(struct device *dev)
> > +void put_cxl_topology_host(struct device *dev)
> >  {
> >  	WARN_ON(dev != cxl_topology_host);
> >  	up_read(&topology_host_sem);
> >  }
> > +EXPORT_SYMBOL_NS_GPL(put_cxl_topology_host, CXL);
> >  
> >  static int decoder_match(struct device *dev, void *data)
> >  {
> > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> > index 24fa16157d5e..f8354241c5a3 100644
> > --- a/drivers/cxl/cxl.h
> > +++ b/drivers/cxl/cxl.h
> > @@ -170,6 +170,8 @@ void __iomem *devm_cxl_iomap_block(struct device *dev, resource_size_t addr,
> >  
> >  int cxl_register_topology_host(struct device *host);
> >  void cxl_unregister_topology_host(struct device *host);
> > +struct device *get_cxl_topology_host(void);
> > +void put_cxl_topology_host(struct device *dev);
> >  
> >  /*
> >   * cxl_decoder flags that define the type of memory / devices this
> > diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
> > index 3c03131517af..7a1fc726fe9f 100644
> > --- a/drivers/cxl/port.c
> > +++ b/drivers/cxl/port.c
> > @@ -233,12 +233,64 @@ static int enumerate_hdm_decoders(struct cxl_port *port,
> >  	return 0;
> >  }
> >  
> > +/*
> > + * Per the CXL specification (8.2.5.12 CXL HDM Decoder Capability Structure)
> > + * single ported host-bridges need not publish a decoder capability when a
> > + * passthrough decode can be assumed, i.e. all transactions that the uport sees
> > + * are claimed and passed to the single dport. Disable the range until the first
> > + * CXL region is enumerated / activated.
> > + */
> > +static int add_passthrough_decoder(struct cxl_port *port)
> > +{
> > +	int single_port_map[1], rc;
> > +	struct cxl_decoder *cxld;
> > +	struct cxl_dport *dport;
> > +
> > +	device_lock_assert(&port->dev);
> > +
> > +	cxld = cxl_decoder_alloc(port, 1);
> > +	if (IS_ERR(cxld))
> > +		return PTR_ERR(cxld);
> > +
> > +	cxld->interleave_ways = 1;
> > +	cxld->interleave_granularity = PAGE_SIZE;
> > +	cxld->target_type = CXL_DECODER_EXPANDER;
> > +	cxld->platform_res = (struct resource)DEFINE_RES_MEM(0, 0);
> > +
> > +	dport = list_first_entry(&port->dports, typeof(*dport), list);
> > +	single_port_map[0] = dport->port_id;
> > +
> > +	rc = cxl_decoder_add_locked(cxld, single_port_map);
> > +	if (rc)
> > +		put_device(&cxld->dev);
> 
> I would handle this error path entirely here, or use a goto rather
> than messing up the good path with conditionals on each element,
> particularly as there isn't much to do in the error paths.
> I guess this might get more complicated in later patches though.
> 
> Obviously that tidy up would make this more complex than simply moving
> the code. (I might have commented on this before, but too long ago to remember ;)
> 
> 	if (rc) {
> 		put_device(&cxld->dev);
> 		return rc;
> 	}
> 	rc = cxl_decoder...
> 	if (rc)
> 		return rc;
> 
> 	dev_dbg(..
> 
> 	return 0;
> 

Since I changed this in v2 for the port driver, in the last patch, I think it
makes sense to do the move, and then cleanup.

Thanks.

> > +	else
> > +		rc = cxl_decoder_autoremove(&port->dev, cxld);
> > +
> > +	if (rc == 0)
> > +		dev_dbg(&port->dev, "add: %s\n", dev_name(&cxld->dev));
> > +
> > +	return rc;
> > +}
> > +