On 21-11-22 18:20:15, Jonathan Cameron wrote:
> 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>
>
Thanks. I noticed another commit message bug, s/ACPI0016/ACPI0017\/CEDT above.
> > ---
> > 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);
> >
>
