Re: [PATCH 10/19] cxl/memdev: Indicate probe deferral

Re: [PATCH 10/19] cxl/memdev: Indicate probe deferral

[kernel test robot]

BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <168592155270.1948938.11536845108449547920.stgit@dwillia2-xfh.jf.intel.com>
References: <168592155270.1948938.11536845108449547920.stgit@dwillia2-xfh.jf.intel.com>
TO: Dan Williams <dan.j.williams@intel.com>

Hi Dan,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 9561de3a55bed6bdd44a12820ba81ec416e705a7]

url:    https://github.com/intel-lab-lkp/linux/commits/Dan-Williams/cxl-regs-Clarify-when-a-struct-cxl_register_map-is-input-vs-output/20230605-073402
base:   9561de3a55bed6bdd44a12820ba81ec416e705a7
patch link:    https://lore.kernel.org/r/168592155270.1948938.11536845108449547920.stgit%40dwillia2-xfh.jf.intel.com
patch subject: [PATCH 10/19] cxl/memdev: Indicate probe deferral
:::::: branch date: 4 days ago
:::::: commit date: 4 days ago
config: i386-randconfig-m021-20230608 (https://download.01.org/0day-ci/archive/20230609/202306090732.SLD5PmoG-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Closes: https://lore.kernel.org/r/202306090732.SLD5PmoG-lkp@intel.com/

smatch warnings:
drivers/cxl/core/memdev.c:642 cxl_acquire_endpoint() warn: inconsistent returns '&cxlmd->dev.mutex'.

vim +642 drivers/cxl/core/memdev.c

3d135db510240f Ben Widawsky 2021-08-02  611  
126ed1e685e817 Dan Williams 2023-06-04  612  /*
126ed1e685e817 Dan Williams 2023-06-04  613   * Try to get a locked reference on a memdev's CXL port topology
126ed1e685e817 Dan Williams 2023-06-04  614   * connection. Be careful to observe when cxl_mem_probe() has deposited
126ed1e685e817 Dan Williams 2023-06-04  615   * a probe deferral awaiting the arrival of the CXL root driver
126ed1e685e817 Dan Williams 2023-06-04  616   */
126ed1e685e817 Dan Williams 2023-06-04  617  struct cxl_port *cxl_acquire_endpoint(struct cxl_memdev *cxlmd)
126ed1e685e817 Dan Williams 2023-06-04  618  {
126ed1e685e817 Dan Williams 2023-06-04  619  	struct cxl_port *endpoint;
126ed1e685e817 Dan Williams 2023-06-04  620  	int rc = -ENXIO;
126ed1e685e817 Dan Williams 2023-06-04  621  
126ed1e685e817 Dan Williams 2023-06-04  622  	device_lock(&cxlmd->dev);
126ed1e685e817 Dan Williams 2023-06-04  623  	endpoint = cxlmd->endpoint;
126ed1e685e817 Dan Williams 2023-06-04  624  	if (!endpoint)
126ed1e685e817 Dan Williams 2023-06-04  625  		goto err;
126ed1e685e817 Dan Williams 2023-06-04  626  
126ed1e685e817 Dan Williams 2023-06-04  627  	if (IS_ERR(endpoint)) {
126ed1e685e817 Dan Williams 2023-06-04  628  		rc = PTR_ERR(endpoint);
126ed1e685e817 Dan Williams 2023-06-04  629  		goto err;
126ed1e685e817 Dan Williams 2023-06-04  630  	}
126ed1e685e817 Dan Williams 2023-06-04  631  
126ed1e685e817 Dan Williams 2023-06-04  632  	device_lock(&endpoint->dev);
126ed1e685e817 Dan Williams 2023-06-04  633  	if (!endpoint->dev.driver)
126ed1e685e817 Dan Williams 2023-06-04  634  		goto err_endpoint;
126ed1e685e817 Dan Williams 2023-06-04  635  
126ed1e685e817 Dan Williams 2023-06-04  636  	return endpoint;
126ed1e685e817 Dan Williams 2023-06-04  637  
126ed1e685e817 Dan Williams 2023-06-04  638  err_endpoint:
126ed1e685e817 Dan Williams 2023-06-04  639  	device_unlock(&endpoint->dev);
126ed1e685e817 Dan Williams 2023-06-04  640  err:
126ed1e685e817 Dan Williams 2023-06-04  641  	device_unlock(&cxlmd->dev);
126ed1e685e817 Dan Williams 2023-06-04 @642  	return ERR_PTR(rc);
126ed1e685e817 Dan Williams 2023-06-04  643  }
126ed1e685e817 Dan Williams 2023-06-04  644  EXPORT_SYMBOL_NS(cxl_acquire_endpoint, CXL);
126ed1e685e817 Dan Williams 2023-06-04  645  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH 00/19] cxl: Device memory setup

[Dan Williams]

There are 2 models for implementing CXL memory. To date the CXL enabling
has been focused on the common class-device case (Type-3). The
class-code mandates the implementation of standard mechanisms like the
mailbox and mandatory commands. The other model (Type-2) is
implementation specific memory typically associated with local device
memory for an accelerator. Start the support for Type-2 and take the
opportunity to better prepare the CXL core for "a la carte" enabling of
optional CXL features.

Now, to date there has not been any engagement on the list for an
accelerator driver that wants to reuse the CXL core, but I think it is
worth moving ahead with these patches for the following reasons:

1/ The refactoring of region creation is needed by Persistent Memory
   support where the kernel needs to create regions from labels, not sysfs
   input.

2/ The 'struct cxl_dev_state' object carries infrastructure that is
   optional outside of CXL memory-device class-code devices. That makes it
   difficult to even start the discussion with accelerator driver authors
   that want to evaluate what pieces of the CXL core are suitable to reuse.

3/ The example type-2 driver in cxl_test protects against
   type-3-exclusive assumptions from leaking back into the code base.

In other words it is difficult to start the "type-2" discussion when the
kernel is ~1500 lines of change from the baseline such a driver might
need, and the cleanups make the code more maintainable independent of an
immediate non-test user.

The first 9 patches are general cleanups, the last 10 are focused on
refactoring region creation in support of driver-instantiated CXL memory
regions.

---

Dan Williams (19):
      cxl/regs: Clarify when a 'struct cxl_register_map' is input vs output
      tools/testing/cxl: Remove unused @cxlds argument
      cxl/mbox: Move mailbox related driver state to its own data structure
      cxl/memdev: Make mailbox functionality optional
      cxl/port: Rename CXL_DECODER_{EXPANDER,ACCELERATOR} => {HOSTMEM,DEVMEM}
      cxl/hdm: Default CXL_DEVTYPE_DEVMEM decoders to CXL_DECODER_DEVMEM
      cxl/region: Manage decoder target_type at decoder-attach time
      cxl/port: Enumerate flit mode capability
      cxl/memdev: Formalize endpoint port linkage
      cxl/memdev: Indicate probe deferral
      cxl/region: Factor out construct_region_{begin,end} and drop_region() for reuse
      cxl/region: Factor out interleave ways setup
      cxl/region: Factor out interleave granularity setup
      cxl/region: Clarify locking requirements of cxl_region_attach()
      cxl/region: Specify host-only vs device memory at region creation time
      cxl/hdm: Define a driver interface for DPA allocation
      cxl/region: Define a driver interface for HPA free space enumeration
      cxl/region: Define a driver interface for region creation
      tools/testing/cxl: Emulate a CXL accelerator with local memory


 drivers/cxl/acpi.c           |    2 
 drivers/cxl/core/hdm.c       |  164 +++++++++++++---
 drivers/cxl/core/mbox.c      |  277 ++++++++++++++-------------
 drivers/cxl/core/memdev.c    |  108 +++++++++-
 drivers/cxl/core/pci.c       |   84 ++++++++
 drivers/cxl/core/pmem.c      |    2 
 drivers/cxl/core/port.c      |   19 +-
 drivers/cxl/core/region.c    |  437 ++++++++++++++++++++++++++++++++++++------
 drivers/cxl/core/regs.c      |    8 -
 drivers/cxl/cxl.h            |   21 ++
 drivers/cxl/cxlmem.h         |  124 ++++++++----
 drivers/cxl/cxlpci.h         |   25 ++
 drivers/cxl/mem.c            |   17 +-
 drivers/cxl/pci.c            |  114 ++++++-----
 drivers/cxl/pmem.c           |   35 ++-
 drivers/cxl/port.c           |    5 
 drivers/cxl/security.c       |   24 +-
 tools/testing/cxl/test/cxl.c |   20 ++
 tools/testing/cxl/test/mem.c |  214 ++++++++++++++-------
 19 files changed, 1245 insertions(+), 455 deletions(-)

base-commit: 9561de3a55bed6bdd44a12820ba81ec416e705a7

[PATCH 10/19] cxl/memdev: Indicate probe deferral

[Dan Williams]

The first stop for a CXL accelerator driver that wants to establish new
CXL.mem regions is to register a 'struct cxl_memdev'. That kicks off
cxl_mem_probe() to enumerate all 'struct cxl_port' instances in the
topology up to the root.

If the root driver has not attached yet the expectation is that the
driver waits until that link is established. The common cxl_pci driver
has reason to keep the 'struct cxl_memdev' device attached to the bus
until the root driver attaches. An accelerator may want to instead defer
probing until CXL resources can be acquired.

Use the @endpoint attribute of a 'struct cxl_memdev' to convey when
accelerator driver probing should be deferred vs failed. Provide that
indication via a new cxl_acquire_endpoint() API that can retrieve the
probe status of the memdev.

The first consumer of this API is a test driver that exercises the CXL
Type-2 flow.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/memdev.c |   41 +++++++++++++++++++++++++++++++++++++++++
 drivers/cxl/core/port.c   |    2 +-
 drivers/cxl/cxlmem.h      |    2 ++
 drivers/cxl/mem.c         |    7 +++++--
 4 files changed, 49 insertions(+), 3 deletions(-)

diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
index 65a685e5616f..859c43c340bb 100644
--- a/drivers/cxl/core/memdev.c
+++ b/drivers/cxl/core/memdev.c
@@ -609,6 +609,47 @@ struct cxl_memdev *devm_cxl_add_memdev(struct cxl_dev_state *cxlds)
 }
 EXPORT_SYMBOL_NS_GPL(devm_cxl_add_memdev, CXL);
 
+/*
+ * Try to get a locked reference on a memdev's CXL port topology
+ * connection. Be careful to observe when cxl_mem_probe() has deposited
+ * a probe deferral awaiting the arrival of the CXL root driver
+ */
+struct cxl_port *cxl_acquire_endpoint(struct cxl_memdev *cxlmd)
+{
+	struct cxl_port *endpoint;
+	int rc = -ENXIO;
+
+	device_lock(&cxlmd->dev);
+	endpoint = cxlmd->endpoint;
+	if (!endpoint)
+		goto err;
+
+	if (IS_ERR(endpoint)) {
+		rc = PTR_ERR(endpoint);
+		goto err;
+	}
+
+	device_lock(&endpoint->dev);
+	if (!endpoint->dev.driver)
+		goto err_endpoint;
+
+	return endpoint;
+
+err_endpoint:
+	device_unlock(&endpoint->dev);
+err:
+	device_unlock(&cxlmd->dev);
+	return ERR_PTR(rc);
+}
+EXPORT_SYMBOL_NS(cxl_acquire_endpoint, CXL);
+
+void cxl_release_endpoint(struct cxl_memdev *cxlmd, struct cxl_port *endpoint)
+{
+	device_unlock(&endpoint->dev);
+	device_unlock(&cxlmd->dev);
+}
+EXPORT_SYMBOL_NS(cxl_release_endpoint, CXL);
+
 __init int cxl_memdev_init(void)
 {
 	dev_t devt;
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 6720ab22a494..5e21b53362e6 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1336,7 +1336,7 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
 		 */
 		dev_dbg(&cxlmd->dev, "%s is a root dport\n",
 			dev_name(dport_dev));
-		return -ENXIO;
+		return -EPROBE_DEFER;
 	}
 
 	parent_port = find_cxl_port(dparent, &parent_dport);
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 7ee78e79933c..e3bcd6d12a1c 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -83,6 +83,8 @@ static inline bool is_cxl_endpoint(struct cxl_port *port)
 	return is_cxl_memdev(port->uport);
 }
 
+struct cxl_port *cxl_acquire_endpoint(struct cxl_memdev *cxlmd);
+void cxl_release_endpoint(struct cxl_memdev *cxlmd, struct cxl_port *endpoint);
 struct cxl_memdev *devm_cxl_add_memdev(struct cxl_dev_state *cxlds);
 int devm_cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled,
 			 resource_size_t base, resource_size_t len,
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index 584f9eec57e4..2470c6f2621c 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -154,13 +154,16 @@ static int cxl_mem_probe(struct device *dev)
 		return rc;
 
 	rc = devm_cxl_enumerate_ports(cxlmd);
-	if (rc)
+	if (rc) {
+		cxlmd->endpoint = ERR_PTR(rc);
 		return rc;
+	}
 
 	parent_port = cxl_mem_find_port(cxlmd, &dport);
 	if (!parent_port) {
 		dev_err(dev, "CXL port topology not found\n");
-		return -ENXIO;
+		cxlmd->endpoint = ERR_PTR(-EPROBE_DEFER);
+		return -EPROBE_DEFER;
 	}
 
 	if (dport->rch)

Re: [PATCH 10/19] cxl/memdev: Indicate probe deferral

[Jonathan Cameron]

On Sun, 04 Jun 2023 16:32:32 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> The first stop for a CXL accelerator driver that wants to establish new
> CXL.mem regions is to register a 'struct cxl_memdev'. That kicks off
> cxl_mem_probe() to enumerate all 'struct cxl_port' instances in the
> topology up to the root.
> 
> If the root driver has not attached yet the expectation is that the
> driver waits until that link is established. The common cxl_pci driver
> has reason to keep the 'struct cxl_memdev' device attached to the bus
> until the root driver attaches. An accelerator may want to instead defer
> probing until CXL resources can be acquired.
> 
> Use the @endpoint attribute of a 'struct cxl_memdev' to convey when
> accelerator driver probing should be deferred vs failed. Provide that
> indication via a new cxl_acquire_endpoint() API that can retrieve the
> probe status of the memdev.
> 
> The first consumer of this API is a test driver that exercises the CXL
> Type-2 flow.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/cxl/core/memdev.c |   41 +++++++++++++++++++++++++++++++++++++++++
>  drivers/cxl/core/port.c   |    2 +-
>  drivers/cxl/cxlmem.h      |    2 ++
>  drivers/cxl/mem.c         |    7 +++++--
>  4 files changed, 49 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index 65a685e5616f..859c43c340bb 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -609,6 +609,47 @@ struct cxl_memdev *devm_cxl_add_memdev(struct cxl_dev_state *cxlds)
>  }
>  EXPORT_SYMBOL_NS_GPL(devm_cxl_add_memdev, CXL);
>  
> +/*
> + * Try to get a locked reference on a memdev's CXL port topology
> + * connection. Be careful to observe when cxl_mem_probe() has deposited
> + * a probe deferral awaiting the arrival of the CXL root driver
> + */
> +struct cxl_port *cxl_acquire_endpoint(struct cxl_memdev *cxlmd)
> +{
> +	struct cxl_port *endpoint;
> +	int rc = -ENXIO;
> +
> +	device_lock(&cxlmd->dev);
> +	endpoint = cxlmd->endpoint;
> +	if (!endpoint)
> +		goto err;
> +
> +	if (IS_ERR(endpoint)) {
> +		rc = PTR_ERR(endpoint);

Whilst the above comment talks about deferred, there are paths where it's
potentially another error pointer currently.  Maybe suppress those at
source so the comment is 'precise' or make the comment above incorporate
those other error pointers.

> +		goto err;
> +	}
> +
> +	device_lock(&endpoint->dev);
> +	if (!endpoint->dev.driver)
> +		goto err_endpoint;
> +
> +	return endpoint;
> +
> +err_endpoint:
> +	device_unlock(&endpoint->dev);
> +err:
> +	device_unlock(&cxlmd->dev);
> +	return ERR_PTR(rc);
> +}
> +EXPORT_SYMBOL_NS(cxl_acquire_endpoint, CXL);
> +
> +void cxl_release_endpoint(struct cxl_memdev *cxlmd, struct cxl_port *endpoint)
> +{
> +	device_unlock(&endpoint->dev);
> +	device_unlock(&cxlmd->dev);
> +}
> +EXPORT_SYMBOL_NS(cxl_release_endpoint, CXL);
> +
>  __init int cxl_memdev_init(void)
>  {
>  	dev_t devt;
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 6720ab22a494..5e21b53362e6 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -1336,7 +1336,7 @@ static int add_port_attach_ep(struct cxl_memdev *cxlmd,
>  		 */
>  		dev_dbg(&cxlmd->dev, "%s is a root dport\n",
>  			dev_name(dport_dev));
> -		return -ENXIO;
> +		return -EPROBE_DEFER;
>  	}
>  
>  	parent_port = find_cxl_port(dparent, &parent_dport);
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 7ee78e79933c..e3bcd6d12a1c 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -83,6 +83,8 @@ static inline bool is_cxl_endpoint(struct cxl_port *port)
>  	return is_cxl_memdev(port->uport);
>  }
>  
> +struct cxl_port *cxl_acquire_endpoint(struct cxl_memdev *cxlmd);
> +void cxl_release_endpoint(struct cxl_memdev *cxlmd, struct cxl_port *endpoint);
>  struct cxl_memdev *devm_cxl_add_memdev(struct cxl_dev_state *cxlds);
>  int devm_cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled,
>  			 resource_size_t base, resource_size_t len,
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 584f9eec57e4..2470c6f2621c 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -154,13 +154,16 @@ static int cxl_mem_probe(struct device *dev)
>  		return rc;
>  
>  	rc = devm_cxl_enumerate_ports(cxlmd);
> -	if (rc)
> +	if (rc) {
> +		cxlmd->endpoint = ERR_PTR(rc);

This can be other errors than defer.  Which seems inconsistent
for the above check.

>  		return rc;
> +	}
>  
>  	parent_port = cxl_mem_find_port(cxlmd, &dport);
>  	if (!parent_port) {
>  		dev_err(dev, "CXL port topology not found\n");
> -		return -ENXIO;
> +		cxlmd->endpoint = ERR_PTR(-EPROBE_DEFER);
> +		return -EPROBE_DEFER;

This is a little inelegant as we aren't setting this in the same
function as were the endpoint is otherwise set up.  
Partly that's a naming thing.  In wonder if can pull out the bit
related to the endpoint from cxl_mem_probe() to a new
cxl_mem_ep_probe() at which case this will feel more logical.
That would be fine for this case, but the one above doesn't
really work for that...

Perhaps this is the best that can be done.


>  	}
>  
>  	if (dport->rch)
>