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

[Leonardo Bras <leobras.c@gmail.com>]

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!