Messages in this thread | | | Date | Wed, 20 Jul 2022 08:06:27 +0200 | From | Uwe Kleine-König <> | Subject | Re: pm_runtime_resume_and_get in .remove callbacks |
| |
Hello Rafael,
On Wed, Jul 13, 2022 at 07:47:39PM +0200, Rafael J. Wysocki wrote: > 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);
Why would you not disable the clk if the resume failed?
Is it an option to not call one of the resume variants at all and only call pm_runtime_disable()?
Thanks for your input, Uwe
-- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [unhandled content-type:application/pgp-signature] | |