From: Magnus Damm <magnus.damm@gmail.com> To: "Rafael J. Wysocki" <rjw@sisk.pl> Cc: Alan Stern <stern@rowland.harvard.edu>, Oliver Neukum <oliver@neukum.org>, Magnus Damm <damm@igel.co.jp>, pm list <linux-pm@lists.linux-foundation.org>, LKML <linux-kernel@vger.kernel.org>, Ingo Molnar <mingo@elte.hu>, ACPI Devel Maling List <linux-acpi@vger.kernel.org> Subject: Re: [PATCH] PM: Introduce core framework for run-time PM of I/O devices Date: Sun, 14 Jun 2009 18:41:04 +0900 [thread overview] Message-ID: <aec7e5c30906140241w174e9585y3c5ebac2f4e8537d@mail.gmail.com> (raw) In-Reply-To: <200906140023.36147.rjw@sisk.pl> Hi Rafael, On Sun, Jun 14, 2009 at 7:23 AM, Rafael J. Wysocki<rjw@sisk.pl> 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. 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(). > --- 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. > +/** > + * 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? > +/** > + * 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? > +/** > + * 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? Looking good, thanks a lot for your work on this! Any chance we can get this included in -rc1? / magnus -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
WARNING: multiple messages have this Message-ID (diff)
From: Magnus Damm <magnus.damm@gmail.com> To: "Rafael J. Wysocki" <rjw@sisk.pl> Cc: Alan Stern <stern@rowland.harvard.edu>, Oliver Neukum <oliver@neukum.org>, Magnus Damm <damm@igel.co.jp>, pm list <linux-pm@lists.linux-foundation.org>, LKML <linux-kernel@vger.kernel.org>, Ingo Molnar <mingo@elte.hu>, ACPI Devel Maling List <linux-acpi@vger.kernel.org> Subject: Re: [PATCH] PM: Introduce core framework for run-time PM of I/O devices Date: Sun, 14 Jun 2009 18:41:04 +0900 [thread overview] Message-ID: <aec7e5c30906140241w174e9585y3c5ebac2f4e8537d@mail.gmail.com> (raw) In-Reply-To: <200906140023.36147.rjw@sisk.pl> Hi Rafael, On Sun, Jun 14, 2009 at 7:23 AM, Rafael J. Wysocki<rjw@sisk.pl> 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. 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(). > --- 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. > +/** > + * 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? > +/** > + * 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? > +/** > + * 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? Looking good, thanks a lot for your work on this! Any chance we can get this included in -rc1? / magnus
next prev parent reply other threads:[~2009-06-14 9:41 UTC|newest] Thread overview: 118+ messages / expand[flat|nested] mbox.gz Atom feed top 2009-06-13 22:23 [PATCH] PM: Introduce core framework for run-time PM of I/O devices Rafael J. Wysocki 2009-06-14 9:41 ` Magnus Damm 2009-06-14 9:41 ` Magnus Damm [this message] 2009-06-14 9:41 ` Magnus Damm 2009-06-14 10:29 ` Rafael J. Wysocki 2009-06-14 10:29 ` Rafael J. Wysocki 2009-06-14 9:58 ` [linux-pm] " Rafael J. Wysocki 2009-06-14 22:57 ` [patch update] " Rafael J. Wysocki 2009-06-14 23:18 ` Arjan van de Ven 2009-06-15 20:02 ` Rafael J. Wysocki 2009-06-15 20:02 ` Rafael J. Wysocki 2009-06-14 23:18 ` Arjan van de Ven 2009-06-15 21:08 ` Alan Stern 2009-06-15 21:08 ` Alan Stern 2009-06-15 21:08 ` Alan Stern 2009-06-15 23:21 ` Rafael J. Wysocki 2009-06-16 14:30 ` Alan Stern 2009-06-16 14:30 ` Alan Stern 2009-06-16 14:30 ` Alan Stern 2009-06-16 21:30 ` [patch update 2] " Rafael J. Wysocki 2009-06-16 21:30 ` Rafael J. Wysocki 2009-06-16 22:33 ` [patch update 2 fix] " Rafael J. Wysocki 2009-06-17 20:08 ` Alan Stern 2009-06-17 20:08 ` Alan Stern 2009-06-17 23:07 ` Rafael J. Wysocki 2009-06-18 18:17 ` Alan Stern 2009-06-18 18:17 ` Alan Stern 2009-06-18 18:17 ` Alan Stern 2009-06-19 0:38 ` Rafael J. Wysocki 2009-06-19 0:38 ` Rafael J. Wysocki 2009-06-19 16:25 ` Alan Stern 2009-06-19 16:25 ` Alan Stern 2009-06-19 16:25 ` Alan Stern 2009-06-19 22:42 ` Rafael J. Wysocki 2009-06-20 2:34 ` Alan Stern 2009-06-20 2:34 ` Alan Stern 2009-06-20 2:34 ` Alan Stern 2009-06-20 14:30 ` Alan Stern 2009-06-20 14:30 ` [linux-pm] " Alan Stern 2009-06-20 23:48 ` Rafael J. Wysocki 2009-06-20 23:48 ` [linux-pm] " Rafael J. Wysocki 2009-06-21 2:30 ` Alan Stern 2009-06-21 2:30 ` [linux-pm] " Alan Stern 2009-06-21 11:32 ` Rafael J. Wysocki 2009-06-21 11:32 ` [linux-pm] " Rafael J. Wysocki 2009-06-22 14:16 ` Alan Stern 2009-06-22 15:27 ` Rafael J. Wysocki 2009-06-22 15:27 ` [linux-pm] " Rafael J. Wysocki 2009-06-22 15:39 ` Alan Stern 2009-06-22 15:53 ` Rafael J. Wysocki 2009-06-22 15:53 ` [linux-pm] " Rafael J. Wysocki 2009-06-22 15:39 ` Alan Stern 2009-06-22 14:16 ` Alan Stern 2009-06-22 6:20 ` [linux-pm] " Magnus Damm 2009-06-22 6:20 ` Magnus Damm 2009-06-22 6:43 ` Arjan van de Ven 2009-06-22 6:43 ` Arjan van de Ven 2009-06-22 7:27 ` Magnus Damm 2009-06-22 7:27 ` [linux-pm] " Magnus Damm 2009-06-22 13:49 ` Arjan van de Ven 2009-06-22 13:49 ` [linux-pm] " Arjan van de Ven 2009-06-22 13:49 ` Arjan van de Ven 2009-06-22 15:39 ` Rafael J. Wysocki 2009-06-22 15:39 ` [linux-pm] " Rafael J. Wysocki 2009-06-22 15:33 ` Rafael J. Wysocki 2009-06-22 15:33 ` Rafael J. Wysocki 2009-06-22 6:43 ` Arjan van de Ven 2009-06-22 8:15 ` [linux-pm] " Oliver Neukum 2009-06-22 8:15 ` Oliver Neukum 2009-06-22 6:20 ` Magnus Damm 2009-06-20 23:38 ` [patch update 3] " Rafael J. Wysocki 2009-06-21 2:23 ` Alan Stern 2009-06-21 2:23 ` Alan Stern 2009-06-21 2:23 ` Alan Stern 2009-06-21 12:46 ` Rafael J. Wysocki 2009-06-21 12:46 ` Rafael J. Wysocki 2009-06-22 15:01 ` Alan Stern 2009-06-22 15:01 ` Alan Stern 2009-06-22 15:49 ` Rafael J. Wysocki 2009-06-22 15:49 ` Rafael J. Wysocki 2009-06-22 16:28 ` Alan Stern 2009-06-22 16:28 ` Alan Stern 2009-06-22 16:28 ` Alan Stern 2009-06-22 23:02 ` Rafael J. Wysocki 2009-06-22 23:02 ` Rafael J. Wysocki 2009-06-23 17:02 ` Alan Stern 2009-06-23 17:02 ` Alan Stern 2009-06-23 17:02 ` Alan Stern 2009-06-23 17:45 ` Rafael J. Wysocki 2009-06-23 18:26 ` Alan Stern 2009-06-23 18:26 ` Alan Stern 2009-06-23 18:26 ` Alan Stern 2009-06-24 0:17 ` Rafael J. Wysocki 2009-06-24 0:17 ` Rafael J. Wysocki 2009-06-24 14:51 ` Alan Stern 2009-06-24 14:51 ` Alan Stern 2009-06-24 19:14 ` Rafael J. Wysocki 2009-06-24 19:14 ` Rafael J. Wysocki 2009-06-24 20:19 ` Alan Stern 2009-06-24 20:19 ` Alan Stern 2009-06-24 21:23 ` Rafael J. Wysocki 2009-06-24 21:23 ` Rafael J. Wysocki 2009-06-23 17:45 ` Rafael J. Wysocki 2009-06-20 23:38 ` Rafael J. Wysocki 2009-06-19 22:42 ` [patch update 2 fix] " Rafael J. Wysocki 2009-06-17 23:07 ` Rafael J. Wysocki 2009-06-17 20:08 ` Alan Stern 2009-06-16 22:33 ` Rafael J. Wysocki 2009-06-15 23:21 ` [patch update] " Rafael J. Wysocki 2009-06-24 15:04 ` Pavel Machek 2009-06-27 21:52 ` Rafael J. Wysocki 2009-07-06 8:28 ` Pavel Machek 2009-07-06 8:28 ` Pavel Machek 2009-06-27 21:52 ` Rafael J. Wysocki 2009-06-24 15:04 ` Pavel Machek 2009-06-14 22:57 ` Rafael J. Wysocki 2009-06-14 9:58 ` [PATCH] " Rafael J. Wysocki -- strict thread matches above, loose matches on Subject: below -- 2009-06-13 22:23 Rafael J. Wysocki
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=aec7e5c30906140241w174e9585y3c5ebac2f4e8537d@mail.gmail.com \ --to=magnus.damm@gmail.com \ --cc=damm@igel.co.jp \ --cc=linux-acpi@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pm@lists.linux-foundation.org \ --cc=mingo@elte.hu \ --cc=oliver@neukum.org \ --cc=rjw@sisk.pl \ --cc=stern@rowland.harvard.edu \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.