From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: Roger Quadros <rogerq@kernel.org>
Cc: "David S. Miller" <davem@davemloft.net>,
Eric Dumazet <edumazet@google.com>,
Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
Rob Herring <robh@kernel.org>,
kernel@pengutronix.de, netdev@vger.kernel.org,
Yunsheng Lin <linyunsheng@huawei.com>,
Ravi Gunasekaran <r-gunasekaran@ti.com>,
Marek Majtyka <alardam@gmail.com>,
Stanislav Fomichev <sdf@google.com>,
Simon Horman <horms@kernel.org>,
Siddharth Vadapalli <s-vadapalli@ti.com>,
linux-omap@vger.kernel.org
Subject: Re: [PATCH 2/7] net: ethernet: ti: cpsw: Don't error out in .remove()
Date: Sun, 19 Nov 2023 10:40:34 +0100 [thread overview]
Message-ID: <20231119094034.qxsq3n6nuxn3e4je@pengutronix.de> (raw)
In-Reply-To: <4cabffc1-7ff9-4277-b508-5902f42b47cb@kernel.org>
[-- 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 --]
next prev parent reply other threads:[~2023-11-19 9:41 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-17 9:16 [PATCH 0/7] net: ethernet: Convert to platform remove callback Uwe Kleine-König
2023-11-17 9:16 ` [PATCH 2/7] net: ethernet: ti: cpsw: Don't error out in .remove() Uwe Kleine-König
2023-11-18 10:00 ` Roger Quadros
2023-11-18 10:05 ` Roger Quadros
2023-11-19 9:40 ` Uwe Kleine-König [this message]
2023-11-17 9:16 ` [PATCH 3/7] net: ethernet: ti: cpsw-new: " Uwe Kleine-König
2023-11-18 7:21 ` Ilias Apalodimas
2023-11-17 9:17 ` [PATCH 5/7] net: ethernet: ti: cpsw: Convert to platform remove callback returning void Uwe Kleine-König
2023-11-18 10:06 ` Roger Quadros
2023-11-17 9:17 ` [PATCH 6/7] net: ethernet: ti: cpsw-new: " Uwe Kleine-König
2023-11-18 10:06 ` Roger Quadros
2023-11-17 9:27 ` [PATCH 0/7] net: ethernet: Convert to platform remove callback Uwe Kleine-König
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20231119094034.qxsq3n6nuxn3e4je@pengutronix.de \
--to=u.kleine-koenig@pengutronix.de \
--cc=alardam@gmail.com \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=horms@kernel.org \
--cc=kernel@pengutronix.de \
--cc=kuba@kernel.org \
--cc=linux-omap@vger.kernel.org \
--cc=linyunsheng@huawei.com \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=r-gunasekaran@ti.com \
--cc=robh@kernel.org \
--cc=rogerq@kernel.org \
--cc=s-vadapalli@ti.com \
--cc=sdf@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).