Re: [PATCH 6/6] cxl/mem: Introduce a memdev creation ->probe() operation

[<dan.j.williams@intel.com>]

dan.j.williams@ wrote:
[..]
> That is functionally equivalent to a new devm_cxl_add_memdev() flag, and
> is something that can come later when such a driver arrives.

Here are the fixups collected for a v2, I added some documentation of
the expectations around @ops:

1:  aa399f4e13c5 = 1:  1368388728fd cxl/mem: Fix devm_cxl_memdev_edac_release() confusion
2:  120ac819e8cb = 2:  f56b7c7ec1cb cxl/mem: Arrange for always-synchronous memdev attach
3:  bcb0109994a3 = 3:  1fe83e925d59 cxl/port: Arrange for always synchronous endpoint attach
4:  74c426d1dd5d ! 4:  9bd03230d6f3 cxl/mem: Convert devm_cxl_add_memdev() to scope-based-cleanup
    @@ drivers/cxl/core/memdev.c: static const struct file_operations cxl_memdev_fops =
     +
     +DEFINE_FREE(put_cxlmd, struct cxl_memdev *,
     +	    if (!IS_ERR_OR_NULL(_T)) put_device(&_T->dev);)
    ++
    ++static struct cxl_memdev *cxl_memdev_autoremove(struct cxl_memdev *cxlmd)
    ++{
    ++	int rc;
    ++
    ++	rc = devm_add_action_or_reset(cxlmd->cxlds->dev, cxl_memdev_unregister,
    ++				      cxlmd);
    ++	if (rc)
    ++		return ERR_PTR(rc);
    ++
    ++	return cxlmd;
    ++}
     +
      /*
       * Core helper for devm_cxl_add_memdev() that wants to both create a device and
    @@ drivers/cxl/core/memdev.c: static const struct file_operations cxl_memdev_fops =
     -	 */
     -	cxlmd->cxlds = cxlds;
     -	cxlds->cxlmd = cxlmd;
    -+		return ERR_PTR(rc);
    - 
    +-
     -	cdev = &cxlmd->cdev;
     -	rc = cdev_device_add(cdev, dev);
    -+	rc = cxlmd_add(cxlmd, cxlds);
    - 	if (rc)
    +-	if (rc)
     -		goto err;
     +		return ERR_PTR(rc);
      
     -	rc = devm_add_action_or_reset(host, cxl_memdev_unregister, cxlmd);
    -+	rc = devm_add_action_or_reset(host, cxl_memdev_unregister,
    -+				      no_free_ptr(cxlmd));
    ++	rc = cxlmd_add(cxlmd, cxlds);
      	if (rc)
      		return ERR_PTR(rc);
    - 	return cxlmd;
    --
    +-	return cxlmd;
    + 
     -err:
     -	/*
     -	 * The cdev was briefly live, shutdown any ioctl operations that
    @@ drivers/cxl/core/memdev.c: static const struct file_operations cxl_memdev_fops =
     -	cxl_memdev_shutdown(dev);
     -	put_device(dev);
     -	return ERR_PTR(rc);
    ++	return cxl_memdev_autoremove(no_free_ptr(cxlmd));
      }
      EXPORT_SYMBOL_FOR_MODULES(__devm_cxl_add_memdev, "cxl_mem");
      
5:  b48acca13cdb ! 5:  e636831b1ff9 cxl/mem: Drop @host argument to devm_cxl_add_memdev()
    @@ drivers/cxl/cxlmem.h: static inline bool is_cxl_endpoint(struct cxl_port *port)
      struct cxl_memdev_state;
     
      ## drivers/cxl/core/memdev.c ##
    -@@ drivers/cxl/core/memdev.c: DEFINE_FREE(put_cxlmd, struct cxl_memdev *,
    +@@ drivers/cxl/core/memdev.c: static struct cxl_memdev *cxl_memdev_autoremove(struct cxl_memdev *cxlmd)
       * Core helper for devm_cxl_add_memdev() that wants to both create a device and
       * assert to the caller that upon return cxl_mem::probe() has been invoked.
       */
    @@ drivers/cxl/core/memdev.c: DEFINE_FREE(put_cxlmd, struct cxl_memdev *,
      {
      	struct device *dev;
      	int rc;
    -@@ drivers/cxl/core/memdev.c: struct cxl_memdev *__devm_cxl_add_memdev(struct device *host,
    - 	if (rc)
    - 		return ERR_PTR(rc);
    - 
    --	rc = devm_add_action_or_reset(host, cxl_memdev_unregister,
    -+	rc = devm_add_action_or_reset(cxlds->dev, cxl_memdev_unregister,
    - 				      no_free_ptr(cxlmd));
    - 	if (rc)
    - 		return ERR_PTR(rc);
     
      ## drivers/cxl/mem.c ##
     @@ drivers/cxl/mem.c: static int cxl_mem_probe(struct device *dev)
6:  f7e58dea4878 ! 6:  13c07d702c92 cxl/mem: Introduce a memdev creation ->probe() operation
    @@ drivers/cxl/core/memdev.c: static struct cxl_memdev *cxl_memdev_alloc(struct cxl
      
      	dev = &cxlmd->dev;
      	device_initialize(dev);
    -@@ drivers/cxl/core/memdev.c: static int cxlmd_add(struct cxl_memdev *cxlmd, struct cxl_dev_state *cxlds)
    - DEFINE_FREE(put_cxlmd, struct cxl_memdev *,
    - 	    if (!IS_ERR_OR_NULL(_T)) put_device(&_T->dev);)
    +@@ drivers/cxl/core/memdev.c: static struct cxl_memdev *cxl_memdev_autoremove(struct cxl_memdev *cxlmd)
    + {
    + 	int rc;
      
    -+static struct cxl_memdev *cxl_memdev_autoremove(struct cxl_memdev *cxlmd)
    -+{
    -+	struct cxl_memdev *ret = cxlmd;
    -+	int rc;
    -+
     +	/*
    -+	 * If ops is provided fail if the driver is not attached upon
    -+	 * return. The ->endpoint ERR_PTR may have a more precise error
    -+	 * code to convey. Note that failure here could be the result of
    -+	 * a race to teardown the CXL port topology. I.e.
    -+	 * cxl_mem_probe() could have succeeded and then cxl_mem unbound
    -+	 * before the lock is acquired.
    ++	 * If ops is provided fail if the driver is not attached upon return.
    ++	 * Note that failure here could be the result of a race to teardown the
    ++	 * CXL port topology. I.e. cxl_mem_probe() could have succeeded and then
    ++	 * cxl_mem unbound before the lock is acquired.
     +	 */
     +	guard(device)(&cxlmd->dev);
     +	if (cxlmd->ops && !cxlmd->dev.driver) {
    -+		ret = ERR_PTR(-ENXIO);
    -+		if (IS_ERR(cxlmd->endpoint))
    -+			ret = ERR_CAST(cxlmd->endpoint);
     +		cxl_memdev_unregister(cxlmd);
    -+		return ret;
    ++		return ERR_PTR(-ENXIO);
     +	}
     +
    -+	rc = devm_add_action_or_reset(cxlmd->cxlds->dev, cxl_memdev_unregister,
    -+				      cxlmd);
    -+	if (rc)
    -+		return ERR_PTR(rc);
    -+
    -+	return ret;
    -+}
    -+
    - /*
    + 	rc = devm_add_action_or_reset(cxlmd->cxlds->dev, cxl_memdev_unregister,
    + 				      cxlmd);
    + 	if (rc)
    +@@ drivers/cxl/core/memdev.c: static struct cxl_memdev *cxl_memdev_autoremove(struct cxl_memdev *cxlmd)
       * Core helper for devm_cxl_add_memdev() that wants to both create a device and
       * assert to the caller that upon return cxl_mem::probe() has been invoked.
       */
    @@ drivers/cxl/core/memdev.c: static int cxlmd_add(struct cxl_memdev *cxlmd, struct
      	if (IS_ERR(cxlmd))
      		return cxlmd;
      
    -@@ drivers/cxl/core/memdev.c: struct cxl_memdev *__devm_cxl_add_memdev(struct cxl_dev_state *cxlds)
    - 	if (rc)
    - 		return ERR_PTR(rc);
    - 
    --	rc = devm_add_action_or_reset(cxlds->dev, cxl_memdev_unregister,
    --				      no_free_ptr(cxlmd));
    --	if (rc)
    --		return ERR_PTR(rc);
    --	return cxlmd;
    -+	return cxl_memdev_autoremove(no_free_ptr(cxlmd));
    - }
    - EXPORT_SYMBOL_FOR_MODULES(__devm_cxl_add_memdev, "cxl_mem");
    - 
     
      ## drivers/cxl/mem.c ##
     @@ drivers/cxl/mem.c: static int cxl_mem_probe(struct device *dev)
    @@ drivers/cxl/mem.c: static int cxl_mem_probe(struct device *dev)
      /**
       * devm_cxl_add_memdev - Add a CXL memory device
       * @cxlds: CXL device state to associate with the memdev
    -+ * @ops: optional operations to run in cxl_mem::{probe,remove}() context
    ++ * @ops: optional operations to run in cxl_mem_probe() context
       *
       * Upon return the device will have had a chance to attach to the
    -  * cxl_mem driver, but may fail if the CXL topology is not ready
    -@@ drivers/cxl/mem.c: static int cxl_mem_probe(struct device *dev)
    +- * cxl_mem driver, but may fail if the CXL topology is not ready
    +- * (hardware CXL link down, or software platform CXL root not attached)
    ++ * cxl_mem driver, but may fail to attach if the CXL topology is not ready
    ++ * (hardware CXL link down, or software platform CXL root not attached).
    ++ *
    ++ * When @ops is NULL it indicates the caller wants the memdev to remain
    ++ * registered even if it does not immediately attach to the CXL hierarchy. When
    ++ * @ops is provided a cxl_mem_probe() failure leads to failure of this routine.
    +  *
       * The parent of the resulting device and the devm context for allocations is
       * @cxlds->dev.
       */