Re: [RFC/RFT][PATCH v2 6/7] PM / runtime: Use device links

From: Rafael J. Wysocki
Date: Mon Sep 12 2016 - 09:51:03 EST

On Monday, September 12, 2016 11:47:58 AM Lukas Wunner wrote:
> On Thu, Sep 08, 2016 at 11:30:26PM +0200, Rafael J. Wysocki wrote:
> > Modify the runtime PM framework to use device links to ensure that
> > supplier devices will not be suspended if any of their consumer
> > devices are active.
> I think it's inconsistent to runtime resume/suspend suppliers in
> __rpm_callback() whereas the parent is treated in rpm_suspend()
> and rpm_resume().

The reason why I did that this way is the rollback needed in case of
errors and that led to duplicated code if done elsewhere.

> Instead I'd suggest to amend struct dev_pm_ops with:
> atomic_t consumer_count;
> Amend rpm_check_suspend_allowed() with:
> else if (atomic_read(&dev->power.consumer_count) > 0)
> retval = -EBUSY;

That is a good idea, though (from the first look at least).

> Amend rpm_suspend(), rpm_resume() and __pm_runtime_set_status()
> to decrement/increment consumer_count where we're doing the same
> for the parent's child_count, and runtime resume/idle suppliers
> as necessary.
> > The idea is to reference count suppliers on the consumer's resume
> > and drop references to them on its suspend. The information on
> > whether or not the supplier has been reference counted by the
> > consumer's (runtime) resume is stored in a new field (rpm_active)
> > in the link object for each link.
> So the rpm_active variable indicates if the runtime ref on the
> supplier is currently held. I don't see why this is needed.
> If DEVICE_LINK_PM_RUNTIME is not set, we never acquire a runtime
> ref in the first place. If it's set, a ref is acquired upon
> resuming the consumer and released upon suspending it. So whether
> the ref is held is discernable from the consumer's runtime PM state.
> Why do you need to track this in a separate variable?

Please see pm_runtime_clean_up_links().

> > @@ -718,8 +718,12 @@ enum device_link_status {
> > * Device link flags.
> > *
> > * PERSISTENT: Do not delete the link on consumer device driver unbind.
> > + * PM_RUNTIME: If set, the runtime PM framework will use this link.
> > + * RPM_ACTIVE: Run pm_runtime_get_sync() on the supplier during link creation.
> > */
> > #define DEVICE_LINK_PERSISTENT (1 << 0)
> > +#define DEVICE_LINK_PM_RUNTIME (1 << 1)
> > +#define DEVICE_LINK_RPM_ACTIVE (1 << 2)
> I don't understand the need for DEVICE_LINK_RPM_ACTIVE: If the
> consumer is in runtime resumed state when the link is added and
> DEVICE_LINK_PM_RUNTIME is set, then of course the supplier needs
> to be in runtime resumed state as well. Conversely if the consumer
> is in runtime suspended state, the supplier need not be in runtime
> resumed state either. So the value of the flag can be derived from
> the consumer's runtime PM state. Why do we need the flag at all?

That's because device_link_add() doesn't know that runtime PM states of
the supplier/consumer and the flag tells it what to do (with the assumption
that the caller knows the situation, of course).