Re: [PATCH v4 6/6] PCI: rcar: Shut the PHY down in failpath

[Lorenzo Pieralisi]

On Thu, May 24, 2018 at 04:36:24PM +0200, Marek Vasut wrote:
> If anything fails past phy_init_fn() and the system is a Gen3 with
> a PHY, the PHY will be left on and inited. This is caused by the
> phy_init_fn, which is in fact a pointer to rcar_pcie_phy_init_gen3()
> function, which starts the PHY, yet has no counterpart in the failpath.
> Add that counterpart.
> 
> 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
> Fixes: 517ca93a7159 ("PCI: rcar: Add R-Car gen3 PHY support")
> ---
> V4: New patch
> ---
>  drivers/pci/host/pcie-rcar.c | 10 --
>  1 file changed, 8 insertions(+), 2 deletions(-)

Applied to pci/controller-fixes to be tentatively merged at -rc4,
thanks.

Lorenzo

> diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c
> index 636c3c5095d2..695781934f0a 100644
> --- a/drivers/pci/host/pcie-rcar.c
> +++ b/drivers/pci/host/pcie-rcar.c
> @@ -1163,7 +1163,7 @@ static int rcar_pcie_probe(struct platform_device *pdev)
>   if (rcar_pcie_hw_init(pcie)) {
>   dev_info(dev, "PCIe link down\n");
>   err = -ENODEV;
> - goto err_clk_disable;
> + goto err_phy_shutdown;
>   }
>  
>   data = rcar_pci_read_reg(pcie, MACSR);
> @@ -1175,7 +1175,7 @@ static int rcar_pcie_probe(struct platform_device *pdev)
>   dev_err(dev,
>   "failed to enable MSI support: %d\n",
>   err);
> - goto err_clk_disable;
> + goto err_phy_shutdown;
>   }
>   }
>  
> @@ -1189,6 +1189,12 @@ static int rcar_pcie_probe(struct platform_device 
> *pdev)
>   if (IS_ENABLED(CONFIG_PCI_MSI))
>   rcar_pcie_teardown_msi(pcie);
>  
> +err_phy_shutdown:
> + if (pcie->phy) {
> + phy_power_off(pcie->phy);
> + phy_exit(pcie->phy);
> + }
> +
>  err_clk_disable:
>   clk_disable_unprepare(pcie->bus_clk);
>  
> -- 
> 2.16.2
> 

Re: [PATCH v4 6/6] PCI: rcar: Shut the PHY down in failpath

[Marek Vasut]

On 05/28/2018 10:48 AM, Simon Horman wrote:
> On Thu, May 24, 2018 at 04:36:24PM +0200, Marek Vasut wrote:
>> If anything fails past phy_init_fn() and the system is a Gen3 with
>> a PHY, the PHY will be left on and inited. This is caused by the
>> phy_init_fn, which is in fact a pointer to rcar_pcie_phy_init_gen3()
>> function, which starts the PHY, yet has no counterpart in the failpath.
>> Add that counterpart.
>>
>> 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
>> Fixes: 517ca93a7159 ("PCI: rcar: Add R-Car gen3 PHY support")
> 
> Acked-by: Simon Horman 

While I see the first 5 in today's next, I don't see this one there, so
it'd be nice to add it too.

-- 
Best regards,
Marek Vasut

Re: [PATCH v4 6/6] PCI: rcar: Shut the PHY down in failpath

[Simon Horman]

On Thu, May 24, 2018 at 04:36:24PM +0200, Marek Vasut wrote:
> If anything fails past phy_init_fn() and the system is a Gen3 with
> a PHY, the PHY will be left on and inited. This is caused by the
> phy_init_fn, which is in fact a pointer to rcar_pcie_phy_init_gen3()
> function, which starts the PHY, yet has no counterpart in the failpath.
> Add that counterpart.
> 
> 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
> Fixes: 517ca93a7159 ("PCI: rcar: Add R-Car gen3 PHY support")

Acked-by: Simon Horman 

> ---
> V4: New patch
> ---
>  drivers/pci/host/pcie-rcar.c | 10 --
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c
> index 636c3c5095d2..695781934f0a 100644
> --- a/drivers/pci/host/pcie-rcar.c
> +++ b/drivers/pci/host/pcie-rcar.c
> @@ -1163,7 +1163,7 @@ static int rcar_pcie_probe(struct platform_device *pdev)
>   if (rcar_pcie_hw_init(pcie)) {
>   dev_info(dev, "PCIe link down\n");
>   err = -ENODEV;
> - goto err_clk_disable;
> + goto err_phy_shutdown;
>   }
>  
>   data = rcar_pci_read_reg(pcie, MACSR);
> @@ -1175,7 +1175,7 @@ static int rcar_pcie_probe(struct platform_device *pdev)
>   dev_err(dev,
>   "failed to enable MSI support: %d\n",
>   err);
> - goto err_clk_disable;
> + goto err_phy_shutdown;
>   }
>   }
>  
> @@ -1189,6 +1189,12 @@ static int rcar_pcie_probe(struct platform_device 
> *pdev)
>   if (IS_ENABLED(CONFIG_PCI_MSI))
>   rcar_pcie_teardown_msi(pcie);
>  
> +err_phy_shutdown:
> + if (pcie->phy) {
> + phy_power_off(pcie->phy);
> + phy_exit(pcie->phy);
> + }
> +
>  err_clk_disable:
>   clk_disable_unprepare(pcie->bus_clk);
>  
> -- 
> 2.16.2
> 

Re: [PATCH v4 6/6] PCI: rcar: Shut the PHY down in failpath

[Marek Vasut]

On 05/25/2018 12:09 PM, Geert Uytterhoeven wrote:
> On Thu, May 24, 2018 at 4:36 PM, Marek Vasut  wrote:
>> If anything fails past phy_init_fn() and the system is a Gen3 with
>> a PHY, the PHY will be left on and inited. This is caused by the
>> phy_init_fn, which is in fact a pointer to rcar_pcie_phy_init_gen3()
>> function, which starts the PHY, yet has no counterpart in the failpath.
>> Add that counterpart.
>>
>> Signed-off-by: Marek Vasut 
> 
> Reviewed-by: Geert Uytterhoeven 
> 
> Note that there's another unhandled failure mode: rcar_pcie_phy_init_gen3()
> doesn't call phy_exit() if phy_power_on() fails.

Should be fixed by subsequent patch, thanks.

-- 
Best regards,
Marek Vasut

Re: [PATCH v4 6/6] PCI: rcar: Shut the PHY down in failpath

[Geert Uytterhoeven]

On Thu, May 24, 2018 at 4:36 PM, Marek Vasut  wrote:
> If anything fails past phy_init_fn() and the system is a Gen3 with
> a PHY, the PHY will be left on and inited. This is caused by the
> phy_init_fn, which is in fact a pointer to rcar_pcie_phy_init_gen3()
> function, which starts the PHY, yet has no counterpart in the failpath.
> Add that counterpart.
>
> Signed-off-by: Marek Vasut 

Reviewed-by: Geert Uytterhoeven 

Note that there's another unhandled failure mode: rcar_pcie_phy_init_gen3()
doesn't call phy_exit() if phy_power_on() fails.

Gr{oetje,eeting}s,

Geert

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

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

[PATCH v4 6/6] PCI: rcar: Shut the PHY down in failpath

[Marek Vasut]

If anything fails past phy_init_fn() and the system is a Gen3 with
a PHY, the PHY will be left on and inited. This is caused by the
phy_init_fn, which is in fact a pointer to rcar_pcie_phy_init_gen3()
function, which starts the PHY, yet has no counterpart in the failpath.
Add that counterpart.

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
Fixes: 517ca93a7159 ("PCI: rcar: Add R-Car gen3 PHY support")
---
V4: New patch
---
 drivers/pci/host/pcie-rcar.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c
index 636c3c5095d2..695781934f0a 100644
--- a/drivers/pci/host/pcie-rcar.c
+++ b/drivers/pci/host/pcie-rcar.c
@@ -1163,7 +1163,7 @@ static int rcar_pcie_probe(struct platform_device *pdev)
if (rcar_pcie_hw_init(pcie)) {
dev_info(dev, "PCIe link down\n");
err = -ENODEV;
-   goto err_clk_disable;
+   goto err_phy_shutdown;
}
 
data = rcar_pci_read_reg(pcie, MACSR);
@@ -1175,7 +1175,7 @@ static int rcar_pcie_probe(struct platform_device *pdev)
dev_err(dev,
"failed to enable MSI support: %d\n",
err);
-   goto err_clk_disable;
+   goto err_phy_shutdown;
}
}
 
@@ -1189,6 +1189,12 @@ static int rcar_pcie_probe(struct platform_device *pdev)
if (IS_ENABLED(CONFIG_PCI_MSI))
rcar_pcie_teardown_msi(pcie);
 
+err_phy_shutdown:
+   if (pcie->phy) {
+   phy_power_off(pcie->phy);
+   phy_exit(pcie->phy);
+   }
+
 err_clk_disable:
clk_disable_unprepare(pcie->bus_clk);
 
-- 
2.16.2