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

[Geert Uytterhoeven <geert@linux-m68k.org>]

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?
Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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