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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Sun, 19 Nov 2023 10:40:34 +0100
From: Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To: Roger Quadros <rogerq@...nel.org>
Cc: "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Rob Herring <robh@...nel.org>, kernel@...gutronix.de,
	netdev@...r.kernel.org, Yunsheng Lin <linyunsheng@...wei.com>,
	Ravi Gunasekaran <r-gunasekaran@...com>,
	Marek Majtyka <alardam@...il.com>,
	Stanislav Fomichev <sdf@...gle.com>,
	Simon Horman <horms@...nel.org>,
	Siddharth Vadapalli <s-vadapalli@...com>,
	linux-omap@...r.kernel.org
Subject: Re: [PATCH 2/7] net: ethernet: ti: cpsw: Don't error out in .remove()

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/ |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)