Message-ID: <18fd94d2-4365-16d1-7c85-af07d5c9a0f3@ozlabs.ru>
Date:   Tue, 14 Jul 2020 14:52:10 +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
Subject: Re: [PATCH v3 4/6] powerpc/pseries/iommu: Remove default DMA window
 before creating DDW



On 14/07/2020 12:40, Leonardo Bras wrote:
> Thank you for this feedback Alexey!
> 
> On Mon, 2020-07-13 at 17:33 +1000, Alexey Kardashevskiy wrote:
>> [...]
>>> -	int len, ret;
>>> +	int len, ret, reset_win_ext;
>>
>> Make it "reset_token".
> 
> Oh, it's not a token here, it just checks if the reset_win extension
> exists. The token would be returned in *value, but since we did not
> need it here, it's not copied.

ah right, so it is a bool actually.


> 
>>> [...]
>>> -out_failed:
>>> +out_restore_defwin:
>>> +	if (default_win && reset_win_ext == 0)
>>
>> reset_win_ext potentially may be uninitialized here. Yeah I know it is
>> tied to default_win but still.
> 
> I can't see it being used uninitialized here, as you said it's tied to
> default_win. 

Where it is declared - it is not initialized so in theory it can skip
"if (query.windows_available == 0)".


> Could you please tell me how it can be used uninitialized here, or what
> is bad by doing this way?
> 
>> After looking at this function for a few minutes, it could use some
>> refactoring (way too many gotos)  such as:
> 
> Yes, I agree.
> 
>> 1. move (query.page_size & xx) checks before "if
>> (query.windows_available == 0)"
> 
> Moving 'page_size selection' above 'checking windows available' will
> need us to duplicate the 'page_size selection' after the new query,
> inside the if.

page_size selection is not going to change, why?


> I mean, as query will be done again, it will need to get the (new) page
> size.
> 
>> 2. move "win64 = kzalloc(sizeof(struct property), GFP_KERNEL)" before
>> "if (query.windows_available == 0)"
> 
>> 3. call "reset_dma_window(dev, pdn)" inside the "if
>> (query.windows_available == 0)" branch.
> 
>> Then you can drop all "goto out_restore_defwin" and move default_win and
>> reset_win_ext inside "if (query.windows_available == 0)".
> 
> I did all changes suggested locally and did some analysis in the
> result:
> 
> I did not see a way to put default_win and reset_win_ext inside 
> "if (query.windows_available == 0)", because if we still need a way to
> know if the default window was removed, and if so, restore in case
> anything ever fails ahead (like creating the node property). 

Ah, I missed that new out_restore_defwin label is between other exit
labels. Sorry :-/


> But from that analysis I noted it's possible to remove all the new
> "goto out_restore_defwin", if we do default_win = NULL if
> ddw_read_ext() fails. 
> 
> So testing only default_win should always be enough to say if the
> default window was deleted, and reset_win_ext could be moved inside "if
> (query.windows_available == 0)".
> Also, it would avoid reset_win_ext being 'used uninitialized' and
> "out_restore_defwin:" would not be needed.
> 
> Against the current patch, we would have something like this:
> 
> #####
> 
>  static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn)
>  {
> -       int len, ret, reset_win_ext;
> +       int len, ret;
>         struct ddw_query_response query;
>         struct ddw_create_response create;
>         int page_shift;
> @@ -1173,25 +1173,28 @@ static u64 enable_ddw(struct pci_dev *dev,
> struct device_node *pdn)
>          * for extensions presence.
>          */
>         if (query.windows_available == 0) {
> +               int reset_win_ext;
>                 default_win = of_find_property(pdn, "ibm,dma-window",
> NULL);
>                 if (!default_win)
>                         goto out_failed;
>  
>                 reset_win_ext = ddw_read_ext(pdn,
> DDW_EXT_RESET_DMA_WIN, NULL);
> -               if (reset_win_ext)
> +               if (reset_win_ext){
> +                       default_win = NULL;
>                         goto out_failed;
> +               }


This says "if we can reset, then we fail", no?

>                 remove_dma_window(pdn, ddw_avail, default_win);


I think you can do "default_win=NULL" here and later at
out_restore_defwin check if it is NULL - then call reset.

>                 /* Query again, to check if the window is available */
>                 ret = query_ddw(dev, ddw_avail, &query, pdn);
>                 if (ret != 0)
> -                       goto out_restore_defwin;
> +                       goto out_failed;
>  
>                 if (query.windows_available == 0) {
>                         /* no windows are available for this device. */
>                         dev_dbg(&dev->dev, "no free dynamic windows");
> -                       goto out_restore_defwin;
> +                       goto out_failed;
>                 }
>         }
>         if (query.page_size & 4) {
> @@ -1203,7 +1206,7 @@ static u64 enable_ddw(struct pci_dev *dev, struct
> device_node *pdn)
>         } else {
>                 dev_dbg(&dev->dev, "no supported direct page size in
> mask %x",
>                           query.page_size);
> -               goto out_restore_defwin;
> +               goto out_failed;
>         }
>         /* verify the window * number of ptes will map the partition */
>         /* check largest block * page size > max memory hotplug addr */
> @@ -1212,14 +1215,14 @@ static u64 enable_ddw(struct pci_dev *dev,
> struct device_node *pdn)
>                 dev_dbg(&dev->dev, "can't map partition max 0x%llx with
> %llu "
>                           "%llu-sized pages\n",
> max_addr,  query.largest_available_block,
>                           1ULL << page_shift);
> -               goto out_restore_defwin;
> +               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");
> -               goto out_restore_defwin;
> +               goto out_failed;
>         }
>         win64->name = kstrdup(DIRECT64_PROPNAME, GFP_KERNEL);
>         win64->value = ddwprop = kmalloc(sizeof(*ddwprop), GFP_KERNEL);
> @@ -1282,11 +1285,10 @@ static u64 enable_ddw(struct pci_dev *dev,
> struct device_node *pdn)
>         kfree(win64->value);
>         kfree(win64);
>  
> -out_restore_defwin:
> -       if (default_win && reset_win_ext == 0)
> +out_failed:
> +       if (default_win)
>                 reset_dma_window(dev, pdn);
>  
> -out_failed:
>         fpdn = kzalloc(sizeof(*fpdn), GFP_KERNEL);
>         if (!fpdn)
>                 goto out_unlock;
> 
> #####
> 
> What do you think?
> 
> 
> 
>> The rest of the series is good as it is,
> 
> Thank you :)
> 
>>  however it may conflict with
>> https://patchwork.ozlabs.org/project/linuxppc-dev/patch/20200713062348.100552-1-aik@ozlabs.ru/
>> and the patchset it is made on top of -
>> https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=188385 .
> 
> <From the message of the first link>
>> (do not rush, let me finish reviewing this first) 
> 
> Ok, I have no problem rebasing on top of those patchsets, but what
> would you suggest to be done?

Polish this patch one more time and if by the time when you reposted it
the other patchset is not in upstream, I'll ask Michael to take yours first.


> Would it be ok doing a big multi-author patchset, so we guarantee it
> being applied in the correct order?
>> (You probably want me to rebase my patchset on top of Hellwig + yours,
> right?) 

Nah, at least not yet.


-- 
Alexey

Powered by blists - more mailing lists