On Fri, 19 Nov 2021 16:02:41 -0800
Ben Widawsky <ben.widawsky@xxxxxxxxx> 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@xxxxxxxxx>
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@xxxxxxxxxx>
> ---
> 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);
>
