1. Patchew
  2. linux
  3. [RFC PATCH 0/6] Add support for root port RAS error handling
  4. View patch

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

Li Ming posted 6 patches 1 year, 9 months ago
[RFC PATCH 5/6] cxl: Use __free() for cxl_pci/mem_find_port() to drop put_device()
Posted by Li Ming 1 year, 9 months ago 
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)
 		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;
 
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;
-
 	component_reg_phys = cxl_rcd_component_reg_phys(&pdev->dev, dport);
 
-	put_device(&port->dev);
-
 	if (component_reg_phys == CXL_RESOURCE_NONE)
 		return -ENXIO;
 
-- 
2.40.1
Re: [RFC PATCH 5/6] cxl: Use __free() for cxl_pci/mem_find_port() to drop put_device()
Posted by Dan Williams 1 year, 9 months ago 
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.
Re: [RFC PATCH 5/6] cxl: Use __free() for cxl_pci/mem_find_port() to drop put_device()
Posted by Li, Ming 1 year, 9 months ago 
On 3/15/2024 10:24 AM, Dan Williams wrote:
> 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.

Thanks, I will do that.

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.