Re: pm_runtime_resume_and_get in .remove callbacks

[Rafael J. Wysocki]

On Wed, Jul 20, 2022 at 8:06 AM Uwe Kleine-König
<u.kleine-koenig@xxxxxxxxxxxxxx> wrote:
>
> 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?

I thought that it might lead to problems if the device that failed to
resume was expected to be accessible.

If that's not the case, you can simply do (1).

> Is it an option to not call one of the resume variants at all and only
> call pm_runtime_disable()?

That depends on whether or not you need to manipulate the hardware in
the del/disable part.  If you need to access it there, it is better to
resume I think.  Otherwise, you don't have to do anything, but then
the next probe needs to be prepared for finding the device in the
suspended state.