Re: [PATCH v3 4/6] powerpc/pseries/iommu: Remove default DMA window before creating DDW
From: Leonardo Bras
Date: Mon Jul 13 2020 - 22:40:39 EST
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@xxxxxxxxx/
> 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!