| Message ID | 20230510200020.1534610-1-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f816b9829b19394d318e01953aa3b2721bca040d |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: fec: Better handle pm_runtime_get() failing in .remove() | expand |
On Wed, May 10, 2023 at 10:00:20PM +0200, Uwe Kleine-König wrote: > In the (unlikely) event that pm_runtime_get() (disguised as > pm_runtime_resume_and_get()) fails, the remove callback returned an > error early. The problem with this is that the driver core ignores the > error value and continues removing the device. This results in a > resource leak. Worse the devm allocated resources are freed and so if a > callback of the driver is called later the register mapping is already > gone which probably results in a crash. > > Fixes: a31eda65ba21 ("net: fec: fix clock count mis-match") > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
Hello: This patch was applied to netdev/net.git (main) by Jakub Kicinski <kuba@kernel.org>: On Wed, 10 May 2023 22:00:20 +0200 you wrote: > In the (unlikely) event that pm_runtime_get() (disguised as > pm_runtime_resume_and_get()) fails, the remove callback returned an > error early. The problem with this is that the driver core ignores the > error value and continues removing the device. This results in a > resource leak. Worse the devm allocated resources are freed and so if a > callback of the driver is called later the register mapping is already > gone which probably results in a crash. > > [...] Here is the summary with links: - [net] net: fec: Better handle pm_runtime_get() failing in .remove() https://git.kernel.org/netdev/net/c/f816b9829b19 You are awesome, thank you!
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c index 42ec6ca3bf03..241df41d500f 100644 --- a/drivers/net/ethernet/freescale/fec_main.c +++ b/drivers/net/ethernet/freescale/fec_main.c @@ -4478,9 +4478,11 @@ fec_drv_remove(struct platform_device *pdev) struct device_node *np = pdev->dev.of_node; int ret; - ret = pm_runtime_resume_and_get(&pdev->dev); + ret = pm_runtime_get_sync(&pdev->dev); if (ret < 0) - return ret; + dev_err(&pdev->dev, + "Failed to resume device in remove callback (%pe)\n", + ERR_PTR(ret)); cancel_work_sync(&fep->tx_timeout_work); fec_ptp_stop(pdev); @@ -4493,8 +4495,13 @@ fec_drv_remove(struct platform_device *pdev) of_phy_deregister_fixed_link(np); of_node_put(fep->phy_node); - clk_disable_unprepare(fep->clk_ahb); - clk_disable_unprepare(fep->clk_ipg); + /* After pm_runtime_get_sync() failed, the clks are still off, so skip + * disabling them again. + */ + if (ret >= 0) { + clk_disable_unprepare(fep->clk_ahb); + clk_disable_unprepare(fep->clk_ipg); + } pm_runtime_put_noidle(&pdev->dev); pm_runtime_disable(&pdev->dev);
In the (unlikely) event that pm_runtime_get() (disguised as pm_runtime_resume_and_get()) fails, the remove callback returned an error early. The problem with this is that the driver core ignores the error value and continues removing the device. This results in a resource leak. Worse the devm allocated resources are freed and so if a callback of the driver is called later the register mapping is already gone which probably results in a crash. Fixes: a31eda65ba21 ("net: fec: fix clock count mis-match") Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/net/ethernet/freescale/fec_main.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) base-commit: ac9a78681b921877518763ba0e89202254349d1b