From: Ulf Hansson <ulf.hansson@linaro.org> To: "Rafael J. Wysocki" <rjw@rjwysocki.net> Cc: "Rafael J. Wysocki" <rafael@kernel.org>, Wolfram Sang <wsa@the-dreams.de>, Len Brown <lenb@kernel.org>, ACPI Devel Maling List <linux-acpi@vger.kernel.org>, Linux PM <linux-pm@vger.kernel.org>, Kevin Hilman <khilman@kernel.org>, Jarkko Nikula <jarkko.nikula@linux.intel.com>, Andy Shevchenko <andriy.shevchenko@linux.intel.com>, Mika Westerberg <mika.westerberg@linux.intel.com>, Jisheng Zhang <jszhang@marvell.com>, John Stultz <john.stultz@linaro.org>, Guodong Xu <guodong.xu@linaro.org>, Sumit Semwal <sumit.semwal@linaro.org>, Haojian Zhuang <haojian.zhuang@linaro.org>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>, linux-i2c <linux-i2c@vger.kernel.org> Subject: Re: [PATCH v2 5/9] PM / ACPI: Provide option to disable direct_complete for ACPI devices Date: Fri, 25 Aug 2017 11:28:25 +0200 [thread overview] Message-ID: <CAPDyKFrFUgE0FJE==ht4Z5utj+Pht=9UGn-LYBR9hP4ncofreg@mail.gmail.com> (raw) In-Reply-To: <12333551.k204JYSgao@aspire.rjw.lan> [...] >> >> The above change would offer an improvement, however there are more >> benefits from the runtime PM centric path that it doesn't cover. > > The idea, though, is not to make it instead of, but in addition to the > runtime PM centric path. > > So a driver using the runtime PM centric (or rather runtime PM aware) > callbacks can set power.force_suspend and benefit from having direct_complete > set conditional on whether or not the device is runtime resumed before the > late suspend. > > If device_complete remains set in __device_suspend_late(), no system > suspend/resume callbacks will be invoked from that point on. If it isn't > set, pm_runtime_force_suspend() should work just fine and then > pm_runtime_force_resume() should do the right thing on the way back. > > All boils down to (a) setting a flag and (b) using the right callbacks > for system suspend which is the case with your patches either. You make it sound simple. :-) I try to get inspired of your ideas and see what I can come up with. However, I really want us to avoid making it more difficult to understand the behavior of the core, so I probably look start changing the ACPI PM domain first, then trying to adopt the behavior of the core on top of those changes. Does that sound reasonable to you? > >> The below is pasted from the changelog for patch9 (I will make sure to >> fold in this in the changelog for $subject patch next version). >> >> In case the device is/gets runtime resumed before the device_suspend() >> phase is entered, the PM core doesn't run the direct_complete path for >> the device during system sleep. During system resume, this lead to >> that the device will always be brought back to full power when the >> i2c-dw-plat driver's ->resume() callback is invoked. This may not be >> necessary, thus increasing the resume time for the device and wasting >> power. > > Well, I guess that depends on what is there in its resume callbacks. > >> In the runtime PM centric path, the pm_runtime_force_resume() helper >> takes better care, as it resumes the device only in case it's really >> needed. Normally this means it can be postponed to be dealt with via >> regular calls to runtime PM (pm_runtime_get*()) instead. In other >> words, the device remains in its low power state until someone request >> a new i2c transfer, whenever that may be. >> >> As far as I can think of, the direct_complete path can't be adjusted >> to support this. Or can it? > > It surely can, it's just software. :-) The question is how complicated that > is going to be in the end, however. > > So the patch I sent didn't address the dependency issue, but it allows the > core to deal with the issue which is a core issue, not an ACPI PM domain > issue, because the core disables runtime PM for devices with direct_complete > set, not the ACPI PM domain or PCI etc. > > [BTW, it is not entirely clear to me why it ever is necessary to runtime resume > a device with direct_complete set after __device_suspend(), because it can only > have direct_complete set at that point if all of the hierarchy below it has > this flag set too and so runtime PM has to be disabled for all of those > devices as well.] > > It can be made slightly better by adding a piece to avoid clearing the > parent's direct_complete if it had the new flag set, so below is a new > version (with the new flag renamed to safe_suspend which seemed better to > me). > > Of course, having safe_suspend set will still cause the parent's > direct_complete to be cleared unless the parent has safe_suspend set too, > but that's better than not allowing the parent to use direct_complete at all. Well, as I replied in the earlier response, deploying the runtime PM centric path also for the parent, offers additional optimizations while comparing to try to make the direct_complete path still to work. We could also pick that approach, which would mean additional arguments of moving drivers to the runtime PM centric path, of course only where it makes sense. > > --- > drivers/acpi/device_pm.c | 8 ++++++++ > drivers/base/power/main.c | 15 ++++++++++++--- > include/linux/pm.h | 1 + > 3 files changed, 21 insertions(+), 3 deletions(-) > > Index: linux-pm/drivers/base/power/main.c > =================================================================== > --- linux-pm.orig/drivers/base/power/main.c > +++ linux-pm/drivers/base/power/main.c > @@ -1271,9 +1271,16 @@ static int __device_suspend_late(struct > goto Complete; > } > > - if (dev->power.syscore || dev->power.direct_complete) > + if (dev->power.syscore) > goto Complete; > > + if (dev->power.direct_complete) { > + if (pm_runtime_status_suspended(dev)) > + goto Complete; > + > + dev->power.direct_complete = false; > + } > + > if (dev->pm_domain) { > info = "late power domain "; > callback = pm_late_early_op(&dev->pm_domain->ops, state); > @@ -1482,7 +1489,7 @@ static int __device_suspend(struct devic > if (dev->power.syscore) > goto Complete; > > - if (dev->power.direct_complete) { > + if (dev->power.direct_complete && !dev->power.safe_suspend) { > if (pm_runtime_status_suspended(dev)) { > pm_runtime_disable(dev); > if (pm_runtime_status_suspended(dev)) > @@ -1549,7 +1556,9 @@ static int __device_suspend(struct devic > if (parent) { > spin_lock_irq(&parent->power.lock); > > - dev->parent->power.direct_complete = false; > + if (!dev->parent->power.safe_suspend) > + dev->parent->power.direct_complete = false; > + > if (dev->power.wakeup_path > && !dev->parent->power.ignore_children) > dev->parent->power.wakeup_path = true; > Index: linux-pm/include/linux/pm.h > =================================================================== > --- linux-pm.orig/include/linux/pm.h > +++ linux-pm/include/linux/pm.h > @@ -554,6 +554,7 @@ struct dev_pm_info { > pm_message_t power_state; > unsigned int can_wakeup:1; > unsigned int async_suspend:1; > + unsigned int safe_suspend:1; > bool in_dpm_list:1; /* Owned by the PM core */ > bool is_prepared:1; /* Owned by the PM core */ > bool is_suspended:1; /* Ditto */ > Index: linux-pm/drivers/acpi/device_pm.c > =================================================================== > --- linux-pm.orig/drivers/acpi/device_pm.c > +++ linux-pm/drivers/acpi/device_pm.c > @@ -1025,9 +1025,17 @@ EXPORT_SYMBOL_GPL(acpi_subsys_prepare); > * > * Follow PCI and resume devices suspended at run time before running their > * system suspend callbacks. > + * > + * If the power.direct_complete flag is set for the device, though, skip all > + * that, because the device doesn't need to be resumed then and if it is > + * resumed via runtime PM, the core will notice that and will carry out the > + * late suspend for it. > */ > int acpi_subsys_suspend(struct device *dev) > { > + if (dev->power.direct_complete) > + return 0; > + > pm_runtime_resume(dev); > return pm_generic_suspend(dev); > } > Kind regards Uffe
WARNING: multiple messages have this Message-ID (diff)
From: ulf.hansson@linaro.org (Ulf Hansson) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v2 5/9] PM / ACPI: Provide option to disable direct_complete for ACPI devices Date: Fri, 25 Aug 2017 11:28:25 +0200 [thread overview] Message-ID: <CAPDyKFrFUgE0FJE==ht4Z5utj+Pht=9UGn-LYBR9hP4ncofreg@mail.gmail.com> (raw) In-Reply-To: <12333551.k204JYSgao@aspire.rjw.lan> [...] >> >> The above change would offer an improvement, however there are more >> benefits from the runtime PM centric path that it doesn't cover. > > The idea, though, is not to make it instead of, but in addition to the > runtime PM centric path. > > So a driver using the runtime PM centric (or rather runtime PM aware) > callbacks can set power.force_suspend and benefit from having direct_complete > set conditional on whether or not the device is runtime resumed before the > late suspend. > > If device_complete remains set in __device_suspend_late(), no system > suspend/resume callbacks will be invoked from that point on. If it isn't > set, pm_runtime_force_suspend() should work just fine and then > pm_runtime_force_resume() should do the right thing on the way back. > > All boils down to (a) setting a flag and (b) using the right callbacks > for system suspend which is the case with your patches either. You make it sound simple. :-) I try to get inspired of your ideas and see what I can come up with. However, I really want us to avoid making it more difficult to understand the behavior of the core, so I probably look start changing the ACPI PM domain first, then trying to adopt the behavior of the core on top of those changes. Does that sound reasonable to you? > >> The below is pasted from the changelog for patch9 (I will make sure to >> fold in this in the changelog for $subject patch next version). >> >> In case the device is/gets runtime resumed before the device_suspend() >> phase is entered, the PM core doesn't run the direct_complete path for >> the device during system sleep. During system resume, this lead to >> that the device will always be brought back to full power when the >> i2c-dw-plat driver's ->resume() callback is invoked. This may not be >> necessary, thus increasing the resume time for the device and wasting >> power. > > Well, I guess that depends on what is there in its resume callbacks. > >> In the runtime PM centric path, the pm_runtime_force_resume() helper >> takes better care, as it resumes the device only in case it's really >> needed. Normally this means it can be postponed to be dealt with via >> regular calls to runtime PM (pm_runtime_get*()) instead. In other >> words, the device remains in its low power state until someone request >> a new i2c transfer, whenever that may be. >> >> As far as I can think of, the direct_complete path can't be adjusted >> to support this. Or can it? > > It surely can, it's just software. :-) The question is how complicated that > is going to be in the end, however. > > So the patch I sent didn't address the dependency issue, but it allows the > core to deal with the issue which is a core issue, not an ACPI PM domain > issue, because the core disables runtime PM for devices with direct_complete > set, not the ACPI PM domain or PCI etc. > > [BTW, it is not entirely clear to me why it ever is necessary to runtime resume > a device with direct_complete set after __device_suspend(), because it can only > have direct_complete set at that point if all of the hierarchy below it has > this flag set too and so runtime PM has to be disabled for all of those > devices as well.] > > It can be made slightly better by adding a piece to avoid clearing the > parent's direct_complete if it had the new flag set, so below is a new > version (with the new flag renamed to safe_suspend which seemed better to > me). > > Of course, having safe_suspend set will still cause the parent's > direct_complete to be cleared unless the parent has safe_suspend set too, > but that's better than not allowing the parent to use direct_complete at all. Well, as I replied in the earlier response, deploying the runtime PM centric path also for the parent, offers additional optimizations while comparing to try to make the direct_complete path still to work. We could also pick that approach, which would mean additional arguments of moving drivers to the runtime PM centric path, of course only where it makes sense. > > --- > drivers/acpi/device_pm.c | 8 ++++++++ > drivers/base/power/main.c | 15 ++++++++++++--- > include/linux/pm.h | 1 + > 3 files changed, 21 insertions(+), 3 deletions(-) > > Index: linux-pm/drivers/base/power/main.c > =================================================================== > --- linux-pm.orig/drivers/base/power/main.c > +++ linux-pm/drivers/base/power/main.c > @@ -1271,9 +1271,16 @@ static int __device_suspend_late(struct > goto Complete; > } > > - if (dev->power.syscore || dev->power.direct_complete) > + if (dev->power.syscore) > goto Complete; > > + if (dev->power.direct_complete) { > + if (pm_runtime_status_suspended(dev)) > + goto Complete; > + > + dev->power.direct_complete = false; > + } > + > if (dev->pm_domain) { > info = "late power domain "; > callback = pm_late_early_op(&dev->pm_domain->ops, state); > @@ -1482,7 +1489,7 @@ static int __device_suspend(struct devic > if (dev->power.syscore) > goto Complete; > > - if (dev->power.direct_complete) { > + if (dev->power.direct_complete && !dev->power.safe_suspend) { > if (pm_runtime_status_suspended(dev)) { > pm_runtime_disable(dev); > if (pm_runtime_status_suspended(dev)) > @@ -1549,7 +1556,9 @@ static int __device_suspend(struct devic > if (parent) { > spin_lock_irq(&parent->power.lock); > > - dev->parent->power.direct_complete = false; > + if (!dev->parent->power.safe_suspend) > + dev->parent->power.direct_complete = false; > + > if (dev->power.wakeup_path > && !dev->parent->power.ignore_children) > dev->parent->power.wakeup_path = true; > Index: linux-pm/include/linux/pm.h > =================================================================== > --- linux-pm.orig/include/linux/pm.h > +++ linux-pm/include/linux/pm.h > @@ -554,6 +554,7 @@ struct dev_pm_info { > pm_message_t power_state; > unsigned int can_wakeup:1; > unsigned int async_suspend:1; > + unsigned int safe_suspend:1; > bool in_dpm_list:1; /* Owned by the PM core */ > bool is_prepared:1; /* Owned by the PM core */ > bool is_suspended:1; /* Ditto */ > Index: linux-pm/drivers/acpi/device_pm.c > =================================================================== > --- linux-pm.orig/drivers/acpi/device_pm.c > +++ linux-pm/drivers/acpi/device_pm.c > @@ -1025,9 +1025,17 @@ EXPORT_SYMBOL_GPL(acpi_subsys_prepare); > * > * Follow PCI and resume devices suspended at run time before running their > * system suspend callbacks. > + * > + * If the power.direct_complete flag is set for the device, though, skip all > + * that, because the device doesn't need to be resumed then and if it is > + * resumed via runtime PM, the core will notice that and will carry out the > + * late suspend for it. > */ > int acpi_subsys_suspend(struct device *dev) > { > + if (dev->power.direct_complete) > + return 0; > + > pm_runtime_resume(dev); > return pm_generic_suspend(dev); > } > Kind regards Uffe
next prev parent reply other threads:[~2017-08-25 9:28 UTC|newest] Thread overview: 94+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-08-23 14:42 [PATCH v2 0/9] PM / ACPI / i2c: Deploy runtime PM centric path for system sleep Ulf Hansson 2017-08-23 14:42 ` Ulf Hansson 2017-08-23 14:42 ` [PATCH v2 1/9] PM / ACPI: Restore acpi_subsys_complete() Ulf Hansson 2017-08-23 14:42 ` Ulf Hansson 2017-08-23 22:41 ` Rafael J. Wysocki 2017-08-23 22:41 ` Rafael J. Wysocki 2017-08-23 14:42 ` [PATCH v2 2/9] PM / Sleep: Remove pm_complete_with_resume_check() Ulf Hansson 2017-08-23 14:42 ` Ulf Hansson 2017-08-23 14:42 ` [PATCH v2 3/9] PM / ACPI: Split code validating need for runtime resume in ->prepare() Ulf Hansson 2017-08-23 14:42 ` Ulf Hansson 2017-08-23 14:42 ` [PATCH v2 4/9] PM / ACPI: Split acpi_lpss_suspend_late|resume_early() Ulf Hansson 2017-08-23 14:42 ` Ulf Hansson 2017-08-23 14:42 ` [PATCH v2 5/9] PM / ACPI: Provide option to disable direct_complete for ACPI devices Ulf Hansson 2017-08-23 14:42 ` Ulf Hansson 2017-08-23 23:39 ` Rafael J. Wysocki 2017-08-23 23:39 ` Rafael J. Wysocki 2017-08-24 0:13 ` Rafael J. Wysocki 2017-08-24 0:13 ` Rafael J. Wysocki 2017-08-24 0:20 ` Rafael J. Wysocki 2017-08-24 0:20 ` Rafael J. Wysocki 2017-08-24 1:03 ` Rafael J. Wysocki 2017-08-24 1:03 ` Rafael J. Wysocki 2017-08-24 9:15 ` Ulf Hansson 2017-08-24 9:15 ` Ulf Hansson 2017-08-24 16:35 ` Rafael J. Wysocki 2017-08-24 16:35 ` Rafael J. Wysocki 2017-08-24 21:50 ` Rafael J. Wysocki 2017-08-24 21:50 ` Rafael J. Wysocki 2017-08-25 13:42 ` Rafael J. Wysocki 2017-08-25 13:42 ` Rafael J. Wysocki 2017-08-28 1:30 ` Rafael J. Wysocki 2017-08-28 1:30 ` Rafael J. Wysocki 2017-08-28 8:31 ` Ulf Hansson 2017-08-28 8:31 ` Ulf Hansson 2017-08-28 12:39 ` Rafael J. Wysocki 2017-08-28 12:39 ` Rafael J. Wysocki 2017-08-28 12:54 ` Ulf Hansson 2017-08-28 12:54 ` Ulf Hansson 2017-08-28 13:40 ` Rafael J. Wysocki 2017-08-28 13:40 ` Rafael J. Wysocki 2017-08-28 14:24 ` Ulf Hansson 2017-08-28 14:24 ` Ulf Hansson 2017-08-28 21:14 ` Rafael J. Wysocki 2017-08-28 21:14 ` Rafael J. Wysocki 2017-08-25 9:28 ` Ulf Hansson [this message] 2017-08-25 9:28 ` Ulf Hansson 2017-08-25 12:23 ` Rafael J. Wysocki 2017-08-25 12:23 ` Rafael J. Wysocki 2017-08-24 8:19 ` Ulf Hansson 2017-08-24 8:19 ` Ulf Hansson 2017-08-24 14:57 ` Rafael J. Wysocki 2017-08-24 14:57 ` Rafael J. Wysocki 2017-08-25 9:04 ` Ulf Hansson 2017-08-25 9:04 ` Ulf Hansson 2017-08-23 14:42 ` [PATCH v2 6/9] PM / ACPI: Enable the runtime PM centric approach for system sleep Ulf Hansson 2017-08-23 14:42 ` Ulf Hansson 2017-08-23 14:42 ` [PATCH v2 7/9] PM / ACPI: Avoid runtime resuming device in acpi_subsys_suspend|freeze() Ulf Hansson 2017-08-23 14:42 ` Ulf Hansson 2017-08-23 14:42 ` [PATCH v2 8/9] i2c: designware: Don't resume device in the ->complete() callback Ulf Hansson 2017-08-23 14:42 ` Ulf Hansson 2017-08-23 14:42 ` [PATCH v2 9/9] i2c: designware: Deploy the runtime PM centric approach for system sleep Ulf Hansson 2017-08-23 14:42 ` Ulf Hansson 2017-08-25 14:10 ` [PATCH v2 0/9] PM / ACPI / i2c: Deploy runtime PM centric path " Jarkko Nikula 2017-08-25 14:10 ` Jarkko Nikula 2017-08-29 0:18 ` [PATCH 0/3] PM / ACPI / i2c: Runtime PM aware system sleep handling Rafael J. Wysocki 2017-08-29 0:18 ` Rafael J. Wysocki 2017-08-29 0:20 ` [PATCH 1/3] PM / core: Add SAFE_SUSPEND driver flag Rafael J. Wysocki 2017-08-29 0:20 ` Rafael J. Wysocki 2017-08-29 14:57 ` Ulf Hansson 2017-08-29 14:57 ` Ulf Hansson 2017-08-29 15:02 ` Rafael J. Wysocki 2017-08-29 15:02 ` Rafael J. Wysocki 2017-08-29 0:59 ` [PATCH 2/3] PM / ACPI: Use SAFE_SUSPEND in the generic ACPI PM domain Rafael J. Wysocki 2017-08-29 0:59 ` Rafael J. Wysocki 2017-08-29 0:59 ` [PATCH 3/3] PM: i2c-designware-platdrv: System sleep handling rework Rafael J. Wysocki 2017-08-29 0:59 ` Rafael J. Wysocki 2017-08-29 16:38 ` Rafael J. Wysocki 2017-08-29 16:38 ` Rafael J. Wysocki 2017-08-29 16:40 ` Rafael J. Wysocki 2017-08-29 16:40 ` Rafael J. Wysocki 2017-08-29 10:29 ` [PATCH 0/3] PM / ACPI / i2c: Runtime PM aware system sleep handling Johannes Stezenbach 2017-08-29 10:29 ` Johannes Stezenbach 2017-08-29 11:44 ` Ulf Hansson 2017-08-29 11:44 ` Ulf Hansson 2017-08-29 13:53 ` Johannes Stezenbach 2017-08-29 13:53 ` Johannes Stezenbach 2017-08-29 14:43 ` Rafael J. Wysocki 2017-08-29 14:43 ` Rafael J. Wysocki 2017-08-29 15:05 ` Ulf Hansson 2017-08-29 15:05 ` Ulf Hansson 2017-08-29 16:44 ` Rafael J. Wysocki 2017-08-29 16:44 ` Rafael J. Wysocki 2017-08-29 14:49 ` Rafael J. Wysocki 2017-08-29 14:49 ` 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='CAPDyKFrFUgE0FJE==ht4Z5utj+Pht=9UGn-LYBR9hP4ncofreg@mail.gmail.com' \ --to=ulf.hansson@linaro.org \ --cc=andriy.shevchenko@linux.intel.com \ --cc=guodong.xu@linaro.org \ --cc=haojian.zhuang@linaro.org \ --cc=jarkko.nikula@linux.intel.com \ --cc=john.stultz@linaro.org \ --cc=jszhang@marvell.com \ --cc=khilman@kernel.org \ --cc=lenb@kernel.org \ --cc=linux-acpi@vger.kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-i2c@vger.kernel.org \ --cc=linux-pm@vger.kernel.org \ --cc=mika.westerberg@linux.intel.com \ --cc=rafael@kernel.org \ --cc=rjw@rjwysocki.net \ --cc=sumit.semwal@linaro.org \ --cc=wsa@the-dreams.de \ /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.