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

Reply via email to