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

[Marek Vasut]

On 04/19/2018 12:00 PM, Geert Uytterhoeven wrote:
> Hi Marek,
> 
> On Tue, Apr 10, 2018 at 6:17 PM, Marek Vasut  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 
> 
> 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

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

[Marek Vasut]

On 05/21/2018 03:03 PM, Lorenzo Pieralisi wrote:
> On Mon, May 21, 2018 at 01:08:36PM +0200, Marek Vasut wrote:
>> On 05/14/2018 05:49 PM, Lorenzo Pieralisi wrote:
>>> On Mon, May 14, 2018 at 05:32:04PM +0200, Marek Vasut wrote:
 On 05/01/2018 12:55 PM, Lorenzo Pieralisi wrote:
> 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.
>>
>> Reviewed-by: Simon Horman 
>
> Applied to pci/rcar for v4.18, thanks.

 Is there any reason why this patch isnt in next yet ?
>>>
>>> Bjorn will merge it into -next this week.
>>
>> Seems this got missed last week ?
> 
> It will go into -next as soon as a new -next release will
> be available:
> 
> https://marc.info/?l=linux-next=152654084627146=2

Cool, thanks, I have a few more fixes coming.

-- 
Best regards,
Marek Vasut

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

[Lorenzo Pieralisi]

On Mon, May 21, 2018 at 01:08:36PM +0200, Marek Vasut wrote:
> On 05/14/2018 05:49 PM, Lorenzo Pieralisi wrote:
> > On Mon, May 14, 2018 at 05:32:04PM +0200, Marek Vasut wrote:
> >> On 05/01/2018 12:55 PM, Lorenzo Pieralisi wrote:
> >>> 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.
> 
>  Reviewed-by: Simon Horman 
> >>>
> >>> Applied to pci/rcar for v4.18, thanks.
> >>
> >> Is there any reason why this patch isnt in next yet ?
> > 
> > Bjorn will merge it into -next this week.
> 
> Seems this got missed last week ?

It will go into -next as soon as a new -next release will
be available:

https://marc.info/?l=linux-next=152654084627146=2

Lorenzo

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

[Marek Vasut]

On 05/14/2018 05:49 PM, Lorenzo Pieralisi wrote:
> On Mon, May 14, 2018 at 05:32:04PM +0200, Marek Vasut wrote:
>> On 05/01/2018 12:55 PM, Lorenzo Pieralisi wrote:
>>> 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.

 Reviewed-by: Simon Horman 
>>>
>>> Applied to pci/rcar for v4.18, thanks.
>>
>> Is there any reason why this patch isnt in next yet ?
> 
> Bjorn will merge it into -next this week.

Seems this got missed last week ?

-- 
Best regards,
Marek Vasut

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

[Lorenzo Pieralisi]

On Mon, May 14, 2018 at 05:32:04PM +0200, Marek Vasut wrote:
> On 05/01/2018 12:55 PM, Lorenzo Pieralisi wrote:
> > 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.
> >>
> >> Reviewed-by: Simon Horman 
> > 
> > Applied to pci/rcar for v4.18, thanks.
> 
> Is there any reason why this patch isnt in next yet ?

Bjorn will merge it into -next this week.

Thanks,
Lorenzo

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

[Marek Vasut]

On 05/01/2018 12:55 PM, Lorenzo Pieralisi wrote:
> 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.
>>
>> Reviewed-by: Simon Horman 
> 
> Applied to pci/rcar for v4.18, thanks.

Is there any reason why this patch isnt in next yet ?

-- 
Best regards,
Marek Vasut

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

[Lorenzo Pieralisi]

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.
> 
> Reviewed-by: Simon Horman 

Applied to pci/rcar for v4.18, thanks.

Lorenzo

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

[Geert Uytterhoeven]

Hi Marek,

On Tue, Apr 10, 2018 at 6:17 PM, Marek Vasut  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 

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

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

[Lorenzo Pieralisi]

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

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

[Simon Horman]

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 

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

[Marek Vasut]

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

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

[Geert Uytterhoeven]

Hi Marek,

On Tue, Apr 10, 2018 at 5:25 PM, Marek Vasut  wrote:
> On 04/10/2018 04:42 PM, Geert Uytterhoeven wrote:
>> On Tue, Apr 10, 2018 at 4:31 PM, Marek Vasut  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  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  
 wrote:
> From: Dien Pham 
>
> The controller clock can be switched off during suspend/resume,
> let runtime PM take care of that.
>
> Signed-off-by: Dien Pham 
> Signed-off-by: Hien Dang 
> Signed-off-by: Marek Vasut 
> Cc: Geert Uytterhoeven 
> Cc: Lorenzo Pieralisi 
> Cc: Phil Edworthy 
> Cc: Simon Horman 
> Cc: Wolfram Sang 
> Cc: linux-renesas-soc@vger.kernel.org
> To: linux-...@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(>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 +,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 

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

[Marek Vasut]

On 04/10/2018 04:42 PM, Geert Uytterhoeven wrote:
> Hi Marek,
> 
> On Tue, Apr 10, 2018 at 4:31 PM, Marek Vasut  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  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  
>>> wrote:
 From: Dien Pham 

 The controller clock can be switched off during suspend/resume,
 let runtime PM take care of that.

 Signed-off-by: Dien Pham 
 Signed-off-by: Hien Dang 
 Signed-off-by: Marek Vasut 
 Cc: Geert Uytterhoeven 
 Cc: Lorenzo Pieralisi 
 Cc: Phil Edworthy 
 Cc: Simon Horman 
 Cc: Wolfram Sang 
 Cc: linux-renesas-soc@vger.kernel.org
 To: linux-...@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(>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 +,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 

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

[Geert Uytterhoeven]

Hi Marek,

On Tue, Apr 10, 2018 at 4:31 PM, Marek Vasut  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  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  
>> wrote:
>>> From: Dien Pham 
>>>
>>> The controller clock can be switched off during suspend/resume,
>>> let runtime PM take care of that.
>>>
>>> Signed-off-by: Dien Pham 
>>> Signed-off-by: Hien Dang 
>>> Signed-off-by: Marek Vasut 
>>> Cc: Geert Uytterhoeven 
>>> Cc: Lorenzo Pieralisi 
>>> Cc: Phil Edworthy 
>>> Cc: Simon Horman 
>>> Cc: Wolfram Sang 
>>> Cc: linux-renesas-soc@vger.kernel.org
>>> To: linux-...@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(>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 +,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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@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.
 

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

[Marek Vasut]

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  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  wrote:
>> From: Dien Pham 
>>
>> The controller clock can be switched off during suspend/resume,
>> let runtime PM take care of that.
>>
>> Signed-off-by: Dien Pham 
>> Signed-off-by: Hien Dang 
>> Signed-off-by: Marek Vasut 
>> Cc: Geert Uytterhoeven 
>> Cc: Lorenzo Pieralisi 
>> Cc: Phil Edworthy 
>> Cc: Simon Horman 
>> Cc: Wolfram Sang 
>> Cc: linux-renesas-soc@vger.kernel.org
>> To: linux-...@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(>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 +,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 ...

-- 
Best regards,
Marek Vasut

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

[Simon Horman]

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  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  
> >> > wrote:
> >> >> From: Dien Pham 
> >> >>
> >> >> The controller clock can be switched off during suspend/resume,
> >> >> let runtime PM take care of that.
> >> >>
> >> >> Signed-off-by: Dien Pham 
> >> >> Signed-off-by: Hien Dang 
> >> >> Signed-off-by: Marek Vasut 
> >> >> Cc: Geert Uytterhoeven 
> >> >> Cc: Lorenzo Pieralisi 
> >> >> Cc: Phil Edworthy 
> >> >> Cc: Simon Horman 
> >> >> Cc: Wolfram Sang 
> >> >> Cc: linux-renesas-soc@vger.kernel.org
> >> >> To: linux-...@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(>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 +,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?

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

[Geert Uytterhoeven]

Hi Simon, Marek,

On Mon, Apr 9, 2018 at 1:41 PM, Simon Horman  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  wrote:
>> >> From: Dien Pham 
>> >>
>> >> The controller clock can be switched off during suspend/resume,
>> >> let runtime PM take care of that.
>> >>
>> >> Signed-off-by: Dien Pham 
>> >> Signed-off-by: Hien Dang 
>> >> Signed-off-by: Marek Vasut 
>> >> Cc: Geert Uytterhoeven 
>> >> Cc: Lorenzo Pieralisi 
>> >> Cc: Phil Edworthy 
>> >> Cc: Simon Horman 
>> >> Cc: Wolfram Sang 
>> >> Cc: linux-renesas-soc@vger.kernel.org
>> >> To: linux-...@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(>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 +,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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@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

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

[Simon Horman]

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  wrote:
> >> From: Dien Pham 
> >>
> >> The controller clock can be switched off during suspend/resume,
> >> let runtime PM take care of that.
> >>
> >> Signed-off-by: Dien Pham 
> >> Signed-off-by: Hien Dang 
> >> Signed-off-by: Marek Vasut 
> >> Cc: Geert Uytterhoeven 
> >> Cc: Lorenzo Pieralisi 
> >> Cc: Phil Edworthy 
> >> Cc: Simon Horman 
> >> Cc: Wolfram Sang 
> >> Cc: linux-renesas-soc@vger.kernel.org
> >> To: linux-...@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(>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 +,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?

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

[Marek Vasut]

On 04/09/2018 10:07 AM, Geert Uytterhoeven wrote:
> Hi Marek,
> 
> On Sun, Apr 8, 2018 at 3:09 PM, Marek Vasut  wrote:
>> From: Dien Pham 
>>
>> The controller clock can be switched off during suspend/resume,
>> let runtime PM take care of that.
>>
>> Signed-off-by: Dien Pham 
>> Signed-off-by: Hien Dang 
>> Signed-off-by: Marek Vasut 
>> Cc: Geert Uytterhoeven 
>> Cc: Lorenzo Pieralisi 
>> Cc: Phil Edworthy 
>> Cc: Simon Horman 
>> Cc: Wolfram Sang 
>> Cc: linux-renesas-soc@vger.kernel.org
>> To: linux-...@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(>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 +,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 ?

-- 
Best regards,
Marek Vasut

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

[Geert Uytterhoeven]

Hi Marek,

On Sun, Apr 8, 2018 at 3:09 PM, Marek Vasut  wrote:
> From: Dien Pham 
>
> The controller clock can be switched off during suspend/resume,
> let runtime PM take care of that.
>
> Signed-off-by: Dien Pham 
> Signed-off-by: Hien Dang 
> Signed-off-by: Marek Vasut 
> Cc: Geert Uytterhoeven 
> Cc: Lorenzo Pieralisi 
> Cc: Phil Edworthy 
> Cc: Simon Horman 
> Cc: Wolfram Sang 
> Cc: linux-renesas-soc@vger.kernel.org
> To: linux-...@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(>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 +,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(>resources);
> +
>  err_free_bridge:
> pci_free_host_bridge(bridge);

Gr{oetje,eeting}s,

Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@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

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

[Marek Vasut]

From: Dien Pham 

The controller clock can be switched off during suspend/resume,
let runtime PM take care of that.

Signed-off-by: Dien Pham 
Signed-off-by: Hien Dang 
Signed-off-by: Marek Vasut 
Cc: Geert Uytterhoeven 
Cc: Lorenzo Pieralisi 
Cc: Phil Edworthy 
Cc: Simon Horman 
Cc: Wolfram Sang 
Cc: linux-renesas-soc@vger.kernel.org
To: linux-...@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(>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(-)

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_headresources;
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 +,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(>resources);
+
 err_free_bridge:
pci_free_host_bridge(bridge);
 
-- 
2.16.2