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

Alexey Kardashevskiy
2020-08-24T03:47:02Z

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