Re: [PATCH 14/23] cxl: Introduce topology host registration

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Fri, 19 Nov 2021 16:02:41 -0800
Ben Widawsky <ben.widawsky@intel.com> wrote:

> The description of the CXL topology will be conveyed by a platform
> specific entity that is expected to be a singleton. For ACPI based
> systems, this is ACPI0017. When the topology host goes away, which as of
> now can only be triggered by module unload, it is desirable to have the
> entire topology cleaned up. Regular devm unwinding handles most
> situations already, but what's missing is the ability to teardown the
> root port. Since the root port is platform specific, the core needs a
> set of APIs to allow platform specific drivers to register their root
> ports. With that, all the automatic teardown can occur.
> 
> cxl_test makes for an interesting case. cxl_test creates an alternate
> universe where there are possibly two root topology hosts (a real
> ACPI0016, and a fake ACPI0016). For this to work in the future, cxl_acpi
> (or some future platform host driver) will need to be unloaded first.
> 
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
This is a little unusual looking but having followed through how it is used
it seems like a sensible approach to me.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
> The topology lock can be used for more things. I'd like to save that as
> an add-on patch since it's extra risk for no reward, at this point.
> ---
>  drivers/cxl/acpi.c     | 18 ++++++++++---
>  drivers/cxl/core/bus.c | 57 +++++++++++++++++++++++++++++++++++++++---
>  drivers/cxl/cxl.h      |  5 +++-
>  3 files changed, 73 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index 3415184a2e61..82cc42ab38c6 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -224,8 +224,7 @@ static int add_host_bridge_uport(struct device *match, void *arg)
>  		return 0;
>  	}
>  
> -	port = devm_cxl_add_port(host, match, dport->component_reg_phys,
> -				 root_port);
> +	port = devm_cxl_add_port(match, dport->component_reg_phys, root_port);
>  	if (IS_ERR(port))
>  		return PTR_ERR(port);
>  	dev_dbg(host, "%s: add: %s\n", dev_name(match), dev_name(&port->dev));
> @@ -376,6 +375,11 @@ static int add_root_nvdimm_bridge(struct device *match, void *data)
>  	return 1;
>  }
>  
> +static void clear_topology_host(void *data)
> +{
> +	cxl_unregister_topology_host(data);
> +}
> +
>  static int cxl_acpi_probe(struct platform_device *pdev)
>  {
>  	int rc;
> @@ -384,7 +388,15 @@ static int cxl_acpi_probe(struct platform_device *pdev)
>  	struct acpi_device *adev = ACPI_COMPANION(host);
>  	struct cxl_cfmws_context ctx;
>  
> -	root_port = devm_cxl_add_port(host, host, CXL_RESOURCE_NONE, NULL);
> +	rc = cxl_register_topology_host(host);
> +	if (rc)
> +		return rc;
> +
> +	rc = devm_add_action_or_reset(host, clear_topology_host, host);
> +	if (rc)
> +		return rc;
> +
> +	root_port = devm_cxl_add_port(host, CXL_RESOURCE_NONE, root_port);
>  	if (IS_ERR(root_port))
>  		return PTR_ERR(root_port);
>  	dev_dbg(host, "add: %s\n", dev_name(&root_port->dev));
> diff --git a/drivers/cxl/core/bus.c b/drivers/cxl/core/bus.c
> index cd6fe7823c69..2ad38167796d 100644
> --- a/drivers/cxl/core/bus.c
> +++ b/drivers/cxl/core/bus.c
> @@ -25,6 +25,53 @@
>   */
>  
>  static DEFINE_IDA(cxl_port_ida);
> +static DECLARE_RWSEM(topology_host_sem);
> +
> +static struct device *cxl_topology_host;
> +
> +int cxl_register_topology_host(struct device *host)
> +{
> +	down_write(&topology_host_sem);
> +	if (cxl_topology_host) {
> +		up_write(&topology_host_sem);
> +		pr_warn("%s host currently in use. Please try unloading %s",
> +			dev_name(cxl_topology_host), host->driver->name);
> +		return -EBUSY;
> +	}
> +
> +	cxl_topology_host = host;
> +	up_write(&topology_host_sem);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_register_topology_host, CXL);
> +
> +void cxl_unregister_topology_host(struct device *host)
> +{
> +	down_write(&topology_host_sem);
> +	if (cxl_topology_host == host)
> +		cxl_topology_host = NULL;
> +	else
> +		pr_warn("topology host in use by %s\n",
> +			cxl_topology_host->driver->name);
> +	up_write(&topology_host_sem);
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_unregister_topology_host, CXL);
> +
> +static struct device *get_cxl_topology_host(void)
> +{
> +	down_read(&topology_host_sem);
> +	if (cxl_topology_host)
> +		return cxl_topology_host;
> +	up_read(&topology_host_sem);
> +	return NULL;
> +}
> +
> +static void put_cxl_topology_host(struct device *dev)
> +{
> +	WARN_ON(dev != cxl_topology_host);
> +	up_read(&topology_host_sem);
> +}
>  
>  static ssize_t devtype_show(struct device *dev, struct device_attribute *attr,
>  			    char *buf)
> @@ -362,17 +409,16 @@ static struct cxl_port *cxl_port_alloc(struct device *uport,
>  
>  /**
>   * devm_cxl_add_port - register a cxl_port in CXL memory decode hierarchy
> - * @host: host device for devm operations
>   * @uport: "physical" device implementing this upstream port
>   * @component_reg_phys: (optional) for configurable cxl_port instances
>   * @parent_port: next hop up in the CXL memory decode hierarchy
>   */
> -struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
> +struct cxl_port *devm_cxl_add_port(struct device *uport,
>  				   resource_size_t component_reg_phys,
>  				   struct cxl_port *parent_port)
>  {
> +	struct device *dev, *host;
>  	struct cxl_port *port;
> -	struct device *dev;
>  	int rc;
>  
>  	port = cxl_port_alloc(uport, component_reg_phys, parent_port);
> @@ -391,7 +437,12 @@ struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
>  	if (rc)
>  		goto err;
>  
> +	host = get_cxl_topology_host();
> +	if (!host)
> +		return ERR_PTR(-ENODEV);
> +
>  	rc = devm_add_action_or_reset(host, unregister_port, port);
> +	put_cxl_topology_host(host);
>  	if (rc)
>  		return ERR_PTR(rc);
>  
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 2c5627fa8a34..6fac4826d22b 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -152,6 +152,9 @@ int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type,
>  #define CXL_RESOURCE_NONE ((resource_size_t) -1)
>  #define CXL_TARGET_STRLEN 20
>  
> +int cxl_register_topology_host(struct device *host);
> +void cxl_unregister_topology_host(struct device *host);
> +
>  /*
>   * cxl_decoder flags that define the type of memory / devices this
>   * decoder supports as well as configuration lock status See "CXL 2.0
> @@ -279,7 +282,7 @@ struct cxl_dport {
>  };
>  
>  struct cxl_port *to_cxl_port(struct device *dev);
> -struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
> +struct cxl_port *devm_cxl_add_port(struct device *uport,
>  				   resource_size_t component_reg_phys,
>  				   struct cxl_port *parent_port);
>