diff mbox series

PCI: kirin: Fix an error path in kirin_pcie_probe()

Message ID e2b83334ab204ee905fe36ac01cfc297a5a2a7be.1698654061.git.christophe.jaillet@wanadoo.fr
State New
Headers
Series PCI: kirin: Fix an error path in kirin_pcie_probe() |

Commit Message

Christophe JAILLET Oct. 30, 2023, 8:21 a.m. UTC 
  If an error occurs after a successful kirin_pcie_power_on(),
kirin_pcie_power_off() should be called, as already done in the remove
function.

Fixes: fc5165db245a ("PCI: kirin: Add HiSilicon Kirin SoC PCIe controller driver")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
Not sure of the Fixes tag.
---
 drivers/pci/controller/dwc/pcie-kirin.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Comments

Serge Semin Oct. 30, 2023, 11:32 p.m. UTC | #1 
On Mon, Oct 30, 2023 at 09:21:16AM +0100, Christophe JAILLET wrote:
> If an error occurs after a successful kirin_pcie_power_on(),
> kirin_pcie_power_off() should be called, as already done in the remove
> function.
> 
> Fixes: fc5165db245a ("PCI: kirin: Add HiSilicon Kirin SoC PCIe controller driver")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> Not sure of the Fixes tag.
> ---
>  drivers/pci/controller/dwc/pcie-kirin.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
> index 2ee146767971..0b93de9d2d06 100644
> --- a/drivers/pci/controller/dwc/pcie-kirin.c
> +++ b/drivers/pci/controller/dwc/pcie-kirin.c
> @@ -813,7 +813,15 @@ static int kirin_pcie_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	return dw_pcie_host_init(&pci->pp);
> +	ret = dw_pcie_host_init(&pci->pp);
> +	if (ret)
> +		goto err_power_off;
> +
> +	return 0;
> +
> +err_power_off:
> +	kirin_pcie_power_off(kirin_pcie);
> +	return ret;

From the current driver implementation point of view this looks
correct. So
Reviewed-by: Serge Semin <fancer.lancer@gmail.com>

But the design of the power on/off procedures seems very unfortunate:
1. Calling antagonist from the respective protagonist is a bad
solution for maintainability, because shall you need to add something
to the protagonist you'll need to somehow take into account that it is
reverted in the antagonist only if it was executed, which in its
turn will get to be impossible if there are several conditional
steps need to be implemented.
2. There is a logical split up between the hi3660 and other
controllers. Wherein the hi3660-specific code is implemented as a set
of various coherent functions, meanwhile the code for the other
controllers is placed directly to the kirin_pcie_power_on() and
kirin_pcie_power_off() functions. It looks clumsy and hard-readable.
3. kirin_pcie->gpio_id_dwc_perst is requested and switched to output,
but is never freed and got back to input or level zero.

-Serge(y)

>  }
>  
>  static struct platform_driver kirin_pcie_driver = {
> -- 
> 2.34.1
>
Manivannan Sadhasivam Oct. 31, 2023, 8:35 a.m. UTC | #2 
On Mon, Oct 30, 2023 at 09:21:16AM +0100, Christophe JAILLET wrote:
> If an error occurs after a successful kirin_pcie_power_on(),
> kirin_pcie_power_off() should be called, as already done in the remove
> function.
> 

PERST# assert (gpio_id_dwc_perst) is missing from kirin_pcie_power_off(). So
first you need to add that in a separate patch and then this patch can come
next.

Also, this driver is using legacy GPIO APIs and needs cleanup too (Kudos if you
are willing to do that).

- Mani

> Fixes: fc5165db245a ("PCI: kirin: Add HiSilicon Kirin SoC PCIe controller driver")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> Not sure of the Fixes tag.
> ---
>  drivers/pci/controller/dwc/pcie-kirin.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
> index 2ee146767971..0b93de9d2d06 100644
> --- a/drivers/pci/controller/dwc/pcie-kirin.c
> +++ b/drivers/pci/controller/dwc/pcie-kirin.c
> @@ -813,7 +813,15 @@ static int kirin_pcie_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	return dw_pcie_host_init(&pci->pp);
> +	ret = dw_pcie_host_init(&pci->pp);
> +	if (ret)
> +		goto err_power_off;
> +
> +	return 0;
> +
> +err_power_off:
> +	kirin_pcie_power_off(kirin_pcie);
> +	return ret;
>  }
>  
>  static struct platform_driver kirin_pcie_driver = {
> -- 
> 2.34.1
>
diff mbox series

Patch

diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
index 2ee146767971..0b93de9d2d06 100644
--- a/drivers/pci/controller/dwc/pcie-kirin.c
+++ b/drivers/pci/controller/dwc/pcie-kirin.c
@@ -813,7 +813,15 @@  static int kirin_pcie_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	return dw_pcie_host_init(&pci->pp);
+	ret = dw_pcie_host_init(&pci->pp);
+	if (ret)
+		goto err_power_off;
+
+	return 0;
+
+err_power_off:
+	kirin_pcie_power_off(kirin_pcie);
+	return ret;
 }
 
 static struct platform_driver kirin_pcie_driver = {