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

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

On 04/19/2018 12:00 PM, Geert Uytterhoeven wrote:
> Hi Marek,
> 
> On Tue, Apr 10, 2018 at 6:17 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>> On 04/10/2018 05:28 PM, Geert Uytterhoeven wrote:
>>>>>> 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.
> 
> You are right, your changes are correct, and the naming of these functions
> is confusing. Perhaps it should be changed, to avoid misleading the (not so)
> casual reviewer?
> 
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> BTW, while diving deeper, I noticed a few other pre-existing issues in error
> handling:
> 
> 1. If anything fails after rcar_pcie_get_resources(), the bus clock is never
>    disabled,
> 2. The error path of rcar_pcie_enable_msi() does not call
>    irq_dispose_mapping() before irq_domain_remove(),
> 3. If rcar_pcie_enable() fails, none of the setup done in
>    rcar_pcie_enable_msi() is reverted.
>    Apart from the IRQ domain handling in 2, that includes freeing msi->pages
>    (should this be allocated using the DMA API?), and undoing the related HW
>    setup, to prevent the HW from scribbling the former MSI page in the future.
> 
> Care to fix these, too?

Yes, patchset is coming.

-- 
Best regards,
Marek Vasut