From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Rafael J. Wysocki" Subject: Re: [PATCH] PM: Introduce core framework for run-time PM of I/O devices Date: Sun, 14 Jun 2009 12:29:05 +0200 Message-ID: <200906141229.06561.rjw__21213.5141635828$1244975454$gmane$org@sisk.pl> References: <200906140023.36147.rjw@sisk.pl> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Content-Disposition: inline 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: Magnus Damm Cc: LKML , ACPI Devel Maling List , Magnus Damm , pm list , Ingo Molnar List-Id: linux-pm@vger.kernel.org On Sunday 14 June 2009, Magnus Damm wrote: > Hi Rafael, > > On Sun, Jun 14, 2009 at 7:23 AM, Rafael J. Wysocki wrote: > > Below is the current version of my "run-time PM for I/O devices" patch. > > > > I've done my best to address the comments received during the recent > > discussions, but at the same time I've tried to make the patch only contain > > the most essential things. For this reason, for example, the sysfs interface > > is not there and it's going to be added in a separate patch. > > Good decision. Let's do this step by step. > > > Please let me know if you want me to change anything in this patch or to add > > anything new to it. [Magnus, I remember you wanted something like > > ->runtime_wakeup() along with ->runtime_idle(), but I'm not sure it's really > > necessary. Please let me know if you have any particular usage scenario for > > it.] > > I will keep on building my arch specific platform bus code on top of > the latest version of this patch. > > However, to begin with I'll not make use of the ->runtime_idle() > callback in the bus code. This because rearranging the existing > platform devices into a tree will require a lot of rewriting, and I'm > not convinced it's the right approach. I'd rather focus on getting > basic functionality in place at this point. So if no one else needs > ->runtime_idle(), feel free to exclude the ->runtime_idle() part if > you want to make the patch even leaner to begin with. I think it's going be useful in general. If not, we can just drop it. > Together with the bus specific callbacks I plan to modify device > drivers to include pm_runtime_suspend() / pm_runtime_resume() calls to > notify the bus code when they are idle and when they need wakeup, > similar to my earlier proposal with > platform_device_idle()/platform_device_wakeup(). That sounds like a good plan. > > --- linux-2.6.orig/include/linux/pm.h > > +++ linux-2.6/include/linux/pm.h > > @@ -182,6 +205,11 @@ struct dev_pm_ops { > > int (*thaw_noirq)(struct device *dev); > > int (*poweroff_noirq)(struct device *dev); > > int (*restore_noirq)(struct device *dev); > > +#ifdef CONFIG_PM_RUNTIME > > + int (*runtime_suspend)(struct device *dev); > > + int (*runtime_resume)(struct device *dev); > > + void (*runtime_idle)(struct device *dev); > > +#endif > > Do we really need to wrap these in CONFIG_PM_RUNTIME? The callbacks > for STR and STD are not wrapped in CONFIG_SUSPEND and > CONFIG_HIBERNATION, right? > > > --- /dev/null > > +++ linux-2.6/drivers/base/power/runtime.c > [snip] > > +/** > > + * pm_runtime_suspend - Run a device bus type's runtime_suspend() callback. > > + * @dev: Device to suspend. > > + * > > + * Check if the status of the device is appropriate and run the > > + * ->runtime_suspend() callback provided by the device's bus type driver. > > + * Update the run-time PM flags in the device object to reflect the current > > + * status of the device. > > + */ > > +int pm_runtime_suspend(struct device *dev) > > +{ > > + int error = 0; > > I'm sure you put a lot of thought into this already, but is it really > the best approach to assume that busses without runtime pm callbacks > can be suspended? I'd go with an error value by default and only > return 0 as callback return value. Hmm, yes. I think you're right. > > +/** > > + * pm_cancel_suspend - Cancel a pending suspend request for given device. > > + * @dev: Device to cancel the suspend request for. > > + * > > + * Should be called under pm_lock_device() and only if we are sure that the > > + * ->autosuspend() callback hasn't started to yet. > > + */ > > +static void pm_cancel_suspend(struct device *dev) > > +{ > > + dev->power.suspend_aborted = true; > > + cancel_delayed_work(&dev->power.runtime_work); > > + dev->power.runtime_status = RPM_ACTIVE; > > +} > > This pm_lock_device() comment seems to come from old code, no? Correct, I'll fix the comments. > > +/** > > + * pm_runtime_resume - Run a device bus type's runtime_resume() callback. > > + * @dev: Device to resume. > > + * > > + * Check if the device is really suspended and run the ->runtime_resume() > > + * callback provided by the device's bus type driver. Update the run-time PM > > + * flags in the device object to reflect the current status of the device. If > > + * runtime suspend is in progress while this function is being run, wait for it > > + * to finish before resuming the device. If runtime suspend is scheduled, but > > + * it hasn't started yet, cancel it and we're done. > > + */ > > +int pm_runtime_resume(struct device *dev) > > +{ > > + int error = 0; > > Same here, does non-existing runtime pm callbacks really mean we can resume? Well, in fact if we get to the callback and it doesn't exist, that will be a bug. So, I think it's a good idea to return error code in such a case. > > +/** > > + * pm_runtime_disable - Disable run-time power management for given device. > > + * @dev: Device to handle. > > + * > > + * Increase the depth field in the device's dev_pm_info structure, which will > > + * cause the run-time PM functions above to return without doing anything. > > + * If there is a run-time PM operation in progress, wait for it to complete. > > + */ > > +void pm_runtime_disable(struct device *dev) > > +{ > > + might_sleep(); > > + > > + atomic_inc(&dev->power.depth); > > + > > + if (dev->power.runtime_status & RPM_IN_PROGRESS) > > + wait_for_completion(&dev->power.work_done); > > +} > > +EXPORT_SYMBOL_GPL(pm_runtime_disable); > > + > > +/** > > + * pm_runtime_enable - Disable run-time power management for given device. > > + * @dev: Device to handle. > > + * > > + * Enable run-time power management for given device by decreasing the depth > > + * field in its dev_pm_info structure. > > + */ > > +void pm_runtime_enable(struct device *dev) > > +{ > > + if (!atomic_add_unless(&dev->power.depth, -1, 0)) > > + dev_warn(dev, "PM: Excessive pm_runtime_enable()!\n"); > > +} > > +EXPORT_SYMBOL_GPL(pm_runtime_enable); > > Any thoughts on performing ->runtime_resume()/->runtime_suspend() in > enable() and disable()? I guess it's performed too early/late to make > sense from the driver point of view? Some thoughts, yes. As for an implementation, I'd like to wait until at least one bus type uses the framework. > Looking good, thanks a lot for your work on this! Thanks for your comments. > Any chance we can get this included in -rc1? Well, in fact I have already pushed all of the changes I wanted in 2.6.31. Also, I'd like to receive some comments on the $subject patch from the other people. That said, the merge window is still open, so if the comments are supportive and there's a chance to put the final version into linux-next for a couple of days before the merge window ends, I may try to push it to Linus. After all, the patch is not going to introduce any regressions. ;-) Best, Rafael