Re: [PATCH v2 04/10] cxl: Clean up adapter MMIO unmap path.

[Cyril Bur <cyrilbur@gmail.com>]

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

>  - MMIO pointer unmapping is guarded by a null pointer check.
>    However, iounmap doesn't null the pointer, just invalidate it.
>    Therefore, explicitly null the pointer after unmapping.
> 
>  - afu_desc_mmio also needs to be unmapped.
> 
>  - PCI regions are allocated in cxl_map_adapter_regs.
>    Therefore they should be released in unmap, not elsewhere.
> 

You've changed the order in which cxl_remove_adapter() does its work, which,
I'm sure you've considered and it's fine, best to check.

Acked-by: Cyril Bur <cyrilbur@gmail.com>

> Signed-off-by: Daniel Axtens <dja@axtens.net>
> ---
>  drivers/misc/cxl/pci.c | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c
> index 1849c1785b49..adcf938f2fdb 100644
> --- a/drivers/misc/cxl/pci.c
> +++ b/drivers/misc/cxl/pci.c
> @@ -539,10 +539,18 @@ err:
>  
>  static void cxl_unmap_slice_regs(struct cxl_afu *afu)
>  {
> -	if (afu->p2n_mmio)
> +	if (afu->p2n_mmio) {
>  		iounmap(afu->p2n_mmio);
> -	if (afu->p1n_mmio)
> +		afu->p2n_mmio = NULL;
> +	}
> +	if (afu->p1n_mmio) {
>  		iounmap(afu->p1n_mmio);
> +		afu->p1n_mmio = NULL;
> +	}
> +	if (afu->afu_desc_mmio) {
> +		iounmap(afu->afu_desc_mmio);
> +		afu->afu_desc_mmio = NULL;
> +	}
>  }
>  
>  static void cxl_release_afu(struct device *dev)
> @@ -920,10 +928,16 @@ err1:
>  
>  static void cxl_unmap_adapter_regs(struct cxl *adapter)
>  {
> -	if (adapter->p1_mmio)
> +	if (adapter->p1_mmio) {
>  		iounmap(adapter->p1_mmio);
> -	if (adapter->p2_mmio)
> +		adapter->p1_mmio = NULL;
> +		pci_release_region(to_pci_dev(adapter->dev.parent), 2);
> +	}
> +	if (adapter->p2_mmio) {
>  		iounmap(adapter->p2_mmio);
> +		adapter->p2_mmio = NULL;
> +		pci_release_region(to_pci_dev(adapter->dev.parent), 0);
> +	}
>  }
>  
>  static int cxl_read_vsec(struct cxl *adapter, struct pci_dev *dev)
> @@ -1132,8 +1146,6 @@ static void cxl_remove_adapter(struct cxl *adapter)
>  
>  	device_unregister(&adapter->dev);
>  
> -	pci_release_region(pdev, 0);
> -	pci_release_region(pdev, 2);
>  	pci_disable_device(pdev);
>  }
>