From: Leonardo Bras <leobras.c@gmail.com>
To: Alexey Kardashevskiy <aik@ozlabs.ru>,
Michael Ellerman <mpe@ellerman.id.au>,
Benjamin Herrenschmidt <benh@kernel.crashing.org>,
Paul Mackerras <paulus@samba.org>,
Thiago Jung Bauermann <bauerman@linux.ibm.com>,
Ram Pai <linuxram@us.ibm.com>
Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 4/6] powerpc/pseries/iommu: Remove default DMA window before creating DDW
Date: Mon, 13 Jul 2020 23:40:30 -0300 [thread overview]
Message-ID: <8c29be499e8741e7d77d53ca005034a2ca0179ac.camel@gmail.com> (raw)
In-Reply-To: <cc15a81d-04d9-3ee4-4fdb-093618f6e635@ozlabs.ru>
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.
> > [...]
> > -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.
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.
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).
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;
+ }
remove_dma_window(pdn, ddw_avail, default_win);
/* 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?
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?)
> thanks,
Thank you!
next prev parent reply other threads:[~2020-07-14 2:40 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-03 6:18 [PATCH v3 0/6] Remove default DMA window before creating DDW Leonardo Bras
2020-07-03 6:18 ` [PATCH v3 1/6] powerpc/pseries/iommu: Create defines for operations in ibm,ddw-applicable Leonardo Bras
2020-07-03 6:18 ` [PATCH v3 2/6] powerpc/pseries/iommu: Update call to ibm,query-pe-dma-windows Leonardo Bras
2020-07-03 6:18 ` [PATCH v3 3/6] powerpc/pseries/iommu: Move window-removing part of remove_ddw into remove_dma_window Leonardo Bras
2020-07-03 6:18 ` [PATCH v3 4/6] powerpc/pseries/iommu: Remove default DMA window before creating DDW Leonardo Bras
2020-07-13 7:33 ` Alexey Kardashevskiy
2020-07-14 2:40 ` Leonardo Bras [this message]
2020-07-14 4:52 ` Alexey Kardashevskiy
2020-07-14 6:30 ` Leonardo Bras
2020-07-14 6:46 ` Leonardo Bras
2020-07-03 6:18 ` [PATCH v3 5/6] powerpc/pseries/iommu: Make use of DDW even if it does not map the partition Leonardo Bras
2020-07-03 6:18 ` [PATCH v3 6/6] powerpc/pseries/iommu: Rename "direct window" to "dma window" Leonardo Bras
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8c29be499e8741e7d77d53ca005034a2ca0179ac.camel@gmail.com \
--to=leobras.c@gmail.com \
--cc=aik@ozlabs.ru \
--cc=bauerman@linux.ibm.com \
--cc=benh@kernel.crashing.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=linuxram@us.ibm.com \
--cc=mpe@ellerman.id.au \
--cc=paulus@samba.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).