[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20231119094034.qxsq3n6nuxn3e4je@pengutronix.de>
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)
Powered by blists - more mailing lists