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

[Alan Stern <stern@rowland.harvard.edu>]

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.

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.

> +/**
> + * 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?

> +/**
> + * 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.

> +/**
> + * 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.

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

Alan Stern