Re: pm_runtime_resume_and_get in .remove callbacks

[Uwe Kleine-König]

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@xxxxxxxxxxxxxx> 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/ |

Attachment: signature.asc
Description: PGP signature