[PATCH net 0/7] net: cpsw: fix leaks and probe deferral

[PATCH net 0/7] net: cpsw: fix leaks and probe deferral

[Johan Hovold]

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


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 | 88 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 71 insertions(+), 17 deletions(-)

-- 
2.7.3

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

[Johan Hovold]

Make sure to resume the platform device to enable clocks before
accessing the CPSW registers in the probe error path (e.g. for deferred
probe).

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)

Note that in the unlikely event of a runtime-resume failure, we'll leak
the ale struct.

Fixes: df828598a755 ("netdev: driver: ethernet: Add TI CPSW driver")
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/net/ethernet/ti/cpsw.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index c6cff3d2ff05..5bc5e6189661 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -2818,7 +2818,12 @@ static int cpsw_probe(struct platform_device *pdev)
 	return 0;
 
 clean_ale_ret:
-	cpsw_ale_destroy(cpsw->ale);
+	if (pm_runtime_get_sync(&pdev->dev) < 0) {
+		pm_runtime_put_noidle(&pdev->dev);
+	} else {
+		cpsw_ale_destroy(cpsw->ale);
+		pm_runtime_put_sync(&pdev->dev);
+	}
 clean_dma_ret:
 	cpdma_ctlr_destroy(cpsw->dma);
 clean_runtime_disable_ret:
-- 
2.7.3

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

[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 <johan@kernel.org>
---
 drivers/net/ethernet/ti/cpsw.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 5bc5e6189661..ee1fae914abc 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -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",
-- 
2.7.3

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

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

Fixes: 1fb19aa730e4 ("net: cpsw: Add parent<->child relation support
between cpsw and mdio")
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/net/ethernet/ti/cpsw.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index ee1fae914abc..4374ba05610b 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -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;
@@ -2588,7 +2593,7 @@ static int cpsw_probe(struct platform_device *pdev)
 	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 +2614,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 +2626,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,7 +2634,7 @@ 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;
 
@@ -2639,7 +2644,7 @@ static int cpsw_probe(struct platform_device *pdev)
 	ret = pm_runtime_get_sync(&pdev->dev);
 	if (ret < 0) {
 		pm_runtime_put_noidle(&pdev->dev);
-		goto clean_runtime_disable_ret;
+		goto clean_dt_ret;
 	}
 	cpsw->version = readl(&cpsw->regs->id_ver);
 	pm_runtime_put_sync(&pdev->dev);
@@ -2648,7 +2653,7 @@ static int cpsw_probe(struct platform_device *pdev)
 	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_dt_ret;
 	}
 
 	memset(&dma_params, 0, sizeof(dma_params));
@@ -2685,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_runtime_disable_ret;
+		goto clean_dt_ret;
 	}
 	for (i = 0; i < cpsw->data.slaves; i++) {
 		struct cpsw_slave *slave = &cpsw->slaves[i];
@@ -2714,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_runtime_disable_ret;
+		goto clean_dt_ret;
 	}
 
 	cpsw->txch[0] = cpdma_chan_create(cpsw->dma, 0, cpsw_tx_handler, 0);
@@ -2827,7 +2832,8 @@ static int cpsw_probe(struct platform_device *pdev)
 	}
 clean_dma_ret:
 	cpdma_ctlr_destroy(cpsw->dma);
-clean_runtime_disable_ret:
+clean_dt_ret:
+	cpsw_remove_dt(pdev);
 	pm_runtime_disable(&pdev->dev);
 clean_ndev_ret:
 	free_netdev(priv->ndev);
@@ -2852,7 +2858,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)
-- 
2.7.3

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

[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 <johan@kernel.org>
---
 drivers/net/ethernet/ti/cpsw.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 4374ba05610b..4bd96732291c 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -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);
 }
 
-- 
2.7.3

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

[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 <johan@kernel.org>
---
 drivers/net/ethernet/ti/cpsw.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 4bd96732291c..421da97a5be6 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -2852,12 +2852,14 @@ 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;
 		}
 	}
 
 	return 0;
 
+clean_unregister_netdev_ret:
+	unregister_netdev(ndev);
 clean_ale_ret:
 	if (pm_runtime_get_sync(&pdev->dev) < 0) {
 		pm_runtime_put_noidle(&pdev->dev);
-- 
2.7.3

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

[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 <johan@kernel.org>
---
 drivers/net/ethernet/ti/cpsw.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 421da97a5be6..411be060fee6 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -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);
-- 
2.7.3

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

[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 <johan@kernel.org>
---
 drivers/net/ethernet/ti/cpsw.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 411be060fee6..cf8f1afa46bf 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -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;
@@ -2628,11 +2631,10 @@ static int cpsw_probe(struct platform_device *pdev)
 	/* Select default pin state */
 	pinctrl_pm_select_default_state(&pdev->dev);
 
-	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;
-- 
2.7.3

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

[Grygorii Strashko]

On 11/16/2016 08:35 AM, Johan Hovold wrote:
> Make sure to resume the platform device to enable clocks before
> accessing the CPSW registers in the probe error path (e.g. for deferred
> probe).
> 
> 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)
> 
> Note that in the unlikely event of a runtime-resume failure, we'll leak
> the ale struct.
> 
> Fixes: df828598a755 ("netdev: driver: ethernet: Add TI CPSW driver")
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  drivers/net/ethernet/ti/cpsw.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index c6cff3d2ff05..5bc5e6189661 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
> @@ -2818,7 +2818,12 @@ static int cpsw_probe(struct platform_device *pdev)
>  	return 0;
>  
>  clean_ale_ret:
> -	cpsw_ale_destroy(cpsw->ale);
> +	if (pm_runtime_get_sync(&pdev->dev) < 0) {
> +		pm_runtime_put_noidle(&pdev->dev);
> +	} else {
> +		cpsw_ale_destroy(cpsw->ale);
> +		pm_runtime_put_sync(&pdev->dev);
> +	}
>  clean_dma_ret:
>  	cpdma_ctlr_destroy(cpsw->dma);
>  clean_runtime_disable_ret:
> 

I think, wouldn't it be logically more simple to just keep CPSW PM runtime enabled during probe?
Like in below diff (not tested):

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 0548e56..deaac1b 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -2657,13 +2657,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_runtime_put_ret;
        }
 
        memset(&dma_params, 0, sizeof(dma_params));
@@ -2700,7 +2699,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_runtime_put_ret;
        }
        for (i = 0; i < cpsw->data.slaves; i++) {
                struct cpsw_slave *slave = &cpsw->slaves[i];
@@ -2729,7 +2728,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_runtime_put_ret;
        }
 
        cpsw->txch[0] = cpdma_chan_create(cpsw->dma, 0, cpsw_tx_handler, 0);
@@ -2831,12 +2830,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_runtime_put_ret:
+       pm_runtime_put_sync(&pdev->dev);
 clean_runtime_disable_ret:
        pm_runtime_disable(&pdev->dev);
 clean_ndev_ret:

-- 
regards,
-grygorii

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

[Grygorii Strashko]

On 11/16/2016 08:35 AM, Johan Hovold wrote:
> Make sure to resume the platform device to enable clocks before
> accessing the CPSW registers in the probe error path (e.g. for deferred
> probe).
> 
> 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)
> 
> Note that in the unlikely event of a runtime-resume failure, we'll leak
> the ale struct.
> 
> Fixes: df828598a755 ("netdev: driver: ethernet: Add TI CPSW driver")
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  drivers/net/ethernet/ti/cpsw.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index c6cff3d2ff05..5bc5e6189661 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
> @@ -2818,7 +2818,12 @@ static int cpsw_probe(struct platform_device *pdev)
>  	return 0;
>  
>  clean_ale_ret:
> -	cpsw_ale_destroy(cpsw->ale);
> +	if (pm_runtime_get_sync(&pdev->dev) < 0) {
> +		pm_runtime_put_noidle(&pdev->dev);
> +	} else {
> +		cpsw_ale_destroy(cpsw->ale);
> +		pm_runtime_put_sync(&pdev->dev);
> +	}
>  clean_dma_ret:
>  	cpdma_ctlr_destroy(cpsw->dma);
>  clean_runtime_disable_ret:
> 

I think, wouldn't it be logically more simple to just keep CPSW PM runtime enabled during probe?
Like in below diff (not tested):

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 0548e56..deaac1b 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -2657,13 +2657,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_runtime_put_ret;
        }
 
        memset(&dma_params, 0, sizeof(dma_params));
@@ -2700,7 +2699,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_runtime_put_ret;
        }
        for (i = 0; i < cpsw->data.slaves; i++) {
                struct cpsw_slave *slave = &cpsw->slaves[i];
@@ -2729,7 +2728,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_runtime_put_ret;
        }
 
        cpsw->txch[0] = cpdma_chan_create(cpsw->dma, 0, cpsw_tx_handler, 0);
@@ -2831,12 +2830,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_runtime_put_ret:
+       pm_runtime_put_sync(&pdev->dev);
 clean_runtime_disable_ret:
        pm_runtime_disable(&pdev->dev);
 clean_ndev_ret:

-- 
regards,
-grygorii

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

[Johan Hovold]

On Wed, Nov 16, 2016 at 02:33:18PM -0600, Grygorii Strashko wrote:
> On 11/16/2016 08:35 AM, Johan Hovold wrote:
> > Make sure to resume the platform device to enable clocks before
> > accessing the CPSW registers in the probe error path (e.g. for deferred
> > probe).
> > 
> > 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)
> > 
> > Note that in the unlikely event of a runtime-resume failure, we'll leak
> > the ale struct.
> > 
> > Fixes: df828598a755 ("netdev: driver: ethernet: Add TI CPSW driver")
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> > ---
> >  drivers/net/ethernet/ti/cpsw.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> > index c6cff3d2ff05..5bc5e6189661 100644
> > --- a/drivers/net/ethernet/ti/cpsw.c
> > +++ b/drivers/net/ethernet/ti/cpsw.c
> > @@ -2818,7 +2818,12 @@ static int cpsw_probe(struct platform_device *pdev)
> >  	return 0;
> >  
> >  clean_ale_ret:
> > -	cpsw_ale_destroy(cpsw->ale);
> > +	if (pm_runtime_get_sync(&pdev->dev) < 0) {
> > +		pm_runtime_put_noidle(&pdev->dev);
> > +	} else {
> > +		cpsw_ale_destroy(cpsw->ale);
> > +		pm_runtime_put_sync(&pdev->dev);
> > +	}
> >  clean_dma_ret:
> >  	cpdma_ctlr_destroy(cpsw->dma);
> >  clean_runtime_disable_ret:
> > 
> 
> I think, wouldn't it be logically more simple to just keep CPSW PM
> runtime enabled during probe?

Indeed it would. I'll do that in a v2.

Thanks,
Johan