Messages in this thread Patch in this message | | | Subject | Re: [PATCH v1 08/10] powerpc/pseries/iommu: Add ddw_property_create() and refactor enable_ddw() | From | Alexey Kardashevskiy <> | Date | Mon, 24 Aug 2020 15:07:04 +1000 |
| |
On 18/08/2020 09:40, Leonardo Bras wrote: > Code used to create a ddw property that was previously scattered in > enable_ddw() is now gathered in ddw_property_create(), which deals with > allocation and filling the property, letting it ready for > of_property_add(), which now occurs in sequence. > > This created an opportunity to reorganize the second part of enable_ddw(): > > Without this patch enable_ddw() does, in order: > kzalloc() property & members, create_ddw(), fill ddwprop inside property, > ddw_list_add(), do tce_setrange_multi_pSeriesLP_walk in all memory, > of_add_property(). > > With this patch enable_ddw() does, in order: > create_ddw(), ddw_property_create(), of_add_property(), ddw_list_add(), > do tce_setrange_multi_pSeriesLP_walk in all memory. > > This change requires of_remove_property() in case anything fails after > of_add_property(), but we get to do tce_setrange_multi_pSeriesLP_walk > in all memory, which looks the most expensive operation, only if > everything else succeeds. > > Signed-off-by: Leonardo Bras <leobras.c@gmail.com> > --- > arch/powerpc/platforms/pseries/iommu.c | 97 +++++++++++++++----------- > 1 file changed, 57 insertions(+), 40 deletions(-) > > diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c > index 4031127c9537..3a1ef02ad9d5 100644 > --- a/arch/powerpc/platforms/pseries/iommu.c > +++ b/arch/powerpc/platforms/pseries/iommu.c > @@ -1123,6 +1123,31 @@ static void reset_dma_window(struct pci_dev *dev, struct device_node *par_dn) > ret); > } > > +static int ddw_property_create(struct property **ddw_win, const char *propname,
@propname is always the same, do you really want to pass it every time?
> + u32 liobn, u64 dma_addr, u32 page_shift, u32 window_shift) > +{ > + struct dynamic_dma_window_prop *ddwprop; > + struct property *win64; > + > + *ddw_win = win64 = kzalloc(sizeof(*win64), GFP_KERNEL); > + if (!win64) > + return -ENOMEM; > + > + win64->name = kstrdup(propname, GFP_KERNEL);
Not clear why "win64->name = DIRECT64_PROPNAME" would not work here, the generic OF code does not try kfree() it but it is probably out of scope here.
> + ddwprop = kzalloc(sizeof(*ddwprop), GFP_KERNEL); > + win64->value = ddwprop; > + win64->length = sizeof(*ddwprop); > + if (!win64->name || !win64->value) > + return -ENOMEM;
Up to 2 memory leaks here. I see the cleanup at "out_free_prop:" but still looks fragile. Instead you could simply return win64 as the only error possible here is -ENOMEM and returning NULL is equally good.
> + > + ddwprop->liobn = cpu_to_be32(liobn); > + ddwprop->dma_base = cpu_to_be64(dma_addr); > + ddwprop->tce_shift = cpu_to_be32(page_shift); > + ddwprop->window_shift = cpu_to_be32(window_shift); > + > + return 0; > +} > + > /* > * If the PE supports dynamic dma windows, and there is space for a table > * that can map all pages in a linear offset, then setup such a table, > @@ -1140,12 +1165,11 @@ static bool enable_ddw(struct pci_dev *dev, struct device_node *pdn) > struct ddw_query_response query; > struct ddw_create_response create; > int page_shift; > - u64 max_addr; > + u64 max_addr, win_addr; > struct device_node *dn; > u32 ddw_avail[DDW_APPLICABLE_SIZE]; > struct direct_window *window; > - struct property *win64; > - struct dynamic_dma_window_prop *ddwprop; > + struct property *win64 = NULL; > struct failed_ddw_pdn *fpdn; > bool default_win_removed = false; > > @@ -1244,38 +1268,34 @@ static bool enable_ddw(struct pci_dev *dev, struct device_node *pdn) > goto out_failed; > } > len = order_base_2(max_addr); > - win64 = kzalloc(sizeof(struct property), GFP_KERNEL); > - if (!win64) { > - dev_info(&dev->dev, > - "couldn't allocate property for 64bit dma window\n"); > + > + ret = create_ddw(dev, ddw_avail, &create, page_shift, len); > + if (ret != 0)
It is usually just "if (ret)"
> goto out_failed; > - } > - win64->name = kstrdup(DIRECT64_PROPNAME, GFP_KERNEL); > - win64->value = ddwprop = kmalloc(sizeof(*ddwprop), GFP_KERNEL); > - win64->length = sizeof(*ddwprop); > - if (!win64->name || !win64->value) { > + > + dev_dbg(&dev->dev, "created tce table LIOBN 0x%x for %pOF\n", > + create.liobn, dn); > + > + win_addr = ((u64)create.addr_hi << 32) | create.addr_lo; > + ret = ddw_property_create(&win64, DIRECT64_PROPNAME, create.liobn, win_addr, > + page_shift, len); > + if (ret) { > dev_info(&dev->dev, > - "couldn't allocate property name and value\n"); > + "couldn't allocate property, property name, or value\n"); > goto out_free_prop; > } > > - ret = create_ddw(dev, ddw_avail, &create, page_shift, len); > - if (ret != 0) > + ret = of_add_property(pdn, win64); > + if (ret) { > + dev_err(&dev->dev, "unable to add dma window property for %pOF: %d", > + pdn, ret); > goto out_free_prop; > - > - ddwprop->liobn = cpu_to_be32(create.liobn); > - ddwprop->dma_base = cpu_to_be64(((u64)create.addr_hi << 32) | > - create.addr_lo); > - ddwprop->tce_shift = cpu_to_be32(page_shift); > - ddwprop->window_shift = cpu_to_be32(len); > - > - dev_dbg(&dev->dev, "created tce table LIOBN 0x%x for %pOF\n", > - create.liobn, dn); > + } > > /* Add new window to existing DDW list */ > - window = ddw_list_add(pdn, ddwprop); > + window = ddw_list_add(pdn, win64->value); > if (!window) > - goto out_clear_window; > + goto out_prop_del; > > ret = walk_system_ram_range(0, memblock_end_of_DRAM() >> PAGE_SHIFT, > win64->value, tce_setrange_multi_pSeriesLP_walk); > @@ -1285,14 +1305,7 @@ static bool enable_ddw(struct pci_dev *dev, struct device_node *pdn) > goto out_free_window; > } > > - ret = of_add_property(pdn, win64); > - if (ret) { > - dev_err(&dev->dev, "unable to add dma window property for %pOF: %d", > - pdn, ret); > - goto out_free_window; > - } > - > - dev->dev.archdata.dma_offset = be64_to_cpu(ddwprop->dma_base); > + dev->dev.archdata.dma_offset = win_addr; > goto out_unlock; > > out_free_window: > @@ -1302,14 +1315,18 @@ static bool enable_ddw(struct pci_dev *dev, struct device_node *pdn) > > kfree(window); > > -out_clear_window: > - remove_ddw(pdn, true); > +out_prop_del: > + of_remove_property(pdn, win64); > > out_free_prop: > - kfree(win64->name); > - kfree(win64->value); > - kfree(win64); > - win64 = NULL; > + if (win64) { > + kfree(win64->name); > + kfree(win64->value); > + kfree(win64); > + win64 = NULL; > + } > + > + remove_ddw(pdn, true); > > out_failed: > if (default_win_removed) >
-- Alexey
| |