Re: [PATCH V5] PCI: rcar: Use runtime PM to control controller clock

[Marek Vasut <marek.vasut@gmail.com>]

On 04/10/2018 04:42 PM, Geert Uytterhoeven wrote:
> Hi Marek,
> 
> On Tue, Apr 10, 2018 at 4:31 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>> On 04/09/2018 02:26 PM, Simon Horman wrote:
>>> On Mon, Apr 09, 2018 at 01:47:38PM +0200, Geert Uytterhoeven wrote:
>>>> On Mon, Apr 9, 2018 at 1:41 PM, Simon Horman <horms@verge.net.au> wrote:
>>>>> On Mon, Apr 09, 2018 at 10:20:05AM +0200, Marek Vasut wrote:
>>>>>> On 04/09/2018 10:07 AM, Geert Uytterhoeven wrote:
>>>>>>> On Sun, Apr 8, 2018 at 3:09 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>>>> From: Dien Pham <dien.pham.ry@rvc.renesas.com>
>>>>>>>>
>>>>>>>> The controller clock can be switched off during suspend/resume,
>>>>>>>> let runtime PM take care of that.
>>>>>>>>
>>>>>>>> Signed-off-by: Dien Pham <dien.pham.ry@rvc.renesas.com>
>>>>>>>> Signed-off-by: Hien Dang <hien.dang.eb@renesas.com>
>>>>>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>>>>>>>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
>>>>>>>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>>>>>>>> Cc: Phil Edworthy <phil.edworthy@renesas.com>
>>>>>>>> Cc: Simon Horman <horms+renesas@verge.net.au>
>>>>>>>> Cc: Wolfram Sang <wsa@the-dreams.de>
>>>>>>>> Cc: linux-renesas-soc@vger.kernel.org
>>>>>>>> To: linux-pci@vger.kernel.org
>>>>>>>> ---
>>>>>>>> V2: - Reorder the fail path in rcar_pcie_probe() to cater for the
>>>>>>>>       reordering of function calls in probe
>>>>>>>>     - Dispose of fail_clk in rcar_pcie_get_resources()
>>>>>>>> V3: - Fix up the failpath in probe function
>>>>>>>> V4: - Rebase on recent linux-next
>>>>>>>> V5: - Do not call pci_free_resource_list(&pcie->resources) if
>>>>>>>>       rcar_pcie_parse_request_of_pci_ranges() fails, since that
>>>>>>>>       functiona calls pci_free_resource_list() already.
>>>>>>>
>>>>>>> Thanks for the update!
>>>>>>>
>>>>>>>> --- a/drivers/pci/host/pcie-rcar.c
>>>>>>>> +++ b/drivers/pci/host/pcie-rcar.c
>>>>>>>
>>>>>>>> @@ -1124,22 +1111,22 @@ static int rcar_pcie_probe(struct platform_device *pdev)
>>>>>>>>         if (err)
>>>>>>>>                 goto err_free_bridge;
>>>>>>>>
>>>>>>>> +       pm_runtime_enable(pcie->dev);
>>>>>>>> +       err = pm_runtime_get_sync(pcie->dev);
>>>>>>>> +       if (err < 0) {
>>>>>>>> +               dev_err(pcie->dev, "pm_runtime_get_sync failed\n");
>>>>>>>> +               goto err_pm_disable;
>>>>>>>> +       }
>>>>>>>> +
>>>>>>>
>>>>>>> As you moved the pm_runtime setup up...
>>>>>>>
>>>>>>>>         err = rcar_pcie_get_resources(pcie);
>>>>>>>>         if (err < 0) {
>>>>>>>>                 dev_err(dev, "failed to request resources: %d\n", err);
>>>>>>>> -               goto err_free_resource_list;
>>>>>>>> +               goto err_pm_put;
>>>>>>>>         }
>>>>>>>>
>>>>>>>>         err = rcar_pcie_parse_map_dma_ranges(pcie, dev->of_node);
>>>>>>>>         if (err)
>>>>>>>> -               goto err_free_resource_list;
>>>>>>>> -
>>>>>>>> -       pm_runtime_enable(dev);
>>>>>>>> -       err = pm_runtime_get_sync(dev);
>>>>>>>> -       if (err < 0) {
>>>>>>>> -               dev_err(dev, "pm_runtime_get_sync failed\n");
>>>>>>>> -               goto err_pm_disable;
>>>>>>>> -       }
>>>>>>>> +               goto err_pm_put;
>>>>>>>>
>>>>>>>>         /* Failure to get a link might just be that no cards are inserted */
>>>>>>>>         hw_init_fn = of_device_get_match_data(dev);
>>>>>>>> @@ -1174,9 +1161,8 @@ static int rcar_pcie_probe(struct platform_device *pdev)
>>>>>>>>
>>>>>>>>  err_pm_disable:
>>>>>>>>         pm_runtime_disable(dev);
>>>>>>>
>>>>>>> ... shouldn't it be moved down here, for symmetry?
>>>>>>
>>>>>> I am reasonably certain the failpath should be correct now. Did I still
>>>>>> miss something ?
>>>>>
>>>>> It looks correct to me too. Geert are Marek and I missing something?
>>>>
>>>> Probably it will still work fine, but after this patch, Runtime PM is enabled
>>>> early, and disabled early, which is not symmetrical.
>>>>
>>>> I like symmetry ;-)
>>>
>>> Understood. I think that is reasonable.
>>> Marek, would you care to respin?
>>
>> I am looking into the driver, but I fail to see what Geert is trying to
>> make me change here.
>>
>> 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 ?

-- 
Best regards,
Marek Vasut