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

Message ID 20180408130925.19088-1-marek.vasut+renesas@gmail.com
State New
Delegated to: Lorenzo Pieralisi
Headers show
Series
  • [V5] PCI: rcar: Use runtime PM to control controller clock
Related show

Commit Message

Marek Vasut April 8, 2018, 1:09 p.m.
From: Dien Pham <dien.pham.ry@rvc.renesas.com>

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@rvc.renesas.com>
Signed-off-by: Hien Dang <hien.dang.eb@renesas.com>
Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Phil Edworthy <phil.edworthy@renesas.com>
Cc: Simon Horman <horms+renesas@verge.net.au>
Cc: Wolfram Sang <wsa@the-dreams.de>
Cc: linux-renesas-soc@vger.kernel.org
To: linux-pci@vger.kernel.org
---
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.
---
 drivers/pci/host/pcie-rcar.c | 38 ++++++++++++--------------------------
 1 file changed, 12 insertions(+), 26 deletions(-)

Comments

Geert Uytterhoeven April 9, 2018, 8:07 a.m. | #1
Hi Marek,

On Sun, Apr 8, 2018 at 3:09 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> From: Dien Pham <dien.pham.ry@rvc.renesas.com>
>
> 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@rvc.renesas.com>
> Signed-off-by: Hien Dang <hien.dang.eb@renesas.com>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Phil Edworthy <phil.edworthy@renesas.com>
> Cc: Simon Horman <horms+renesas@verge.net.au>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Cc: linux-renesas-soc@vger.kernel.org
> To: linux-pci@vger.kernel.org
> ---
> 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?

> -
> -err_free_resource_list:
>         pci_free_resource_list(&pcie->resources);
> +
>  err_free_bridge:
>         pci_free_host_bridge(bridge);

Gr{oetje,eeting}s,

                        Geert
Marek Vasut April 9, 2018, 8:20 a.m. | #2
On 04/09/2018 10:07 AM, Geert Uytterhoeven wrote:
> Hi Marek,
> 
> On Sun, Apr 8, 2018 at 3:09 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>> From: Dien Pham <dien.pham.ry@rvc.renesas.com>
>>
>> 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@rvc.renesas.com>
>> Signed-off-by: Hien Dang <hien.dang.eb@renesas.com>
>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>> Cc: Phil Edworthy <phil.edworthy@renesas.com>
>> Cc: Simon Horman <horms+renesas@verge.net.au>
>> Cc: Wolfram Sang <wsa@the-dreams.de>
>> Cc: linux-renesas-soc@vger.kernel.org
>> To: linux-pci@vger.kernel.org
>> ---
>> 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 ?
Simon Horman April 9, 2018, 11:41 a.m. | #3
On Mon, Apr 09, 2018 at 10:20:05AM +0200, Marek Vasut wrote:
> On 04/09/2018 10:07 AM, Geert Uytterhoeven wrote:
> > Hi Marek,
> > 
> > On Sun, Apr 8, 2018 at 3:09 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> >> From: Dien Pham <dien.pham.ry@rvc.renesas.com>
> >>
> >> 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@rvc.renesas.com>
> >> Signed-off-by: Hien Dang <hien.dang.eb@renesas.com>
> >> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> >> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> >> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> >> Cc: Phil Edworthy <phil.edworthy@renesas.com>
> >> Cc: Simon Horman <horms+renesas@verge.net.au>
> >> Cc: Wolfram Sang <wsa@the-dreams.de>
> >> Cc: linux-renesas-soc@vger.kernel.org
> >> To: linux-pci@vger.kernel.org
> >> ---
> >> 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?
Geert Uytterhoeven April 9, 2018, 11:47 a.m. | #4
Hi Simon, Marek,

On Mon, Apr 9, 2018 at 1:41 PM, Simon Horman <horms@verge.net.au> 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@gmail.com> wrote:
>> >> From: Dien Pham <dien.pham.ry@rvc.renesas.com>
>> >>
>> >> 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@rvc.renesas.com>
>> >> Signed-off-by: Hien Dang <hien.dang.eb@renesas.com>
>> >> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>> >> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
>> >> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>> >> Cc: Phil Edworthy <phil.edworthy@renesas.com>
>> >> Cc: Simon Horman <horms+renesas@verge.net.au>
>> >> Cc: Wolfram Sang <wsa@the-dreams.de>
>> >> Cc: linux-renesas-soc@vger.kernel.org
>> >> To: linux-pci@vger.kernel.org
>> >> ---
>> >> 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 ;-)

Gr{oetje,eeting}s,

                        Geert
Simon Horman April 9, 2018, 12:26 p.m. | #5
On Mon, Apr 09, 2018 at 01:47:38PM +0200, Geert Uytterhoeven wrote:
> Hi Simon, Marek,
> 
> On Mon, Apr 9, 2018 at 1:41 PM, Simon Horman <horms@verge.net.au> 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@gmail.com> wrote:
> >> >> From: Dien Pham <dien.pham.ry@rvc.renesas.com>
> >> >>
> >> >> 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@rvc.renesas.com>
> >> >> Signed-off-by: Hien Dang <hien.dang.eb@renesas.com>
> >> >> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> >> >> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> >> >> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> >> >> Cc: Phil Edworthy <phil.edworthy@renesas.com>
> >> >> Cc: Simon Horman <horms+renesas@verge.net.au>
> >> >> Cc: Wolfram Sang <wsa@the-dreams.de>
> >> >> Cc: linux-renesas-soc@vger.kernel.org
> >> >> To: linux-pci@vger.kernel.org
> >> >> ---
> >> >> 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?
Marek Vasut April 10, 2018, 2:31 p.m. | #6
On 04/09/2018 02:26 PM, Simon Horman wrote:
> On Mon, Apr 09, 2018 at 01:47:38PM +0200, Geert Uytterhoeven wrote:
>> Hi Simon, Marek,
>>
>> On Mon, Apr 9, 2018 at 1:41 PM, Simon Horman <horms@verge.net.au> 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@gmail.com> wrote:
>>>>>> From: Dien Pham <dien.pham.ry@rvc.renesas.com>
>>>>>>
>>>>>> 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@rvc.renesas.com>
>>>>>> Signed-off-by: Hien Dang <hien.dang.eb@renesas.com>
>>>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>>>>>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
>>>>>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>>>>>> Cc: Phil Edworthy <phil.edworthy@renesas.com>
>>>>>> Cc: Simon Horman <horms+renesas@verge.net.au>
>>>>>> Cc: Wolfram Sang <wsa@the-dreams.de>
>>>>>> Cc: linux-renesas-soc@vger.kernel.org
>>>>>> To: linux-pci@vger.kernel.org
>>>>>> ---
>>>>>> 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()
| | |
| | '- pm_runtime_put()
| '- pm_runtime_disable() + pci_free_resource_list()
'- pci_free_host_bridge()

It looks symmetric to me ...
Geert Uytterhoeven April 10, 2018, 2:42 p.m. | #7
Hi Marek,

On Tue, Apr 10, 2018 at 4:31 PM, Marek Vasut <marek.vasut@gmail.com> 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@verge.net.au> 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@gmail.com> wrote:
>>>>>>> From: Dien Pham <dien.pham.ry@rvc.renesas.com>
>>>>>>>
>>>>>>> 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@rvc.renesas.com>
>>>>>>> Signed-off-by: Hien Dang <hien.dang.eb@renesas.com>
>>>>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>>>>>>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
>>>>>>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>>>>>>> Cc: Phil Edworthy <phil.edworthy@renesas.com>
>>>>>>> Cc: Simon Horman <horms+renesas@verge.net.au>
>>>>>>> Cc: Wolfram Sang <wsa@the-dreams.de>
>>>>>>> Cc: linux-renesas-soc@vger.kernel.org
>>>>>>> To: linux-pci@vger.kernel.org
>>>>>>> ---
>>>>>>> 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

> | | |
> | | '- pm_runtime_put()
> | '- pm_runtime_disable() + pci_free_resource_list()

pci_free_resource_list() is called while the device is runtime-disabled.

> '- 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.

Gr{oetje,eeting}s,

                        Geert
Marek Vasut April 10, 2018, 3:25 p.m. | #8
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@gmail.com> 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@verge.net.au> 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@gmail.com> wrote:
>>>>>>>> From: Dien Pham <dien.pham.ry@rvc.renesas.com>
>>>>>>>>
>>>>>>>> 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@rvc.renesas.com>
>>>>>>>> Signed-off-by: Hien Dang <hien.dang.eb@renesas.com>
>>>>>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>>>>>>>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
>>>>>>>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>>>>>>>> Cc: Phil Edworthy <phil.edworthy@renesas.com>
>>>>>>>> Cc: Simon Horman <horms+renesas@verge.net.au>
>>>>>>>> Cc: Wolfram Sang <wsa@the-dreams.de>
>>>>>>>> Cc: linux-renesas-soc@vger.kernel.org
>>>>>>>> To: linux-pci@vger.kernel.org
>>>>>>>> ---
>>>>>>>> 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 ?
Geert Uytterhoeven April 10, 2018, 3:28 p.m. | #9
Hi Marek,

On Tue, Apr 10, 2018 at 5:25 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> On 04/10/2018 04:42 PM, Geert Uytterhoeven wrote:
>> On Tue, Apr 10, 2018 at 4:31 PM, Marek Vasut <marek.vasut@gmail.com> 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@verge.net.au> 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@gmail.com> wrote:
>>>>>>>>> From: Dien Pham <dien.pham.ry@rvc.renesas.com>
>>>>>>>>>
>>>>>>>>> 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@rvc.renesas.com>
>>>>>>>>> Signed-off-by: Hien Dang <hien.dang.eb@renesas.com>
>>>>>>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>>>>>>>>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
>>>>>>>>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>>>>>>>>> Cc: Phil Edworthy <phil.edworthy@renesas.com>
>>>>>>>>> Cc: Simon Horman <horms+renesas@verge.net.au>
>>>>>>>>> Cc: Wolfram Sang <wsa@the-dreams.de>
>>>>>>>>> Cc: linux-renesas-soc@vger.kernel.org
>>>>>>>>> To: linux-pci@vger.kernel.org
>>>>>>>>> ---
>>>>>>>>> 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
Marek Vasut April 10, 2018, 4:17 p.m. | #10
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.
Simon Horman April 13, 2018, 12:48 p.m. | #11
On Tue, Apr 10, 2018 at 06:17:04PM +0200, Marek Vasut wrote:
> On 04/10/2018 05:28 PM, Geert Uytterhoeven wrote:

...

> >>> 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.

Hi,

thanks everyone for their efforts in preparing/reviewing this patch.

It seems there are some differences of opinion on how best to handle the
error paths but unlike earlier versions this one seems correct to me. If
that turns out to be false we can address it. But I don't think its likely
things will be enhanced by continuing this review.

Lorenzo, please consider taking this patch in its current form.

Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
Lorenzo Pieralisi April 13, 2018, 5:48 p.m. | #12
On Fri, Apr 13, 2018 at 02:48:19PM +0200, Simon Horman wrote:
> On Tue, Apr 10, 2018 at 06:17:04PM +0200, Marek Vasut wrote:
> > On 04/10/2018 05:28 PM, Geert Uytterhoeven wrote:
> 
> ...
> 
> > >>> 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.
> 
> Hi,
> 
> thanks everyone for their efforts in preparing/reviewing this patch.
> 
> It seems there are some differences of opinion on how best to handle the
> error paths but unlike earlier versions this one seems correct to me. If
> that turns out to be false we can address it. But I don't think its likely
> things will be enhanced by continuing this review.
> 
> Lorenzo, please consider taking this patch in its current form.

I will as soon as we restart queueing patches for v4.18, thanks for
the heads-up.

Lorenzo
Geert Uytterhoeven April 19, 2018, 10 a.m. | #13
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

Patch

diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c
index 6ab28f29ac6a..25f68305322c 100644
--- a/drivers/pci/host/pcie-rcar.c
+++ b/drivers/pci/host/pcie-rcar.c
@@ -142,7 +142,6 @@  struct rcar_pcie {
 	void __iomem		*base;
 	struct list_head	resources;
 	int			root_bus_nr;
-	struct clk		*clk;
 	struct clk		*bus_clk;
 	struct			rcar_msi msi;
 };
@@ -914,24 +913,14 @@  static int rcar_pcie_get_resources(struct rcar_pcie *pcie)
 	if (IS_ERR(pcie->base))
 		return PTR_ERR(pcie->base);
 
-	pcie->clk = devm_clk_get(dev, "pcie");
-	if (IS_ERR(pcie->clk)) {
-		dev_err(dev, "cannot get platform clock\n");
-		return PTR_ERR(pcie->clk);
-	}
-	err = clk_prepare_enable(pcie->clk);
-	if (err)
-		return err;
-
 	pcie->bus_clk = devm_clk_get(dev, "pcie_bus");
 	if (IS_ERR(pcie->bus_clk)) {
 		dev_err(dev, "cannot get pcie bus clock\n");
-		err = PTR_ERR(pcie->bus_clk);
-		goto fail_clk;
+		return PTR_ERR(pcie->bus_clk);
 	}
 	err = clk_prepare_enable(pcie->bus_clk);
 	if (err)
-		goto fail_clk;
+		return err;
 
 	i = irq_of_parse_and_map(dev->of_node, 0);
 	if (!i) {
@@ -953,8 +942,6 @@  static int rcar_pcie_get_resources(struct rcar_pcie *pcie)
 
 err_map_reg:
 	clk_disable_unprepare(pcie->bus_clk);
-fail_clk:
-	clk_disable_unprepare(pcie->clk);
 
 	return err;
 }
@@ -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;
+	}
+
 	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);
-
-err_free_resource_list:
 	pci_free_resource_list(&pcie->resources);
+
 err_free_bridge:
 	pci_free_host_bridge(bridge);