Skip to contentSkip to conversation

net: cpsw: fix leaks and probe deferral

PATCH In review Johan Hovold 2 participants 13 messages Nov 17, 2016 – Nov 18, 2016

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

Johan Hovold

author
#0 lore raw

v2 cover net: cpsw: fix leaks and probe deferral

This series fixes as number of leaks and issues in the cpsw probe-error and driver-unbind paths, some which specifically prevented deferred probing.

Johan

v2
- Keep platform device runtime-resumed throughout probe instead of resuming in the probe error path as suggested by Grygorii (patch 1/7).

- Runtime-resume platform device before registering any children in order to make sure it is synchronously suspended after deregistering children in the error path (patch 3/7).

Johan Hovold (7):
net: ethernet: ti: cpsw: fix bad register access in probe error path
net: ethernet: ti: cpsw: fix mdio device reference leak
net: ethernet: ti: cpsw: fix deferred probe
net: ethernet: ti: cpsw: fix of_node and phydev leaks
net: ethernet: ti: cpsw: fix secondary-emac probe error path
net: ethernet: ti: cpsw: add missing sanity check
net: ethernet: ti: cpsw: fix fixed-link phy probe deferral

 drivers/net/ethernet/ti/cpsw.c | 95 ++++++++++++++++++++++++++++++++----------
 1 file changed, 74 insertions(+), 21 deletions(-)
Signature · 1 line
2.7.3
author
#1 lore raw

PATCH v2 1/7 net: ethernet: ti: cpsw: fix bad register access in probe error path

Make sure to keep the platform device runtime-resumed throughout probe to avoid accessing the CPSW registers in the error path (e.g. for deferred probe) with clocks disabled:

Unhandled fault: external abort on non-linefetch (0x1008) at 0xd0872d08
...
[<c04fabcc>] (cpsw_ale_control_set) from [<c04fb8b4>] (cpsw_ale_destroy+0x2c/0x44) [<c04fb8b4>] (cpsw_ale_destroy) from [<c04fea58>] (cpsw_probe+0xbd0/0x10c4) [<c04fea58>] (cpsw_probe) from [<c047b2a0>] (platform_drv_probe+0x5c/0xc0)

Fixes df828598a755 ("netdev: driver: ethernet: Add TI CPSW driver")
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/net/ethernet/ti/cpsw.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)
drivers/net/ethernet/ti/cpsw.c +7 -4
index c6cff3d2ff05..f60f8ab7c1e3 100644
@@ -2641,13 +2641,12 @@ static int cpsw_probe(struct platform_device *pdev)
 		goto clean_runtime_disable_ret;
 	}
 	cpsw->version = readl(&cpsw->regs->id_ver);
-	pm_runtime_put_sync(&pdev->dev);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
 	cpsw->wr_regs = devm_ioremap_resource(&pdev->dev, res);
 	if (IS_ERR(cpsw->wr_regs)) {
 		ret = PTR_ERR(cpsw->wr_regs);
-		goto clean_runtime_disable_ret;
+		goto clean_pm_runtime_put_ret;
 	}
 
 	memset(&dma_params, 0, sizeof(dma_params));

@@ -2684,7 +2683,7 @@ static int cpsw_probe(struct platform_device *pdev)
 	default:
 		dev_err(priv->dev, "unknown version 0x%08x\n", cpsw->version);
 		ret = -ENODEV;
-		goto clean_runtime_disable_ret;
+		goto clean_pm_runtime_put_ret;
 	}
 	for (i = 0; i < cpsw->data.slaves; i++) {
 		struct cpsw_slave *slave = &cpsw->slaves[i];

@@ -2713,7 +2712,7 @@ static int cpsw_probe(struct platform_device *pdev)
 	if (!cpsw->dma) {
 		dev_err(priv->dev, "error initializing dma\n");
 		ret = -ENOMEM;
-		goto clean_runtime_disable_ret;
+		goto clean_pm_runtime_put_ret;
 	}
 
 	cpsw->txch[0] = cpdma_chan_create(cpsw->dma, 0, cpsw_tx_handler, 0);

@@ -2815,12 +2814,16 @@ static int cpsw_probe(struct platform_device *pdev)
 		}
 	}
 
+	pm_runtime_put(&pdev->dev);
+
 	return 0;
 
 clean_ale_ret:
 	cpsw_ale_destroy(cpsw->ale);
 clean_dma_ret:
 	cpdma_ctlr_destroy(cpsw->dma);
+clean_pm_runtime_put_ret:
+	pm_runtime_put_sync(&pdev->dev);
 clean_runtime_disable_ret:
 	pm_runtime_disable(&pdev->dev);
 clean_ndev_ret:
Signature · 1 line
2.7.3
author
#2 lore raw

PATCH v2 2/7 net: ethernet: ti: cpsw: fix mdio device reference leak

In reply to Johan Hovold

Make sure to drop the reference taken by of_find_device_by_node() when looking up an mdio device from a phy_id property during probe.

Fixes 549985ee9c72 ("cpsw: simplify the setup of the register

pointers")

Signed-off-by Johan Hovold <[email protected]>
---
 drivers/net/ethernet/ti/cpsw.c | 1 +
 1 file changed, 1 insertion(+)
drivers/net/ethernet/ti/cpsw.c +1 -0
index f60f8ab7c1e3..84c5d214557e 100644
@@ -2397,6 +2397,7 @@ static int cpsw_probe_dt(struct cpsw_platform_data *data,
 			}
 			snprintf(slave_data->phy_id, sizeof(slave_data->phy_id),
 				 PHY_ID_FMT, mdio->name, phyid);
+			put_device(&mdio->dev);
 		} else {
 			dev_err(&pdev->dev,
 				"No slave[%d] phy_id, phy-handle, or fixed-link property\n",
Signature · 1 line
2.7.3
author
#3 lore raw

PATCH v2 3/7 net: ethernet: ti: cpsw: fix deferred probe

In reply to Johan Hovold

Make sure to deregister all child devices also on probe errors to avoid leaks and to fix probe deferral:

cpsw 4a100000.ethernet: omap_device: omap_device_enable() called from invalid state 1 cpsw 4a100000.ethernet: use pm_runtime_put_sync_suspend() in driver?
cpsw: probe of 4a100000.ethernet failed with error -22

Add generic helper to undo the effects of cpsw_probe_dt(), which will also be used in a follow-on patch to fix further leaks that have been introduced more recently.

Note that the platform device is now runtime-resumed before registering any child devices in order to make sure that it is synchronously suspended after having deregistered the children in the error path.

Fixes 1fb19aa730e4 ("net: cpsw: Add parent<->child relation support

between cpsw and mdio")

Signed-off-by Johan Hovold <[email protected]>
---
 drivers/net/ethernet/ti/cpsw.c | 41 ++++++++++++++++++++++++-----------------
 1 file changed, 24 insertions(+), 17 deletions(-)
drivers/net/ethernet/ti/cpsw.c +24 -17
index 84c5d214557e..5d14abb06486 100644
@@ -2441,6 +2441,11 @@ static int cpsw_probe_dt(struct cpsw_platform_data *data,
 	return 0;
 }
 
+static void cpsw_remove_dt(struct platform_device *pdev)
+{
+	of_platform_depopulate(&pdev->dev);
+}
+
 static int cpsw_probe_dual_emac(struct cpsw_priv *priv)
 {
 	struct cpsw_common		*cpsw = priv->cpsw;

@@ -2585,10 +2590,19 @@ static int cpsw_probe(struct platform_device *pdev)
 	/* Select default pin state */
 	pinctrl_pm_select_default_state(&pdev->dev);
 
+	/* Need to enable clocks with runtime PM api to access module
+	 * registers
+	 */
+	ret = pm_runtime_get_sync(&pdev->dev);
+	if (ret < 0) {
+		pm_runtime_put_noidle(&pdev->dev);
+		goto clean_runtime_disable_ret;
+	}
+
 	if (cpsw_probe_dt(&cpsw->data, pdev)) {
 		dev_err(&pdev->dev, "cpsw: platform data missing\n");
 		ret = -ENODEV;
-		goto clean_runtime_disable_ret;
+		goto clean_dt_ret;
 	}
 	data = &cpsw->data;
 	cpsw->rx_ch_num = 1;

@@ -2609,7 +2623,7 @@ static int cpsw_probe(struct platform_device *pdev)
 				    GFP_KERNEL);
 	if (!cpsw->slaves) {
 		ret = -ENOMEM;
-		goto clean_runtime_disable_ret;
+		goto clean_dt_ret;
 	}
 	for (i = 0; i < data->slaves; i++)
 		cpsw->slaves[i].slave_num = i;

@@ -2621,7 +2635,7 @@ static int cpsw_probe(struct platform_device *pdev)
 	if (IS_ERR(clk)) {
 		dev_err(priv->dev, "fck is not found\n");
 		ret = -ENODEV;
-		goto clean_runtime_disable_ret;
+		goto clean_dt_ret;
 	}
 	cpsw->bus_freq_mhz = clk_get_rate(clk) / 1000000;
 

@@ -2629,25 +2643,17 @@ static int cpsw_probe(struct platform_device *pdev)
 	ss_regs = devm_ioremap_resource(&pdev->dev, ss_res);
 	if (IS_ERR(ss_regs)) {
 		ret = PTR_ERR(ss_regs);
-		goto clean_runtime_disable_ret;
+		goto clean_dt_ret;
 	}
 	cpsw->regs = ss_regs;
 
-	/* Need to enable clocks with runtime PM api to access module
-	 * registers
-	 */
-	ret = pm_runtime_get_sync(&pdev->dev);
-	if (ret < 0) {
-		pm_runtime_put_noidle(&pdev->dev);
-		goto clean_runtime_disable_ret;
-	}
 	cpsw->version = readl(&cpsw->regs->id_ver);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
 	cpsw->wr_regs = devm_ioremap_resource(&pdev->dev, res);
 	if (IS_ERR(cpsw->wr_regs)) {
 		ret = PTR_ERR(cpsw->wr_regs);
-		goto clean_pm_runtime_put_ret;
+		goto clean_dt_ret;
 	}
 
 	memset(&dma_params, 0, sizeof(dma_params));

@@ -2684,7 +2690,7 @@ static int cpsw_probe(struct platform_device *pdev)
 	default:
 		dev_err(priv->dev, "unknown version 0x%08x\n", cpsw->version);
 		ret = -ENODEV;
-		goto clean_pm_runtime_put_ret;
+		goto clean_dt_ret;
 	}
 	for (i = 0; i < cpsw->data.slaves; i++) {
 		struct cpsw_slave *slave = &cpsw->slaves[i];

@@ -2713,7 +2719,7 @@ static int cpsw_probe(struct platform_device *pdev)
 	if (!cpsw->dma) {
 		dev_err(priv->dev, "error initializing dma\n");
 		ret = -ENOMEM;
-		goto clean_pm_runtime_put_ret;
+		goto clean_dt_ret;
 	}
 
 	cpsw->txch[0] = cpdma_chan_create(cpsw->dma, 0, cpsw_tx_handler, 0);

@@ -2823,7 +2829,8 @@ static int cpsw_probe(struct platform_device *pdev)
 	cpsw_ale_destroy(cpsw->ale);
 clean_dma_ret:
 	cpdma_ctlr_destroy(cpsw->dma);
-clean_pm_runtime_put_ret:
+clean_dt_ret:
+	cpsw_remove_dt(pdev);
 	pm_runtime_put_sync(&pdev->dev);
 clean_runtime_disable_ret:
 	pm_runtime_disable(&pdev->dev);

@@ -2850,7 +2857,7 @@ static int cpsw_remove(struct platform_device *pdev)
 
 	cpsw_ale_destroy(cpsw->ale);
 	cpdma_ctlr_destroy(cpsw->dma);
-	of_platform_depopulate(&pdev->dev);
+	cpsw_remove_dt(pdev);
 	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 	if (cpsw->data.dual_emac)
Signature · 1 line
2.7.3
author
#4 lore raw

PATCH v2 4/7 net: ethernet: ti: cpsw: fix of_node and phydev leaks

In reply to Johan Hovold

Make sure to drop references taken and deregister devices registered during probe on probe errors (including deferred probe) and driver unbind.

Specifically, PHY of-node references were never released and fixed-link PHY devices were never deregistered.

Fixes 9e42f715264f ("drivers: net: cpsw: add phy-handle parsing")
Fixes 1f71e8c96fc6 ("drivers: net: cpsw: Add support for fixed-link

PHY")

Signed-off-by Johan Hovold <[email protected]>
---
 drivers/net/ethernet/ti/cpsw.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)
drivers/net/ethernet/ti/cpsw.c +35 -0
index 5d14abb06486..c3b78bc4fe58 100644
@@ -2443,6 +2443,41 @@ static int cpsw_probe_dt(struct cpsw_platform_data *data,
 
 static void cpsw_remove_dt(struct platform_device *pdev)
 {
+	struct net_device *ndev = platform_get_drvdata(pdev);
+	struct cpsw_common *cpsw = ndev_to_cpsw(ndev);
+	struct cpsw_platform_data *data = &cpsw->data;
+	struct device_node *node = pdev->dev.of_node;
+	struct device_node *slave_node;
+	int i = 0;
+
+	for_each_available_child_of_node(node, slave_node) {
+		struct cpsw_slave_data *slave_data = &data->slave_data[i];
+
+		if (strcmp(slave_node->name, "slave"))
+			continue;
+
+		if (of_phy_is_fixed_link(slave_node)) {
+			struct phy_device *phydev;
+
+			phydev = of_phy_find_device(slave_node);
+			if (phydev) {
+				fixed_phy_unregister(phydev);
+				/* Put references taken by
+				 * of_phy_find_device() and
+				 * of_phy_register_fixed_link().
+				 */
+				phy_device_free(phydev);
+				phy_device_free(phydev);
+			}
+		}
+
+		of_node_put(slave_data->phy_node);
+
+		i++;
+		if (i == data->slaves)
+			break;
+	}
+
 	of_platform_depopulate(&pdev->dev);
 }
Signature · 1 line
2.7.3
author
#5 lore raw

PATCH v2 5/7 net: ethernet: ti: cpsw: fix secondary-emac probe error path

In reply to Johan Hovold

Make sure to deregister the primary device in case the secondary emac fails to probe.

kernel BUG at /home/johan/work/omicron/src/linux/net/core/dev.c:7743!
...
[<c05b3dec>] (free_netdev) from [<c04fe6c0>] (cpsw_probe+0x9cc/0xe50) [<c04fe6c0>] (cpsw_probe) from [<c047b28c>] (platform_drv_probe+0x5c/0xc0)

Fixes d9ba8f9e6298 ("driver: net: ethernet: cpsw: dual emac interface

implementation")

Signed-off-by Johan Hovold <[email protected]>
---
 drivers/net/ethernet/ti/cpsw.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/net/ethernet/ti/cpsw.c +3 -1
index c3b78bc4fe58..11b2daef3158 100644
@@ -2852,7 +2852,7 @@ static int cpsw_probe(struct platform_device *pdev)
 		ret = cpsw_probe_dual_emac(priv);
 		if (ret) {
 			cpsw_err(priv, probe, "error probe slave 2 emac interface\n");
-			goto clean_ale_ret;
+			goto clean_unregister_netdev_ret;
 		}
 	}
 

@@ -2860,6 +2860,8 @@ static int cpsw_probe(struct platform_device *pdev)
 
 	return 0;
 
+clean_unregister_netdev_ret:
+	unregister_netdev(ndev);
 clean_ale_ret:
 	cpsw_ale_destroy(cpsw->ale);
 clean_dma_ret:
Signature · 1 line
2.7.3
author
#6 lore raw

PATCH v2 6/7 net: ethernet: ti: cpsw: add missing sanity check

In reply to Johan Hovold

Make sure to check for allocation failures before dereferencing a NULL-pointer during probe.

Fixes 649a1688c960 ("net: ethernet: ti: cpsw: create common struct to

hold shared driver data")

Signed-off-by Johan Hovold <[email protected]>
---
 drivers/net/ethernet/ti/cpsw.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/net/ethernet/ti/cpsw.c +3 -0
index 11b2daef3158..1387299030e4 100644
@@ -2588,6 +2588,9 @@ static int cpsw_probe(struct platform_device *pdev)
 	int irq;
 
 	cpsw = devm_kzalloc(&pdev->dev, sizeof(struct cpsw_common), GFP_KERNEL);
+	if (!cpsw)
+		return -ENOMEM;
+
 	cpsw->dev = &pdev->dev;
 
 	ndev = alloc_etherdev_mq(sizeof(struct cpsw_priv), CPSW_MAX_QUEUES);
Signature · 1 line
2.7.3
author
#7 lore raw

PATCH v2 7/7 net: ethernet: ti: cpsw: fix fixed-link phy probe deferral

In reply to Johan Hovold

Make sure to propagate errors from of_phy_register_fixed_link() which can fail with -EPROBE_DEFER.

Fixes 1f71e8c96fc6 ("drivers: net: cpsw: Add support for fixed-link

PHY")

Signed-off-by Johan Hovold <[email protected]>
---
 drivers/net/ethernet/ti/cpsw.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)
drivers/net/ethernet/ti/cpsw.c +7 -5
index 1387299030e4..58947aae31c7 100644
@@ -2375,8 +2375,11 @@ static int cpsw_probe_dt(struct cpsw_platform_data *data,
 			 * to the PHY is the Ethernet MAC DT node.
 			 */
 			ret = of_phy_register_fixed_link(slave_node);
-			if (ret)
+			if (ret) {
+				if (ret != -EPROBE_DEFER)
+					dev_err(&pdev->dev, "failed to register fixed-link phy: %d\n", ret);
 				return ret;
+			}
 			slave_data->phy_node = of_node_get(slave_node);
 		} else if (parp) {
 			u32 phyid;

@@ -2637,11 +2640,10 @@ static int cpsw_probe(struct platform_device *pdev)
 		goto clean_runtime_disable_ret;
 	}
 
-	if (cpsw_probe_dt(&cpsw->data, pdev)) {
-		dev_err(&pdev->dev, "cpsw: platform data missing\n");
-		ret = -ENODEV;
+	ret = cpsw_probe_dt(&cpsw->data, pdev);
+	if (ret)
 		goto clean_dt_ret;
-	}
+
 	data = &cpsw->data;
 	cpsw->rx_ch_num = 1;
 	cpsw->tx_ch_num = 1;
Signature · 1 line
2.7.3
maintainer
#8 lore raw

Re: [PATCH net v2 7/7] net: ethernet: ti: cpsw: fix fixed-link phy probe deferral

From: Johan Hovold <[email protected]>
Date: Thu, 17 Nov 2016 17:40:04 +0100

Quoted 6 lines

Make sure to propagate errors from of_phy_register_fixed_link() which can fail with -EPROBE_DEFER.

Fixes 1f71e8c96fc6 ("drivers: net: cpsw: Add support for fixed-link

PHY")

Signed-off-by Johan Hovold <[email protected]>

Johan, when you update a patch within a series you must post the entire series freshly to the lists, cover posting and all.

Thank you.

author
#9 lore raw

Re: [PATCH net v2 7/7] net: ethernet: ti: cpsw: fix fixed-link phy probe deferral

Quoted 6 lines · David Miller

From: Johan Hovold <[email protected]>
Date: Thu, 17 Nov 2016 17:40:04 +0100

Quoted 3 lines

Make sure to propagate errors from of_phy_register_fixed_link() which can fail with -EPROBE_DEFER.

Quoted 6 lines
Quoted 3 lines
Fixes 1f71e8c96fc6 ("drivers: net: cpsw: Add support for fixed-link

PHY")

Signed-off-by Johan Hovold <[email protected]>

Johan, when you update a patch within a series you must post the entire series freshly to the lists, cover posting and all.

I'm quite sure that is exactly what I did. Did you only get this last patch out of the seven?

Johan

author
#10 lore raw

Re: [PATCH net v2 7/7] net: ethernet: ti: cpsw: fix fixed-link phy probe deferral

Quoted 11 lines · Johan Hovold
Quoted 10 lines · David Miller

From: Johan Hovold <[email protected]>
Date: Thu, 17 Nov 2016 17:40:04 +0100

Quoted 6 lines

Make sure to propagate errors from of_phy_register_fixed_link() which can fail with -EPROBE_DEFER.

Fixes 1f71e8c96fc6 ("drivers: net: cpsw: Add support for fixed-link

PHY")

Signed-off-by Johan Hovold <[email protected]>
Quoted 5 lines
Quoted 2 lines

Johan, when you update a patch within a series you must post the entire series freshly to the lists, cover posting and all.

I'm quite sure that is exactly what I did. Did you only get this last patch out of the seven?

The full series has made it to the lists:

	https://marc.info/?l=linux-netdev&m=147940433115984&w=2

Perhaps some messages just got held up.

Let me know if you still want me to resend.

Thanks,
Johan

maintainer
#11 lore raw

Re: [PATCH net v2 7/7] net: ethernet: ti: cpsw: fix fixed-link phy probe deferral

In reply to Johan Hovold

From: Johan Hovold <[email protected]>
Date: Thu, 17 Nov 2016 18:19:20 +0100

Quoted 11 lines · David Miller
Quoted 10 lines

From: Johan Hovold <[email protected]>
Date: Thu, 17 Nov 2016 17:40:04 +0100

Quoted 6 lines

Make sure to propagate errors from of_phy_register_fixed_link() which can fail with -EPROBE_DEFER.

Fixes 1f71e8c96fc6 ("drivers: net: cpsw: Add support for fixed-link

PHY")

Signed-off-by Johan Hovold <[email protected]>
Quoted 5 lines
Quoted 2 lines

Johan, when you update a patch within a series you must post the entire series freshly to the lists, cover posting and all.

I'm quite sure that is exactly what I did. Did you only get this last patch out of the seven?

I ended up getting it delayed, thanks.

maintainer
#12 lore raw
In reply to Johan Hovold

From: Johan Hovold <[email protected]>
Date: Thu, 17 Nov 2016 17:39:57 +0100

Quoted 3 lines

This series fixes as number of leaks and issues in the cpsw probe-error and driver-unbind paths, some which specifically prevented deferred probing.

...

Quoted 8 lines

v2
- Keep platform device runtime-resumed throughout probe instead of resuming in the probe error path as suggested by Grygorii (patch 1/7).

- Runtime-resume platform device before registering any children in order to make sure it is synchronously suspended after deregistering children in the error path (patch 3/7).

Series applied, thanks.