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 <leobra...@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

Reply via email to