Re: [PATCH 3/3] cxl/port: Refactor __devm_cxl_add_port() to drop goto pattern

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

On 8/22/2024 6:01 AM, Ira Weiny wrote:
> Li Ming wrote:
>> The "goto error" pattern is not recommended, it can be removed via
>> refactoring. In __devm_cxl_add_port(), there is a 'goto' to call
>> put_device() for the error cases between device_initialize() and
>> device_add() to dereference the 'struct device' of a new cxl_port.
>> The refactoring is introducing a new function called cxl_port_add()
>> which is used to add the 'struct device' of a new cxl_port to
>> device hierarchy, moving the functions needing the help of above
>> 'goto' into cxl_port_add(), and using a scope-based resource management
>> __free() to drop the open coded put_device() and 'goto' for the error
>> cases.
>>
>> Suggested-by: Dan Williams <dan.j.williams@intel.com>
>> Signed-off-by: Li Ming <ming4.li@intel.com>
>> ---
>>  drivers/cxl/core/port.c | 59 ++++++++++++++++++++++++-----------------
>>  1 file changed, 35 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>> index 53e2593daa95..a886b16b2610 100644
>> --- a/drivers/cxl/core/port.c
>> +++ b/drivers/cxl/core/port.c
>> @@ -828,27 +828,20 @@ static void cxl_debugfs_create_dport_dir(struct cxl_dport *dport)
>>  			    &cxl_einj_inject_fops);
>>  }
>>  
>> -static struct cxl_port *__devm_cxl_add_port(struct device *host,
>> -					    struct device *uport_dev,
>> -					    resource_size_t component_reg_phys,
>> -					    struct cxl_dport *parent_dport)
>> +static int cxl_port_add(struct cxl_port *port,
>> +			resource_size_t component_reg_phys,
>> +			struct cxl_dport *parent_dport)
>>  {
>> -	struct cxl_port *port;
>> -	struct device *dev;
>> +	struct device *dev __free(put_device) = &port->dev;
>>  	int rc;
>>  
>> -	port = cxl_port_alloc(uport_dev, parent_dport);
>> -	if (IS_ERR(port))
>> -		return port;
>> -
>> -	dev = &port->dev;
>> -	if (is_cxl_memdev(uport_dev)) {
>> -		struct cxl_memdev *cxlmd = to_cxl_memdev(uport_dev);
>> +	if (is_cxl_memdev(port->uport_dev)) {
>> +		struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport_dev);
>>  		struct cxl_dev_state *cxlds = cxlmd->cxlds;
>>  
>>  		rc = dev_set_name(dev, "endpoint%d", port->id);
>>  		if (rc)
>> -			goto err;
>> +			return rc;
>>  
>>  		/*
>>  		 * The endpoint driver already enumerated the component and RAS
>> @@ -861,19 +854,41 @@ static struct cxl_port *__devm_cxl_add_port(struct device *host,
>>  	} else if (parent_dport) {
>>  		rc = dev_set_name(dev, "port%d", port->id);
>>  		if (rc)
>> -			goto err;
>> +			return rc;
>>  
>>  		rc = cxl_port_setup_regs(port, component_reg_phys);
>>  		if (rc)
>> -			goto err;
>> -	} else
>> +			return rc;
>> +	} else {
>>  		rc = dev_set_name(dev, "root%d", port->id);
>> -	if (rc)
>> -		goto err;
>> +		if (rc)
>> +			return rc;
>> +	}
>>  
>>  	rc = device_add(dev);
>>  	if (rc)
>> -		goto err;
>> +		return rc;
>> +
>> +	/* Inhibit the cleanup function invoked */
>> +	dev = NULL;
> I'm tempted to say we should use no_free_ptr() here.  But I don't think
> there is any magic we need in there.

Yes, I also think using no_free_ptr() is better. but I have to add a extra local variable to store the return value of no_free_ptr(), otherwise there should be a compilation warning.

@@ -870,7 +870,7 @@ static int cxl_port_add(struct cxl_port *port,
                return rc;

        /* Inhibit the cleanup function invoked */
-       dev = NULL;
+       no_free_ptr(dev);
        return 0;
 }

compilation warning:

drivers/cxl/core/port.c: In function ‘cxl_port_add’:
./include/linux/cleanup.h:76:22: warning: ignoring return value of ‘__must_check_fn’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
   76 |         ((typeof(p)) __must_check_fn(__get_and_null_ptr(p)))
      |                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/cxl/core/port.c:873:9: note: in expansion of macro ‘no_free_ptr’
  873 |         no_free_ptr(dev);
      |         ^~~~~~~~~~~
  LD [M]  drivers/cxl/core/cxl_core.o
  MODPOST Module.symvers


> So.
>
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
>
>> +	return 0;
>> +}
>> +
>> +static struct cxl_port *__devm_cxl_add_port(struct device *host,
>> +					    struct device *uport_dev,
>> +					    resource_size_t component_reg_phys,
>> +					    struct cxl_dport *parent_dport)
>> +{
>> +	struct cxl_port *port;
>> +	int rc;
>> +
>> +	port = cxl_port_alloc(uport_dev, parent_dport);
>> +	if (IS_ERR(port))
>> +		return port;
>> +
>> +	rc = cxl_port_add(port, component_reg_phys, parent_dport);
>> +	if (rc)
>> +		return ERR_PTR(rc);
>>  
>>  	rc = devm_add_action_or_reset(host, unregister_port, port);
>>  	if (rc)
>> @@ -891,10 +906,6 @@ static struct cxl_port *__devm_cxl_add_port(struct device *host,
>>  		port->pci_latency = cxl_pci_get_latency(to_pci_dev(uport_dev));
>>  
>>  	return port;
>> -
>> -err:
>> -	put_device(dev);
>> -	return ERR_PTR(rc);
>>  }
>>  
>>  /**
>> -- 
>> 2.40.1
>>