[PATCH net-next 0/2] cpsw: fix resource leak for v3.8

[PATCH net-next 0/2] cpsw: fix resource leak for v3.8

[Richard Cochran]

While looking at the idea of removing all of the register offsets in
the CPSW's device tree, I noticed that the driver would be leaking IO
mappings. Although this is, strictly speaking, a bug fix, still it can
wait to appear in v3.8, since there is no way to use the driver in
v3.7 (or earlier) anyhow.

Thanks,
Richard


Richard Cochran (2):
  cpsw: rename register banks to match the reference manual, part 2
  cpsw: fix leaking IO mappings

 drivers/net/ethernet/ti/cpsw.c |   39 +++++++++++++++++++--------------------
 1 files changed, 19 insertions(+), 20 deletions(-)

-- 
1.7.2.5

[PATCH net-next 1/2] cpsw: rename register banks to match the reference manual, part 2

[Richard Cochran]

The code mixes up the CPSW_SS and the CPSW_WR register naming. This patch
changes the names to conform to the published Technical Reference Manual
from TI, in order to make working on the code less confusing.

Signed-off-by: Richard Cochran <richardcochran@gmail.com>
---
 drivers/net/ethernet/ti/cpsw.c |   26 +++++++++++++-------------
 1 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 023d439..6215246 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -286,7 +286,7 @@ struct cpsw_priv {
 	struct platform_device		*pdev;
 	struct net_device		*ndev;
 	struct resource			*cpsw_res;
-	struct resource			*cpsw_ss_res;
+	struct resource			*cpsw_wr_res;
 	struct napi_struct		napi;
 	struct device			*dev;
 	struct cpsw_platform_data	data;
@@ -1270,25 +1270,25 @@ static int __devinit cpsw_probe(struct platform_device *pdev)
 	priv->host_port_regs = regs + data->host_port_reg_ofs;
 	priv->cpts.reg = regs + data->cpts_reg_ofs;
 
-	priv->cpsw_ss_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
-	if (!priv->cpsw_ss_res) {
+	priv->cpsw_wr_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (!priv->cpsw_wr_res) {
 		dev_err(priv->dev, "error getting i/o resource\n");
 		ret = -ENOENT;
 		goto clean_clk_ret;
 	}
 
-	if (!request_mem_region(priv->cpsw_ss_res->start,
-			resource_size(priv->cpsw_ss_res), ndev->name)) {
+	if (!request_mem_region(priv->cpsw_wr_res->start,
+			resource_size(priv->cpsw_wr_res), ndev->name)) {
 		dev_err(priv->dev, "failed request i/o region\n");
 		ret = -ENXIO;
 		goto clean_clk_ret;
 	}
 
-	regs = ioremap(priv->cpsw_ss_res->start,
-				resource_size(priv->cpsw_ss_res));
+	regs = ioremap(priv->cpsw_wr_res->start,
+				resource_size(priv->cpsw_wr_res));
 	if (!regs) {
 		dev_err(priv->dev, "unable to map i/o region\n");
-		goto clean_cpsw_ss_iores_ret;
+		goto clean_cpsw_wr_iores_ret;
 	}
 	priv->wr_regs = regs;
 
@@ -1409,9 +1409,9 @@ clean_dma_ret:
 	cpdma_ctlr_destroy(priv->dma);
 clean_iomap_ret:
 	iounmap(priv->regs);
-clean_cpsw_ss_iores_ret:
-	release_mem_region(priv->cpsw_ss_res->start,
-			   resource_size(priv->cpsw_ss_res));
+clean_cpsw_wr_iores_ret:
+	release_mem_region(priv->cpsw_wr_res->start,
+			   resource_size(priv->cpsw_wr_res));
 clean_cpsw_iores_ret:
 	release_mem_region(priv->cpsw_res->start,
 			   resource_size(priv->cpsw_res));
@@ -1442,8 +1442,8 @@ static int __devexit cpsw_remove(struct platform_device *pdev)
 	iounmap(priv->regs);
 	release_mem_region(priv->cpsw_res->start,
 			   resource_size(priv->cpsw_res));
-	release_mem_region(priv->cpsw_ss_res->start,
-			   resource_size(priv->cpsw_ss_res));
+	release_mem_region(priv->cpsw_wr_res->start,
+			   resource_size(priv->cpsw_wr_res));
 	pm_runtime_disable(&pdev->dev);
 	clk_put(priv->clk);
 	kfree(priv->slaves);
-- 
1.7.2.5

[PATCH net-next 2/2] cpsw: fix leaking IO mappings

[Richard Cochran]

The CPSW driver remaps two different IO regions, but fails to unmap them
both. This patch fixes the issue by calling iounmap in the appropriate
places.

Signed-off-by: Richard Cochran <richardcochran@gmail.com>
---
 drivers/net/ethernet/ti/cpsw.c |   17 ++++++++---------
 1 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 6215246..7654a62 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -1252,14 +1252,12 @@ static int __devinit cpsw_probe(struct platform_device *pdev)
 		ret = -ENOENT;
 		goto clean_clk_ret;
 	}
-
 	if (!request_mem_region(priv->cpsw_res->start,
 				resource_size(priv->cpsw_res), ndev->name)) {
 		dev_err(priv->dev, "failed request i/o region\n");
 		ret = -ENXIO;
 		goto clean_clk_ret;
 	}
-
 	regs = ioremap(priv->cpsw_res->start, resource_size(priv->cpsw_res));
 	if (!regs) {
 		dev_err(priv->dev, "unable to map i/o region\n");
@@ -1274,16 +1272,14 @@ static int __devinit cpsw_probe(struct platform_device *pdev)
 	if (!priv->cpsw_wr_res) {
 		dev_err(priv->dev, "error getting i/o resource\n");
 		ret = -ENOENT;
-		goto clean_clk_ret;
+		goto clean_iomap_ret;
 	}
-
 	if (!request_mem_region(priv->cpsw_wr_res->start,
 			resource_size(priv->cpsw_wr_res), ndev->name)) {
 		dev_err(priv->dev, "failed request i/o region\n");
 		ret = -ENXIO;
-		goto clean_clk_ret;
+		goto clean_iomap_ret;
 	}
-
 	regs = ioremap(priv->cpsw_wr_res->start,
 				resource_size(priv->cpsw_wr_res));
 	if (!regs) {
@@ -1326,7 +1322,7 @@ static int __devinit cpsw_probe(struct platform_device *pdev)
 	if (!priv->dma) {
 		dev_err(priv->dev, "error initializing dma\n");
 		ret = -ENOMEM;
-		goto clean_iomap_ret;
+		goto clean_wr_iomap_ret;
 	}
 
 	priv->txch = cpdma_chan_create(priv->dma, tx_chan_num(0),
@@ -1407,11 +1403,13 @@ clean_dma_ret:
 	cpdma_chan_destroy(priv->txch);
 	cpdma_chan_destroy(priv->rxch);
 	cpdma_ctlr_destroy(priv->dma);
-clean_iomap_ret:
-	iounmap(priv->regs);
+clean_wr_iomap_ret:
+	iounmap(priv->wr_regs);
 clean_cpsw_wr_iores_ret:
 	release_mem_region(priv->cpsw_wr_res->start,
 			   resource_size(priv->cpsw_wr_res));
+clean_iomap_ret:
+	iounmap(priv->regs);
 clean_cpsw_iores_ret:
 	release_mem_region(priv->cpsw_res->start,
 			   resource_size(priv->cpsw_res));
@@ -1442,6 +1440,7 @@ static int __devexit cpsw_remove(struct platform_device *pdev)
 	iounmap(priv->regs);
 	release_mem_region(priv->cpsw_res->start,
 			   resource_size(priv->cpsw_res));
+	iounmap(priv->wr_regs);
 	release_mem_region(priv->cpsw_wr_res->start,
 			   resource_size(priv->cpsw_wr_res));
 	pm_runtime_disable(&pdev->dev);
-- 
1.7.2.5

Re: [PATCH net-next 2/2] cpsw: fix leaking IO mappings

[Cyril Chemparathy]

On 11/03/2012 09:25 AM, Richard Cochran wrote:
> The CPSW driver remaps two different IO regions, but fails to unmap them
> both. This patch fixes the issue by calling iounmap in the appropriate
> places.

Any thoughts on using devres helpers to keep the bail out path simpler?

> Signed-off-by: Richard Cochran <richardcochran@gmail.com>
> ---
>   drivers/net/ethernet/ti/cpsw.c |   17 ++++++++---------
>   1 files changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index 6215246..7654a62 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
> @@ -1252,14 +1252,12 @@ static int __devinit cpsw_probe(struct platform_device *pdev)
>   		ret = -ENOENT;
>   		goto clean_clk_ret;
>   	}
> -
>   	if (!request_mem_region(priv->cpsw_res->start,
>   				resource_size(priv->cpsw_res), ndev->name)) {
>   		dev_err(priv->dev, "failed request i/o region\n");
>   		ret = -ENXIO;
>   		goto clean_clk_ret;
>   	}
> -
>   	regs = ioremap(priv->cpsw_res->start, resource_size(priv->cpsw_res));
>   	if (!regs) {
>   		dev_err(priv->dev, "unable to map i/o region\n");
> @@ -1274,16 +1272,14 @@ static int __devinit cpsw_probe(struct platform_device *pdev)
>   	if (!priv->cpsw_wr_res) {
>   		dev_err(priv->dev, "error getting i/o resource\n");
>   		ret = -ENOENT;
> -		goto clean_clk_ret;
> +		goto clean_iomap_ret;
>   	}
> -
>   	if (!request_mem_region(priv->cpsw_wr_res->start,
>   			resource_size(priv->cpsw_wr_res), ndev->name)) {
>   		dev_err(priv->dev, "failed request i/o region\n");
>   		ret = -ENXIO;
> -		goto clean_clk_ret;
> +		goto clean_iomap_ret;
>   	}
> -
>   	regs = ioremap(priv->cpsw_wr_res->start,
>   				resource_size(priv->cpsw_wr_res));
>   	if (!regs) {
> @@ -1326,7 +1322,7 @@ static int __devinit cpsw_probe(struct platform_device *pdev)
>   	if (!priv->dma) {
>   		dev_err(priv->dev, "error initializing dma\n");
>   		ret = -ENOMEM;
> -		goto clean_iomap_ret;
> +		goto clean_wr_iomap_ret;
>   	}
>   
>   	priv->txch = cpdma_chan_create(priv->dma, tx_chan_num(0),
> @@ -1407,11 +1403,13 @@ clean_dma_ret:
>   	cpdma_chan_destroy(priv->txch);
>   	cpdma_chan_destroy(priv->rxch);
>   	cpdma_ctlr_destroy(priv->dma);
> -clean_iomap_ret:
> -	iounmap(priv->regs);
> +clean_wr_iomap_ret:
> +	iounmap(priv->wr_regs);
>   clean_cpsw_wr_iores_ret:
>   	release_mem_region(priv->cpsw_wr_res->start,
>   			   resource_size(priv->cpsw_wr_res));
> +clean_iomap_ret:
> +	iounmap(priv->regs);
>   clean_cpsw_iores_ret:
>   	release_mem_region(priv->cpsw_res->start,
>   			   resource_size(priv->cpsw_res));
> @@ -1442,6 +1440,7 @@ static int __devexit cpsw_remove(struct platform_device *pdev)
>   	iounmap(priv->regs);
>   	release_mem_region(priv->cpsw_res->start,
>   			   resource_size(priv->cpsw_res));
> +	iounmap(priv->wr_regs);
>   	release_mem_region(priv->cpsw_wr_res->start,
>   			   resource_size(priv->cpsw_wr_res));
>   	pm_runtime_disable(&pdev->dev);

Re: [PATCH net-next 2/2] cpsw: fix leaking IO mappings

[Richard Cochran]

On Sun, Nov 04, 2012 at 02:23:58PM +0100, Cyril Chemparathy wrote:
> On 11/03/2012 09:25 AM, Richard Cochran wrote:
> >The CPSW driver remaps two different IO regions, but fails to unmap them
> >both. This patch fixes the issue by calling iounmap in the appropriate
> >places.
> 
> Any thoughts on using devres helpers to keep the bail out path simpler?

I wasn't aware of devres before, but I just read through the doc. It
looks interesting, and if I were writting a new driver, then I might
use it.

But for this driver, it really isn't very complex. It only looks that
way, but I think this just accidental. I guess that the code was
adapted from the davinci emac driver.

Thanks,
Richard

Re: [PATCH net-next 0/2] cpsw: fix resource leak for v3.8

[David Miller]

From: Richard Cochran <richardcochran@gmail.com>
Date: Sat,  3 Nov 2012 09:25:28 +0100

> While looking at the idea of removing all of the register offsets in
> the CPSW's device tree, I noticed that the driver would be leaking IO
> mappings. Although this is, strictly speaking, a bug fix, still it can
> wait to appear in v3.8, since there is no way to use the driver in
> v3.7 (or earlier) anyhow.

All applied, thanks.

RE: [PATCH net-next 0/2] cpsw: fix resource leak for v3.8

[N, Mugunthan V]

> -----Original Message-----
> From: Richard Cochran [mailto:richardcochran@gmail.com]
> Sent: Saturday, November 03, 2012 1:55 PM
> To: netdev@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org; David Miller; Chemparathy,
> Cyril; N, Mugunthan V; Hiremath, Vaibhav
> Subject: [PATCH net-next 0/2] cpsw: fix resource leak for v3.8
> 
> While looking at the idea of removing all of the register offsets in
> the CPSW's device tree, I noticed that the driver would be leaking IO
> mappings. Although this is, strictly speaking, a bug fix, still it can
> wait to appear in v3.8, since there is no way to use the driver in
> v3.7 (or earlier) anyhow.
> 
> Thanks,
> Richard
> 
> 
> Richard Cochran (2):
>   cpsw: rename register banks to match the reference manual, part 2
>   cpsw: fix leaking IO mappings
> 
>  drivers/net/ethernet/ti/cpsw.c |   39 +++++++++++++++++++-------------
> -------
>  1 files changed, 19 insertions(+), 20 deletions(-)
> 
> --
> 1.7.2.5

The patches look good to me.

Acked-by: Mugunthan V N <mugunthanvnm@ti.com>