diff mbox series

[14/23] cxl: Introduce topology host registration

Message ID 20211120000250.1663391-15-ben.widawsky@intel.com
State Superseded
Headers show
Series Add drivers for CXL ports and mem devices | expand

Commit Message

Ben Widawsky Nov. 20, 2021, 12:02 a.m. UTC 
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>
---
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(-)

Comments

Jonathan Cameron Nov. 22, 2021, 6:20 p.m. UTC | #1 
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);
>
Ben Widawsky Nov. 22, 2021, 10:30 p.m. UTC | #2 
On 21-11-22 18:20:15, Jonathan Cameron wrote:
> 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>
> 

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);
> >  
>
Dan Williams Nov. 25, 2021, 1:09 a.m. UTC | #3 
On Fri, Nov 19, 2021 at 4:03 PM 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.

Wait, no, that was one of the original motivations, but then we
discussed here [1] that devm teardown of a topology can happen
naturally / hierarchically.

[1]: https://lore.kernel.org/r/CAPcyv4ikVFFqyfH2zLhBVJ28N1_gufGHd2gVbP2h+Rv2cZEpeA@mail.gmail.com

No, the reason for the cxl_topology_host is as a constraint for when
CXL.mem connectivity can be verified from root to endpoint. Given that
endpoints can attach at any point in time relative to when the root
arrives CXL.mem connectivity needs to be revalidated at every topology
host arrival / depart event.
Dan Carpenter Nov. 29, 2021, 11:42 a.m. UTC | #4 
Hi Ben,

url:    https://github.com/0day-ci/linux/commits/Ben-Widawsky/Add-drivers-for-CXL-ports-and-mem-devices/20211120-080513
base:   53989fad1286e652ea3655ae3367ba698da8d2ff
config: x86_64-randconfig-m001-20211118 (https://download.01.org/0day-ci/archive/20211126/202111260523.BAvGTRJR-lkp@intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/cxl/acpi.c:399 cxl_acpi_probe() error: uninitialized symbol 'root_port'.

vim +/root_port +399 drivers/cxl/acpi.c

4812be97c015bd Dan Williams     2021-06-09  383  static int cxl_acpi_probe(struct platform_device *pdev)
4812be97c015bd Dan Williams     2021-06-09  384  {
3b94ce7b7bc1b4 Dan Williams     2021-06-09  385  	int rc;
4812be97c015bd Dan Williams     2021-06-09  386  	struct cxl_port *root_port;
4812be97c015bd Dan Williams     2021-06-09  387  	struct device *host = &pdev->dev;
7d4b5ca2e2cb5d Dan Williams     2021-06-09  388  	struct acpi_device *adev = ACPI_COMPANION(host);
f4ce1f766f1ebf Dan Williams     2021-10-29  389  	struct cxl_cfmws_context ctx;
4812be97c015bd Dan Williams     2021-06-09  390  
6b4661f8037e4f Ben Widawsky     2021-11-19  391  	rc = cxl_register_topology_host(host);
6b4661f8037e4f Ben Widawsky     2021-11-19  392  	if (rc)
6b4661f8037e4f Ben Widawsky     2021-11-19  393  		return rc;
6b4661f8037e4f Ben Widawsky     2021-11-19  394  
6b4661f8037e4f Ben Widawsky     2021-11-19  395  	rc = devm_add_action_or_reset(host, clear_topology_host, host);
6b4661f8037e4f Ben Widawsky     2021-11-19  396  	if (rc)
6b4661f8037e4f Ben Widawsky     2021-11-19  397  		return rc;
6b4661f8037e4f Ben Widawsky     2021-11-19  398  
6b4661f8037e4f Ben Widawsky     2021-11-19 @399  	root_port = devm_cxl_add_port(host, CXL_RESOURCE_NONE, root_port);
                                                                                                               ^^^^^^^^^^
Uninitialized.

4812be97c015bd Dan Williams     2021-06-09  400  	if (IS_ERR(root_port))
4812be97c015bd Dan Williams     2021-06-09  401  		return PTR_ERR(root_port);
4812be97c015bd Dan Williams     2021-06-09  402  	dev_dbg(host, "add: %s\n", dev_name(&root_port->dev));
4812be97c015bd Dan Williams     2021-06-09  403  
3b94ce7b7bc1b4 Dan Williams     2021-06-09  404  	rc = bus_for_each_dev(adev->dev.bus, NULL, root_port,
7d4b5ca2e2cb5d Dan Williams     2021-06-09  405  			      add_host_bridge_dport);
f4ce1f766f1ebf Dan Williams     2021-10-29  406  	if (rc < 0)
f4ce1f766f1ebf Dan Williams     2021-10-29  407  		return rc;
3b94ce7b7bc1b4 Dan Williams     2021-06-09  408  
f4ce1f766f1ebf Dan Williams     2021-10-29  409  	ctx = (struct cxl_cfmws_context) {
f4ce1f766f1ebf Dan Williams     2021-10-29  410  		.dev = host,
f4ce1f766f1ebf Dan Williams     2021-10-29  411  		.root_port = root_port,
f4ce1f766f1ebf Dan Williams     2021-10-29  412  	};
f4ce1f766f1ebf Dan Williams     2021-10-29  413  	acpi_table_parse_cedt(ACPI_CEDT_TYPE_CFMWS, cxl_parse_cfmws, &ctx);
3e23d17ce1980c Alison Schofield 2021-06-17  414  
3b94ce7b7bc1b4 Dan Williams     2021-06-09  415  	/*
3b94ce7b7bc1b4 Dan Williams     2021-06-09  416  	 * Root level scanned with host-bridge as dports, now scan host-bridges
3b94ce7b7bc1b4 Dan Williams     2021-06-09  417  	 * for their role as CXL uports to their CXL-capable PCIe Root Ports.
3b94ce7b7bc1b4 Dan Williams     2021-06-09  418  	 */
8fdcb1704f61a8 Dan Williams     2021-06-15  419  	rc = bus_for_each_dev(adev->dev.bus, NULL, root_port,
3b94ce7b7bc1b4 Dan Williams     2021-06-09  420  			      add_host_bridge_uport);
f4ce1f766f1ebf Dan Williams     2021-10-29  421  	if (rc < 0)
f4ce1f766f1ebf Dan Williams     2021-10-29  422  		return rc;
8fdcb1704f61a8 Dan Williams     2021-06-15  423  
8fdcb1704f61a8 Dan Williams     2021-06-15  424  	if (IS_ENABLED(CONFIG_CXL_PMEM))
8fdcb1704f61a8 Dan Williams     2021-06-15  425  		rc = device_for_each_child(&root_port->dev, root_port,
8fdcb1704f61a8 Dan Williams     2021-06-15  426  					   add_root_nvdimm_bridge);
8fdcb1704f61a8 Dan Williams     2021-06-15  427  	if (rc < 0)
8fdcb1704f61a8 Dan Williams     2021-06-15  428  		return rc;
f4ce1f766f1ebf Dan Williams     2021-10-29  429  
8fdcb1704f61a8 Dan Williams     2021-06-15  430  	return 0;
4812be97c015bd Dan Williams     2021-06-09  431  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Ben Widawsky Nov. 29, 2021, 9:23 p.m. UTC | #5 
On 21-11-24 17:09:03, Dan Williams wrote:
> On Fri, Nov 19, 2021 at 4:03 PM 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.
> 
> Wait, no, that was one of the original motivations, but then we
> discussed here [1] that devm teardown of a topology can happen
> naturally / hierarchically.
> 
> [1]: https://lore.kernel.org/r/CAPcyv4ikVFFqyfH2zLhBVJ28N1_gufGHd2gVbP2h+Rv2cZEpeA@mail.gmail.com
> 
> No, the reason for the cxl_topology_host is as a constraint for when
> CXL.mem connectivity can be verified from root to endpoint. Given that
> endpoints can attach at any point in time relative to when the root
> arrives CXL.mem connectivity needs to be revalidated at every topology
> host arrival / depart event.

Oops. I forgot to update the commit message, I will take what you wrote with
slight modification.
diff mbox series

Patch

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