Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

Rafael J. Wysocki
2014-06-20T13:02:56Z

On Thursday, June 19, 2014 04:13:07 PM Alan Stern wrote:
> On Thu, 19 Jun 2014, Kevin Hilman wrote:
>
> > Alan Stern <stern@rowland.harvard.edu> writes:
> >
> > > On Thu, 19 Jun 2014, Allen Yu wrote:
> > >
> > >> So what's the exact state of device if dev->power.is_suspended flag
> > >> is set and runtime_status is RPM_ACTIVE? Is it a state like
> > >> "suspended but still can be accessed"?
> > >>
> > >> I'm just afraid the existing code would cause a device hang if we
> > >> allow it to be accessed even though it's suspended (at this point
> > >> RPM_ACTIVE could be meaningless). I don't understand the original
> > >> motivation of these code. If it's a valid case, most likely it should
> > >> be handled in the specific device driver instead of the PM core.
> > >
> > > You should read the Changelog for commit 6f3c77b040f (PM / Runtime:
> > > let rpm_resume() succeed if RPM_ACTIVE, even when disabled, v2). It
> > > explains why the code looks the way it does.
> > >
> > > However, I'm starting to think the reasoning in that commit may not be
> > > valid. While perhaps it is okay for some I2C devices (mentioned in the
> > > commit log), it probably isn't okay in general.
> >
> > Why not?
>
> See below.
>
> > > Kevin, do have any comments on this matter? What do you think about
> > > making the following change to rpm_resume():
> > >
> > > repeat:
> > > if (dev->power.runtime_error)
> > > retval = -EINVAL;
> > > - else if (dev->power.disable_depth == 1 && dev->power.is_suspended
> > > + else if (dev->power.disable_depth > 0
> > > && dev->power.runtime_status == RPM_ACTIVE)
> > > retval = 1;
> > > else if (dev->power.disable_depth > 0)
> > >
> > > Or even:
> > >
> > > + else if (dev->power.disable_depth > 0 && !dev->power.is_suspended
> > >
> > > although this would require the I2C driver you mentioned in your commit
> > > to change.
> >
> > My change was introduced to catch a very specific case. Namely, when we
> > know that the core has successfully asked the device to do a system suspend
> > (dev->power.is_suspended == true) *and* we know that runtime PM was
> > disabled *only* by the PM core (disable_depth == 0) while the device was
> > still active (runtime_status == RPM_ACTIVE.)
>
> For a general device, the fact that dev->power.is_suspended is set
> means the device _has_ been powered down. Even though the
> runtime_status may not have changed, the PM core has to assume the
> device is not available for use.

This seems to go a bit too far. What power.is_suspended actually means is
that __device_suspend() has run for the device successfully. What the
implications of that are depends on the bus type (or subsystem in general)
and device driver.

> While your I2C devices may be useable even after the ->suspend callback
> returns, for most devices this isn't true. So we shouldn't allow
> rpm_resume() to return imediately when is_suspended is set.

I can agree with that.

> > In your first idea above, it would allow a _get() to succeed even if
> > someone other than the core (including the driver itself) had called
> > pm_runtime_disable(). I don't think we want that.
>
> Why not? The fact that the device is disabled for runtime PM means
> that the PM core mustn't try to change its power state. But if the
> runtime status is RPM_ACTIVE then the device should already be powered
> up, so there's no harm in letting runtime_resume() succeed.
>
> To put it another way, disabled_depth > 0 means that the PM core isn't
> allowed to invoke any of the runtime PM callbacks. But when
> runtime_status == RPM_ACTIVE, runtime_resume() can run successfully
> without invoking any callbacks.

Theoretically. That is, unless someone changes the status from RPM_SUSPENDED
to RPM_ACTIVE while runtime PM is disabled for the device, which is documented
as a *valid* thing to do.

So really, perhaps we should go back to thinking that runtime_status is
meaningless while runtime PM is disabled, which really is the case?

Rafael