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

["Li, Ming" <ming4.li@intel.com>]

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.