[PATCH 0/7] net: ethernet: Convert to platform remove callback

[PATCH 0/7] net: ethernet: Convert to platform remove callback

[Uwe Kleine-König]

Hello,

after three fixes this series converts the remaining four platform
drivers below drivers/net/ethernet that don't use .remove_new yet to do
that.

See commit 5c5a7680e67b ("platform: Provide a remove callback that
returns no value") for an extended explanation and the eventual goal.
The TL;DR; is to prevent bugs like the three fixed here.

Best regards
Uwe

Uwe Kleine-König (7):
  net: ethernet: ti: am65-cpsw: Don't error out in .remove()
  net: ethernet: ti: cpsw: Don't error out in .remove()
  net: ethernet: ti: cpsw-new: Don't error out in .remove()
  net: ethernet: ti: am65-cpsw: Convert to platform remove callback
    returning void
  net: ethernet: ti: cpsw: Convert to platform remove callback returning
    void
  net: ethernet: ti: cpsw-new: Convert to platform remove callback
    returning void
  net: ethernet: ezchip: Convert to platform remove callback returning
    void

 drivers/net/ethernet/ezchip/nps_enet.c   |  6 ++----
 drivers/net/ethernet/ti/am65-cpsw-nuss.c | 12 +++++++-----
 drivers/net/ethernet/ti/cpsw.c           | 21 ++++++++++++++-------
 drivers/net/ethernet/ti/cpsw_new.c       | 21 ++++++++++++++-------
 4 files changed, 37 insertions(+), 23 deletions(-)

base-commit: eff99d8edbed7918317331ebd1e365d8e955d65e
-- 
2.42.0

[PATCH 1/7] net: ethernet: ti: am65-cpsw: Don't error out in .remove()

[Uwe Kleine-König]

Returning early from .remove() with an error code still results in the
driver unbinding the device. So the driver core ignores the returned error
code and the resources that were not freed are never catched up. In
combination with devm this also often results in use-after-free bugs.

In case of the am65-cpsw-nuss driver there is an error path, but it's never
taken because am65_cpts_resume() never fails (which however might be
another problem). Still make this explicit and drop the early return in
exchange for an error message (that is more useful than the error the
driver core emits when .remove() returns non-zero).

This prepares changing am65_cpsw_nuss_remove() to return void.

Fixes: 93a76530316a ("net: ethernet: ti: introduce am65x/j721e gigabit eth subsystem driver")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/net/ethernet/ti/am65-cpsw-nuss.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
index ece9f8df98ae..960cb3fa0754 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
@@ -3007,9 +3007,12 @@ static int am65_cpsw_nuss_remove(struct platform_device *pdev)
 
 	common = dev_get_drvdata(dev);
 
-	ret = pm_runtime_resume_and_get(&pdev->dev);
+	ret = pm_runtime_get_sync(&pdev->dev);
 	if (ret < 0)
-		return ret;
+		/* am65_cpts_resume() doesn't fail, so handling ret < 0 is only
+		 * for the sake of completeness.
+		 */
+		dev_err(dev, "runtime resume failed (%pe)\n", ERR_PTR(ret));
 
 	am65_cpsw_unregister_devlink(common);
 	am65_cpsw_unregister_notifiers(common);
-- 
2.42.0

[PATCH 2/7] net: ethernet: ti: cpsw: Don't error out in .remove()

[Uwe Kleine-König]

Returning early from .remove() with an error code still results in the
driver unbinding the device. So the driver core ignores the returned error
code and the resources that were not freed are never catched up. In
combination with devm this also often results in use-after-free bugs.

If runtime resume fails, it's still important to free all resources, so
don't return with an error code, but emit an error message and continue
freeing acquired stuff.

This prepares changing cpsw_remove() to return void.

Fixes: 8a0b6dc958fd ("drivers: net: cpsw: fix wrong regs access in cpsw_remove")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/net/ethernet/ti/cpsw.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index ca4d4548f85e..db5a2ba8a6d4 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -1727,16 +1727,24 @@ static int cpsw_remove(struct platform_device *pdev)
 	struct cpsw_common *cpsw = platform_get_drvdata(pdev);
 	int i, ret;
 
-	ret = pm_runtime_resume_and_get(&pdev->dev);
+	ret = pm_runtime_get_sync(&pdev->dev);
 	if (ret < 0)
-		return ret;
+		/* There is no need to do something about that. The important
+		 * thing is to not exit early, but do all cleanup that doesn't
+		 * require register access.
+		 */
+		dev_err(&pdev->dev, "runtime resume failed (%pe)\n",
+			ERR_PTR(ret));
 
 	for (i = 0; i < cpsw->data.slaves; i++)
 		if (cpsw->slaves[i].ndev)
 			unregister_netdev(cpsw->slaves[i].ndev);
 
-	cpts_release(cpsw->cpts);
-	cpdma_ctlr_destroy(cpsw->dma);
+	if (ret >= 0) {
+		cpts_release(cpsw->cpts);
+		cpdma_ctlr_destroy(cpsw->dma);
+	}
+
 	cpsw_remove_dt(pdev);
 	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
-- 
2.42.0

[PATCH 3/7] net: ethernet: ti: cpsw-new: Don't error out in .remove()

[Uwe Kleine-König]

Returning early from .remove() with an error code still results in the
driver unbinding the device. So the driver core ignores the returned error
code and the resources that were not freed are never catched up. In
combination with devm this also often results in use-after-free bugs.

If runtime resume fails, it's still important to free all resources, so
don't return with an error code, but emit an error message and continue
freeing acquired stuff.

This prepares changing cpsw_remove() to return void.

Fixes: ed3525eda4c4 ("net: ethernet: ti: introduce cpsw switchdev based driver part 1 - dual-emac")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/net/ethernet/ti/cpsw_new.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw_new.c b/drivers/net/ethernet/ti/cpsw_new.c
index 0e4f526b1753..a6ce409f563c 100644
--- a/drivers/net/ethernet/ti/cpsw_new.c
+++ b/drivers/net/ethernet/ti/cpsw_new.c
@@ -2042,16 +2042,24 @@ static int cpsw_remove(struct platform_device *pdev)
 	struct cpsw_common *cpsw = platform_get_drvdata(pdev);
 	int ret;
 
-	ret = pm_runtime_resume_and_get(&pdev->dev);
+	ret = pm_runtime_get_sync(&pdev->dev);
 	if (ret < 0)
-		return ret;
+		/* There is no need to do something about that. The important
+		 * thing is to not exit early, but do all cleanup that doesn't
+		 * requrie register access.
+		 */
+		dev_err(&pdev->dev, "runtime resume failed (%pe)\n",
+			ERR_PTR(ret));
 
 	cpsw_unregister_notifiers(cpsw);
 	cpsw_unregister_devlink(cpsw);
 	cpsw_unregister_ports(cpsw);
 
-	cpts_release(cpsw->cpts);
-	cpdma_ctlr_destroy(cpsw->dma);
+	if (ret >= 0) {
+		cpts_release(cpsw->cpts);
+		cpdma_ctlr_destroy(cpsw->dma);
+	}
+
 	cpsw_remove_dt(cpsw);
 	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
-- 
2.42.0

[PATCH 4/7] net: ethernet: ti: am65-cpsw: Convert to platform remove callback returning void

[Uwe Kleine-König]

The .remove() callback for a platform driver returns an int which makes
many driver authors wrongly assume it's possible to do error handling by
returning an error code. However the value returned is ignored (apart
from emitting a warning) and this typically results in resource leaks.

To improve here there is a quest to make the remove callback return
void. In the first step of this quest all drivers are converted to
.remove_new(), which already returns void. Eventually after all drivers
are converted, .remove_new() will be renamed to .remove().

Trivially convert this driver from always returning zero in the remove
callback to the void returning variant.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/net/ethernet/ti/am65-cpsw-nuss.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
index 960cb3fa0754..83730b566dff 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
@@ -2999,7 +2999,7 @@ static int am65_cpsw_nuss_probe(struct platform_device *pdev)
 	return ret;
 }
 
-static int am65_cpsw_nuss_remove(struct platform_device *pdev)
+static void am65_cpsw_nuss_remove(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct am65_cpsw_common *common;
@@ -3030,7 +3030,6 @@ static int am65_cpsw_nuss_remove(struct platform_device *pdev)
 
 	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
-	return 0;
 }
 
 static int am65_cpsw_nuss_suspend(struct device *dev)
@@ -3130,7 +3129,7 @@ static struct platform_driver am65_cpsw_nuss_driver = {
 		.pm = &am65_cpsw_nuss_dev_pm_ops,
 	},
 	.probe = am65_cpsw_nuss_probe,
-	.remove = am65_cpsw_nuss_remove,
+	.remove_new = am65_cpsw_nuss_remove,
 };
 
 module_platform_driver(am65_cpsw_nuss_driver);
-- 
2.42.0

[PATCH 5/7] net: ethernet: ti: cpsw: Convert to platform remove callback returning void

[Uwe Kleine-König]

The .remove() callback for a platform driver returns an int which makes
many driver authors wrongly assume it's possible to do error handling by
returning an error code. However the value returned is ignored (apart
from emitting a warning) and this typically results in resource leaks.

To improve here there is a quest to make the remove callback return
void. In the first step of this quest all drivers are converted to
.remove_new(), which already returns void. Eventually after all drivers
are converted, .remove_new() will be renamed to .remove().

Trivially convert this driver from always returning zero in the remove
callback to the void returning variant.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/net/ethernet/ti/cpsw.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index db5a2ba8a6d4..ed29a76420f8 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -1722,7 +1722,7 @@ static int cpsw_probe(struct platform_device *pdev)
 	return ret;
 }
 
-static int cpsw_remove(struct platform_device *pdev)
+static void cpsw_remove(struct platform_device *pdev)
 {
 	struct cpsw_common *cpsw = platform_get_drvdata(pdev);
 	int i, ret;
@@ -1748,7 +1748,6 @@ static int cpsw_remove(struct platform_device *pdev)
 	cpsw_remove_dt(pdev);
 	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
-	return 0;
 }
 
 #ifdef CONFIG_PM_SLEEP
@@ -1803,7 +1802,7 @@ static struct platform_driver cpsw_driver = {
 		.of_match_table = cpsw_of_mtable,
 	},
 	.probe = cpsw_probe,
-	.remove = cpsw_remove,
+	.remove_new = cpsw_remove,
 };
 
 module_platform_driver(cpsw_driver);
-- 
2.42.0

[PATCH 6/7] net: ethernet: ti: cpsw-new: Convert to platform remove callback returning void

[Uwe Kleine-König]

The .remove() callback for a platform driver returns an int which makes
many driver authors wrongly assume it's possible to do error handling by
returning an error code. However the value returned is ignored (apart
from emitting a warning) and this typically results in resource leaks.

To improve here there is a quest to make the remove callback return
void. In the first step of this quest all drivers are converted to
.remove_new(), which already returns void. Eventually after all drivers
are converted, .remove_new() will be renamed to .remove().

Trivially convert this driver from always returning zero in the remove
callback to the void returning variant.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/net/ethernet/ti/cpsw_new.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw_new.c b/drivers/net/ethernet/ti/cpsw_new.c
index a6ce409f563c..dae8d0203021 100644
--- a/drivers/net/ethernet/ti/cpsw_new.c
+++ b/drivers/net/ethernet/ti/cpsw_new.c
@@ -2037,7 +2037,7 @@ static int cpsw_probe(struct platform_device *pdev)
 	return ret;
 }
 
-static int cpsw_remove(struct platform_device *pdev)
+static void cpsw_remove(struct platform_device *pdev)
 {
 	struct cpsw_common *cpsw = platform_get_drvdata(pdev);
 	int ret;
@@ -2063,7 +2063,6 @@ static int cpsw_remove(struct platform_device *pdev)
 	cpsw_remove_dt(cpsw);
 	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
-	return 0;
 }
 
 static int __maybe_unused cpsw_suspend(struct device *dev)
@@ -2124,7 +2123,7 @@ static struct platform_driver cpsw_driver = {
 		.of_match_table = cpsw_of_mtable,
 	},
 	.probe = cpsw_probe,
-	.remove = cpsw_remove,
+	.remove_new = cpsw_remove,
 };
 
 module_platform_driver(cpsw_driver);
-- 
2.42.0

[PATCH 7/7] net: ethernet: ezchip: Convert to platform remove callback returning void

[Uwe Kleine-König]

The .remove() callback for a platform driver returns an int which makes
many driver authors wrongly assume it's possible to do error handling by
returning an error code. However the value returned is ignored (apart
from emitting a warning) and this typically results in resource leaks.

To improve here there is a quest to make the remove callback return
void. In the first step of this quest all drivers are converted to
.remove_new(), which already returns void. Eventually after all drivers
are converted, .remove_new() will be renamed to .remove().

Trivially convert this driver from always returning zero in the remove
callback to the void returning variant.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/net/ethernet/ezchip/nps_enet.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/ezchip/nps_enet.c b/drivers/net/ethernet/ezchip/nps_enet.c
index 4d7184d46824..07c2b701b5fa 100644
--- a/drivers/net/ethernet/ezchip/nps_enet.c
+++ b/drivers/net/ethernet/ezchip/nps_enet.c
@@ -633,7 +633,7 @@ static s32 nps_enet_probe(struct platform_device *pdev)
 	return err;
 }
 
-static s32 nps_enet_remove(struct platform_device *pdev)
+static void nps_enet_remove(struct platform_device *pdev)
 {
 	struct net_device *ndev = platform_get_drvdata(pdev);
 	struct nps_enet_priv *priv = netdev_priv(ndev);
@@ -641,8 +641,6 @@ static s32 nps_enet_remove(struct platform_device *pdev)
 	unregister_netdev(ndev);
 	netif_napi_del(&priv->napi);
 	free_netdev(ndev);
-
-	return 0;
 }
 
 static const struct of_device_id nps_enet_dt_ids[] = {
@@ -653,7 +651,7 @@ MODULE_DEVICE_TABLE(of, nps_enet_dt_ids);
 
 static struct platform_driver nps_enet_driver = {
 	.probe = nps_enet_probe,
-	.remove = nps_enet_remove,
+	.remove_new = nps_enet_remove,
 	.driver = {
 		.name = DRV_NAME,
 		.of_match_table  = nps_enet_dt_ids,
-- 
2.42.0

Re: [PATCH 0/7] net: ethernet: Convert to platform remove callback

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 937 bytes --]

Hello,

On Fri, Nov 17, 2023 at 10:16:56AM +0100, Uwe Kleine-König wrote:
> after three fixes this series converts the remaining four platform
> drivers below drivers/net/ethernet that don't use .remove_new yet to do
> that.
> 
> See commit 5c5a7680e67b ("platform: Provide a remove callback that
> returns no value") for an extended explanation and the eventual goal.
> The TL;DR; is to prevent bugs like the three fixed here.

I completely barfed this series, sorry for that.

I forgot to mention "net-next" in the subject. The first three patches
are fixes, but I don't think they are urgent enough to fasttrack them.

And somehow To: got empty, there must be something fishy in my scripts.
I will take care that this won't happen again.

Mea culpa,
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/7] net: ethernet: ti: am65-cpsw: Don't error out in .remove()

[Roger Quadros]

On 17/11/2023 11:16, Uwe Kleine-König wrote:
> Returning early from .remove() with an error code still results in the
> driver unbinding the device. So the driver core ignores the returned error
> code and the resources that were not freed are never catched up. In
> combination with devm this also often results in use-after-free bugs.
> 
> In case of the am65-cpsw-nuss driver there is an error path, but it's never
> taken because am65_cpts_resume() never fails (which however might be
> another problem). Still make this explicit and drop the early return in
> exchange for an error message (that is more useful than the error the
> driver core emits when .remove() returns non-zero).
> 
> This prepares changing am65_cpsw_nuss_remove() to return void.
> 
> Fixes: 93a76530316a ("net: ethernet: ti: introduce am65x/j721e gigabit eth subsystem driver")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/net/ethernet/ti/am65-cpsw-nuss.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> index ece9f8df98ae..960cb3fa0754 100644
> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> @@ -3007,9 +3007,12 @@ static int am65_cpsw_nuss_remove(struct platform_device *pdev)
>  
>  	common = dev_get_drvdata(dev);
>  
> -	ret = pm_runtime_resume_and_get(&pdev->dev);
> +	ret = pm_runtime_get_sync(&pdev->dev);

Unrelated to this patch but I see pm_runtime_resume_and_get()
used at multiple places in this driver.

Would it be wise to replace them all with pm_runtime_get_sync()?

>  	if (ret < 0)
> -		return ret;
> +		/* am65_cpts_resume() doesn't fail, so handling ret < 0 is only
> +		 * for the sake of completeness.
> +		 */
> +		dev_err(dev, "runtime resume failed (%pe)\n", ERR_PTR(ret));
>  
>  	am65_cpsw_unregister_devlink(common);
>  	am65_cpsw_unregister_notifiers(common);

-- 
cheers,
-roger

Re: [PATCH 1/7] net: ethernet: ti: am65-cpsw: Don't error out in .remove()

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2254 bytes --]

Hello,

On Fri, Nov 17, 2023 at 11:51:56AM +0200, Roger Quadros wrote:
> On 17/11/2023 11:16, Uwe Kleine-König wrote:
> > Returning early from .remove() with an error code still results in the
> > driver unbinding the device. So the driver core ignores the returned error
> > code and the resources that were not freed are never catched up. In
> > combination with devm this also often results in use-after-free bugs.
> > 
> > In case of the am65-cpsw-nuss driver there is an error path, but it's never
> > taken because am65_cpts_resume() never fails (which however might be
> > another problem). Still make this explicit and drop the early return in
> > exchange for an error message (that is more useful than the error the
> > driver core emits when .remove() returns non-zero).
> > 
> > This prepares changing am65_cpsw_nuss_remove() to return void.
> > 
> > Fixes: 93a76530316a ("net: ethernet: ti: introduce am65x/j721e gigabit eth subsystem driver")
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  drivers/net/ethernet/ti/am65-cpsw-nuss.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> > index ece9f8df98ae..960cb3fa0754 100644
> > --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> > +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> > @@ -3007,9 +3007,12 @@ static int am65_cpsw_nuss_remove(struct platform_device *pdev)
> >  
> >  	common = dev_get_drvdata(dev);
> >  
> > -	ret = pm_runtime_resume_and_get(&pdev->dev);
> > +	ret = pm_runtime_get_sync(&pdev->dev);
> 
> Unrelated to this patch but I see pm_runtime_resume_and_get()
> used at multiple places in this driver.
> 
> Would it be wise to replace them all with pm_runtime_get_sync()?

No, in general pm_runtime_resume_and_get() is the variant that is easier
to use because it drops the usage count in the error path. Here however
the error isn't simply propagated and so pm_runtime_get_sync() is
better.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/7] net: ethernet: ti: cpsw-new: Don't error out in .remove()

[Ilias Apalodimas]

On Fri, 17 Nov 2023 at 11:17, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Returning early from .remove() with an error code still results in the
> driver unbinding the device. So the driver core ignores the returned error
> code and the resources that were not freed are never catched up. In
> combination with devm this also often results in use-after-free bugs.
>
> If runtime resume fails, it's still important to free all resources, so
> don't return with an error code, but emit an error message and continue
> freeing acquired stuff.
>
> This prepares changing cpsw_remove() to return void.
>
> Fixes: ed3525eda4c4 ("net: ethernet: ti: introduce cpsw switchdev based driver part 1 - dual-emac")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/net/ethernet/ti/cpsw_new.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/ti/cpsw_new.c b/drivers/net/ethernet/ti/cpsw_new.c
> index 0e4f526b1753..a6ce409f563c 100644
> --- a/drivers/net/ethernet/ti/cpsw_new.c
> +++ b/drivers/net/ethernet/ti/cpsw_new.c
> @@ -2042,16 +2042,24 @@ static int cpsw_remove(struct platform_device *pdev)
>         struct cpsw_common *cpsw = platform_get_drvdata(pdev);
>         int ret;
>
> -       ret = pm_runtime_resume_and_get(&pdev->dev);
> +       ret = pm_runtime_get_sync(&pdev->dev);
>         if (ret < 0)
> -               return ret;
> +               /* There is no need to do something about that. The important
> +                * thing is to not exit early, but do all cleanup that doesn't
> +                * requrie register access.
> +                */
> +               dev_err(&pdev->dev, "runtime resume failed (%pe)\n",
> +                       ERR_PTR(ret));
>
>         cpsw_unregister_notifiers(cpsw);
>         cpsw_unregister_devlink(cpsw);
>         cpsw_unregister_ports(cpsw);
>
> -       cpts_release(cpsw->cpts);
> -       cpdma_ctlr_destroy(cpsw->dma);
> +       if (ret >= 0) {
> +               cpts_release(cpsw->cpts);
> +               cpdma_ctlr_destroy(cpsw->dma);
> +       }
> +
>         cpsw_remove_dt(cpsw);
>         pm_runtime_put_sync(&pdev->dev);
>         pm_runtime_disable(&pdev->dev);
> --
> 2.42.0
>

Acked-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Re: [PATCH 1/7] net: ethernet: ti: am65-cpsw: Don't error out in .remove()

[Roger Quadros]

On 17/11/2023 11:16, Uwe Kleine-König wrote:
> Returning early from .remove() with an error code still results in the
> driver unbinding the device. So the driver core ignores the returned error
> code and the resources that were not freed are never catched up. In
> combination with devm this also often results in use-after-free bugs.
> 
> In case of the am65-cpsw-nuss driver there is an error path, but it's never
> taken because am65_cpts_resume() never fails (which however might be
> another problem). Still make this explicit and drop the early return in
> exchange for an error message (that is more useful than the error the
> driver core emits when .remove() returns non-zero).
> 
> This prepares changing am65_cpsw_nuss_remove() to return void.
> 
> Fixes: 93a76530316a ("net: ethernet: ti: introduce am65x/j721e gigabit eth subsystem driver")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/net/ethernet/ti/am65-cpsw-nuss.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> index ece9f8df98ae..960cb3fa0754 100644
> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> @@ -3007,9 +3007,12 @@ static int am65_cpsw_nuss_remove(struct platform_device *pdev)
>  
>  	common = dev_get_drvdata(dev);
>  
> -	ret = pm_runtime_resume_and_get(&pdev->dev);
> +	ret = pm_runtime_get_sync(&pdev->dev);
>  	if (ret < 0)
> -		return ret;
> +		/* am65_cpts_resume() doesn't fail, so handling ret < 0 is only
> +		 * for the sake of completeness.
> +		 */
> +		dev_err(dev, "runtime resume failed (%pe)\n", ERR_PTR(ret));

If the pm_runtime_get_sync() call fails then
am65_cpts_release()->am65_cpts_disable() will cause a bus error
as we are accessing the module with its power domain turned off.

So, the am65_cpts_disable() call needs to be avoided in
the pm_runtime_get_sync() failure path.

>  
>  	am65_cpsw_unregister_devlink(common);
>  	am65_cpsw_unregister_notifiers(common);

-- 
cheers,
-roger

Re: [PATCH 2/7] net: ethernet: ti: cpsw: Don't error out in .remove()

[Roger Quadros]

On 17/11/2023 11:16, Uwe Kleine-König wrote:
> Returning early from .remove() with an error code still results in the
> driver unbinding the device. So the driver core ignores the returned error
> code and the resources that were not freed are never catched up. In
> combination with devm this also often results in use-after-free bugs.
> 
> If runtime resume fails, it's still important to free all resources, so
> don't return with an error code, but emit an error message and continue
> freeing acquired stuff.
> 
> This prepares changing cpsw_remove() to return void.
> 
> Fixes: 8a0b6dc958fd ("drivers: net: cpsw: fix wrong regs access in cpsw_remove")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/net/ethernet/ti/cpsw.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index ca4d4548f85e..db5a2ba8a6d4 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
> @@ -1727,16 +1727,24 @@ static int cpsw_remove(struct platform_device *pdev)
>  	struct cpsw_common *cpsw = platform_get_drvdata(pdev);
>  	int i, ret;
>  
> -	ret = pm_runtime_resume_and_get(&pdev->dev);
> +	ret = pm_runtime_get_sync(&pdev->dev);
>  	if (ret < 0)
> -		return ret;
> +		/* There is no need to do something about that. The important
> +		 * thing is to not exit early, but do all cleanup that doesn't
> +		 * require register access.
> +		 */
> +		dev_err(&pdev->dev, "runtime resume failed (%pe)\n",
> +			ERR_PTR(ret));
>  
>  	for (i = 0; i < cpsw->data.slaves; i++)
>  		if (cpsw->slaves[i].ndev)
>  			unregister_netdev(cpsw->slaves[i].ndev);
>  
> -	cpts_release(cpsw->cpts);
> -	cpdma_ctlr_destroy(cpsw->dma);
> +	if (ret >= 0) {
> +		cpts_release(cpsw->cpts);

cpts_release() only does clk_unprepare().
Why not do that in the error path as well?

> +		cpdma_ctlr_destroy(cpsw->dma);

cpdma_ctrl_destroy() not only stops the DMA controller
but also frees up the channel and calls dma_free_coherent?

We still want to free up the channel and dma_free_coherent in the
error path?

> +	}
> +
>  	cpsw_remove_dt(pdev);
>  	pm_runtime_put_sync(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);

-- 
cheers,
-roger

Re: [PATCH 2/7] net: ethernet: ti: cpsw: Don't error out in .remove()

[Roger Quadros]

On 18/11/2023 12:00, Roger Quadros wrote:
> 
> 
> On 17/11/2023 11:16, Uwe Kleine-König wrote:
>> Returning early from .remove() with an error code still results in the
>> driver unbinding the device. So the driver core ignores the returned error
>> code and the resources that were not freed are never catched up. In
>> combination with devm this also often results in use-after-free bugs.
>>
>> If runtime resume fails, it's still important to free all resources, so
>> don't return with an error code, but emit an error message and continue
>> freeing acquired stuff.
>>
>> This prepares changing cpsw_remove() to return void.
>>
>> Fixes: 8a0b6dc958fd ("drivers: net: cpsw: fix wrong regs access in cpsw_remove")
>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>> ---
>>  drivers/net/ethernet/ti/cpsw.c | 16 ++++++++++++----
>>  1 file changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
>> index ca4d4548f85e..db5a2ba8a6d4 100644
>> --- a/drivers/net/ethernet/ti/cpsw.c
>> +++ b/drivers/net/ethernet/ti/cpsw.c
>> @@ -1727,16 +1727,24 @@ static int cpsw_remove(struct platform_device *pdev)
>>  	struct cpsw_common *cpsw = platform_get_drvdata(pdev);
>>  	int i, ret;
>>  
>> -	ret = pm_runtime_resume_and_get(&pdev->dev);
>> +	ret = pm_runtime_get_sync(&pdev->dev);
>>  	if (ret < 0)
>> -		return ret;
>> +		/* There is no need to do something about that. The important
>> +		 * thing is to not exit early, but do all cleanup that doesn't
>> +		 * require register access.
>> +		 */
>> +		dev_err(&pdev->dev, "runtime resume failed (%pe)\n",
>> +			ERR_PTR(ret));
>>  
>>  	for (i = 0; i < cpsw->data.slaves; i++)
>>  		if (cpsw->slaves[i].ndev)
>>  			unregister_netdev(cpsw->slaves[i].ndev);
>>  
>> -	cpts_release(cpsw->cpts);
>> -	cpdma_ctlr_destroy(cpsw->dma);
>> +	if (ret >= 0) {
>> +		cpts_release(cpsw->cpts);
> 
> cpts_release() only does clk_unprepare().
> Why not do that in the error path as well?
> 
>> +		cpdma_ctlr_destroy(cpsw->dma);
> 
> cpdma_ctrl_destroy() not only stops the DMA controller
> but also frees up the channel and calls dma_free_coherent?
> 
> We still want to free up the channel and dma_free_coherent in the
> error path?

cpdma_chan_destroy() does a cpdma_chan_stop() which does register
accesses so I suppose it cannot be called in the error path.

which leaves only the cpdma_desc_pool_destroy() call.

> 
>> +	}
>> +
>>  	cpsw_remove_dt(pdev);
>>  	pm_runtime_put_sync(&pdev->dev);
>>  	pm_runtime_disable(&pdev->dev);
> 

-- 
cheers,
-roger

Re: [PATCH 4/7] net: ethernet: ti: am65-cpsw: Convert to platform remove callback returning void

[Roger Quadros]

On 17/11/2023 11:17, Uwe Kleine-König wrote:
> The .remove() callback for a platform driver returns an int which makes
> many driver authors wrongly assume it's possible to do error handling by
> returning an error code. However the value returned is ignored (apart
> from emitting a warning) and this typically results in resource leaks.
> 
> To improve here there is a quest to make the remove callback return
> void. In the first step of this quest all drivers are converted to
> .remove_new(), which already returns void. Eventually after all drivers
> are converted, .remove_new() will be renamed to .remove().
> 
> Trivially convert this driver from always returning zero in the remove
> callback to the void returning variant.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Reviewed-by: Roger Quadros <rogerq@kernel.org>

Re: [PATCH 5/7] net: ethernet: ti: cpsw: Convert to platform remove callback returning void

[Roger Quadros]

On 17/11/2023 11:17, Uwe Kleine-König wrote:
> The .remove() callback for a platform driver returns an int which makes
> many driver authors wrongly assume it's possible to do error handling by
> returning an error code. However the value returned is ignored (apart
> from emitting a warning) and this typically results in resource leaks.
> 
> To improve here there is a quest to make the remove callback return
> void. In the first step of this quest all drivers are converted to
> .remove_new(), which already returns void. Eventually after all drivers
> are converted, .remove_new() will be renamed to .remove().
> 
> Trivially convert this driver from always returning zero in the remove
> callback to the void returning variant.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Reviewed-by: Roger Quadros <rogerq@kernel.org>

Re: [PATCH 6/7] net: ethernet: ti: cpsw-new: Convert to platform remove callback returning void

[Roger Quadros]

On 17/11/2023 11:17, Uwe Kleine-König wrote:
> The .remove() callback for a platform driver returns an int which makes
> many driver authors wrongly assume it's possible to do error handling by
> returning an error code. However the value returned is ignored (apart
> from emitting a warning) and this typically results in resource leaks.
> 
> To improve here there is a quest to make the remove callback return
> void. In the first step of this quest all drivers are converted to
> .remove_new(), which already returns void. Eventually after all drivers
> are converted, .remove_new() will be renamed to .remove().
> 
> Trivially convert this driver from always returning zero in the remove
> callback to the void returning variant.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Reviewed-by: Roger Quadros <rogerq@kernel.org>

Re: [PATCH 2/7] net: ethernet: ti: cpsw: Don't error out in .remove()

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2607 bytes --]

Hello Roger,

[dropping Mugunthan V N from Cc, their address bounces, and adding the
net maintainers that I failed to do for the original submission]

On Sat, Nov 18, 2023 at 12:00:08PM +0200, Roger Quadros wrote:
> > -	cpts_release(cpsw->cpts);
> > -	cpdma_ctlr_destroy(cpsw->dma);
> > +	if (ret >= 0) {
> > +		cpts_release(cpsw->cpts);
> 
> cpts_release() only does clk_unprepare().
> Why not do that in the error path as well?

I thought I saw the pm suspend do a clk_unprepare, but I don't find
that. Indeed this step shouldn't be skipped.

> > +		cpdma_ctlr_destroy(cpsw->dma);
> 
> cpdma_ctrl_destroy() not only stops the DMA controller
> but also frees up the channel and calls dma_free_coherent?
> 
> We still want to free up the channel and dma_free_coherent in the
> error path?

Then we need to split the function and only do the software part. I
don't have hardware to test this change and getting it right without
testing seems to be hard.

May I suggest that only do the conversion to remove_new (that doesn't
modify the resource leak behaviour) and you care for fixing the leaks?

My change would then just look as follows:

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index ca4d4548f85e..1251160b563b 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -1722,14 +1722,16 @@ static int cpsw_probe(struct platform_device *pdev)
 	return ret;
 }
 
-static int cpsw_remove(struct platform_device *pdev)
+static void cpsw_remove(struct platform_device *pdev)
 {
 	struct cpsw_common *cpsw = platform_get_drvdata(pdev);
 	int i, ret;
 
 	ret = pm_runtime_resume_and_get(&pdev->dev);
-	if (ret < 0)
-		return ret;
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Failed to resume device (%pe)\n",
+			ERR_PTR(ret))
+	}
 
 	for (i = 0; i < cpsw->data.slaves; i++)
 		if (cpsw->slaves[i].ndev)
@@ -1740,7 +1742,6 @@ static int cpsw_remove(struct platform_device *pdev)
 	cpsw_remove_dt(pdev);
 	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
-	return 0;
 }
 
 #ifdef CONFIG_PM_SLEEP
@@ -1795,7 +1796,7 @@ static struct platform_driver cpsw_driver = {
 		.of_match_table = cpsw_of_mtable,
 	},
 	.probe = cpsw_probe,
-	.remove = cpsw_remove,
+	.remove_new = cpsw_remove,
 };
 
 module_platform_driver(cpsw_driver);

A similar question for the other two cpsw drivers.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]