Hi Marek,
On Tue, Apr 10, 2018 at 5:25 PM, Marek Vasut <[email protected]> wrote:
> On 04/10/2018 04:42 PM, Geert Uytterhoeven wrote:
>> On Tue, Apr 10, 2018 at 4:31 PM, Marek Vasut <[email protected]> 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 <[email protected]> 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 <[email protected]>
>>>>>>>> wrote:
>>>>>>>>> From: Dien Pham <[email protected]>
>>>>>>>>>
>>>>>>>>> The controller clock can be switched off during suspend/resume,
>>>>>>>>> let runtime PM take care of that.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Dien Pham <[email protected]>
>>>>>>>>> Signed-off-by: Hien Dang <[email protected]>
>>>>>>>>> Signed-off-by: Marek Vasut <[email protected]>
>>>>>>>>> Cc: Geert Uytterhoeven <[email protected]>
>>>>>>>>> Cc: Lorenzo Pieralisi <[email protected]>
>>>>>>>>> Cc: Phil Edworthy <[email protected]>
>>>>>>>>> Cc: Simon Horman <[email protected]>
>>>>>>>>> Cc: Wolfram Sang <[email protected]>
>>>>>>>>> Cc: [email protected]
>>>>>>>>> To: [email protected]
>>>>>>>>> ---
>>>>>>>>> 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 ?
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.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
