[PATCH 1/2] PCI: imx6: Remove unneeded check of platform_get_resource()

[PATCH 1/2] PCI: imx6: Remove unneeded check of platform_get_resource()

[Fabio Estevam]

From: Fabio Estevam <fabio.estevam@freescale.com>

When using devm_ioremap_resource(), we do not need to check the return value of
platform_get_resource(), so just remove it.

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

diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index bd70af8..5002e23 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -426,11 +426,6 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
 		"imprecise external abort");
 
 	dbi_base = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!dbi_base) {
-		dev_err(&pdev->dev, "dbi_base memory resource not found\n");
-		return -ENODEV;
-	}
-
 	pp->dbi_base = devm_ioremap_resource(&pdev->dev, dbi_base);
 	if (IS_ERR(pp->dbi_base)) {
 		ret = PTR_ERR(pp->dbi_base);
-- 
1.8.1.2

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

[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>
---
 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

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

[Marek Vasut]

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

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

[Fabio Estevam]

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

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

[Marek Vasut]

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

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

[Fabio Estevam]

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

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

[Bjorn Helgaas]

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
> 

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

[Jingoo Han]

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

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

[Marek Vasut]

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

Re: [PATCH 1/2] PCI: imx6: Remove unneeded check of platform_get_resource()

[Marek Vasut]

Dear Fabio Estevam,

> From: Fabio Estevam <fabio.estevam@freescale.com>
> 
> When using devm_ioremap_resource(), we do not need to check the return
> value of platform_get_resource(), so just remove it.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>

Acked-by: Marek Vasut <marex@denx.de>

> ---
>  drivers/pci/host/pci-imx6.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
> index bd70af8..5002e23 100644
> --- a/drivers/pci/host/pci-imx6.c
> +++ b/drivers/pci/host/pci-imx6.c
> @@ -426,11 +426,6 @@ static int __init imx6_pcie_probe(struct
> platform_device *pdev) "imprecise external abort");
> 
>  	dbi_base = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (!dbi_base) {
> -		dev_err(&pdev->dev, "dbi_base memory resource not found\n");
> -		return -ENODEV;
> -	}
> -
>  	pp->dbi_base = devm_ioremap_resource(&pdev->dev, dbi_base);
>  	if (IS_ERR(pp->dbi_base)) {
>  		ret = PTR_ERR(pp->dbi_base);

Best regards,
Marek Vasut

Re: [PATCH 1/2] PCI: imx6: Remove unneeded check of platform_get_resource()

[Bjorn Helgaas]

[+cc Shawn]

On Mon, Dec 02, 2013 at 01:39:34AM -0200, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
> 
> When using devm_ioremap_resource(), we do not need to check the return value of
> platform_get_resource(), so just remove it.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>

I applied this with Marek's ack to pci/host-imx6 for v3.14.

Shawn, let me know if you object.  It looks correct to me because
devm_ioremap_resource() checks for res being NULL.

> ---
>  drivers/pci/host/pci-imx6.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
> index bd70af8..5002e23 100644
> --- a/drivers/pci/host/pci-imx6.c
> +++ b/drivers/pci/host/pci-imx6.c
> @@ -426,11 +426,6 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
>  		"imprecise external abort");
>  
>  	dbi_base = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (!dbi_base) {
> -		dev_err(&pdev->dev, "dbi_base memory resource not found\n");
> -		return -ENODEV;
> -	}
> -
>  	pp->dbi_base = devm_ioremap_resource(&pdev->dev, dbi_base);
>  	if (IS_ERR(pp->dbi_base)) {
>  		ret = PTR_ERR(pp->dbi_base);
> -- 
> 1.8.1.2
>