Skip to contentSkip to conversation

cpsw: fix resource leak for v3.8

PATCH In review Richard Cochran 4 participants 7 messages Nov 3, 2012 – Nov 5, 2012

Subsystems (3)
Networking Network drivers TI Ethernet switch driver (cpsw)
lore
Share & tools
lore

Richard Cochran

author
#0 lore raw

cover 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(-)
Signature · 1 line
1.7.2.5
author
#1 lore raw

PATCH 1/2 cpsw: rename register banks to match the reference manual, part 2

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 <[email protected]>
---
 drivers/net/ethernet/ti/cpsw.c |   26 +++++++++++++-------------
 1 files changed, 13 insertions(+), 13 deletions(-)
drivers/net/ethernet/ti/cpsw.c +13 -13
index 023d439..6215246 100644
@@ -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);
Signature · 1 line
1.7.2.5
author
#2 lore raw

PATCH 2/2 cpsw: fix leaking IO mappings

In reply to 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 <[email protected]>
---
 drivers/net/ethernet/ti/cpsw.c |   17 ++++++++---------
 1 files changed, 8 insertions(+), 9 deletions(-)
drivers/net/ethernet/ti/cpsw.c +8 -9
index 6215246..7654a62 100644
@@ -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);
Signature · 1 line
1.7.2.5
maintainer
#5 lore raw
In reply to Richard Cochran

From: Richard Cochran <[email protected]>
Date: Sat, 3 Nov 2012 09:25:28 +0100

Quoted 5 lines

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.

#3 lore raw

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

In reply to Richard Cochran
Quoted 3 lines · 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.

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

Quoted 76 lines
Signed-off-by Richard Cochran <[email protected]>
---
  drivers/net/ethernet/ti/cpsw.c |   17 ++++++++---------
  1 files changed, 8 insertions(+), 9 deletions(-)
drivers/net/ethernet/ti/cpsw.c +8 -9
index 6215246..7654a62 100644
@@ -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);
author
#4 lore raw

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

Quoted 6 lines · Cyril Chemparathy
Quoted 3 lines · 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.

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

Acked-by on the v1 series
#6 lore raw
In reply to Richard Cochran
Quoted 22 lines

-----Original Message-----
From: Richard Cochran [mailto:richardcochran at gmail.com]
Sent: Saturday, November 03, 2012 1:55 PM
To: netdev at vger.kernel.org
Cc: linux-arm-kernel at 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

Quoted 6 lines
 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 <[email protected]>