Re: [PATCH v2 3/3] cxl: Clean up cxl_nvdimm devices when removing nvdimm_bus

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

Dave Jiang wrote:
> In the current implementation, when the nvdimm_bus is removed, the
> cxl_nvdimm devices only have the driver unbound which leaves orphaned
> cxl_nvdimm devices around. Add removal of cxl_nvdimm devices when the
> nvdimm_bus object is removed on cxl_nvb driver removal.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/core/pmem.c | 6 +++++-
>  drivers/cxl/cxl.h       | 1 +
>  drivers/cxl/pmem.c      | 5 +++++
>  3 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/pmem.c b/drivers/cxl/core/pmem.c
> index f7f784047450..4edc87ceb993 100644
> --- a/drivers/cxl/core/pmem.c
> +++ b/drivers/cxl/core/pmem.c
> @@ -142,18 +142,22 @@ static struct cxl_nvdimm *cxl_nvdimm_alloc(struct cxl_nvdimm_bridge *cxl_nvb,
>  	return cxl_nvd;
>  }
>  
> -static void cxlmd_release_nvdimm(void *_cxlmd)
> +void cxlmd_release_nvdimm(void *_cxlmd)
>  {
>  	struct cxl_memdev *cxlmd = _cxlmd;
>  	struct cxl_nvdimm *cxl_nvd = cxlmd->cxl_nvd;
>  	struct cxl_nvdimm_bridge *cxl_nvb = cxlmd->cxl_nvb;
>  
> +	dev_info(&cxlmd->dev, "XXX release nvdimm %s\n",
> +		 cxl_nvd ? dev_name(&cxl_nvd->dev) : "none");
> +

Stale debug?

>  	cxl_nvd->cxlmd = NULL;
>  	cxlmd->cxl_nvd = NULL;
>  	cxlmd->cxl_nvb = NULL;
>  	device_unregister(&cxl_nvd->dev);
>  	put_device(&cxl_nvb->dev);
>  }
> +EXPORT_SYMBOL_NS_GPL(cxlmd_release_nvdimm, "CXL");
>  
>  /**
>   * devm_cxl_add_nvdimm() - add a bridge between a cxl_memdev and an nvdimm
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 3b79d5c4765a..3f0d927e5772 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -926,6 +926,7 @@ bool is_cxl_nvdimm(struct device *dev);
>  int devm_cxl_add_nvdimm(struct device *host, struct cxl_port *port,
>  			struct cxl_memdev *cxlmd);
>  struct cxl_nvdimm_bridge *cxl_find_nvdimm_bridge(struct cxl_port *port);
> +void cxlmd_release_nvdimm(void *cxlmd);
>  
>  #ifdef CONFIG_CXL_REGION
>  bool is_cxl_pmem_region(struct device *dev);
> diff --git a/drivers/cxl/pmem.c b/drivers/cxl/pmem.c
> index 0b200be1fd36..edce5269a682 100644
> --- a/drivers/cxl/pmem.c
> +++ b/drivers/cxl/pmem.c
> @@ -397,6 +397,7 @@ static int cxl_pmem_ctl(struct nvdimm_bus_descriptor *nd_desc,
>  static int detach_nvdimm(struct device *dev, void *data)
>  {
>  	struct cxl_nvdimm *cxl_nvd;
> +	struct cxl_memdev *cxlmd;
>  	bool release = false;
>  
>  	if (!is_cxl_nvdimm(dev))
> @@ -411,6 +412,10 @@ static int detach_nvdimm(struct device *dev, void *data)
>  	}
>  	if (release)
>  		device_release_driver(dev);
> +
> +	cxlmd = cxl_nvd->cxlmd;
> +	devm_release_action(&cxlmd->dev, cxlmd_release_nvdimm, cxlmd);

You need to hold the cxlmd device lock and check against dev.driver
before invoking its release actions. I worry that lockdep will not like
that, but worth a shot.

It might be sufficient to just set an INVALIDATED flag in the cxl_nvdimm
object that cxl_nvdimm_probe() can check to give up early on trying to
re-enable the device.

This would be similar to this check in cxl_mem_probe():

        if (work_pending(&cxlmd->detach_work))
                return -EBUSY;

...which prevents reattach while detach cleanup is pending.