On Tuesday, September 5, 2017 4:40:22 PM CEST Jarkko Nikula wrote:
> Hi
>
> On 09/04/2017 02:08 AM, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> >
> > The power management handling in dw_i2c_plat_probe() is somewhat
> > messy and it is rather hard to figure out the code intention for
> > the case when pm_disabled is set. In that case, the driver doesn't
> > enable runtime PM at all, but in addition to that it calls
> > pm_runtime_forbid() as though it wasn't sure if runtime PM might
> > be enabled for the device later by someone else.
> >
> > Although that concern doesn't seem to be actually valid, the
> > device is clearly still expected to be PM-capable even in the
> > pm_disabled set case, so a better approach would be to enable
> > runtime PM for it unconditionally and then prevent it from
> > being runtime suspended by using pm_runtime_forbid().
> >
> > Make the driver do that as that will help to clean up its system
> > sleep handling in a relatively straightforward way.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > ---
> > drivers/i2c/busses/i2c-designware-platdrv.c | 36 +++++++++++++++++++---------
> > 1 file changed, 25 insertions(+), 11 deletions(-)
> >
> > Index: linux-pm/drivers/i2c/busses/i2c-designware-platdrv.c
> > ===================================================================
> > --- linux-pm.orig/drivers/i2c/busses/i2c-designware-platdrv.c
> > +++ linux-pm/drivers/i2c/busses/i2c-designware-platdrv.c
> > @@ -249,6 +249,16 @@ static void dw_i2c_set_fifo_size(struct
> > }
> > }
> >
> > +static void dw_i2c_plat_pm_cleanup(struct dw_i2c_dev *dev)
> > +{
> > + pm_runtime_get_noresume(dev->dev);
> > + if (dev->pm_disabled)
> > + pm_runtime_allow(dev->dev);
> > +
> > + pm_runtime_disable(dev->dev);
> > + pm_runtime_put_noidle(dev->dev);
> > +}
> > +
> > static int dw_i2c_plat_probe(struct platform_device *pdev)
> > {
> > struct dw_i2c_platform_data *pdata = dev_get_platdata(&pdev->dev);
> > @@ -362,14 +372,19 @@ static int dw_i2c_plat_probe(struct plat
> > ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
> > adap->dev.of_node = pdev->dev.of_node;
> >
> > - if (dev->pm_disabled) {
> > + /* The code below assumes runtime PM to be disabled. */
> > + WARN_ON(pm_runtime_enabled(&pdev->dev));
> > +
> > + pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> > + pm_runtime_use_autosuspend(&pdev->dev);
> > + pm_runtime_set_active(&pdev->dev);
> > +
> > + pm_runtime_get_noresume(&pdev->dev);
> > + pm_runtime_enable(&pdev->dev);
> > + if (dev->pm_disabled)
> > pm_runtime_forbid(&pdev->dev);
> > - } else {
> > - pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> > - pm_runtime_use_autosuspend(&pdev->dev);
> > - pm_runtime_set_active(&pdev->dev);
> > - pm_runtime_enable(&pdev->dev);
> > - }
> > +
> > + pm_runtime_put_noidle(&pdev->dev);
> >
> Is pm_runtime_get_noresume()/pm_runtime_put_noidle() cycle needed here?
> My vague memory tells platform device won't power off instantly because
> of plain pm_runtime_enable() even if dev->power.usage_count is zero.
Not by itself, but as a result of something running in parallel with the
probe it may in theory.
> I guess it was the pm_request_idle() in driver_probe_device() that
> triggered the power transition after probe.
>
> drivers/base/dd.c: driver_probe_device():
> pm_runtime_barrier(dev);
> ret = really_probe(dev, drv);
> pm_request_idle(dev);
This doesn't prevent runtime PM transitions from occurring in parallel
with really_probe() and the extra counter incrementation/decrementation
doesn't hurt. :-)
To me, the rule of thumb for runtime PM should be quite analogous to the one
for interrupts: expect it to happen immediately after you have enabled it
unless you know for a fact that there are protections in place.
Thanks,
Rafael
