| Message ID | 20231117091655.872426-1-u.kleine-koenig@pengutronix.de |
|---|---|
| Headers | show |
| Series | net: ethernet: Convert to platform remove callback | expand |
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
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>
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); >
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