Re: [RFC PATCH 5/6] cxl: Use __free() for cxl_pci/mem_find_port() to drop put_device()

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

Li Ming wrote:
> Introduce a new helper function called put_cxl_port() to instead of the
> put_device() in order to release the device reference of struct cxl_port
> got via get_device() in cxl_pci/mem_find_port().
> 
> Besides, use scope-based resource management __free() to drop the open
> coded put_device() for each cxl_pci/mem_find_port().
> 
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Li Ming <ming4.li@intel.com>
> ---
>  drivers/cxl/core/pci.c  |  6 ++----
>  drivers/cxl/core/port.c |  9 +++++++++
>  drivers/cxl/cxl.h       |  2 ++
>  drivers/cxl/mem.c       |  5 ++---
>  drivers/cxl/pci.c       | 12 +++++-------
>  5 files changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 6c9c8d92f8f7..7254484330d2 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -902,15 +902,13 @@ static void cxl_handle_rdport_errors(struct cxl_dev_state *cxlds)
>  	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
>  	struct aer_capability_regs aer_regs;
>  	struct cxl_dport *dport;
> -	struct cxl_port *port;
>  	int severity;
> +	struct cxl_port *port __free(put_cxl_port) =
> +		cxl_pci_find_port(pdev, &dport);
>  
> -	port = cxl_pci_find_port(pdev, &dport);
>  	if (!port)

Keep the declaration separated from the rest of the variable
declarations like this:

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 0df09bd79408..2d1ef35fe99c 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -916,12 +916,11 @@ static void cxl_handle_rdport_errors(struct cxl_dev_state *cxlds)
        struct cxl_port *port;
        int severity;
 
-       port = cxl_pci_find_port(pdev, &dport);
+       struct cxl_port *port __free(put_cxl_port) =
+               cxl_pci_find_port(pdev, &dport);
        if (!port)
                return;
 
-       put_device(&port->dev);
-
        if (!cxl_rch_get_aer_info(dport->regs.dport_aer, &aer_regs))
                return;
 
>  		return;
>  
> -	put_device(&port->dev);
> -
>  	if (!cxl_rch_get_aer_info(dport->regs.dport_aer, &aer_regs))
>  		return;
>  
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index e59d9d37aa65..6e2fc2fce7c9 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -1671,6 +1671,15 @@ struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd,
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_mem_find_port, CXL);
>  
> +void put_cxl_port(struct cxl_port *port)
> +{
> +	if (!port)
> +		return;
> +
> +	put_device(&port->dev);
> +}
> +EXPORT_SYMBOL_NS_GPL(put_cxl_port, CXL);
> +
>  static int decoder_populate_targets(struct cxl_switch_decoder *cxlsd,
>  				    struct cxl_port *port, int *target_map)
>  {
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index b6017c0c57b4..476158782e3e 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -743,6 +743,8 @@ struct cxl_port *cxl_pci_find_port(struct pci_dev *pdev,
>  				   struct cxl_dport **dport);
>  struct cxl_port *cxl_mem_find_port(struct cxl_memdev *cxlmd,
>  				   struct cxl_dport **dport);
> +void put_cxl_port(struct cxl_port *port);
> +DEFINE_FREE(put_cxl_port, struct cxl_port *, if (_T) put_cxl_port(_T))
>  bool schedule_cxl_memdev_detach(struct cxl_memdev *cxlmd);
>  
>  struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port,
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index c5c9d8e0d88d..5aaa8ee2a46d 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -109,7 +109,6 @@ static int cxl_mem_probe(struct device *dev)
>  	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
>  	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>  	struct device *endpoint_parent;
> -	struct cxl_port *parent_port;
>  	struct cxl_dport *dport;
>  	struct dentry *dentry;
>  	int rc;
> @@ -146,7 +145,8 @@ static int cxl_mem_probe(struct device *dev)
>  	if (rc)
>  		return rc;
>  
> -	parent_port = cxl_mem_find_port(cxlmd, &dport);
> +	struct cxl_port *parent_port __free(put_cxl_port) =
> +		cxl_mem_find_port(cxlmd, &dport);
>  	if (!parent_port) {
>  		dev_err(dev, "CXL port topology not found\n");
>  		return -ENXIO;
> @@ -170,7 +170,6 @@ static int cxl_mem_probe(struct device *dev)
>  	rc = devm_cxl_add_endpoint(endpoint_parent, cxlmd, dport);
>  unlock:
>  	device_unlock(endpoint_parent);
> -	put_device(&parent_port->dev);
>  	if (rc)
>  		return rc;

When converting a function to use cleanup helpers, it should try to
remove all gotos, something like:

@@ -159,21 +159,18 @@ static int cxl_mem_probe(struct device *dev)
 
        cxl_setup_parent_dport(dev, dport);
 
-       device_lock(endpoint_parent);
-       if (!endpoint_parent->driver) {
-               dev_err(dev, "CXL port topology %s not enabled\n",
-                       dev_name(endpoint_parent));
-               rc = -ENXIO;
-               goto unlock;
+       scoped_guard(device, endpoint_parent) {
+               if (!endpoint_parent->driver) {
+                       dev_err(dev, "CXL port topology %s not enabled\n",
+                               dev_name(endpoint_parent));
+                       return -ENXIO;
+               }
+
+               rc = devm_cxl_add_endpoint(endpoint_parent, cxlmd, dport);
+               if (rc)
+                       return rc;
        }
 
-       rc = devm_cxl_add_endpoint(endpoint_parent, cxlmd, dport);
-unlock:
-       device_unlock(endpoint_parent);
-       put_device(&parent_port->dev);
-       if (rc)
-               return rc;
-
        if (resource_size(&cxlds->pmem_res) && IS_ENABLED(CONFIG_CXL_PMEM)) {
                rc = devm_cxl_add_nvdimm(cxlmd);
                if (rc == -ENODEV)


>  
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 4fd1f207c84e..d0ec8c5b1e99 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -473,23 +473,21 @@ static bool is_cxl_restricted(struct pci_dev *pdev)
>  static int cxl_rcrb_get_comp_regs(struct pci_dev *pdev,
>  				  struct cxl_register_map *map)
>  {
> -	struct cxl_port *port;
>  	struct cxl_dport *dport;
>  	resource_size_t component_reg_phys;
> +	struct cxl_port *port __free(put_cxl_port) =
> +		cxl_pci_find_port(pdev, &dport);
> +
> +	if (!port)
> +		return -EPROBE_DEFER;
>  
>  	*map = (struct cxl_register_map) {
>  		.host = &pdev->dev,
>  		.resource = CXL_RESOURCE_NONE,
>  	};
>  
> -	port = cxl_pci_find_port(pdev, &dport);
> -	if (!port)
> -		return -EPROBE_DEFER;
> -

Here again don't move the cxl_pci_find_port() earlier in the function,
keep it after the assignment of @map.