Re: [PATCH v1 08/10] powerpc/pseries/iommu: Add ddw_property_create() and refactor enable_ddw()

[Leonardo Bras]

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@xxxxxxxxx>
> > ---
> >  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)
> >