Re: [PATCH V5] PCI: rcar: Use runtime PM to control controller clock
On 04/19/2018 12:00 PM, Geert Uytterhoeven wrote: > Hi Marek, > > On Tue, Apr 10, 2018 at 6:17 PM, Marek Vasutwrote: >> 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
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
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
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
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
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
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 HormanApplied to pci/rcar for v4.18, thanks. Lorenzo
Re: [PATCH V5] PCI: rcar: Use runtime PM to control controller clock
Hi Marek, On Tue, Apr 10, 2018 at 6:17 PM, Marek Vasutwrote: > 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
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
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
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
Hi Marek, On Tue, Apr 10, 2018 at 5:25 PM, Marek Vasutwrote: > 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
On 04/10/2018 04:42 PM, Geert Uytterhoeven wrote: > Hi Marek, > > On Tue, Apr 10, 2018 at 4:31 PM, Marek Vasutwrote: >> 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
Hi Marek, On Tue, Apr 10, 2018 at 4:31 PM, Marek Vasutwrote: > 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
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 Hormanwrote: >>> 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
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 Hormanwrote: > > 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
Hi Simon, Marek, On Mon, Apr 9, 2018 at 1:41 PM, Simon Hormanwrote: > 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
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 Vasutwrote: > >> 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
On 04/09/2018 10:07 AM, Geert Uytterhoeven wrote: > Hi Marek, > > On Sun, Apr 8, 2018 at 3:09 PM, Marek Vasutwrote: >> 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
Hi Marek, On Sun, Apr 8, 2018 at 3:09 PM, Marek Vasutwrote: > 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