Message-ID: <67abf81f4617b_2d1e2946a@dwillia2-xfh.jf.intel.com.notmuch>
Date: Tue, 11 Feb 2025 17:23:43 -0800
From: Dan Williams <dan.j.williams@...el.com>
To: Terry Bowman <terry.bowman@....com>, <linux-cxl@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <linux-pci@...r.kernel.org>,
	<nifan.cxl@...il.com>, <dave@...olabs.net>, <jonathan.cameron@...wei.com>,
	<dave.jiang@...el.com>, <alison.schofield@...el.com>,
	<vishal.l.verma@...el.com>, <dan.j.williams@...el.com>,
	<bhelgaas@...gle.com>, <mahesh@...ux.ibm.com>, <ira.weiny@...el.com>,
	<oohall@...il.com>, <Benjamin.Cheatham@....com>, <rrichter@....com>,
	<nathan.fontenot@....com>, <Smita.KoralahalliChannabasappa@....com>,
	<lukas@...ner.de>, <ming.li@...omail.com>,
	<PradeepVineshReddy.Kodamati@....com>
Subject: Re: [PATCH v7 07/17] cxl/pci: Map CXL PCIe Root Port and Downstream
 Switch Port RAS registers

Terry Bowman wrote:
> The CXL mem driver (cxl_mem) currently maps and caches a pointer to RAS
> registers for the endpoint's Root Port. The same needs to be done for
> each of the CXL Downstream Switch Ports and CXL Root Ports found between
> the endpoint and CXL Host Bridge.
> 
> Introduce cxl_init_ep_ports_aer() to be called for each CXL Port in the
> sub-topology between the endpoint and the CXL Host Bridge. This function
> will determine if there are CXL Downstream Switch Ports or CXL Root Ports
> associated with this Port. The same check will be added in the future for
> upstream switch ports.
> 
> Move the RAS register map logic from cxl_dport_map_ras() into
> cxl_dport_init_ras_reporting(). This eliminates the need for the helper
> function, cxl_dport_map_ras().

Not sure about the motivation here...

> cxl_init_ep_ports_aer() calls cxl_dport_init_ras_reporting() to map
> the RAS registers for CXL Downstream Switch Ports and CXL Root Ports.

Ok, makes sense...

> cxl_dport_init_ras_reporting() must check for previously mapped registers
> before mapping. This is required because multiple Endpoints under a CXL
> switch may share an upstream CXL Root Port, CXL Downstream Switch Port,
> or CXL Downstream Switch Port. Ensure the RAS registers are only mapped
> once.

Sounds broken. Every device upstream-port only has one downstream port.

A CXL switch config looks like this:

           │             
┌──────────┼────────────┐
│SWITCH   ┌┴─┐          │
│         │UP│          │
│         └─┬┘          │
│    ┌──────┼─────┐     │
│    │      │     │     │
│   ┌┴─┐  ┌─┴┐  ┌─┴┐    │
│   │DP│  │DP│  │DP│    │
│   └┬─┘  └─┬┘  └─┬┘    │
└────┼──────┼─────┼─────┘
    ┌┴─┐  ┌─┴┐  ┌─┴┐     
    │EP│  │EP│  │EP│     
    └──┘  └──┘  └──┘     

...so how can an endpoint ever find that its immediate parent downstream
port has already been mapped?

> Introduce a mutex for synchronizing accesses to the cached RAS mapping.

I suspect the motivation for the lock and "previously mapped" check was
due to noticing that the ras registers are not being unmapped, but
that's due to a devm bug below.

Even if it were the case that multiple resources need to share 1 devm
mapping, that would need to look something like the logic around
cxl_detach_ep(). In that arrangement, the first endpoint in the door
sets up the 'struct cxl_port' and its 'struct cxl_dport' instances, and
the last endpoint out the door tears it all down and turns off the
lights.

> Signed-off-by: Terry Bowman <terry.bowman@....com>
> Reviewed-by: Alejandro Lucero <alucerop@....com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> Reviewed-by: Gregory Price <gourry@...rry.net>
> ---
>  drivers/cxl/core/pci.c | 42 ++++++++++++++++++++----------------------
>  drivers/cxl/cxl.h      |  6 ++----
>  drivers/cxl/mem.c      | 31 +++++++++++++++++++++++++++++--
>  3 files changed, 51 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index a5c65f79db18..143c853a52c4 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -24,6 +24,8 @@ static unsigned short media_ready_timeout = 60;
>  module_param(media_ready_timeout, ushort, 0644);
>  MODULE_PARM_DESC(media_ready_timeout, "seconds to wait for media ready");
>  
> +static DEFINE_MUTEX(ras_init_mutex);
> +
>  struct cxl_walk_context {
>  	struct pci_bus *bus;
>  	struct cxl_port *port;
> @@ -749,18 +751,6 @@ static void cxl_dport_map_rch_aer(struct cxl_dport *dport)
>  	}
>  }
>  
> -static void cxl_dport_map_ras(struct cxl_dport *dport)
> -{
> -	struct cxl_register_map *map = &dport->reg_map;
> -	struct device *dev = dport->dport_dev;
> -
> -	if (!map->component_map.ras.valid)
> -		dev_dbg(dev, "RAS registers not found\n");
> -	else if (cxl_map_component_regs(map, &dport->regs.component,
> -					BIT(CXL_CM_CAP_CAP_ID_RAS)))
> -		dev_dbg(dev, "Failed to map RAS capability.\n");
> -}
> -
>  static void cxl_disable_rch_root_ints(struct cxl_dport *dport)
>  {
>  	void __iomem *aer_base = dport->regs.dport_aer;
> @@ -788,22 +778,30 @@ static void cxl_disable_rch_root_ints(struct cxl_dport *dport)
>  /**
>   * cxl_dport_init_ras_reporting - Setup CXL RAS report on this dport
>   * @dport: the cxl_dport that needs to be initialized
> - * @host: host device for devm operations
>   */
> -void cxl_dport_init_ras_reporting(struct cxl_dport *dport, struct device *host)
> +void cxl_dport_init_ras_reporting(struct cxl_dport *dport)
>  {
> -	dport->reg_map.host = host;
> -	cxl_dport_map_ras(dport);
> -
> -	if (dport->rch) {
> -		struct pci_host_bridge *host_bridge = to_pci_host_bridge(dport->dport_dev);
> -
> -		if (!host_bridge->native_aer)
> -			return;
> +	struct device *dport_dev = dport->dport_dev;
> +	struct pci_host_bridge *host_bridge = to_pci_host_bridge(dport_dev);
>  
> +	dport->reg_map.host = dport_dev;

This seems to be confused about how devm works. @host is passed in
because the cxl_memdev instance being probed in cxl_mem_probe() is doing
setup work on behalf of @dport_dev.

When the cxl_memdev goes through a ->remove() event, unbind from
cxl_mem, it tears down that mapping.

However, when using @dport_dev as the devm host, that mapping will not
be torn down until either the @dport_dev goes through a ->remove() event
or the device is unregistered altogether. There is no CXL subsystem
coordination with a driver for @dport_dev. The PCIe portdrv might have
an interest in it, but CXL can not depend on portdrv to map CXL
registers or keep the device bound while CXL has an interest those
registers. The devres_release_all() triggered by a
"device_del(@dport_dev)" is also uncoordinated with any CXL interest. In
general, it is a devm anti-pattern to depend on a device_del() event to
trigger devres_release_all().


> +	if (dport->rch && host_bridge->native_aer) {
>  		cxl_dport_map_rch_aer(dport);
>  		cxl_disable_rch_root_ints(dport);
>  	}
> +
> +	/* dport may have more than 1 downstream EP. Check if already mapped. */
> +	mutex_lock(&ras_init_mutex);

I suspect this lock and check got added to workaround "Failed to request
region" messages coming out of devm_cxl_iomap_block() in testing? Per
above, that's not "more than 1 downstream EPi", that's "failure to clean
up the last mapping for the next cxl_mem_probe() event of the same
endpoint".

Powered by blists - more mailing lists