From: Kevin Hilman <khilman@ti.com> To: "Rafael J. Wysocki" <rjw@sisk.pl> Cc: Alan Stern <stern@rowland.harvard.edu>, "Ohad Ben-Cohen" <ohad@wizery.com>, linux-pm@lists.linux-foundation.org, Johannes Berg <johannes@sipsolutions.net>, linux-wireless@vger.kernel.org, linux-mmc@vger.kernel.org, Ido Yariv <ido@wizery.com> Subject: Re: [linux-pm] subtle pm_runtime_put_sync race and sdio functions Date: Wed, 26 Jan 2011 15:28:20 -0800 [thread overview] Message-ID: <87k4hrtfcb.fsf@ti.com> (raw) In-Reply-To: <201012221329.40251.rjw@sisk.pl> (Rafael J. Wysocki's message of "Wed, 22 Dec 2010 13:29:40 +0100") "Rafael J. Wysocki" <rjw@sisk.pl> writes: > On Wednesday, December 22, 2010, Alan Stern wrote: >> On Tue, 21 Dec 2010, Rafael J. Wysocki wrote: >> >> > It basically goes like this. There's device A that is only resumed when it's >> > needed to carry out an operation and is suspended immediately after that. >> > There's another device B that needs A to do something during its suspend. >> > So, when the suspend of B is started, A is woken up, does its work and is >> > suspended again (using pm_runtime_suspend()). Then B is suspended. >> > >> > We currently require that ->suspend() and ->resume() callbacks be defined >> > for A (presumably pointing to the same code as its runtime callbacks) so that >> > things work correctly, but perhaps we can just relax this requirement a bit? >> > I'm not 100% sure that's a good idea, just considering it. >> >> I still don't know. It would require a lot of special conditions: no >> child devices, not runtime-PM-disabled, not runtime-PM-forbidden... >> Also, A's parent would have to be coded carefully; otherwise A's >> runtime resume would prevent the parent from suspending. >> >> This just doesn't fit very well with the runtime PM model, or at least, >> not in the form you described. >> >> Consider this instead: Since A is required to be functional before B >> can be used, A must be registered before B and hence B gets suspended >> before A. Therefore during the prepare phase we can runtime-resume A >> and leave it powered up; when B needs to suspend, it won't matter that >> the runtime-PM calls are ineffective. > > We don't really need to do that, because the runtime resume _is_ functional > during system suspend. The only thing missing is a ->suspend() callback for A > (and a corresponding ->resume() callback to make sure A will be available to > B during system resume). > OK, I'm finally back to debugging this problem and looking for a final solution. I agree that what is needed is ->suspend() and ->resume() callbacks for A, but the question remains how to implement them. In my case, A doesn't need runtime callbacks, but *does* require that the subsystem callbacks are called because the subsystem actually does all the real PM work. On OMAP, the device PM code (clock mgmt, device low-power states, etc.) is common for all on-chip devices, so is handled in common code at the subsystem level (in this case, platform_bus.) Therefore, what is ideally needed is the ability for A's suspend to simply call pm_runtime_suspend() so the subsystem can do the work. However, since runtime transitions are locked out by this time, that doesn't work. IOW, what is needed is a way for a system suspend to say "please finish the runtime suspend that was already requested." What I've done to work around this in driver A is to manually check pm_runtime_suspended() and directly call the subsystem's runtime suspend/resume (patch below[1]. NOTE, I've used the _noirq methods to ensure device A is available when device B needs it.) While this works, I'm not crazy about it since it requires the driver know about the subsystem (in this case the bus) where the real PM work is done. IMO, it would be much more intuitive (and readable) if the driver's suspend hooks could simply trigger a runtime suspend (either a new one, or one already requested.) FWIW, another hack that I've experimented with is just to just re-enable runtime transitions by doing a pm_runtime_put_sync() in the suspend_noirq method and a pm_runtime_get_noresume() in the resume_noirq method. This allows driver A to suspend since it has already requested runtime, but I don't expect this second approach to be popluar. :) This second approach also doen't address system suspend/resume when the user has disabled runtime PM via /sys/devices/.../power/control. Kevin [1] diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c index b605ff3..a4bc15a 100644 --- a/drivers/i2c/busses/i2c-omap.c +++ b/drivers/i2c/busses/i2c-omap.c @@ -1137,12 +1137,40 @@ omap_i2c_remove(struct platform_device *pdev) return 0; } +#ifdef CONFIG_SUSPEND +static int omap_i2c_suspend(struct device *dev) +{ + if (!pm_runtime_suspended(dev)) + if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_suspend) + dev->bus->pm->runtime_suspend(dev); + + return 0; +} + +static int omap_i2c_resume(struct device *dev) +{ + if (!pm_runtime_suspended(dev)) + if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_resume) + dev->bus->pm->runtime_resume(dev); + + return 0; +} + +static struct dev_pm_ops omap_i2c_pm_ops = { + .suspend_noirq = omap_i2c_suspend, + .resume_noirq = omap_i2c_resume, +}; +#else +#define omap_i2c_pm_ops NULL +#endif + static struct platform_driver omap_i2c_driver = { .probe = omap_i2c_probe, .remove = omap_i2c_remove, .driver = { .name = "omap_i2c", .owner = THIS_MODULE, + .pm = &omap_i2c_pm_ops, }, };
WARNING: multiple messages have this Message-ID (diff)
From: Kevin Hilman <khilman@ti.com> To: "Rafael J. Wysocki" <rjw@sisk.pl> Cc: linux-mmc@vger.kernel.org, linux-wireless@vger.kernel.org, Ido Yariv <ido@wizery.com>, linux-pm@lists.linux-foundation.org, Johannes Berg <johannes@sipsolutions.net> Subject: Re: subtle pm_runtime_put_sync race and sdio functions Date: Wed, 26 Jan 2011 15:28:20 -0800 [thread overview] Message-ID: <87k4hrtfcb.fsf@ti.com> (raw) In-Reply-To: <201012221329.40251.rjw@sisk.pl> (Rafael J. Wysocki's message of "Wed, 22 Dec 2010 13:29:40 +0100") "Rafael J. Wysocki" <rjw@sisk.pl> writes: > On Wednesday, December 22, 2010, Alan Stern wrote: >> On Tue, 21 Dec 2010, Rafael J. Wysocki wrote: >> >> > It basically goes like this. There's device A that is only resumed when it's >> > needed to carry out an operation and is suspended immediately after that. >> > There's another device B that needs A to do something during its suspend. >> > So, when the suspend of B is started, A is woken up, does its work and is >> > suspended again (using pm_runtime_suspend()). Then B is suspended. >> > >> > We currently require that ->suspend() and ->resume() callbacks be defined >> > for A (presumably pointing to the same code as its runtime callbacks) so that >> > things work correctly, but perhaps we can just relax this requirement a bit? >> > I'm not 100% sure that's a good idea, just considering it. >> >> I still don't know. It would require a lot of special conditions: no >> child devices, not runtime-PM-disabled, not runtime-PM-forbidden... >> Also, A's parent would have to be coded carefully; otherwise A's >> runtime resume would prevent the parent from suspending. >> >> This just doesn't fit very well with the runtime PM model, or at least, >> not in the form you described. >> >> Consider this instead: Since A is required to be functional before B >> can be used, A must be registered before B and hence B gets suspended >> before A. Therefore during the prepare phase we can runtime-resume A >> and leave it powered up; when B needs to suspend, it won't matter that >> the runtime-PM calls are ineffective. > > We don't really need to do that, because the runtime resume _is_ functional > during system suspend. The only thing missing is a ->suspend() callback for A > (and a corresponding ->resume() callback to make sure A will be available to > B during system resume). > OK, I'm finally back to debugging this problem and looking for a final solution. I agree that what is needed is ->suspend() and ->resume() callbacks for A, but the question remains how to implement them. In my case, A doesn't need runtime callbacks, but *does* require that the subsystem callbacks are called because the subsystem actually does all the real PM work. On OMAP, the device PM code (clock mgmt, device low-power states, etc.) is common for all on-chip devices, so is handled in common code at the subsystem level (in this case, platform_bus.) Therefore, what is ideally needed is the ability for A's suspend to simply call pm_runtime_suspend() so the subsystem can do the work. However, since runtime transitions are locked out by this time, that doesn't work. IOW, what is needed is a way for a system suspend to say "please finish the runtime suspend that was already requested." What I've done to work around this in driver A is to manually check pm_runtime_suspended() and directly call the subsystem's runtime suspend/resume (patch below[1]. NOTE, I've used the _noirq methods to ensure device A is available when device B needs it.) While this works, I'm not crazy about it since it requires the driver know about the subsystem (in this case the bus) where the real PM work is done. IMO, it would be much more intuitive (and readable) if the driver's suspend hooks could simply trigger a runtime suspend (either a new one, or one already requested.) FWIW, another hack that I've experimented with is just to just re-enable runtime transitions by doing a pm_runtime_put_sync() in the suspend_noirq method and a pm_runtime_get_noresume() in the resume_noirq method. This allows driver A to suspend since it has already requested runtime, but I don't expect this second approach to be popluar. :) This second approach also doen't address system suspend/resume when the user has disabled runtime PM via /sys/devices/.../power/control. Kevin [1] diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c index b605ff3..a4bc15a 100644 --- a/drivers/i2c/busses/i2c-omap.c +++ b/drivers/i2c/busses/i2c-omap.c @@ -1137,12 +1137,40 @@ omap_i2c_remove(struct platform_device *pdev) return 0; } +#ifdef CONFIG_SUSPEND +static int omap_i2c_suspend(struct device *dev) +{ + if (!pm_runtime_suspended(dev)) + if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_suspend) + dev->bus->pm->runtime_suspend(dev); + + return 0; +} + +static int omap_i2c_resume(struct device *dev) +{ + if (!pm_runtime_suspended(dev)) + if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_resume) + dev->bus->pm->runtime_resume(dev); + + return 0; +} + +static struct dev_pm_ops omap_i2c_pm_ops = { + .suspend_noirq = omap_i2c_suspend, + .resume_noirq = omap_i2c_resume, +}; +#else +#define omap_i2c_pm_ops NULL +#endif + static struct platform_driver omap_i2c_driver = { .probe = omap_i2c_probe, .remove = omap_i2c_remove, .driver = { .name = "omap_i2c", .owner = THIS_MODULE, + .pm = &omap_i2c_pm_ops, }, };
next prev parent reply other threads:[~2011-01-26 23:35 UTC|newest] Thread overview: 179+ messages / expand[flat|nested] mbox.gz Atom feed top 2010-12-09 23:37 subtle pm_runtime_put_sync race and sdio functions Ohad Ben-Cohen 2010-12-10 0:00 ` Rafael J. Wysocki 2010-12-10 0:00 ` [linux-pm] " Rafael J. Wysocki 2010-12-10 17:24 ` Alan Stern 2010-12-10 17:24 ` [linux-pm] " Alan Stern 2010-12-10 22:01 ` Rafael J. Wysocki 2010-12-10 22:01 ` [linux-pm] " Rafael J. Wysocki 2010-12-10 23:02 ` Ohad Ben-Cohen 2010-12-10 23:02 ` [linux-pm] " Ohad Ben-Cohen 2010-12-10 22:59 ` Ohad Ben-Cohen 2010-12-10 22:59 ` [linux-pm] " Ohad Ben-Cohen 2010-12-11 1:17 ` Ohad Ben-Cohen 2010-12-11 14:53 ` Alan Stern 2010-12-11 1:17 ` Ohad Ben-Cohen 2010-12-11 14:50 ` Alan Stern 2010-12-18 13:29 ` Ohad Ben-Cohen 2010-12-18 13:29 ` [linux-pm] " Ohad Ben-Cohen 2010-12-18 14:16 ` David Vrabel 2010-12-18 15:12 ` Ohad Ben-Cohen 2010-12-18 15:12 ` Ohad Ben-Cohen 2010-12-18 14:16 ` David Vrabel 2010-12-18 15:07 ` [linux-pm] " Rafael J. Wysocki 2010-12-18 16:00 ` Ohad Ben-Cohen 2010-12-18 16:00 ` [linux-pm] " Ohad Ben-Cohen 2010-12-18 16:40 ` Johannes Berg 2010-12-18 19:08 ` Ohad Ben-Cohen 2010-12-18 19:08 ` Ohad Ben-Cohen 2010-12-18 21:30 ` Alan Stern 2010-12-18 21:30 ` Alan Stern 2010-12-18 22:57 ` Rafael J. Wysocki 2010-12-18 22:57 ` [linux-pm] " Rafael J. Wysocki 2010-12-18 21:30 ` Alan Stern 2010-12-18 22:52 ` Rafael J. Wysocki 2010-12-18 22:52 ` [linux-pm] " Rafael J. Wysocki 2010-12-18 19:08 ` Ohad Ben-Cohen 2010-12-18 21:29 ` [linux-pm] " Alan Stern 2010-12-18 21:29 ` Alan Stern 2010-12-18 21:29 ` Alan Stern 2010-12-18 22:50 ` Rafael J. Wysocki 2010-12-18 22:50 ` [linux-pm] " Rafael J. Wysocki 2010-12-18 16:40 ` Johannes Berg 2010-12-18 22:47 ` [linux-pm] " Rafael J. Wysocki 2010-12-18 22:47 ` Rafael J. Wysocki 2010-12-19 7:48 ` Ohad Ben-Cohen 2010-12-19 7:48 ` Ohad Ben-Cohen 2010-12-19 7:48 ` Ohad Ben-Cohen 2010-12-19 10:22 ` Rafael J. Wysocki 2010-12-19 10:22 ` [linux-pm] " Rafael J. Wysocki 2010-12-20 3:37 ` Alan Stern 2010-12-20 3:37 ` Alan Stern 2010-12-20 21:17 ` Rafael J. Wysocki 2010-12-21 0:57 ` Alan Stern 2010-12-21 0:57 ` Alan Stern 2010-12-21 21:31 ` Rafael J. Wysocki 2010-12-22 1:42 ` Alan Stern 2010-12-22 1:42 ` Alan Stern 2010-12-22 12:29 ` Rafael J. Wysocki 2010-12-22 12:29 ` Rafael J. Wysocki 2011-01-26 23:28 ` Kevin Hilman [this message] 2011-01-26 23:28 ` Kevin Hilman 2011-01-27 18:13 ` Alan Stern 2011-01-27 18:13 ` [linux-pm] " Alan Stern 2011-01-27 18:13 ` Alan Stern 2011-01-27 19:22 ` Kevin Hilman 2011-01-27 19:22 ` [linux-pm] " Kevin Hilman 2011-01-27 19:22 ` Kevin Hilman 2011-01-27 19:49 ` Alan Stern 2011-01-27 19:49 ` [linux-pm] " Alan Stern 2011-01-27 19:49 ` Alan Stern 2011-01-27 20:15 ` Kevin Hilman 2011-01-27 20:15 ` [linux-pm] " Kevin Hilman 2011-01-27 20:15 ` Kevin Hilman 2011-01-27 22:18 ` Vitaly Wool 2011-01-27 22:18 ` Vitaly Wool 2011-01-27 22:18 ` Vitaly Wool 2011-01-27 23:21 ` Rafael J. Wysocki 2011-01-27 23:21 ` [linux-pm] " Rafael J. Wysocki 2011-01-27 23:49 ` Kevin Hilman 2011-01-27 23:49 ` [linux-pm] " Kevin Hilman 2011-01-27 23:11 ` Rafael J. Wysocki 2011-01-27 23:11 ` [linux-pm] " Rafael J. Wysocki 2011-01-27 18:20 ` Vitaly Wool 2011-01-27 18:20 ` [linux-pm] " Vitaly Wool 2011-01-27 18:20 ` Vitaly Wool 2011-01-27 18:54 ` Kevin Hilman 2011-01-27 18:54 ` [linux-pm] " Kevin Hilman 2010-12-22 12:29 ` Rafael J. Wysocki 2010-12-22 1:42 ` Alan Stern 2010-12-21 21:31 ` Rafael J. Wysocki 2010-12-21 0:57 ` Alan Stern 2010-12-20 21:17 ` Rafael J. Wysocki 2010-12-20 3:37 ` Alan Stern 2010-12-21 22:23 ` Kevin Hilman 2010-12-21 22:23 ` [linux-pm] " Kevin Hilman 2010-12-22 1:48 ` Alan Stern 2010-12-22 1:48 ` Alan Stern 2010-12-22 1:48 ` Alan Stern 2010-12-23 7:51 ` Ohad Ben-Cohen 2010-12-23 7:51 ` [linux-pm] " Ohad Ben-Cohen 2010-12-23 16:03 ` Alan Stern 2010-12-23 16:03 ` [linux-pm] " Alan Stern 2010-12-23 16:03 ` Alan Stern 2010-12-25 7:34 ` Ohad Ben-Cohen 2010-12-25 7:34 ` Ohad Ben-Cohen 2010-12-25 16:21 ` Alan Stern 2010-12-25 16:21 ` Alan Stern 2010-12-25 20:58 ` Ohad Ben-Cohen 2010-12-25 20:58 ` [linux-pm] " Ohad Ben-Cohen 2010-12-25 20:58 ` Ohad Ben-Cohen 2010-12-25 21:50 ` Vitaly Wool 2010-12-25 21:50 ` [linux-pm] " Vitaly Wool 2010-12-26 5:27 ` Ohad Ben-Cohen 2010-12-26 5:27 ` [linux-pm] " Ohad Ben-Cohen 2010-12-25 21:54 ` Vitaly Wool 2010-12-25 21:54 ` Vitaly Wool 2010-12-25 21:54 ` Vitaly Wool 2010-12-26 2:48 ` [linux-pm] " Alan Stern 2010-12-26 2:48 ` Alan Stern 2010-12-26 5:55 ` Ohad Ben-Cohen 2010-12-26 5:55 ` [linux-pm] " Ohad Ben-Cohen 2010-12-26 5:55 ` Ohad Ben-Cohen 2010-12-26 11:45 ` Rafael J. Wysocki 2010-12-26 12:43 ` Ohad Ben-Cohen 2010-12-26 12:43 ` [linux-pm] " Ohad Ben-Cohen 2010-12-26 12:43 ` Ohad Ben-Cohen 2010-12-26 18:35 ` Rafael J. Wysocki 2010-12-28 19:11 ` Ohad Ben-Cohen 2010-12-28 19:11 ` [linux-pm] " Ohad Ben-Cohen 2010-12-28 19:21 ` Rafael J. Wysocki 2010-12-28 19:21 ` [linux-pm] " Rafael J. Wysocki 2010-12-28 19:21 ` Rafael J. Wysocki 2010-12-28 19:34 ` Ohad Ben-Cohen 2010-12-28 20:36 ` Rafael J. Wysocki 2010-12-28 20:36 ` [linux-pm] " Rafael J. Wysocki 2010-12-28 19:34 ` Ohad Ben-Cohen 2010-12-26 18:35 ` Rafael J. Wysocki 2010-12-26 14:53 ` [linux-pm] " Ohad Ben-Cohen 2010-12-26 18:37 ` Rafael J. Wysocki 2010-12-26 18:37 ` [linux-pm] " Rafael J. Wysocki 2010-12-28 19:15 ` Ohad Ben-Cohen 2010-12-28 20:04 ` Rafael J. Wysocki 2010-12-28 20:04 ` [linux-pm] " Rafael J. Wysocki 2010-12-28 20:04 ` Rafael J. Wysocki 2010-12-28 20:41 ` Ohad Ben-Cohen 2010-12-28 20:41 ` [linux-pm] " Ohad Ben-Cohen 2010-12-28 20:41 ` Ohad Ben-Cohen 2010-12-28 19:15 ` Ohad Ben-Cohen 2010-12-26 14:53 ` Ohad Ben-Cohen 2010-12-26 11:45 ` Rafael J. Wysocki 2010-12-26 17:00 ` [linux-pm] " Alan Stern 2010-12-26 17:00 ` Alan Stern 2010-12-28 19:04 ` Ohad Ben-Cohen 2010-12-28 19:04 ` [linux-pm] " Ohad Ben-Cohen 2010-12-28 19:04 ` Ohad Ben-Cohen 2010-12-28 21:46 ` Alan Stern 2010-12-28 21:46 ` [linux-pm] " Alan Stern 2010-12-28 21:46 ` Alan Stern 2010-12-29 6:34 ` Ohad Ben-Cohen 2010-12-30 4:25 ` Alan Stern 2010-12-30 4:25 ` Alan Stern 2010-12-30 4:25 ` Alan Stern 2010-12-29 6:34 ` Ohad Ben-Cohen 2010-12-29 8:01 ` Ohad Ben-Cohen 2010-12-29 8:01 ` [linux-pm] " Ohad Ben-Cohen 2010-12-30 4:30 ` Alan Stern 2010-12-30 4:30 ` [linux-pm] " Alan Stern 2010-12-30 4:30 ` Alan Stern 2010-12-26 17:00 ` Alan Stern 2010-12-26 2:48 ` Alan Stern 2010-12-25 16:21 ` Alan Stern 2010-12-25 7:34 ` Ohad Ben-Cohen 2010-12-18 22:47 ` Rafael J. Wysocki 2010-12-18 15:07 ` Rafael J. Wysocki 2010-12-18 21:20 ` [linux-pm] " Alan Stern 2010-12-18 23:03 ` Rafael J. Wysocki 2010-12-18 23:03 ` Rafael J. Wysocki 2010-12-19 10:00 ` [linux-pm] " Ohad Ben-Cohen 2010-12-19 10:00 ` Ohad Ben-Cohen 2010-12-18 21:20 ` Alan Stern
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=87k4hrtfcb.fsf@ti.com \ --to=khilman@ti.com \ --cc=ido@wizery.com \ --cc=johannes@sipsolutions.net \ --cc=linux-mmc@vger.kernel.org \ --cc=linux-pm@lists.linux-foundation.org \ --cc=linux-wireless@vger.kernel.org \ --cc=ohad@wizery.com \ --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.