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

[Ira Weiny <ira.weiny@intel.com>]

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.

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
>