From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alan Stern Subject: Re: [patch update] Re: Run-time PM idea (was: Re: [RFC][PATCH 0/2] PM: Rearrange core suspend code) Date: Wed, 10 Jun 2009 17:14:07 -0400 (EDT) Message-ID: References: <200906101029.27529.rjw@sisk.pl> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <200906101029.27529.rjw@sisk.pl> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-pm-bounces@lists.linux-foundation.org Errors-To: linux-pm-bounces@lists.linux-foundation.org To: "Rafael J. Wysocki" Cc: ACPI Devel Maling List , Linux-pm mailing list , LKML List-Id: linux-pm@vger.kernel.org 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