Messages in this thread | | | From | "Rafael J. Wysocki" <> | Date | Wed, 13 Jul 2022 19:47:39 +0200 | Subject | Re: pm_runtime_resume_and_get in .remove callbacks |
| |
On Wed, Jul 13, 2022 at 10:47 AM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > Hello, > > there is a big bunch of kernel drivers (here: > drivers/i2c/busses/i2c-sprd.c) that have a remove callback that looks as > follows: > > static int sprd_i2c_remove(struct platform_device *pdev) > { > struct sprd_i2c *i2c_dev = platform_get_drvdata(pdev); > int ret; > > ret = pm_runtime_resume_and_get(i2c_dev->dev); > if (ret < 0) > return ret; > > i2c_del_adapter(&i2c_dev->adap); > clk_disable_unprepare(i2c_dev->clk); > > pm_runtime_put_noidle(i2c_dev->dev); > pm_runtime_disable(i2c_dev->dev); > > return 0; > } > > If pm_runtime_resume_and_get fails, the i2c adapter isn't removed, but > as the memory backing i2c_dev goes away---it was allocated using > devm_kzalloc in .probe()---the next i2c action will probably access > freed memory. > > I'm not familiar enough with pm-runtime stuff, but wonder what > can/should be done about that. The obvious (to me) candidates are: > > - log an error if pm_runtime_resume_and_get() fails, but continue to > clean up > - don't check the return value at all > > What do you think?
(1) Use pm_runtime_get_sync() instead of pm_runtime_resume_and_get() and don't check its return value,
or if that is not viable, because something really can run if and only if the device is operational,
(2) do something like
ret = pm_runtime_resume_and_get(i2c_dev->dev); i2c_del_adapter(&i2c_dev->adap); if (ret >= 0) clk_disable_unprepare(i2c_dev->clk);
pm_runtime_put_noidle(i2c_dev->dev); pm_runtime_disable(i2c_dev->dev);
Thanks!
| |