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

[Rafael J. Wysocki]

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).

Thanks,
Rafael