| Message ID | 20180408130925.19088-1-marek.vasut+renesas@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Simon Horman |
| Headers | show |
Hi Marek, On Sun, Apr 8, 2018 at 3:09 PM, Marek Vasut <marek.vasut@gmail.com> wrote: > From: Dien Pham <dien.pham.ry@rvc.renesas.com> > > The controller clock can be switched off during suspend/resume, > let runtime PM take care of that. > > Signed-off-by: Dien Pham <dien.pham.ry@rvc.renesas.com> > Signed-off-by: Hien Dang <hien.dang.eb@renesas.com> > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > Cc: Geert Uytterhoeven <geert+renesas@glider.be> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Cc: Phil Edworthy <phil.edworthy@renesas.com> > Cc: Simon Horman <horms+renesas@verge.net.au> > Cc: Wolfram Sang <wsa@the-dreams.de> > Cc: linux-renesas-soc@vger.kernel.org > To: linux-pci@vger.kernel.org > --- > V2: - Reorder the fail path in rcar_pcie_probe() to cater for the > reordering of function calls in probe > - Dispose of fail_clk in rcar_pcie_get_resources() > V3: - Fix up the failpath in probe function > V4: - Rebase on recent linux-next > V5: - Do not call pci_free_resource_list(&pcie->resources) if > rcar_pcie_parse_request_of_pci_ranges() fails, since that > functiona calls pci_free_resource_list() already. Thanks for the update! > --- a/drivers/pci/host/pcie-rcar.c > +++ b/drivers/pci/host/pcie-rcar.c > @@ -1124,22 +1111,22 @@ static int rcar_pcie_probe(struct platform_device *pdev) > if (err) > goto err_free_bridge; > > + pm_runtime_enable(pcie->dev); > + err = pm_runtime_get_sync(pcie->dev); > + if (err < 0) { > + dev_err(pcie->dev, "pm_runtime_get_sync failed\n"); > + goto err_pm_disable; > + } > + As you moved the pm_runtime setup up... > err = rcar_pcie_get_resources(pcie); > if (err < 0) { > dev_err(dev, "failed to request resources: %d\n", err); > - goto err_free_resource_list; > + goto err_pm_put; > } > > err = rcar_pcie_parse_map_dma_ranges(pcie, dev->of_node); > if (err) > - goto err_free_resource_list; > - > - pm_runtime_enable(dev); > - err = pm_runtime_get_sync(dev); > - if (err < 0) { > - dev_err(dev, "pm_runtime_get_sync failed\n"); > - goto err_pm_disable; > - } > + goto err_pm_put; > > /* Failure to get a link might just be that no cards are inserted */ > hw_init_fn = of_device_get_match_data(dev); > @@ -1174,9 +1161,8 @@ static int rcar_pcie_probe(struct platform_device *pdev) > > err_pm_disable: > pm_runtime_disable(dev); ... shouldn't it be moved down here, for symmetry? > - > -err_free_resource_list: > pci_free_resource_list(&pcie->resources); > + > err_free_bridge: > pci_free_host_bridge(bridge); Gr{oetje,eeting}s, Geert
On 04/09/2018 10:07 AM, Geert Uytterhoeven wrote: > Hi Marek, > > On Sun, Apr 8, 2018 at 3:09 PM, Marek Vasut <marek.vasut@gmail.com> wrote: >> From: Dien Pham <dien.pham.ry@rvc.renesas.com> >> >> The controller clock can be switched off during suspend/resume, >> let runtime PM take care of that. >> >> Signed-off-by: Dien Pham <dien.pham.ry@rvc.renesas.com> >> Signed-off-by: Hien Dang <hien.dang.eb@renesas.com> >> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> >> Cc: Geert Uytterhoeven <geert+renesas@glider.be> >> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> >> Cc: Phil Edworthy <phil.edworthy@renesas.com> >> Cc: Simon Horman <horms+renesas@verge.net.au> >> Cc: Wolfram Sang <wsa@the-dreams.de> >> Cc: linux-renesas-soc@vger.kernel.org >> To: linux-pci@vger.kernel.org >> --- >> V2: - Reorder the fail path in rcar_pcie_probe() to cater for the >> reordering of function calls in probe >> - Dispose of fail_clk in rcar_pcie_get_resources() >> V3: - Fix up the failpath in probe function >> V4: - Rebase on recent linux-next >> V5: - Do not call pci_free_resource_list(&pcie->resources) if >> rcar_pcie_parse_request_of_pci_ranges() fails, since that >> functiona calls pci_free_resource_list() already. > > Thanks for the update! > >> --- a/drivers/pci/host/pcie-rcar.c >> +++ b/drivers/pci/host/pcie-rcar.c > >> @@ -1124,22 +1111,22 @@ static int rcar_pcie_probe(struct platform_device *pdev) >> if (err) >> goto err_free_bridge; >> >> + pm_runtime_enable(pcie->dev); >> + err = pm_runtime_get_sync(pcie->dev); >> + if (err < 0) { >> + dev_err(pcie->dev, "pm_runtime_get_sync failed\n"); >> + goto err_pm_disable; >> + } >> + > > As you moved the pm_runtime setup up... > >> err = rcar_pcie_get_resources(pcie); >> if (err < 0) { >> dev_err(dev, "failed to request resources: %d\n", err); >> - goto err_free_resource_list; >> + goto err_pm_put; >> } >> >> err = rcar_pcie_parse_map_dma_ranges(pcie, dev->of_node); >> if (err) >> - goto err_free_resource_list; >> - >> - pm_runtime_enable(dev); >> - err = pm_runtime_get_sync(dev); >> - if (err < 0) { >> - dev_err(dev, "pm_runtime_get_sync failed\n"); >> - goto err_pm_disable; >> - } >> + goto err_pm_put; >> >> /* Failure to get a link might just be that no cards are inserted */ >> hw_init_fn = of_device_get_match_data(dev); >> @@ -1174,9 +1161,8 @@ static int rcar_pcie_probe(struct platform_device *pdev) >> >> err_pm_disable: >> pm_runtime_disable(dev); > > ... shouldn't it be moved down here, for symmetry? I am reasonably certain the failpath should be correct now. Did I still miss something ?
On Mon, Apr 09, 2018 at 10:20:05AM +0200, Marek Vasut wrote: > On 04/09/2018 10:07 AM, Geert Uytterhoeven wrote: > > Hi Marek, > > > > On Sun, Apr 8, 2018 at 3:09 PM, Marek Vasut <marek.vasut@gmail.com> wrote: > >> From: Dien Pham <dien.pham.ry@rvc.renesas.com> > >> > >> The controller clock can be switched off during suspend/resume, > >> let runtime PM take care of that. > >> > >> Signed-off-by: Dien Pham <dien.pham.ry@rvc.renesas.com> > >> Signed-off-by: Hien Dang <hien.dang.eb@renesas.com> > >> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > >> Cc: Geert Uytterhoeven <geert+renesas@glider.be> > >> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > >> Cc: Phil Edworthy <phil.edworthy@renesas.com> > >> Cc: Simon Horman <horms+renesas@verge.net.au> > >> Cc: Wolfram Sang <wsa@the-dreams.de> > >> Cc: linux-renesas-soc@vger.kernel.org > >> To: linux-pci@vger.kernel.org > >> --- > >> V2: - Reorder the fail path in rcar_pcie_probe() to cater for the > >> reordering of function calls in probe > >> - Dispose of fail_clk in rcar_pcie_get_resources() > >> V3: - Fix up the failpath in probe function > >> V4: - Rebase on recent linux-next > >> V5: - Do not call pci_free_resource_list(&pcie->resources) if > >> rcar_pcie_parse_request_of_pci_ranges() fails, since that > >> functiona calls pci_free_resource_list() already. > > > > Thanks for the update! > > > >> --- a/drivers/pci/host/pcie-rcar.c > >> +++ b/drivers/pci/host/pcie-rcar.c > > > >> @@ -1124,22 +1111,22 @@ static int rcar_pcie_probe(struct platform_device *pdev) > >> if (err) > >> goto err_free_bridge; > >> > >> + pm_runtime_enable(pcie->dev); > >> + err = pm_runtime_get_sync(pcie->dev); > >> + if (err < 0) { > >> + dev_err(pcie->dev, "pm_runtime_get_sync failed\n"); > >> + goto err_pm_disable; > >> + } > >> + > > > > As you moved the pm_runtime setup up... > > > >> err = rcar_pcie_get_resources(pcie); > >> if (err < 0) { > >> dev_err(dev, "failed to request resources: %d\n", err); > >> - goto err_free_resource_list; > >> + goto err_pm_put; > >> } > >> > >> err = rcar_pcie_parse_map_dma_ranges(pcie, dev->of_node); > >> if (err) > >> - goto err_free_resource_list; > >> - > >> - pm_runtime_enable(dev); > >> - err = pm_runtime_get_sync(dev); > >> - if (err < 0) { > >> - dev_err(dev, "pm_runtime_get_sync failed\n"); > >> - goto err_pm_disable; > >> - } > >> + goto err_pm_put; > >> > >> /* Failure to get a link might just be that no cards are inserted */ > >> hw_init_fn = of_device_get_match_data(dev); > >> @@ -1174,9 +1161,8 @@ static int rcar_pcie_probe(struct platform_device *pdev) > >> > >> err_pm_disable: > >> pm_runtime_disable(dev); > > > > ... shouldn't it be moved down here, for symmetry? > > I am reasonably certain the failpath should be correct now. Did I still > miss something ? It looks correct to me too. Geert are Marek and I missing something?
Hi Simon, Marek, On Mon, Apr 9, 2018 at 1:41 PM, Simon Horman <horms@verge.net.au> wrote: > On Mon, Apr 09, 2018 at 10:20:05AM +0200, Marek Vasut wrote: >> On 04/09/2018 10:07 AM, Geert Uytterhoeven wrote: >> > On Sun, Apr 8, 2018 at 3:09 PM, Marek Vasut <marek.vasut@gmail.com> wrote: >> >> From: Dien Pham <dien.pham.ry@rvc.renesas.com> >> >> >> >> The controller clock can be switched off during suspend/resume, >> >> let runtime PM take care of that. >> >> >> >> Signed-off-by: Dien Pham <dien.pham.ry@rvc.renesas.com> >> >> Signed-off-by: Hien Dang <hien.dang.eb@renesas.com> >> >> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> >> >> Cc: Geert Uytterhoeven <geert+renesas@glider.be> >> >> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> >> >> Cc: Phil Edworthy <phil.edworthy@renesas.com> >> >> Cc: Simon Horman <horms+renesas@verge.net.au> >> >> Cc: Wolfram Sang <wsa@the-dreams.de> >> >> Cc: linux-renesas-soc@vger.kernel.org >> >> To: linux-pci@vger.kernel.org >> >> --- >> >> V2: - Reorder the fail path in rcar_pcie_probe() to cater for the >> >> reordering of function calls in probe >> >> - Dispose of fail_clk in rcar_pcie_get_resources() >> >> V3: - Fix up the failpath in probe function >> >> V4: - Rebase on recent linux-next >> >> V5: - Do not call pci_free_resource_list(&pcie->resources) if >> >> rcar_pcie_parse_request_of_pci_ranges() fails, since that >> >> functiona calls pci_free_resource_list() already. >> > >> > Thanks for the update! >> > >> >> --- a/drivers/pci/host/pcie-rcar.c >> >> +++ b/drivers/pci/host/pcie-rcar.c >> > >> >> @@ -1124,22 +1111,22 @@ static int rcar_pcie_probe(struct platform_device *pdev) >> >> if (err) >> >> goto err_free_bridge; >> >> >> >> + pm_runtime_enable(pcie->dev); >> >> + err = pm_runtime_get_sync(pcie->dev); >> >> + if (err < 0) { >> >> + dev_err(pcie->dev, "pm_runtime_get_sync failed\n"); >> >> + goto err_pm_disable; >> >> + } >> >> + >> > >> > As you moved the pm_runtime setup up... >> > >> >> err = rcar_pcie_get_resources(pcie); >> >> if (err < 0) { >> >> dev_err(dev, "failed to request resources: %d\n", err); >> >> - goto err_free_resource_list; >> >> + goto err_pm_put; >> >> } >> >> >> >> err = rcar_pcie_parse_map_dma_ranges(pcie, dev->of_node); >> >> if (err) >> >> - goto err_free_resource_list; >> >> - >> >> - pm_runtime_enable(dev); >> >> - err = pm_runtime_get_sync(dev); >> >> - if (err < 0) { >> >> - dev_err(dev, "pm_runtime_get_sync failed\n"); >> >> - goto err_pm_disable; >> >> - } >> >> + goto err_pm_put; >> >> >> >> /* Failure to get a link might just be that no cards are inserted */ >> >> hw_init_fn = of_device_get_match_data(dev); >> >> @@ -1174,9 +1161,8 @@ static int rcar_pcie_probe(struct platform_device *pdev) >> >> >> >> err_pm_disable: >> >> pm_runtime_disable(dev); >> > >> > ... shouldn't it be moved down here, for symmetry? >> >> I am reasonably certain the failpath should be correct now. Did I still >> miss something ? > > It looks correct to me too. Geert are Marek and I missing something? Probably it will still work fine, but after this patch, Runtime PM is enabled early, and disabled early, which is not symmetrical. I like symmetry ;-) Gr{oetje,eeting}s, Geert
On Mon, Apr 09, 2018 at 01:47:38PM +0200, Geert Uytterhoeven wrote: > Hi Simon, Marek, > > On Mon, Apr 9, 2018 at 1:41 PM, Simon Horman <horms@verge.net.au> wrote: > > On Mon, Apr 09, 2018 at 10:20:05AM +0200, Marek Vasut wrote: > >> On 04/09/2018 10:07 AM, Geert Uytterhoeven wrote: > >> > On Sun, Apr 8, 2018 at 3:09 PM, Marek Vasut <marek.vasut@gmail.com> wrote: > >> >> From: Dien Pham <dien.pham.ry@rvc.renesas.com> > >> >> > >> >> The controller clock can be switched off during suspend/resume, > >> >> let runtime PM take care of that. > >> >> > >> >> Signed-off-by: Dien Pham <dien.pham.ry@rvc.renesas.com> > >> >> Signed-off-by: Hien Dang <hien.dang.eb@renesas.com> > >> >> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > >> >> Cc: Geert Uytterhoeven <geert+renesas@glider.be> > >> >> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > >> >> Cc: Phil Edworthy <phil.edworthy@renesas.com> > >> >> Cc: Simon Horman <horms+renesas@verge.net.au> > >> >> Cc: Wolfram Sang <wsa@the-dreams.de> > >> >> Cc: linux-renesas-soc@vger.kernel.org > >> >> To: linux-pci@vger.kernel.org > >> >> --- > >> >> V2: - Reorder the fail path in rcar_pcie_probe() to cater for the > >> >> reordering of function calls in probe > >> >> - Dispose of fail_clk in rcar_pcie_get_resources() > >> >> V3: - Fix up the failpath in probe function > >> >> V4: - Rebase on recent linux-next > >> >> V5: - Do not call pci_free_resource_list(&pcie->resources) if > >> >> rcar_pcie_parse_request_of_pci_ranges() fails, since that > >> >> functiona calls pci_free_resource_list() already. > >> > > >> > Thanks for the update! > >> > > >> >> --- a/drivers/pci/host/pcie-rcar.c > >> >> +++ b/drivers/pci/host/pcie-rcar.c > >> > > >> >> @@ -1124,22 +1111,22 @@ static int rcar_pcie_probe(struct platform_device *pdev) > >> >> if (err) > >> >> goto err_free_bridge; > >> >> > >> >> + pm_runtime_enable(pcie->dev); > >> >> + err = pm_runtime_get_sync(pcie->dev); > >> >> + if (err < 0) { > >> >> + dev_err(pcie->dev, "pm_runtime_get_sync failed\n"); > >> >> + goto err_pm_disable; > >> >> + } > >> >> + > >> > > >> > As you moved the pm_runtime setup up... > >> > > >> >> err = rcar_pcie_get_resources(pcie); > >> >> if (err < 0) { > >> >> dev_err(dev, "failed to request resources: %d\n", err); > >> >> - goto err_free_resource_list; > >> >> + goto err_pm_put; > >> >> } > >> >> > >> >> err = rcar_pcie_parse_map_dma_ranges(pcie, dev->of_node); > >> >> if (err) > >> >> - goto err_free_resource_list; > >> >> - > >> >> - pm_runtime_enable(dev); > >> >> - err = pm_runtime_get_sync(dev); > >> >> - if (err < 0) { > >> >> - dev_err(dev, "pm_runtime_get_sync failed\n"); > >> >> - goto err_pm_disable; > >> >> - } > >> >> + goto err_pm_put; > >> >> > >> >> /* Failure to get a link might just be that no cards are inserted */ > >> >> hw_init_fn = of_device_get_match_data(dev); > >> >> @@ -1174,9 +1161,8 @@ static int rcar_pcie_probe(struct platform_device *pdev) > >> >> > >> >> err_pm_disable: > >> >> pm_runtime_disable(dev); > >> > > >> > ... shouldn't it be moved down here, for symmetry? > >> > >> I am reasonably certain the failpath should be correct now. Did I still > >> miss something ? > > > > It looks correct to me too. Geert are Marek and I missing something? > > Probably it will still work fine, but after this patch, Runtime PM is enabled > early, and disabled early, which is not symmetrical. > > I like symmetry ;-) Understood. I think that is reasonable. Marek, would you care to respin?
On 04/09/2018 02:26 PM, Simon Horman wrote: > On Mon, Apr 09, 2018 at 01:47:38PM +0200, Geert Uytterhoeven wrote: >> Hi Simon, Marek, >> >> On Mon, Apr 9, 2018 at 1:41 PM, Simon Horman <horms@verge.net.au> wrote: >>> On Mon, Apr 09, 2018 at 10:20:05AM +0200, Marek Vasut wrote: >>>> On 04/09/2018 10:07 AM, Geert Uytterhoeven wrote: >>>>> On Sun, Apr 8, 2018 at 3:09 PM, Marek Vasut <marek.vasut@gmail.com> wrote: >>>>>> From: Dien Pham <dien.pham.ry@rvc.renesas.com> >>>>>> >>>>>> The controller clock can be switched off during suspend/resume, >>>>>> let runtime PM take care of that. >>>>>> >>>>>> Signed-off-by: Dien Pham <dien.pham.ry@rvc.renesas.com> >>>>>> Signed-off-by: Hien Dang <hien.dang.eb@renesas.com> >>>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> >>>>>> Cc: Geert Uytterhoeven <geert+renesas@glider.be> >>>>>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> >>>>>> Cc: Phil Edworthy <phil.edworthy@renesas.com> >>>>>> Cc: Simon Horman <horms+renesas@verge.net.au> >>>>>> Cc: Wolfram Sang <wsa@the-dreams.de> >>>>>> Cc: linux-renesas-soc@vger.kernel.org >>>>>> To: linux-pci@vger.kernel.org >>>>>> --- >>>>>> V2: - Reorder the fail path in rcar_pcie_probe() to cater for the >>>>>> reordering of function calls in probe >>>>>> - Dispose of fail_clk in rcar_pcie_get_resources() >>>>>> V3: - Fix up the failpath in probe function >>>>>> V4: - Rebase on recent linux-next >>>>>> V5: - Do not call pci_free_resource_list(&pcie->resources) if >>>>>> rcar_pcie_parse_request_of_pci_ranges() fails, since that >>>>>> functiona calls pci_free_resource_list() already. >>>>> >>>>> Thanks for the update! >>>>> >>>>>> --- a/drivers/pci/host/pcie-rcar.c >>>>>> +++ b/drivers/pci/host/pcie-rcar.c >>>>> >>>>>> @@ -1124,22 +1111,22 @@ static int rcar_pcie_probe(struct platform_device *pdev) >>>>>> if (err) >>>>>> goto err_free_bridge; >>>>>> >>>>>> + pm_runtime_enable(pcie->dev); >>>>>> + err = pm_runtime_get_sync(pcie->dev); >>>>>> + if (err < 0) { >>>>>> + dev_err(pcie->dev, "pm_runtime_get_sync failed\n"); >>>>>> + goto err_pm_disable; >>>>>> + } >>>>>> + >>>>> >>>>> As you moved the pm_runtime setup up... >>>>> >>>>>> err = rcar_pcie_get_resources(pcie); >>>>>> if (err < 0) { >>>>>> dev_err(dev, "failed to request resources: %d\n", err); >>>>>> - goto err_free_resource_list; >>>>>> + goto err_pm_put; >>>>>> } >>>>>> >>>>>> err = rcar_pcie_parse_map_dma_ranges(pcie, dev->of_node); >>>>>> if (err) >>>>>> - goto err_free_resource_list; >>>>>> - >>>>>> - pm_runtime_enable(dev); >>>>>> - err = pm_runtime_get_sync(dev); >>>>>> - if (err < 0) { >>>>>> - dev_err(dev, "pm_runtime_get_sync failed\n"); >>>>>> - goto err_pm_disable; >>>>>> - } >>>>>> + goto err_pm_put; >>>>>> >>>>>> /* Failure to get a link might just be that no cards are inserted */ >>>>>> hw_init_fn = of_device_get_match_data(dev); >>>>>> @@ -1174,9 +1161,8 @@ static int rcar_pcie_probe(struct platform_device *pdev) >>>>>> >>>>>> err_pm_disable: >>>>>> pm_runtime_disable(dev); >>>>> >>>>> ... shouldn't it be moved down here, for symmetry? >>>> >>>> I am reasonably certain the failpath should be correct now. Did I still >>>> miss something ? >>> >>> It looks correct to me too. Geert are Marek and I missing something? >> >> Probably it will still work fine, but after this patch, Runtime PM is enabled >> early, and disabled early, which is not symmetrical. >> >> I like symmetry ;-) > > Understood. I think that is reasonable. > Marek, would you care to respin? I am looking into the driver, but I fail to see what Geert is trying to make me change here. The pairing looks as follows: .- rcar_pcie_parse_request_of_pci_ranges() | (pm_runtime_enable is here) | .- pm_runtime_get_sync() | | .- rcar_pcie_get_resources() | | | | | '- pm_runtime_put() | '- pm_runtime_disable() + pci_free_resource_list() '- pci_free_host_bridge() It looks symmetric to me ...
Hi Marek, On Tue, Apr 10, 2018 at 4:31 PM, Marek Vasut <marek.vasut@gmail.com> wrote: > On 04/09/2018 02:26 PM, Simon Horman wrote: >> On Mon, Apr 09, 2018 at 01:47:38PM +0200, Geert Uytterhoeven wrote: >>> On Mon, Apr 9, 2018 at 1:41 PM, Simon Horman <horms@verge.net.au> wrote: >>>> On Mon, Apr 09, 2018 at 10:20:05AM +0200, Marek Vasut wrote: >>>>> On 04/09/2018 10:07 AM, Geert Uytterhoeven wrote: >>>>>> On Sun, Apr 8, 2018 at 3:09 PM, Marek Vasut <marek.vasut@gmail.com> wrote: >>>>>>> From: Dien Pham <dien.pham.ry@rvc.renesas.com> >>>>>>> >>>>>>> The controller clock can be switched off during suspend/resume, >>>>>>> let runtime PM take care of that. >>>>>>> >>>>>>> Signed-off-by: Dien Pham <dien.pham.ry@rvc.renesas.com> >>>>>>> Signed-off-by: Hien Dang <hien.dang.eb@renesas.com> >>>>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> >>>>>>> Cc: Geert Uytterhoeven <geert+renesas@glider.be> >>>>>>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> >>>>>>> Cc: Phil Edworthy <phil.edworthy@renesas.com> >>>>>>> Cc: Simon Horman <horms+renesas@verge.net.au> >>>>>>> Cc: Wolfram Sang <wsa@the-dreams.de> >>>>>>> Cc: linux-renesas-soc@vger.kernel.org >>>>>>> To: linux-pci@vger.kernel.org >>>>>>> --- >>>>>>> V2: - Reorder the fail path in rcar_pcie_probe() to cater for the >>>>>>> reordering of function calls in probe >>>>>>> - Dispose of fail_clk in rcar_pcie_get_resources() >>>>>>> V3: - Fix up the failpath in probe function >>>>>>> V4: - Rebase on recent linux-next >>>>>>> V5: - Do not call pci_free_resource_list(&pcie->resources) if >>>>>>> rcar_pcie_parse_request_of_pci_ranges() fails, since that >>>>>>> functiona calls pci_free_resource_list() already. >>>>>> >>>>>> Thanks for the update! >>>>>> >>>>>>> --- a/drivers/pci/host/pcie-rcar.c >>>>>>> +++ b/drivers/pci/host/pcie-rcar.c >>>>>> >>>>>>> @@ -1124,22 +1111,22 @@ static int rcar_pcie_probe(struct platform_device *pdev) >>>>>>> if (err) >>>>>>> goto err_free_bridge; >>>>>>> >>>>>>> + pm_runtime_enable(pcie->dev); >>>>>>> + err = pm_runtime_get_sync(pcie->dev); >>>>>>> + if (err < 0) { >>>>>>> + dev_err(pcie->dev, "pm_runtime_get_sync failed\n"); >>>>>>> + goto err_pm_disable; >>>>>>> + } >>>>>>> + >>>>>> >>>>>> As you moved the pm_runtime setup up... >>>>>> >>>>>>> err = rcar_pcie_get_resources(pcie); >>>>>>> if (err < 0) { >>>>>>> dev_err(dev, "failed to request resources: %d\n", err); >>>>>>> - goto err_free_resource_list; >>>>>>> + goto err_pm_put; >>>>>>> } >>>>>>> >>>>>>> err = rcar_pcie_parse_map_dma_ranges(pcie, dev->of_node); >>>>>>> if (err) >>>>>>> - goto err_free_resource_list; >>>>>>> - >>>>>>> - pm_runtime_enable(dev); >>>>>>> - err = pm_runtime_get_sync(dev); >>>>>>> - if (err < 0) { >>>>>>> - dev_err(dev, "pm_runtime_get_sync failed\n"); >>>>>>> - goto err_pm_disable; >>>>>>> - } >>>>>>> + goto err_pm_put; >>>>>>> >>>>>>> /* Failure to get a link might just be that no cards are inserted */ >>>>>>> hw_init_fn = of_device_get_match_data(dev); >>>>>>> @@ -1174,9 +1161,8 @@ static int rcar_pcie_probe(struct platform_device *pdev) >>>>>>> >>>>>>> err_pm_disable: >>>>>>> pm_runtime_disable(dev); >>>>>> >>>>>> ... shouldn't it be moved down here, for symmetry? >>>>> >>>>> I am reasonably certain the failpath should be correct now. Did I still >>>>> miss something ? >>>> >>>> It looks correct to me too. Geert are Marek and I missing something? >>> >>> Probably it will still work fine, but after this patch, Runtime PM is enabled >>> early, and disabled early, which is not symmetrical. >>> >>> I like symmetry ;-) >> >> Understood. I think that is reasonable. >> Marek, would you care to respin? > > I am looking into the driver, but I fail to see what Geert is trying to > make me change here. > > The pairing looks as follows: > > .- rcar_pcie_parse_request_of_pci_ranges() > | (pm_runtime_enable is here) > | .- pm_runtime_get_sync() > | | .- rcar_pcie_get_resources() rcar_pcie_get_resources() is called while the device is runtime-enabled/resumed > | | | > | | '- pm_runtime_put() > | '- pm_runtime_disable() + pci_free_resource_list() pci_free_resource_list() is called while the device is runtime-disabled. > '- pci_free_host_bridge() > > It looks symmetric to me ... rcar_pcie_get_resources() is called while the device is runtime-enabled/resumed, pci_free_resource_list() is called while the device is runtime-disabled. Gr{oetje,eeting}s, Geert
On 04/10/2018 04:42 PM, Geert Uytterhoeven wrote: > Hi Marek, > > On Tue, Apr 10, 2018 at 4:31 PM, Marek Vasut <marek.vasut@gmail.com> wrote: >> On 04/09/2018 02:26 PM, Simon Horman wrote: >>> On Mon, Apr 09, 2018 at 01:47:38PM +0200, Geert Uytterhoeven wrote: >>>> On Mon, Apr 9, 2018 at 1:41 PM, Simon Horman <horms@verge.net.au> wrote: >>>>> On Mon, Apr 09, 2018 at 10:20:05AM +0200, Marek Vasut wrote: >>>>>> On 04/09/2018 10:07 AM, Geert Uytterhoeven wrote: >>>>>>> On Sun, Apr 8, 2018 at 3:09 PM, Marek Vasut <marek.vasut@gmail.com> wrote: >>>>>>>> From: Dien Pham <dien.pham.ry@rvc.renesas.com> >>>>>>>> >>>>>>>> The controller clock can be switched off during suspend/resume, >>>>>>>> let runtime PM take care of that. >>>>>>>> >>>>>>>> Signed-off-by: Dien Pham <dien.pham.ry@rvc.renesas.com> >>>>>>>> Signed-off-by: Hien Dang <hien.dang.eb@renesas.com> >>>>>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> >>>>>>>> Cc: Geert Uytterhoeven <geert+renesas@glider.be> >>>>>>>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> >>>>>>>> Cc: Phil Edworthy <phil.edworthy@renesas.com> >>>>>>>> Cc: Simon Horman <horms+renesas@verge.net.au> >>>>>>>> Cc: Wolfram Sang <wsa@the-dreams.de> >>>>>>>> Cc: linux-renesas-soc@vger.kernel.org >>>>>>>> To: linux-pci@vger.kernel.org >>>>>>>> --- >>>>>>>> V2: - Reorder the fail path in rcar_pcie_probe() to cater for the >>>>>>>> reordering of function calls in probe >>>>>>>> - Dispose of fail_clk in rcar_pcie_get_resources() >>>>>>>> V3: - Fix up the failpath in probe function >>>>>>>> V4: - Rebase on recent linux-next >>>>>>>> V5: - Do not call pci_free_resource_list(&pcie->resources) if >>>>>>>> rcar_pcie_parse_request_of_pci_ranges() fails, since that >>>>>>>> functiona calls pci_free_resource_list() already. >>>>>>> >>>>>>> Thanks for the update! >>>>>>> >>>>>>>> --- a/drivers/pci/host/pcie-rcar.c >>>>>>>> +++ b/drivers/pci/host/pcie-rcar.c >>>>>>> >>>>>>>> @@ -1124,22 +1111,22 @@ static int rcar_pcie_probe(struct platform_device *pdev) >>>>>>>> if (err) >>>>>>>> goto err_free_bridge; >>>>>>>> >>>>>>>> + pm_runtime_enable(pcie->dev); >>>>>>>> + err = pm_runtime_get_sync(pcie->dev); >>>>>>>> + if (err < 0) { >>>>>>>> + dev_err(pcie->dev, "pm_runtime_get_sync failed\n"); >>>>>>>> + goto err_pm_disable; >>>>>>>> + } >>>>>>>> + >>>>>>> >>>>>>> As you moved the pm_runtime setup up... >>>>>>> >>>>>>>> err = rcar_pcie_get_resources(pcie); >>>>>>>> if (err < 0) { >>>>>>>> dev_err(dev, "failed to request resources: %d\n", err); >>>>>>>> - goto err_free_resource_list; >>>>>>>> + goto err_pm_put; >>>>>>>> } >>>>>>>> >>>>>>>> err = rcar_pcie_parse_map_dma_ranges(pcie, dev->of_node); >>>>>>>> if (err) >>>>>>>> - goto err_free_resource_list; >>>>>>>> - >>>>>>>> - pm_runtime_enable(dev); >>>>>>>> - err = pm_runtime_get_sync(dev); >>>>>>>> - if (err < 0) { >>>>>>>> - dev_err(dev, "pm_runtime_get_sync failed\n"); >>>>>>>> - goto err_pm_disable; >>>>>>>> - } >>>>>>>> + goto err_pm_put; >>>>>>>> >>>>>>>> /* Failure to get a link might just be that no cards are inserted */ >>>>>>>> hw_init_fn = of_device_get_match_data(dev); >>>>>>>> @@ -1174,9 +1161,8 @@ static int rcar_pcie_probe(struct platform_device *pdev) >>>>>>>> >>>>>>>> err_pm_disable: >>>>>>>> pm_runtime_disable(dev); >>>>>>> >>>>>>> ... shouldn't it be moved down here, for symmetry? >>>>>> >>>>>> I am reasonably certain the failpath should be correct now. Did I still >>>>>> miss something ? >>>>> >>>>> It looks correct to me too. Geert are Marek and I missing something? >>>> >>>> Probably it will still work fine, but after this patch, Runtime PM is enabled >>>> early, and disabled early, which is not symmetrical. >>>> >>>> I like symmetry ;-) >>> >>> Understood. I think that is reasonable. >>> Marek, would you care to respin? >> >> I am looking into the driver, but I fail to see what Geert is trying to >> make me change here. >> >> The pairing looks as follows: >> >> .- rcar_pcie_parse_request_of_pci_ranges() >> | (pm_runtime_enable is here) >> | .- pm_runtime_get_sync() >> | | .- rcar_pcie_get_resources() > > rcar_pcie_get_resources() is called while the device is runtime-enabled/resumed Because something may access the device, yes. >> | | | >> | | '- pm_runtime_put() >> | '- pm_runtime_disable() + pci_free_resource_list() > > pci_free_resource_list() is called while the device is runtime-disabled. Because nothing will access the device. >> '- pci_free_host_bridge() >> >> It looks symmetric to me ... > > rcar_pcie_get_resources() is called while the device is > runtime-enabled/resumed, > pci_free_resource_list() is called while the device is runtime-disabled. At this point, I think I'd rather see a diff of changes which you have in mind rather than this endless discussion. Can you provide one against this patch ?
Hi Marek, On Tue, Apr 10, 2018 at 5:25 PM, Marek Vasut <marek.vasut@gmail.com> wrote: > On 04/10/2018 04:42 PM, Geert Uytterhoeven wrote: >> On Tue, Apr 10, 2018 at 4:31 PM, Marek Vasut <marek.vasut@gmail.com> wrote: >>> On 04/09/2018 02:26 PM, Simon Horman wrote: >>>> On Mon, Apr 09, 2018 at 01:47:38PM +0200, Geert Uytterhoeven wrote: >>>>> On Mon, Apr 9, 2018 at 1:41 PM, Simon Horman <horms@verge.net.au> wrote: >>>>>> On Mon, Apr 09, 2018 at 10:20:05AM +0200, Marek Vasut wrote: >>>>>>> On 04/09/2018 10:07 AM, Geert Uytterhoeven wrote: >>>>>>>> On Sun, Apr 8, 2018 at 3:09 PM, Marek Vasut <marek.vasut@gmail.com> wrote: >>>>>>>>> From: Dien Pham <dien.pham.ry@rvc.renesas.com> >>>>>>>>> >>>>>>>>> The controller clock can be switched off during suspend/resume, >>>>>>>>> let runtime PM take care of that. >>>>>>>>> >>>>>>>>> Signed-off-by: Dien Pham <dien.pham.ry@rvc.renesas.com> >>>>>>>>> Signed-off-by: Hien Dang <hien.dang.eb@renesas.com> >>>>>>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> >>>>>>>>> Cc: Geert Uytterhoeven <geert+renesas@glider.be> >>>>>>>>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> >>>>>>>>> Cc: Phil Edworthy <phil.edworthy@renesas.com> >>>>>>>>> Cc: Simon Horman <horms+renesas@verge.net.au> >>>>>>>>> Cc: Wolfram Sang <wsa@the-dreams.de> >>>>>>>>> Cc: linux-renesas-soc@vger.kernel.org >>>>>>>>> To: linux-pci@vger.kernel.org >>>>>>>>> --- >>>>>>>>> V2: - Reorder the fail path in rcar_pcie_probe() to cater for the >>>>>>>>> reordering of function calls in probe >>>>>>>>> - Dispose of fail_clk in rcar_pcie_get_resources() >>>>>>>>> V3: - Fix up the failpath in probe function >>>>>>>>> V4: - Rebase on recent linux-next >>>>>>>>> V5: - Do not call pci_free_resource_list(&pcie->resources) if >>>>>>>>> rcar_pcie_parse_request_of_pci_ranges() fails, since that >>>>>>>>> functiona calls pci_free_resource_list() already. >>>>>>>> >>>>>>>> Thanks for the update! >>>>>>>> >>>>>>>>> --- a/drivers/pci/host/pcie-rcar.c >>>>>>>>> +++ b/drivers/pci/host/pcie-rcar.c >>>>>>>> >>>>>>>>> @@ -1124,22 +1111,22 @@ static int rcar_pcie_probe(struct platform_device *pdev) >>>>>>>>> if (err) >>>>>>>>> goto err_free_bridge; >>>>>>>>> >>>>>>>>> + pm_runtime_enable(pcie->dev); >>>>>>>>> + err = pm_runtime_get_sync(pcie->dev); >>>>>>>>> + if (err < 0) { >>>>>>>>> + dev_err(pcie->dev, "pm_runtime_get_sync failed\n"); >>>>>>>>> + goto err_pm_disable; >>>>>>>>> + } >>>>>>>>> + >>>>>>>> >>>>>>>> As you moved the pm_runtime setup up... >>>>>>>> >>>>>>>>> err = rcar_pcie_get_resources(pcie); >>>>>>>>> if (err < 0) { >>>>>>>>> dev_err(dev, "failed to request resources: %d\n", err); >>>>>>>>> - goto err_free_resource_list; >>>>>>>>> + goto err_pm_put; >>>>>>>>> } >>>>>>>>> >>>>>>>>> err = rcar_pcie_parse_map_dma_ranges(pcie, dev->of_node); >>>>>>>>> if (err) >>>>>>>>> - goto err_free_resource_list; >>>>>>>>> - >>>>>>>>> - pm_runtime_enable(dev); >>>>>>>>> - err = pm_runtime_get_sync(dev); >>>>>>>>> - if (err < 0) { >>>>>>>>> - dev_err(dev, "pm_runtime_get_sync failed\n"); >>>>>>>>> - goto err_pm_disable; >>>>>>>>> - } >>>>>>>>> + goto err_pm_put; >>>>>>>>> >>>>>>>>> /* Failure to get a link might just be that no cards are inserted */ >>>>>>>>> hw_init_fn = of_device_get_match_data(dev); >>>>>>>>> @@ -1174,9 +1161,8 @@ static int rcar_pcie_probe(struct platform_device *pdev) >>>>>>>>> >>>>>>>>> err_pm_disable: >>>>>>>>> pm_runtime_disable(dev); >>>>>>>> >>>>>>>> ... shouldn't it be moved down here, for symmetry? >>>>>>> >>>>>>> I am reasonably certain the failpath should be correct now. Did I still >>>>>>> miss something ? >>>>>> >>>>>> It looks correct to me too. Geert are Marek and I missing something? >>>>> >>>>> Probably it will still work fine, but after this patch, Runtime PM is enabled >>>>> early, and disabled early, which is not symmetrical. >>>>> >>>>> I like symmetry ;-) >>>> >>>> Understood. I think that is reasonable. >>>> Marek, would you care to respin? >>> >>> I am looking into the driver, but I fail to see what Geert is trying to >>> make me change here. >>> >>> The pairing looks as follows: >>> >>> .- rcar_pcie_parse_request_of_pci_ranges() >>> | (pm_runtime_enable is here) >>> | .- pm_runtime_get_sync() >>> | | .- rcar_pcie_get_resources() >> >> rcar_pcie_get_resources() is called while the device is runtime-enabled/resumed > > Because something may access the device, yes. > >>> | | | >>> | | '- pm_runtime_put() >>> | '- pm_runtime_disable() + pci_free_resource_list() >> >> pci_free_resource_list() is called while the device is runtime-disabled. > > Because nothing will access the device. > >>> '- pci_free_host_bridge() >>> >>> It looks symmetric to me ... >> >> rcar_pcie_get_resources() is called while the device is >> runtime-enabled/resumed, >> pci_free_resource_list() is called while the device is runtime-disabled. > > At this point, I think I'd rather see a diff of changes which you have > in mind rather than this endless discussion. Can you provide one against > this patch ? My final comment: If the steps during probing are A..Z, cleanup and removal should undo them in reverse order (Z..A), unless there's a very good reason not to do so. Gr{oetje,eeting}s, Geert
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.
On Tue, Apr 10, 2018 at 06:17:04PM +0200, Marek Vasut wrote: > On 04/10/2018 05:28 PM, Geert Uytterhoeven wrote: ... > >>> rcar_pcie_get_resources() is called while the device is > >>> runtime-enabled/resumed, > >>> pci_free_resource_list() is called while the device is runtime-disabled. > > rcar_pcie_get_resources() is NOT a pair function for > pci_free_resource_list() . rcar_pcie_parse_request_of_pci_ranges() is a > pair function for pci_free_resource_list(). > > rcar_pcie_parse_request_of_pci_ranges() calls > of_pci_get_host_bridge_resources() internally, so every single function > called after successful call of rcar_pcie_parse_request_of_pci_ranges() > must call pci_free_resource_list(). > > Both of_pci_get_host_bridge_resources() and pci_free_resource_list() are > called with runtime PM disabled. > > The naming of the functions is confusing though. Hi, thanks everyone for their efforts in preparing/reviewing this patch. It seems there are some differences of opinion on how best to handle the error paths but unlike earlier versions this one seems correct to me. If that turns out to be false we can address it. But I don't think its likely things will be enhanced by continuing this review. Lorenzo, please consider taking this patch in its current form. Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
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
Hi Marek, On Tue, Apr 10, 2018 at 6:17 PM, Marek Vasut <marek.vasut@gmail.com> wrote: > On 04/10/2018 05:28 PM, Geert Uytterhoeven wrote: >>>>> The pairing looks as follows: >>>>> >>>>> .- rcar_pcie_parse_request_of_pci_ranges() >>>>> | (pm_runtime_enable is here) >>>>> | .- pm_runtime_get_sync() >>>>> | | .- rcar_pcie_get_resources() >>>> >>>> rcar_pcie_get_resources() is called while the device is runtime-enabled/resumed >>> >>> Because something may access the device, yes. >>> >>>>> | | | >>>>> | | '- pm_runtime_put() >>>>> | '- pm_runtime_disable() + pci_free_resource_list() >>>> >>>> pci_free_resource_list() is called while the device is runtime-disabled. >>> >>> Because nothing will access the device. >>> >>>>> '- pci_free_host_bridge() >>>>> >>>>> It looks symmetric to me ... >>>> >>>> rcar_pcie_get_resources() is called while the device is >>>> runtime-enabled/resumed, >>>> pci_free_resource_list() is called while the device is runtime-disabled. >>> >>> At this point, I think I'd rather see a diff of changes which you have >>> in mind rather than this endless discussion. Can you provide one against >>> this patch ? >> >> My final comment: >> >> If the steps during probing are A..Z, cleanup and removal should undo them >> in reverse order (Z..A), unless there's a very good reason not to do so. > > I spent extra time going through the probe function and I just don't see > how it is not done in the exact reverse, I checked every single goto > statement in probe. > > I noticed this though: > >>>> rcar_pcie_get_resources() is called while the device is >>>> runtime-enabled/resumed, >>>> pci_free_resource_list() is called while the device is runtime-disabled. > > rcar_pcie_get_resources() is NOT a pair function for > pci_free_resource_list() . rcar_pcie_parse_request_of_pci_ranges() is a > pair function for pci_free_resource_list(). > > rcar_pcie_parse_request_of_pci_ranges() calls > of_pci_get_host_bridge_resources() internally, so every single function > called after successful call of rcar_pcie_parse_request_of_pci_ranges() > must call pci_free_resource_list(). > > Both of_pci_get_host_bridge_resources() and pci_free_resource_list() are > called with runtime PM disabled. > > The naming of the functions is confusing though. You are right, your changes are correct, and the naming of these functions is confusing. Perhaps it should be changed, to avoid misleading the (not so) casual reviewer? Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> BTW, while diving deeper, I noticed a few other pre-existing issues in error handling: 1. If anything fails after rcar_pcie_get_resources(), the bus clock is never disabled, 2. The error path of rcar_pcie_enable_msi() does not call irq_dispose_mapping() before irq_domain_remove(), 3. If rcar_pcie_enable() fails, none of the setup done in rcar_pcie_enable_msi() is reverted. Apart from the IRQ domain handling in 2, that includes freeing msi->pages (should this be allocated using the DMA API?), and undoing the related HW setup, to prevent the HW from scribbling the former MSI page in the future. Care to fix these, too? Thanks! Gr{oetje,eeting}s, Geert
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 <horms+renesas@verge.net.au> Applied to pci/rcar for v4.18, thanks. Lorenzo
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 <horms+renesas@verge.net.au> > > Applied to pci/rcar for v4.18, thanks. Is there any reason why this patch isnt in next yet ?
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 <horms+renesas@verge.net.au> > > > > 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
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 <horms+renesas@verge.net.au> >>> >>> 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 ?
On 04/19/2018 12:00 PM, Geert Uytterhoeven wrote: > Hi Marek, > > On Tue, Apr 10, 2018 at 6:17 PM, Marek Vasut <marek.vasut@gmail.com> wrote: >> On 04/10/2018 05:28 PM, Geert Uytterhoeven wrote: >>>>>> The pairing looks as follows: >>>>>> >>>>>> .- rcar_pcie_parse_request_of_pci_ranges() >>>>>> | (pm_runtime_enable is here) >>>>>> | .- pm_runtime_get_sync() >>>>>> | | .- rcar_pcie_get_resources() >>>>> >>>>> rcar_pcie_get_resources() is called while the device is runtime-enabled/resumed >>>> >>>> Because something may access the device, yes. >>>> >>>>>> | | | >>>>>> | | '- pm_runtime_put() >>>>>> | '- pm_runtime_disable() + pci_free_resource_list() >>>>> >>>>> pci_free_resource_list() is called while the device is runtime-disabled. >>>> >>>> Because nothing will access the device. >>>> >>>>>> '- pci_free_host_bridge() >>>>>> >>>>>> It looks symmetric to me ... >>>>> >>>>> rcar_pcie_get_resources() is called while the device is >>>>> runtime-enabled/resumed, >>>>> pci_free_resource_list() is called while the device is runtime-disabled. >>>> >>>> At this point, I think I'd rather see a diff of changes which you have >>>> in mind rather than this endless discussion. Can you provide one against >>>> this patch ? >>> >>> My final comment: >>> >>> If the steps during probing are A..Z, cleanup and removal should undo them >>> in reverse order (Z..A), unless there's a very good reason not to do so. >> >> I spent extra time going through the probe function and I just don't see >> how it is not done in the exact reverse, I checked every single goto >> statement in probe. >> >> I noticed this though: >> >>>>> rcar_pcie_get_resources() is called while the device is >>>>> runtime-enabled/resumed, >>>>> pci_free_resource_list() is called while the device is runtime-disabled. >> >> rcar_pcie_get_resources() is NOT a pair function for >> pci_free_resource_list() . rcar_pcie_parse_request_of_pci_ranges() is a >> pair function for pci_free_resource_list(). >> >> rcar_pcie_parse_request_of_pci_ranges() calls >> of_pci_get_host_bridge_resources() internally, so every single function >> called after successful call of rcar_pcie_parse_request_of_pci_ranges() >> must call pci_free_resource_list(). >> >> Both of_pci_get_host_bridge_resources() and pci_free_resource_list() are >> called with runtime PM disabled. >> >> The naming of the functions is confusing though. > > You are right, your changes are correct, and the naming of these functions > is confusing. Perhaps it should be changed, to avoid misleading the (not so) > casual reviewer? > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > BTW, while diving deeper, I noticed a few other pre-existing issues in error > handling: > > 1. If anything fails after rcar_pcie_get_resources(), the bus clock is never > disabled, > 2. The error path of rcar_pcie_enable_msi() does not call > irq_dispose_mapping() before irq_domain_remove(), > 3. If rcar_pcie_enable() fails, none of the setup done in > rcar_pcie_enable_msi() is reverted. > Apart from the IRQ domain handling in 2, that includes freeing msi->pages > (should this be allocated using the DMA API?), and undoing the related HW > setup, to prevent the HW from scribbling the former MSI page in the future. > > Care to fix these, too? Yes, patchset is coming.
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 <horms+renesas@verge.net.au> > >>> > >>> 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&m=152654084627146&w=2 Lorenzo
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 <horms+renesas@verge.net.au> >>>>> >>>>> 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&m=152654084627146&w=2 Cool, thanks, I have a few more fixes coming.
diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c index 6ab28f29ac6a..25f68305322c 100644 --- a/drivers/pci/host/pcie-rcar.c +++ b/drivers/pci/host/pcie-rcar.c @@ -142,7 +142,6 @@ struct rcar_pcie { void __iomem *base; struct list_head resources; int root_bus_nr; - struct clk *clk; struct clk *bus_clk; struct rcar_msi msi; }; @@ -914,24 +913,14 @@ static int rcar_pcie_get_resources(struct rcar_pcie *pcie) if (IS_ERR(pcie->base)) return PTR_ERR(pcie->base); - pcie->clk = devm_clk_get(dev, "pcie"); - if (IS_ERR(pcie->clk)) { - dev_err(dev, "cannot get platform clock\n"); - return PTR_ERR(pcie->clk); - } - err = clk_prepare_enable(pcie->clk); - if (err) - return err; - pcie->bus_clk = devm_clk_get(dev, "pcie_bus"); if (IS_ERR(pcie->bus_clk)) { dev_err(dev, "cannot get pcie bus clock\n"); - err = PTR_ERR(pcie->bus_clk); - goto fail_clk; + return PTR_ERR(pcie->bus_clk); } err = clk_prepare_enable(pcie->bus_clk); if (err) - goto fail_clk; + return err; i = irq_of_parse_and_map(dev->of_node, 0); if (!i) { @@ -953,8 +942,6 @@ static int rcar_pcie_get_resources(struct rcar_pcie *pcie) err_map_reg: clk_disable_unprepare(pcie->bus_clk); -fail_clk: - clk_disable_unprepare(pcie->clk); return err; } @@ -1124,22 +1111,22 @@ static int rcar_pcie_probe(struct platform_device *pdev) if (err) goto err_free_bridge; + pm_runtime_enable(pcie->dev); + err = pm_runtime_get_sync(pcie->dev); + if (err < 0) { + dev_err(pcie->dev, "pm_runtime_get_sync failed\n"); + goto err_pm_disable; + } + err = rcar_pcie_get_resources(pcie); if (err < 0) { dev_err(dev, "failed to request resources: %d\n", err); - goto err_free_resource_list; + goto err_pm_put; } err = rcar_pcie_parse_map_dma_ranges(pcie, dev->of_node); if (err) - goto err_free_resource_list; - - pm_runtime_enable(dev); - err = pm_runtime_get_sync(dev); - if (err < 0) { - dev_err(dev, "pm_runtime_get_sync failed\n"); - goto err_pm_disable; - } + goto err_pm_put; /* Failure to get a link might just be that no cards are inserted */ hw_init_fn = of_device_get_match_data(dev); @@ -1174,9 +1161,8 @@ static int rcar_pcie_probe(struct platform_device *pdev) err_pm_disable: pm_runtime_disable(dev); - -err_free_resource_list: pci_free_resource_list(&pcie->resources); + err_free_bridge: pci_free_host_bridge(bridge);