From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f52.google.com ([74.125.82.52]:54375 "EHLO mail-wm0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751367AbeDJQSB (ORCPT ); Tue, 10 Apr 2018 12:18:01 -0400 From: Marek Vasut Subject: Re: [PATCH V5] PCI: rcar: Use runtime PM to control controller clock To: Geert Uytterhoeven Cc: Simon Horman , linux-pci , Dien Pham , Hien Dang , Marek Vasut , Geert Uytterhoeven , Lorenzo Pieralisi , Phil Edworthy , Wolfram Sang , Linux-Renesas References: <20180408130925.19088-1-marek.vasut+renesas@gmail.com> <3973dcdf-c6d7-5622-0c19-ea6f77899261@gmail.com> <20180409114159.azxeehjkeuinwrwe@verge.net.au> <20180409122636.6rmtzhqqlpqxyear@verge.net.au> Message-ID: Date: Tue, 10 Apr 2018 18:17:04 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Sender: linux-pci-owner@vger.kernel.org List-ID: 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