[v2,05/10] cxl: Refactor adaptor init/teardown

Some aspects of initialisation are done only once in the lifetime of
an adapter: for example, allocating memory for the adapter,
allocating the adapter number, or setting up sysfs/debugfs files.

However, we may want to be able to do some parts of the
initialisation multiple times: for example, in error recovery we
want to be able to tear down and then re-map IO memory and IRQs.

Therefore, refactor CXL init/teardown as follows.

 - Keep the overarching functions 'cxl_init_adapter' and its pair,
   'cxl_remove_adapter'.

 - Move all 'once only' allocation/freeing steps to the existing
   'cxl_alloc_adapter' function, and its pair 'cxl_release_adapter'
   (This involves moving allocation of the adapter number out of
   cxl_init_adapter.)

 - Create two new functions: 'cxl_configure_adapter', and its pair
   'cxl_deconfigure_adapter'. These two functions 'wire up' the
   hardware --- they (de)configure resources that do not need to
   last the entire lifetime of the adapter

Signed-off-by: Daniel Axtens <dja@axtens.net>
---
 drivers/misc/cxl/pci.c | 138 ++++++++++++++++++++++++++++++-------------------
 1 file changed, 85 insertions(+), 53 deletions(-)

On Tue, 28 Jul 2015 15:28:38 +1000
Daniel Axtens <dja@axtens.net> wrote:

> Some aspects of initialisation are done only once in the lifetime of
> an adapter: for example, allocating memory for the adapter,
> allocating the adapter number, or setting up sysfs/debugfs files.
> 
> However, we may want to be able to do some parts of the
> initialisation multiple times: for example, in error recovery we
> want to be able to tear down and then re-map IO memory and IRQs.
> 
> Therefore, refactor CXL init/teardown as follows.
> 
>  - Keep the overarching functions 'cxl_init_adapter' and its pair,
>    'cxl_remove_adapter'.
> 
>  - Move all 'once only' allocation/freeing steps to the existing
>    'cxl_alloc_adapter' function, and its pair 'cxl_release_adapter'
>    (This involves moving allocation of the adapter number out of
>    cxl_init_adapter.)
> 
>  - Create two new functions: 'cxl_configure_adapter', and its pair
>    'cxl_deconfigure_adapter'. These two functions 'wire up' the
>    hardware --- they (de)configure resources that do not need to
>    last the entire lifetime of the adapter
> 

You have a dilema with the use of ugly if (rc = foo()). I don't like it but the
file is littered with it.

Looks like the majority of uses in this file the conditional block is only
one line then it makes sense (or at least in terms of numbers of lines... fair
enough), however, if you have a conditional block spanning multiple lines, I
don't like.

> Signed-off-by: Daniel Axtens <dja@axtens.net>
> ---
>  drivers/misc/cxl/pci.c | 138 ++++++++++++++++++++++++++++++-------------------
>  1 file changed, 85 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c
> index adcf938f2fdb..7f47e2221524 100644
> --- a/drivers/misc/cxl/pci.c
> +++ b/drivers/misc/cxl/pci.c
> @@ -966,7 +966,6 @@ static int cxl_read_vsec(struct cxl *adapter, struct pci_dev *dev)
>  	CXL_READ_VSEC_BASE_IMAGE(dev, vsec, &adapter->base_image);
>  	CXL_READ_VSEC_IMAGE_STATE(dev, vsec, &image_state);
>  	adapter->user_image_loaded = !!(image_state & CXL_VSEC_USER_IMAGE_LOADED);
> -	adapter->perst_loads_image = true;
>  	adapter->perst_select_user = !!(image_state & CXL_VSEC_USER_IMAGE_LOADED);
>  
>  	CXL_READ_VSEC_NAFUS(dev, vsec, &adapter->slices);
> @@ -1026,22 +1025,34 @@ static void cxl_release_adapter(struct device *dev)
>  
>  	pr_devel("cxl_release_adapter\n");
>  
> +	cxl_remove_adapter_nr(adapter);
> +
>  	kfree(adapter);
>  }
>  
> -static struct cxl *cxl_alloc_adapter(struct pci_dev *dev)
> +static struct cxl *cxl_alloc_adapter(void)
>  {
>  	struct cxl *adapter;
> +	int rc;
>  
>  	if (!(adapter = kzalloc(sizeof(struct cxl), GFP_KERNEL)))
>  		return NULL;
>  
> -	adapter->dev.parent = &dev->dev;
> -	adapter->dev.release = cxl_release_adapter;
> -	pci_set_drvdata(dev, adapter);
>  	spin_lock_init(&adapter->afu_list_lock);
>  
> +	if ((rc = cxl_alloc_adapter_nr(adapter)))

Humf

> +		goto err1;
> +
> +	if ((rc = dev_set_name(&adapter->dev, "card%i", adapter->adapter_num)))

Humf
> +		goto err2;
> +
>  	return adapter;
> +
> +err2:
> +	cxl_remove_adapter_nr(adapter);
> +err1:
> +	kfree(adapter);
> +	return NULL;
>  }
>  
>  static int sanitise_adapter_regs(struct cxl *adapter)
> @@ -1050,57 +1061,94 @@ static int sanitise_adapter_regs(struct cxl *adapter)
>  	return cxl_tlb_slb_invalidate(adapter);
>  }
>  
> -static struct cxl *cxl_init_adapter(struct pci_dev *dev)
> +/* This should contain *only* operations that can safely be done in
> + * both creation and recovery.
> + */
> +static int cxl_configure_adapter(struct cxl *adapter, struct pci_dev *dev)
>  {
> -	struct cxl *adapter;
> -	bool free = true;
>  	int rc;
>  
> +	adapter->dev.parent = &dev->dev;
> +	adapter->dev.release = cxl_release_adapter;
> +	pci_set_drvdata(dev, adapter);
>  
> -	if (!(adapter = cxl_alloc_adapter(dev)))
> -		return ERR_PTR(-ENOMEM);
> +	if ((rc = pci_enable_device(dev))) {

Backets...

> +		dev_err(&dev->dev, "pci_enable_device failed: %i\n", rc);
> +		return rc;
> +	}
>  
>  	if ((rc = cxl_read_vsec(adapter, dev)))
> -		goto err1;
> +		return rc;
>  
>  	if ((rc = cxl_vsec_looks_ok(adapter, dev)))
> -		goto err1;
> +	        return rc;
>  
>  	if ((rc = setup_cxl_bars(dev)))
> -		goto err1;
> +		return rc;
>  
>  	if ((rc = switch_card_to_cxl(dev)))
> -		goto err1;
> -
> -	if ((rc = cxl_alloc_adapter_nr(adapter)))
> -		goto err1;
> -
> -	if ((rc = dev_set_name(&adapter->dev, "card%i", adapter->adapter_num)))
> -		goto err2;
> +		return rc;
>  
>  	if ((rc = cxl_update_image_control(adapter)))
> -		goto err2;
> +		return rc;
>  
>  	if ((rc = cxl_map_adapter_regs(adapter, dev)))
> -		goto err2;
> +		return rc;
>  
>  	if ((rc = sanitise_adapter_regs(adapter)))
> -		goto err2;
> +		goto err;
>  
>  	if ((rc = init_implementation_adapter_regs(adapter, dev)))
> -		goto err3;
> +		goto err;
>  
>  	if ((rc = pnv_phb_to_cxl_mode(dev, OPAL_PHB_CAPI_MODE_CAPI)))
> -		goto err3;
> +		goto err;
>  
>  	/* If recovery happened, the last step is to turn on snooping.
>  	 * In the non-recovery case this has no effect */
> -	if ((rc = pnv_phb_to_cxl_mode(dev, OPAL_PHB_CAPI_MODE_SNOOP_ON))) {
> -		goto err3;
> -	}
> +	if ((rc = pnv_phb_to_cxl_mode(dev, OPAL_PHB_CAPI_MODE_SNOOP_ON)))
> +		goto err;
>  
>  	if ((rc = cxl_register_psl_err_irq(adapter)))
> -		goto err3;
> +		goto err;
> +
> +	return 0;
> +
> +err:
> +	cxl_unmap_adapter_regs(adapter);
> +	return rc;
> +
> +}
> +
> +static void cxl_deconfigure_adapter(struct cxl *adapter)
> +{
> +	struct pci_dev *pdev = to_pci_dev(adapter->dev.parent);
> +
> +	cxl_release_psl_err_irq(adapter);
> +	cxl_unmap_adapter_regs(adapter);
> +
> +	pci_disable_device(pdev);
> +}
> +
> +static struct cxl *cxl_init_adapter(struct pci_dev *dev)
> +{
> +	struct cxl *adapter;
> +	int rc;
> +
> +	adapter = cxl_alloc_adapter();
> +	if (!adapter)
> +		return ERR_PTR(-ENOMEM);
> +
> +	/* Set defaults for parameters which need to persist over
> +	 * configure/reconfigure
> +	 */
> +	adapter->perst_loads_image = true;
> +
> +	if ((rc = cxl_configure_adapter(adapter, dev))) {

Brackets

> +		pci_disable_device(dev);
> +		cxl_release_adapter(&adapter->dev);
> +		return ERR_PTR(rc);
> +	}
>  
>  	/* Don't care if this one fails: */
>  	cxl_debugfs_adapter_add(adapter);
> @@ -1118,35 +1166,25 @@ static struct cxl *cxl_init_adapter(struct pci_dev *dev)
>  	return adapter;
>  
>  err_put1:
> -	device_unregister(&adapter->dev);
> -	free = false;
> +	/* This should mirror cxl_remove_adapter, except without the
> +	 * sysfs parts
> +	 */
>  	cxl_debugfs_adapter_remove(adapter);
> -	cxl_release_psl_err_irq(adapter);
> -err3:
> -	cxl_unmap_adapter_regs(adapter);
> -err2:
> -	cxl_remove_adapter_nr(adapter);
> -err1:
> -	if (free)
> -		kfree(adapter);
> +	cxl_deconfigure_adapter(adapter);
> +	device_unregister(&adapter->dev);
>  	return ERR_PTR(rc);
>  }
>  
>  static void cxl_remove_adapter(struct cxl *adapter)
>  {
> -	struct pci_dev *pdev = to_pci_dev(adapter->dev.parent);
> -
> -	pr_devel("cxl_release_adapter\n");
> +	pr_devel("cxl_remove_adapter\n");
>  
>  	cxl_sysfs_adapter_remove(adapter);
>  	cxl_debugfs_adapter_remove(adapter);
> -	cxl_release_psl_err_irq(adapter);
> -	cxl_unmap_adapter_regs(adapter);
> -	cxl_remove_adapter_nr(adapter);
>  
> -	device_unregister(&adapter->dev);
> +	cxl_deconfigure_adapter(adapter);
>  
> -	pci_disable_device(pdev);
> +	device_unregister(&adapter->dev);
>  }
>  
>  static int cxl_probe(struct pci_dev *dev, const struct pci_device_id *id)
> @@ -1160,15 +1198,9 @@ static int cxl_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  	if (cxl_verbose)
>  		dump_cxl_config_space(dev);
>  
> -	if ((rc = pci_enable_device(dev))) {
> -		dev_err(&dev->dev, "pci_enable_device failed: %i\n", rc);
> -		return rc;
> -	}
> -
>  	adapter = cxl_init_adapter(dev);
>  	if (IS_ERR(adapter)) {
>  		dev_err(&dev->dev, "cxl_init_adapter failed: %li\n", PTR_ERR(adapter));
> -		pci_disable_device(dev);
>  		return PTR_ERR(adapter);
>  	}
>

> Looks like the majority of uses in this file the conditional block is only
> one line then it makes sense (or at least in terms of numbers of lines... fair
> enough), however, if you have a conditional block spanning multiple lines, I
> don't like.
> 
Much as this is a massive nit pick, I have split out the two
conditionals that spanned multiple lines.

From: Cyril Bur

> Sent: 11 August 2015 07:01

...
> You have a dilema with the use of ugly if (rc = foo()). I don't like it but the

> file is littered with it.

> 

> Looks like the majority of uses in this file the conditional block is only

> one line then it makes sense (or at least in terms of numbers of lines... fair

> enough), however, if you have a conditional block spanning multiple lines, I

> don't like.

...
> >  	kfree(adapter);

> >  }

> >

> > -static struct cxl *cxl_alloc_adapter(struct pci_dev *dev)

> > +static struct cxl *cxl_alloc_adapter(void)

> >  {

> >  	struct cxl *adapter;

> > +	int rc;

> >

> >  	if (!(adapter = kzalloc(sizeof(struct cxl), GFP_KERNEL)))

> >  		return NULL;

> >

> > -	adapter->dev.parent = &dev->dev;

> > -	adapter->dev.release = cxl_release_adapter;

> > -	pci_set_drvdata(dev, adapter);

> >  	spin_lock_init(&adapter->afu_list_lock);

> >

> > +	if ((rc = cxl_alloc_adapter_nr(adapter)))

> 

> Humf

> 

> > +		goto err1;

> > +

> > +	if ((rc = dev_set_name(&adapter->dev, "card%i", adapter->adapter_num)))

> 

> Humf

> > +		goto err2;

> > +

> >  	return adapter;

> > +

> > +err2:

> > +	cxl_remove_adapter_nr(adapter);

> > +err1:

> > +	kfree(adapter);

> > +	return NULL;

> >  }

...

The function above doesn't even use the 'rc' value.

	David

> The function above doesn't even use the 'rc' value.

Darn, you're right.

I'll fix that in a new version.