From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Rafael J. Wysocki" Subject: Re: [patch update] Re: [linux-pm] Run-time PM idea (was: Re: [RFC][PATCH 0/2] PM: Rearrange core suspend code) Date: Wed, 10 Jun 2009 23:31:13 +0200 Message-ID: <200906102331.14267.rjw@sisk.pl> References: Mime-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Return-path: Received: from ogre.sisk.pl ([217.79.144.158]:50055 "EHLO ogre.sisk.pl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1759021AbZFJVbD (ORCPT ); Wed, 10 Jun 2009 17:31:03 -0400 In-Reply-To: Content-Disposition: inline Sender: linux-acpi-owner@vger.kernel.org List-Id: linux-acpi@vger.kernel.org To: Alan Stern Cc: Oliver Neukum , Linux-pm mailing list , ACPI Devel Maling List , LKML 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