Re: [patch update] Re: [linux-pm] Run-time PM idea (was: Re: [RFC][PATCH 0/2] PM: Rearrange core suspend code)

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Wednesday 10 June 2009, Alan Stern wrote:
> On Wed, 10 Jun 2009, Rafael J. Wysocki wrote:
> 
> > > The idea is that if ->autosuspend() or ->autoresume() returns an error code,
> > > this is a situation the PM core cannot recover from by itself, so it shouldn't
> > > pretend it knows what's happened.  Instead, it marks the device as "I don't
> > > know if it is safe to touch this" and won't handle it until the device driver
> > > or bus type clears the status.
> 
> I'm still not sure this is a good idea.  When would the device driver 
> clear the status?  The autosuspend and autoresume methods run 
> asynchronously, so after they're done the driver doesn't get a chance 
> to do anything.
> 
> It might be best just to set the status to RPM_ACTIVE if a runtime 
> suspend fails and RPM_SUSPENDED if a runtime resume fails.
> 
> > Finally, I decided to follow the Oliver's suggestion that some error codes returned
> > by ->autosuspend() and ->autoresume() may be regarded as "go back to the
> > previous state" information.  I chose to use -EAGAIN and -EBUSY for this
> > purpose.
> 
> Maybe...
> 
> 
> >  struct dev_pm_info {
> >  	pm_message_t		power_state;
> > -	unsigned		can_wakeup:1;
> > -	unsigned		should_wakeup:1;
> > +	unsigned int		can_wakeup:1;
> > +	unsigned int		should_wakeup:1;
> >  	enum dpm_state		status;		/* Owned by the PM core */
> >  #ifdef	CONFIG_PM_SLEEP
> >  	struct list_head	entry;
> >  #endif
> > +#ifdef	CONFIG_PM_RUNTIME
> > +	struct delayed_work	suspend_work;
> > +	unsigned int		suspend_aborted:1;
> > +	struct work_struct	resume_work;
> > +	struct completion	work_done;
> > +	enum rpm_state		runtime_status;
> > +	spinlock_t		lock;
> > +#endif
> >  };
> 
> You know, it doesn't make any sense to have a suspend and a resume 
> both pending at the same time.
>
> So you could add only a delayed_work structure and use its embedded
> work_struct for resume requests.

I thought so too, but I was wrong. ;-)

If resume is requested while the suspend hasn't completed yet, we should
queue it (it's totally valid to request a suspending device to resume IMO), but
the delayed work is still being used by the workqueue code, so we can't modify
it.

> Also, you might borrow a trick from Dave Brownell.  Define the RPM_*
> values so that the individual bits have meanings.  Then instead of
> testing for multiple possible values of runtime_status, you could do a
> simple bit test.

Yes, I'm seriously considering using this approach.

> > +/**
> > + * pm_device_suspended - Check if given device has been suspended at run time.
> > + * @dev: Device to check.
> > + * @data: Ignored.
> > + *
> > + * Returns 0 if the device has been suspended or -EBUSY otherwise.
> > + */
> > +static int pm_device_suspended(struct device *dev, void *data)
> > +{
> > +	int ret;
> > +
> > +	spin_lock(&dev->power.lock);
> > +
> > +	ret = dev->power.runtime_status == RPM_SUSPENDED ? 0 : -EBUSY;
> > +
> > +	spin_unlock(&dev->power.lock);
> 
> How does acquiring the lock help here?

OK, it doesn't.

> > +/**
> > + * pm_check_children - Check if all children of a device have been suspended.
> > + * @dev: Device to check.
> > + *
> > + * Returns 0 if all children of the device have been suspended or -EBUSY
> > + * otherwise.
> > + */
> 
> We might want to do a runtime suspend even if the device's children
> aren't already suspended.  For example, you could suspend a link while
> leaving the device on the other end of the link at full power --
> especially if powering down the device is slow but changing the link's
> power level is fast.

Well, this means that the dependencies between devices in the device tree are
pretty much useless for the run-time PM as far as the core is concerned.  In
which case, why did you mention them at all?

> > +/**
> > + * pm_autosuspend - Run autosuspend callback of given device object's bus type.
> > + * @work: Work structure used for scheduling the execution of this function.
> > + *
> > + * Use @work to get the device object the suspend has been scheduled for,
> > + * check if the suspend request hasn't been cancelled and run the
> > + * ->autosuspend() callback from the device's bus type driver.  Update the
> > + * run-time PM flags in the device object to reflect the current status of the
> > + * device.
> > + */
> > +static void pm_autosuspend(struct work_struct *work)
> 
> Can we call this something else?  "Autosuspend" implies that the 
> suspend originated from within the kernel.  How about "pm_suspend_work" 
> or "pm_runtime_suspend"?  Likewise for the resume routines.

OK

> I haven't checked the details of the code yet.  More later...

OK, thanks.

Best,
Rafael