Re: [PATCH v1 06/10] powerpc/pseries/iommu: Add ddw_list_add() helper

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 18/08/2020 09:40, Leonardo Bras wrote:
> There are two functions adding DDW to the direct_window_list in a
> similar way, so create a ddw_list_add() to avoid duplicity and
> simplify those functions.
> 
> Also, on enable_ddw(), add list_del() on out_free_window to allow
> removing the window from list if any error occurs.
> 
> Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
> ---
>  arch/powerpc/platforms/pseries/iommu.c | 42 ++++++++++++++++----------
>  1 file changed, 26 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
> index 39617ce0ec83..fcdefcc0f365 100644
> --- a/arch/powerpc/platforms/pseries/iommu.c
> +++ b/arch/powerpc/platforms/pseries/iommu.c
> @@ -872,6 +872,24 @@ static u64 find_existing_ddw(struct device_node *pdn)
>  	return dma_addr;
>  }
>  
> +static struct direct_window *ddw_list_add(struct device_node *pdn,
> +					  const struct dynamic_dma_window_prop *dma64)
> +{
> +	struct direct_window *window;
> +
> +	window = kzalloc(sizeof(*window), GFP_KERNEL);
> +	if (!window)
> +		return NULL;
> +
> +	window->device = pdn;
> +	window->prop = dma64;
> +	spin_lock(&direct_window_list_lock);
> +	list_add(&window->list, &direct_window_list);
> +	spin_unlock(&direct_window_list_lock);
> +
> +	return window;
> +}
> +
>  static int find_existing_ddw_windows(void)
>  {
>  	int len;
> @@ -887,18 +905,11 @@ static int find_existing_ddw_windows(void)
>  		if (!direct64)
>  			continue;
>  
> -		window = kzalloc(sizeof(*window), GFP_KERNEL);
> -		if (!window || len < sizeof(struct dynamic_dma_window_prop)) {
> +		window = ddw_list_add(pdn, direct64);
> +		if (!window || len < sizeof(*direct64)) {


Since you are touching this code, it looks like the "len <
sizeof(*direct64)" part should go above to "if (!direct64)".



>  			kfree(window);
>  			remove_ddw(pdn, true);
> -			continue;
>  		}
> -
> -		window->device = pdn;
> -		window->prop = direct64;
> -		spin_lock(&direct_window_list_lock);
> -		list_add(&window->list, &direct_window_list);
> -		spin_unlock(&direct_window_list_lock);
>  	}
>  
>  	return 0;
> @@ -1261,7 +1272,8 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn)
>  	dev_dbg(&dev->dev, "created tce table LIOBN 0x%x for %pOF\n",
>  		  create.liobn, dn);
>  
> -	window = kzalloc(sizeof(*window), GFP_KERNEL);
> +	/* Add new window to existing DDW list */

The comment seems to duplicate what the ddw_list_add name already suggests.


> +	window = ddw_list_add(pdn, ddwprop);
>  	if (!window)
>  		goto out_clear_window;
>  
> @@ -1280,16 +1292,14 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn)
>  		goto out_free_window;
>  	}
>  
> -	window->device = pdn;
> -	window->prop = ddwprop;
> -	spin_lock(&direct_window_list_lock);
> -	list_add(&window->list, &direct_window_list);
> -	spin_unlock(&direct_window_list_lock);

I'd leave these 3 lines here and in find_existing_ddw_windows() (which
would make  ddw_list_add -> ddw_prop_alloc). In general you want to have
less stuff to do on the failure path. kmalloc may fail and needs kfree
but you can safely delay list_add (which cannot fail) and avoid having
the lock help twice in the same function (one of them is hidden inside
ddw_list_add).

Not sure if this change is really needed after all. Thanks,

> -
>  	dma_addr = be64_to_cpu(ddwprop->dma_base);
>  	goto out_unlock;
>  
>  out_free_window:
> +	spin_lock(&direct_window_list_lock);
> +	list_del(&window->list);
> +	spin_unlock(&direct_window_list_lock);
> +
>  	kfree(window);
>  
>  out_clear_window:
> 

-- 
Alexey