On 04/10/2018 05:28 PM, Geert Uytterhoeven wrote: > Hi Marek, Hi, [...] >>>> 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. -- Best regards, Marek Vasut
