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 <[email protected]> > --- > 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
