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@xxxxxxxxx> 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@xxxxxxxxxxxx> 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@xxxxxxxxx> wrote:
>>>>>>>> From: Dien Pham <dien.pham.ry@xxxxxxxxxxxxxxx>
>>>>>>>>
>>>>>>>> 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@xxxxxxxxxxxxxxx>
>>>>>>>> Signed-off-by: Hien Dang <hien.dang.eb@xxxxxxxxxxx>
>>>>>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@xxxxxxxxx>
>>>>>>>> Cc: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
>>>>>>>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>
>>>>>>>> Cc: Phil Edworthy <phil.edworthy@xxxxxxxxxxx>
>>>>>>>> Cc: Simon Horman <horms+renesas@xxxxxxxxxxxx>
>>>>>>>> Cc: Wolfram Sang <wsa@xxxxxxxxxxxxx>
>>>>>>>> Cc: linux-renesas-soc@xxxxxxxxxxxxxxx
>>>>>>>> To: linux-pci@xxxxxxxxxxxxxxx
>>>>>>>> ---
>>>>>>>> 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
