| Message ID | 20200817234033.442511-9-leobras.c@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | DDW indirect mapping | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch powerpc/merge (97a94d178e5876ad49482c42b13b7296cd6803de) |
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch powerpc/next (9123e3a74ec7b934a4a099e98af6a61c2f80bbf5) |
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch linus/master (9123e3a74ec7b934a4a099e98af6a61c2f80bbf5) |
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch powerpc/fixes (388692e943a58f28aac0fe83e75f5994da9ff8a1) |
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch linux-next (0f1fa5848ab32d269a2030caac618bd6a99ab3f3) |
| snowpatch_ozlabs/apply_patch | fail | Failed to apply to any branch |
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) >
On Mon, 2020-08-24 at 15:07 +1000, Alexey Kardashevskiy wrote: > > 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? I think it reads better, like "create a ddw property with this name". Also, it makes possible to create ddw properties with other names, in case we decide to create properties with different names depending on the window created. Also, it's probably optimized / inlined at this point. Is it ok doing it like this? > > > + 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. Yeah, I had that question too. Previous code was like that, and I as trying not to mess too much on how it's done. > > + 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. I agree. It's better if this function have it's own cleaning routine. It will be fixed for next version. > > > > + > > + 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)" It was previously like that, and all query_ddw() checks return value this way. Should I update them all or just this one? Thanks! > > > > 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) > >
On 29/08/2020 01:25, Leonardo Bras wrote: > On Mon, 2020-08-24 at 15:07 +1000, Alexey Kardashevskiy wrote: >> >> 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? > > I think it reads better, like "create a ddw property with this name". This reads as "there are at least two ddw properties". > Also, it makes possible to create ddw properties with other names, in > case we decide to create properties with different names depending on > the window created. It is one window at any given moment, why call it different names... I get the part that it is not always "direct" anymore but still... > Also, it's probably optimized / inlined at this point. > Is it ok doing it like this? > >> >>> + 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. > > Yeah, I had that question too. > Previous code was like that, and I as trying not to mess too much on > how it's done. > >>> + 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. > > I agree. It's better if this function have it's own cleaning routine. > It will be fixed for next version. > >> >> >>> + >>> + 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)" > > It was previously like that, and all query_ddw() checks return value > this way. ah I see. > Should I update them all or just this one? Pick one variant and make sure all new lines use just that. In this patch you add both variants. Thanks, > > Thanks! > >> >> >>> 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) >>> >
On Mon, 2020-08-31 at 14:34 +1000, Alexey Kardashevskiy wrote: > > On 29/08/2020 01:25, Leonardo Bras wrote: > > On Mon, 2020-08-24 at 15:07 +1000, Alexey Kardashevskiy wrote: > > > 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? > > > > I think it reads better, like "create a ddw property with this name". > > This reads as "there are at least two ddw properties". > > > Also, it makes possible to create ddw properties with other names, in > > case we decide to create properties with different names depending on > > the window created. > > It is one window at any given moment, why call it different names... I > get the part that it is not always "direct" anymore but still... > It seems the case as one of the options you suggested on patch [09/10] >> I suspect it breaks kexec (from older kernel to this one) so you >> either need to check for both DT names or just keep the old one. > > > Also, it's probably optimized / inlined at this point. > > Is it ok doing it like this? > > > > > > + 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. > > > > Yeah, I had that question too. > > Previous code was like that, and I as trying not to mess too much on > > how it's done. > > > > > > + 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. > > > > I agree. It's better if this function have it's own cleaning routine. > > It will be fixed for next version. > > > > > > > > > + > > > > + 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)" > > > > It was previously like that, and all query_ddw() checks return value > > this way. > > ah I see. > > > Should I update them all or just this one? > > Pick one variant and make sure all new lines use just that. In this > patch you add both variants. Thanks, Ok, I will do that from now on. Thanks!
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, + 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); + ddwprop = kzalloc(sizeof(*ddwprop), GFP_KERNEL); + win64->value = ddwprop; + win64->length = sizeof(*ddwprop); + if (!win64->name || !win64->value) + return -ENOMEM; + + 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) 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)
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(-)