Message-ID: <887bf30e-ae9e-b0cb-0388-dc555692ff0a@ozlabs.ru>
Date:   Tue, 23 Jun 2020 11:12:22 +1000
From:   Alexey Kardashevskiy <aik@...abs.ru>
To:     Leonardo Bras <leobras.c@...il.com>,
        Michael Ellerman <mpe@...erman.id.au>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        Thiago Jung Bauermann <bauerman@...ux.ibm.com>,
        Ram Pai <linuxram@...ibm.com>
Cc:     linuxppc-dev@...ts.ozlabs.org, linux-kernel@...r.kernel.org,
        Oliver <oohall@...il.com>
Subject: Re: [PATCH 3/4] powerpc/pseries/iommu: Move window-removing part of
 remove_ddw into remove_dma_window



On 23/06/2020 04:59, Leonardo Bras wrote:
> Hello Alexey, thanks for the feedback!
> 
> On Mon, 2020-06-22 at 20:02 +1000, Alexey Kardashevskiy wrote:
>>
>> On 19/06/2020 15:06, Leonardo Bras wrote:
>>> Move the window-removing part of remove_ddw into a new function
>>> (remove_dma_window), so it can be used to remove other DMA windows.
>>>
>>> It's useful for removing DMA windows that don't create DIRECT64_PROPNAME
>>> property, like the default DMA window from the device, which uses
>>> "ibm,dma-window".
>>>
>>> Signed-off-by: Leonardo Bras <leobras.c@...il.com>
>>> ---
>>>  arch/powerpc/platforms/pseries/iommu.c | 53 +++++++++++++++-----------
>>>  1 file changed, 31 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
>>> index 5e1fbc176a37..de633f6ae093 100644
>>> --- a/arch/powerpc/platforms/pseries/iommu.c
>>> +++ b/arch/powerpc/platforms/pseries/iommu.c
>>> @@ -767,25 +767,14 @@ static int __init disable_ddw_setup(char *str)
>>>  
>>>  early_param("disable_ddw", disable_ddw_setup);
>>>  
>>> -static void remove_ddw(struct device_node *np, bool remove_prop)
>>> +static void remove_dma_window(struct device_node *pdn, u32 *ddw_avail,
>>
>> You do not need the entire ddw_avail here, pass just the token you need.
> 
> Well, I just emulated the behavior of create_ddw() and query_ddw() as
> both just pass the array instead of the token, even though they only
> use a single token. 

True, there is a pattern.

> I think it's to make the rest of the code independent of the design of
> the "ibm,ddw-applicable" array, and if it changes, only local changes
> on the functions will be needed.

The helper removes a window, if you are going to call other operations
in remove_dma_window(), then you'll have to change its name ;)


>> Also, despite this particular file, the "pdn" name is usually used for
>> struct pci_dn (not device_node), let's keep it that way.
> 
> Sure, I got confused for some time about this, as we have:
> static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn).
> but on *_ddw() we have "struct pci_dn *pdn".

True again, not the cleanest style here.


> I will also add a patch that renames those 'struct device_node *pdn' to
> something like 'struct device_node *parent_dn'.

I would not go that far, we (well, Oliver) are getting rid of many
occurrences of pci_dn and Oliver may have a stronger opinion here.


> 
>>> +			      struct property *win)
>>>  {
>>>  	struct dynamic_dma_window_prop *dwp;
>>> -	struct property *win64;
>>> -	u32 ddw_avail[3];
>>>  	u64 liobn;
>>> -	int ret = 0;
>>> -
>>> -	ret = of_property_read_u32_array(np, "ibm,ddw-applicable",
>>> -					 &ddw_avail[0], 3);
>>> -
>>> -	win64 = of_find_property(np, DIRECT64_PROPNAME, NULL);
>>> -	if (!win64)
>>> -		return;
>>> -
>>> -	if (ret || win64->length < sizeof(*dwp))
>>> -		goto delprop;
>>> +	int ret;
>>>  
>>> -	dwp = win64->value;
>>> +	dwp = win->value;
>>>  	liobn = (u64)be32_to_cpu(dwp->liobn);
>>>  
>>>  	/* clear the whole window, note the arg is in kernel pages */
>>> @@ -793,24 +782,44 @@ static void remove_ddw(struct device_node *np, bool remove_prop)
>>>  		1ULL << (be32_to_cpu(dwp->window_shift) - PAGE_SHIFT), dwp);
>>>  	if (ret)
>>>  		pr_warn("%pOF failed to clear tces in window.\n",
>>> -			np);
>>> +			pdn);
>>>  	else
>>>  		pr_debug("%pOF successfully cleared tces in window.\n",
>>> -			 np);
>>> +			 pdn);
>>>  
>>>  	ret = rtas_call(ddw_avail[2], 1, 1, NULL, liobn);
>>>  	if (ret)
>>>  		pr_warn("%pOF: failed to remove direct window: rtas returned "
>>>  			"%d to ibm,remove-pe-dma-window(%x) %llx\n",
>>> -			np, ret, ddw_avail[2], liobn);
>>> +			pdn, ret, ddw_avail[2], liobn);
>>>  	else
>>>  		pr_debug("%pOF: successfully removed direct window: rtas returned "
>>>  			"%d to ibm,remove-pe-dma-window(%x) %llx\n",
>>> -			np, ret, ddw_avail[2], liobn);
>>> +			pdn, ret, ddw_avail[2], liobn);
>>> +}
>>> +
>>> +static void remove_ddw(struct device_node *np, bool remove_prop)
>>> +{
>>> +	struct property *win;
>>> +	u32 ddw_avail[3];
>>> +	int ret = 0;
>>> +
>>> +	ret = of_property_read_u32_array(np, "ibm,ddw-applicable",
>>> +					 &ddw_avail[0], 3);
>>> +	if (ret)
>>> +		return;
>>> +
>>> +	win = of_find_property(np, DIRECT64_PROPNAME, NULL);
>>> +	if (!win)
>>> +		return;
>>> +
>>> +	if (win->length >= sizeof(struct dynamic_dma_window_prop))
>>
>> Any good reason not to make it "=="? Is there something optional or we
>> expect extension (which may not grow from the end but may add cells in
>> between). Thanks,
> 
> Well, it comes from the old behavior of remove_ddw():
> -	if (ret || win64->length < sizeof(*dwp))
> -		goto delprop;
> As I reversed the logic from 'if (test) go out' to 'if (!test) do
> stuff', I also reversed (a < b) to !(a < b) => (a >= b).
> 
> I have no problem changing that to '==', but it will produce a
> different behavior than before. 

I missed than, never mind then.


> 
>>
>>
>>> +		remove_dma_window(np, ddw_avail, win);
>>> +
>>> +	if (!remove_prop)
>>> +		return;
>>>  
>>> -delprop:
>>> -	if (remove_prop)
>>> -		ret = of_remove_property(np, win64);
>>> +	ret = of_remove_property(np, win);
>>>  	if (ret)
>>>  		pr_warn("%pOF: failed to remove direct window property: %d\n",
>>>  			np, ret);
>>>
> 
> Best regards,
> Leonardo
> 

-- 
Alexey

Powered by blists - more mailing lists