diff mbox

[2/2] PCI: imx6: Remove unneeded 'goto err'

Message ID 1385955575-25270-2-git-send-email-festevam@gmail.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Fabio Estevam Dec. 2, 2013, 3:39 a.m. UTC 
From: Fabio Estevam <fabio.estevam@freescale.com>

There is no need to use 'goto err' as we can directly return the errors.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 drivers/pci/host/pci-imx6.c | 34 ++++++++++++----------------------
 1 file changed, 12 insertions(+), 22 deletions(-)

Comments

Marek Vasut Dec. 2, 2013, 8:08 a.m. UTC | #1 
Dear Fabio Estevam,

> From: Fabio Estevam <fabio.estevam@freescale.com>
> 
> There is no need to use 'goto err' as we can directly return the errors.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>

I am a bit worried about unwinding of the installation of the DABT hook:

424         /* Added for PCI abort handling */
425         hook_fault_code(16 + 6, imx6q_pcie_abort_handler, SIGBUS, 0,
426                 "imprecise external abort");

This is not handled in the fail path. Instead of this patch, would it be 
possible for you to fix the failpath to take this part into consideration?

Another option, and I think even a better one, would be to move this DABT 
handler installation just before imx6_add_pcie_port() call. You'd still need to 
handle it's de-installation in remove(), but you won't have so many functions in 
probe() goto-ing to fail path.

Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Fabio Estevam Dec. 4, 2013, 6:31 p.m. UTC | #2 
Hi Marek,

On Mon, Dec 2, 2013 at 6:08 AM, Marek Vasut <marex@denx.de> wrote:

> I am a bit worried about unwinding of the installation of the DABT hook:
>
> 424         /* Added for PCI abort handling */
> 425         hook_fault_code(16 + 6, imx6q_pcie_abort_handler, SIGBUS, 0,
> 426                 "imprecise external abort");
>
> This is not handled in the fail path. Instead of this patch, would it be
> possible for you to fix the failpath to take this part into consideration?

IMHO this would be subject of a separate patch. This one does not
change any current behavior.

>
> Another option, and I think even a better one, would be to move this DABT
> handler installation just before imx6_add_pcie_port() call. You'd still need to
> handle it's de-installation in remove(), but you won't have so many functions in
> probe() goto-ing to fail path.

How do we 'uninstall' this DABT handler installation?

Regards,

Fabio Estevam
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marek Vasut Dec. 4, 2013, 11:49 p.m. UTC | #3 
On Wednesday, December 04, 2013 at 07:31:57 PM, Fabio Estevam wrote:
> Hi Marek,
> 
> On Mon, Dec 2, 2013 at 6:08 AM, Marek Vasut <marex@denx.de> wrote:
> > I am a bit worried about unwinding of the installation of the DABT hook:
> > 
> > 424         /* Added for PCI abort handling */
> > 425         hook_fault_code(16 + 6, imx6q_pcie_abort_handler, SIGBUS, 0,
> > 426                 "imprecise external abort");
> > 
> > This is not handled in the fail path. Instead of this patch, would it be
> > possible for you to fix the failpath to take this part into
> > consideration?
> 
> IMHO this would be subject of a separate patch. This one does not
> change any current behavior.

The current behavior is botched then. If this would be compilable as a module 
and you removed this module, this hook would also cease to exist and if 
something triggered this type of DABT, the whole kernel would explode. I suppose 
noone will compile this as a module in the first place, but let's play safe ;-)

> > Another option, and I think even a better one, would be to move this DABT
> > handler installation just before imx6_add_pcie_port() call. You'd still
> > need to handle it's de-installation in remove(), but you won't have so
> > many functions in probe() goto-ing to fail path.
> 
> How do we 'uninstall' this DABT handler installation?

I think you should call hook_fault_code() again, but this time you should 
specify the old values that were in the fault handler table before.

Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Fabio Estevam Dec. 5, 2013, 12:10 a.m. UTC | #4 
On Wed, Dec 4, 2013 at 9:49 PM, Marek Vasut <marex@denx.de> wrote:

> The current behavior is botched then. If this would be compilable as a module

Right, so a separate patch needs to be done in order to fix current behavior.

Regards,

Fabio Estevam
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas Dec. 9, 2013, 10:38 p.m. UTC | #5 
On Mon, Dec 02, 2013 at 01:39:35AM -0200, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
> 
> There is no need to use 'goto err' as we can directly return the errors.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>

Since there's no functional change, I also applied this one to
pci/host-imx6 for v3.14.  I agree that leaving the DABT handler installed
is a bug and should be fixed by another patch.

Bjorn

> ---
>  drivers/pci/host/pci-imx6.c | 34 ++++++++++++----------------------
>  1 file changed, 12 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
> index 5002e23..9fc1cb6 100644
> --- a/drivers/pci/host/pci-imx6.c
> +++ b/drivers/pci/host/pci-imx6.c
> @@ -427,10 +427,8 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
>  
>  	dbi_base = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	pp->dbi_base = devm_ioremap_resource(&pdev->dev, dbi_base);
> -	if (IS_ERR(pp->dbi_base)) {
> -		ret = PTR_ERR(pp->dbi_base);
> -		goto err;
> -	}
> +	if (IS_ERR(pp->dbi_base))
> +		return PTR_ERR(pp->dbi_base);
>  
>  	/* Fetch GPIOs */
>  	imx6_pcie->reset_gpio = of_get_named_gpio(np, "reset-gpio", 0);
> @@ -444,7 +442,7 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
>  				"PCIe reset");
>  	if (ret) {
>  		dev_err(&pdev->dev, "unable to get reset gpio\n");
> -		goto err;
> +		return ret;
>  	}
>  
>  	imx6_pcie->power_on_gpio = of_get_named_gpio(np, "power-on-gpio", 0);
> @@ -455,7 +453,7 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
>  					"PCIe power enable");
>  		if (ret) {
>  			dev_err(&pdev->dev, "unable to get power-on gpio\n");
> -			goto err;
> +			return ret;
>  		}
>  	}
>  
> @@ -467,7 +465,7 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
>  					"PCIe wake up");
>  		if (ret) {
>  			dev_err(&pdev->dev, "unable to get wake-up gpio\n");
> -			goto err;
> +			return ret;
>  		}
>  	}
>  
> @@ -479,7 +477,7 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
>  					"PCIe disable endpoint");
>  		if (ret) {
>  			dev_err(&pdev->dev, "unable to get disable-ep gpio\n");
> -			goto err;
> +			return ret;
>  		}
>  	}
>  
> @@ -488,32 +486,28 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
>  	if (IS_ERR(imx6_pcie->lvds_gate)) {
>  		dev_err(&pdev->dev,
>  			"lvds_gate clock select missing or invalid\n");
> -		ret = PTR_ERR(imx6_pcie->lvds_gate);
> -		goto err;
> +		return PTR_ERR(imx6_pcie->lvds_gate);
>  	}
>  
>  	imx6_pcie->sata_ref_100m = devm_clk_get(&pdev->dev, "sata_ref_100m");
>  	if (IS_ERR(imx6_pcie->sata_ref_100m)) {
>  		dev_err(&pdev->dev,
>  			"sata_ref_100m clock source missing or invalid\n");
> -		ret = PTR_ERR(imx6_pcie->sata_ref_100m);
> -		goto err;
> +		return PTR_ERR(imx6_pcie->sata_ref_100m);
>  	}
>  
>  	imx6_pcie->pcie_ref_125m = devm_clk_get(&pdev->dev, "pcie_ref_125m");
>  	if (IS_ERR(imx6_pcie->pcie_ref_125m)) {
>  		dev_err(&pdev->dev,
>  			"pcie_ref_125m clock source missing or invalid\n");
> -		ret = PTR_ERR(imx6_pcie->pcie_ref_125m);
> -		goto err;
> +		return PTR_ERR(imx6_pcie->pcie_ref_125m);
>  	}
>  
>  	imx6_pcie->pcie_axi = devm_clk_get(&pdev->dev, "pcie_axi");
>  	if (IS_ERR(imx6_pcie->pcie_axi)) {
>  		dev_err(&pdev->dev,
>  			"pcie_axi clock source missing or invalid\n");
> -		ret = PTR_ERR(imx6_pcie->pcie_axi);
> -		goto err;
> +		return PTR_ERR(imx6_pcie->pcie_axi);
>  	}
>  
>  	/* Grab GPR config register range */
> @@ -521,19 +515,15 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
>  		 syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
>  	if (IS_ERR(imx6_pcie->iomuxc_gpr)) {
>  		dev_err(&pdev->dev, "unable to find iomuxc registers\n");
> -		ret = PTR_ERR(imx6_pcie->iomuxc_gpr);
> -		goto err;
> +		return PTR_ERR(imx6_pcie->iomuxc_gpr);
>  	}
>  
>  	ret = imx6_add_pcie_port(pp, pdev);
>  	if (ret < 0)
> -		goto err;
> +		return ret;
>  
>  	platform_set_drvdata(pdev, imx6_pcie);
>  	return 0;
> -
> -err:
> -	return ret;
>  }
>  
>  static const struct of_device_id imx6_pcie_of_match[] = {
> -- 
> 1.8.1.2
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jingoo Han Dec. 10, 2013, 5:54 a.m. UTC | #6 
On Tuesday, December 10, 2013 7:38 AM, Bjorn Helgaas wrote:
> On Mon, Dec 02, 2013 at 01:39:35AM -0200, Fabio Estevam wrote:
> > From: Fabio Estevam <fabio.estevam@freescale.com>
> >
> > There is no need to use 'goto err' as we can directly return the errors.
> >
> > Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> 
> Since there's no functional change, I also applied this one to
> pci/host-imx6 for v3.14.  I agree that leaving the DABT handler installed
> is a bug and should be fixed by another patch.

I also agree with Bjorn's opinion. :-)
Fixing the DABT handler problem can be sent as another patch.

Best regards,
Jingoo Han

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marek Vasut Dec. 10, 2013, 6:48 p.m. UTC | #7 
On Tuesday, December 10, 2013 at 06:54:00 AM, Jingoo Han wrote:
> On Tuesday, December 10, 2013 7:38 AM, Bjorn Helgaas wrote:
> > On Mon, Dec 02, 2013 at 01:39:35AM -0200, Fabio Estevam wrote:
> > > From: Fabio Estevam <fabio.estevam@freescale.com>
> > > 
> > > There is no need to use 'goto err' as we can directly return the
> > > errors.
> > > 
> > > Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> > 
> > Since there's no functional change, I also applied this one to
> > pci/host-imx6 for v3.14.  I agree that leaving the DABT handler installed
> > is a bug and should be fixed by another patch.
> 
> I also agree with Bjorn's opinion. :-)
> Fixing the DABT handler problem can be sent as another patch.

Hm, actually I suspect we cannot un-install the handler. See arch/arm/mm/fault.c 
, the hook_fault_code() call is annotated __init , so it disappears after the 
init sequence finishes. This basically leaves us with disallowing the mx6 pcie 
driver to be removed from kernel (don't let it compile as a module), which I 
dont think is much of a problem anyway.

Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index 5002e23..9fc1cb6 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -427,10 +427,8 @@  static int __init imx6_pcie_probe(struct platform_device *pdev)
 
 	dbi_base = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	pp->dbi_base = devm_ioremap_resource(&pdev->dev, dbi_base);
-	if (IS_ERR(pp->dbi_base)) {
-		ret = PTR_ERR(pp->dbi_base);
-		goto err;
-	}
+	if (IS_ERR(pp->dbi_base))
+		return PTR_ERR(pp->dbi_base);
 
 	/* Fetch GPIOs */
 	imx6_pcie->reset_gpio = of_get_named_gpio(np, "reset-gpio", 0);
@@ -444,7 +442,7 @@  static int __init imx6_pcie_probe(struct platform_device *pdev)
 				"PCIe reset");
 	if (ret) {
 		dev_err(&pdev->dev, "unable to get reset gpio\n");
-		goto err;
+		return ret;
 	}
 
 	imx6_pcie->power_on_gpio = of_get_named_gpio(np, "power-on-gpio", 0);
@@ -455,7 +453,7 @@  static int __init imx6_pcie_probe(struct platform_device *pdev)
 					"PCIe power enable");
 		if (ret) {
 			dev_err(&pdev->dev, "unable to get power-on gpio\n");
-			goto err;
+			return ret;
 		}
 	}
 
@@ -467,7 +465,7 @@  static int __init imx6_pcie_probe(struct platform_device *pdev)
 					"PCIe wake up");
 		if (ret) {
 			dev_err(&pdev->dev, "unable to get wake-up gpio\n");
-			goto err;
+			return ret;
 		}
 	}
 
@@ -479,7 +477,7 @@  static int __init imx6_pcie_probe(struct platform_device *pdev)
 					"PCIe disable endpoint");
 		if (ret) {
 			dev_err(&pdev->dev, "unable to get disable-ep gpio\n");
-			goto err;
+			return ret;
 		}
 	}
 
@@ -488,32 +486,28 @@  static int __init imx6_pcie_probe(struct platform_device *pdev)
 	if (IS_ERR(imx6_pcie->lvds_gate)) {
 		dev_err(&pdev->dev,
 			"lvds_gate clock select missing or invalid\n");
-		ret = PTR_ERR(imx6_pcie->lvds_gate);
-		goto err;
+		return PTR_ERR(imx6_pcie->lvds_gate);
 	}
 
 	imx6_pcie->sata_ref_100m = devm_clk_get(&pdev->dev, "sata_ref_100m");
 	if (IS_ERR(imx6_pcie->sata_ref_100m)) {
 		dev_err(&pdev->dev,
 			"sata_ref_100m clock source missing or invalid\n");
-		ret = PTR_ERR(imx6_pcie->sata_ref_100m);
-		goto err;
+		return PTR_ERR(imx6_pcie->sata_ref_100m);
 	}
 
 	imx6_pcie->pcie_ref_125m = devm_clk_get(&pdev->dev, "pcie_ref_125m");
 	if (IS_ERR(imx6_pcie->pcie_ref_125m)) {
 		dev_err(&pdev->dev,
 			"pcie_ref_125m clock source missing or invalid\n");
-		ret = PTR_ERR(imx6_pcie->pcie_ref_125m);
-		goto err;
+		return PTR_ERR(imx6_pcie->pcie_ref_125m);
 	}
 
 	imx6_pcie->pcie_axi = devm_clk_get(&pdev->dev, "pcie_axi");
 	if (IS_ERR(imx6_pcie->pcie_axi)) {
 		dev_err(&pdev->dev,
 			"pcie_axi clock source missing or invalid\n");
-		ret = PTR_ERR(imx6_pcie->pcie_axi);
-		goto err;
+		return PTR_ERR(imx6_pcie->pcie_axi);
 	}
 
 	/* Grab GPR config register range */
@@ -521,19 +515,15 @@  static int __init imx6_pcie_probe(struct platform_device *pdev)
 		 syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
 	if (IS_ERR(imx6_pcie->iomuxc_gpr)) {
 		dev_err(&pdev->dev, "unable to find iomuxc registers\n");
-		ret = PTR_ERR(imx6_pcie->iomuxc_gpr);
-		goto err;
+		return PTR_ERR(imx6_pcie->iomuxc_gpr);
 	}
 
 	ret = imx6_add_pcie_port(pp, pdev);
 	if (ret < 0)
-		goto err;
+		return ret;
 
 	platform_set_drvdata(pdev, imx6_pcie);
 	return 0;
-
-err:
-	return ret;
 }
 
 static const struct of_device_id imx6_pcie_of_match[] = {