On 04/19/2018 12:00 PM, Geert Uytterhoeven wrote: > Hi Marek, > > On Tue, Apr 10, 2018 at 6:17 PM, Marek Vasut <marek.vasut@xxxxxxxxx> wrote: >> On 04/10/2018 05:28 PM, Geert Uytterhoeven wrote: >>>>>> The pairing looks as follows: >>>>>> >>>>>> .- rcar_pcie_parse_request_of_pci_ranges() >>>>>> | (pm_runtime_enable is here) >>>>>> | .- pm_runtime_get_sync() >>>>>> | | .- rcar_pcie_get_resources() >>>>> >>>>> rcar_pcie_get_resources() is called while the device is runtime-enabled/resumed >>>> >>>> Because something may access the device, yes. >>>> >>>>>> | | | >>>>>> | | '- pm_runtime_put() >>>>>> | '- pm_runtime_disable() + pci_free_resource_list() >>>>> >>>>> pci_free_resource_list() is called while the device is runtime-disabled. >>>> >>>> Because nothing will access the device. >>>> >>>>>> '- pci_free_host_bridge() >>>>>> >>>>>> It looks symmetric to me ... >>>>> >>>>> rcar_pcie_get_resources() is called while the device is >>>>> runtime-enabled/resumed, >>>>> pci_free_resource_list() is called while the device is runtime-disabled. >>>> >>>> At this point, I think I'd rather see a diff of changes which you have >>>> in mind rather than this endless discussion. Can you provide one against >>>> this patch ? >>> >>> My final comment: >>> >>> If the steps during probing are A..Z, cleanup and removal should undo them >>> in reverse order (Z..A), unless there's a very good reason not to do so. >> >> I spent extra time going through the probe function and I just don't see >> how it is not done in the exact reverse, I checked every single goto >> statement in probe. >> >> I noticed this though: >> >>>>> rcar_pcie_get_resources() is called while the device is >>>>> runtime-enabled/resumed, >>>>> pci_free_resource_list() is called while the device is runtime-disabled. >> >> rcar_pcie_get_resources() is NOT a pair function for >> pci_free_resource_list() . rcar_pcie_parse_request_of_pci_ranges() is a >> pair function for pci_free_resource_list(). >> >> rcar_pcie_parse_request_of_pci_ranges() calls >> of_pci_get_host_bridge_resources() internally, so every single function >> called after successful call of rcar_pcie_parse_request_of_pci_ranges() >> must call pci_free_resource_list(). >> >> Both of_pci_get_host_bridge_resources() and pci_free_resource_list() are >> called with runtime PM disabled. >> >> The naming of the functions is confusing though. > > You are right, your changes are correct, and the naming of these functions > is confusing. Perhaps it should be changed, to avoid misleading the (not so) > casual reviewer? > > Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx> > > BTW, while diving deeper, I noticed a few other pre-existing issues in error > handling: > > 1. If anything fails after rcar_pcie_get_resources(), the bus clock is never > disabled, > 2. The error path of rcar_pcie_enable_msi() does not call > irq_dispose_mapping() before irq_domain_remove(), > 3. If rcar_pcie_enable() fails, none of the setup done in > rcar_pcie_enable_msi() is reverted. > Apart from the IRQ domain handling in 2, that includes freeing msi->pages > (should this be allocated using the DMA API?), and undoing the related HW > setup, to prevent the HW from scribbling the former MSI page in the future. > > Care to fix these, too? Yes, patchset is coming. -- Best regards, Marek Vasut
