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-...@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!