From: Ulf Hansson <ulf.hansson@linaro.org> To: Alan Stern <stern@rowland.harvard.edu> Cc: "Rafael J . Wysocki" <rjw@rjwysocki.net>, "Rafael J . Wysocki" <rafael@kernel.org>, linux-pm@vger.kernel.org, Kevin Hilman <khilman@kernel.org>, Maulik Shah <mkshah@codeaurora.org>, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] PM: runtime: Allow rpm_resume() to succeed when runtime PM is disabled Date: Wed, 27 Oct 2021 12:55:43 +0200 [thread overview] Message-ID: <CAPDyKFpgHJA-duQSA2uqhccrDxFqWXO1R1DJxo=aOkT5FyX+Ag@mail.gmail.com> (raw) In-Reply-To: <20211027020235.GA1306582@rowland.harvard.edu> On Wed, 27 Oct 2021 at 04:02, Alan Stern <stern@rowland.harvard.edu> wrote: > > On Wed, Oct 27, 2021 at 12:26:26AM +0200, Ulf Hansson wrote: > > During system suspend, the PM core sets dev->power.is_suspended for the > > device that is being suspended. This flag is also being used in > > rpm_resume(), to allow it to succeed by returning 1, assuming that runtime > > PM has been disabled and the runtime PM status is RPM_ACTIVE, for the > > device. > > > > To make this behaviour a bit more useful, let's drop the check for the > > dev->power.is_suspended flag in rpm_resume(), as it doesn't really need to > > be limited to this anyway. > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > --- > > drivers/base/power/runtime.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c > > index ec94049442b9..fadc278e3a66 100644 > > --- a/drivers/base/power/runtime.c > > +++ b/drivers/base/power/runtime.c > > @@ -742,8 +742,8 @@ static int rpm_resume(struct device *dev, int rpmflags) > > repeat: > > if (dev->power.runtime_error) > > retval = -EINVAL; > > - else if (dev->power.disable_depth == 1 && dev->power.is_suspended > > - && dev->power.runtime_status == RPM_ACTIVE) > > + else if (dev->power.disable_depth > 0 && > > + dev->power.runtime_status == RPM_ACTIVE) > > IIRC there was a good reason why the original code checked for > disable_depth == 1 rather than > 0. But I don't remember exactly what > the reason was. Maybe it had something to do with the fact that during > a system sleep __device_suspend_late calls __pm_runtime_disable, and the > code was checking that there were no other disables in effect. The check was introduced in the below commit: Commit 6f3c77b040fc Author: Kevin Hilman <khilman@ti.com> Date: Fri Sep 21 22:47:34 2012 +0000 PM / Runtime: let rpm_resume() succeed if RPM_ACTIVE, even when disabled, v2 By reading the commit message it's pretty clear to me that the check was added to cover only one specific use case, during system suspend. That is, that a driver may want to call pm_runtime_get_sync() from a late/noirq callback (when the PM core has disabled runtime PM), to understand whether the device is still powered on and accessible. > This is > related to the documented behavior of rpm_resume (it's supposed to fail > with -EACCES if the device is disabled for runtime PM, no matter what > power state the device is in). > > That probably is also the explanation for why dev->power.is_suspended > gets checked: It's how the code tells whether a system sleep is in > progress. Yes, you are certainly correct about the current behaviour. It's there for a reason. On the other hand I would be greatly surprised if this change would cause any issues. Of course, I can't make guarantees, but I am, of course, willing to help to fix problems if those happen. As a matter of fact, I think the current behaviour looks quite inconsistent, as it depends on whether the device is being system suspended. Moreover, for syscore devices (dev->power.syscore is set for them), the PM core doesn't set the "is_suspended" flag. Those can benefit from a common behaviour. Finally, I think the "is_suspended" flag actually needs to be protected by a lock when set by the PM core, as it's being used in two separate execution paths. Although, rather than adding a lock for protection, we can just rely on the "disable_depth" in rpm_resume(). It would be easier and makes the behaviour consistent too. > > So overall, I suspect this change should not be made. But some other > improvement (like a nice comment) might be in order. > > Alan Stern Thanks for reviewing! Kind regards Uffe
WARNING: multiple messages have this Message-ID (diff)
From: Ulf Hansson <ulf.hansson@linaro.org> To: Alan Stern <stern@rowland.harvard.edu> Cc: "Rafael J . Wysocki" <rjw@rjwysocki.net>, "Rafael J . Wysocki" <rafael@kernel.org>, linux-pm@vger.kernel.org, Kevin Hilman <khilman@kernel.org>, Maulik Shah <mkshah@codeaurora.org>, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] PM: runtime: Allow rpm_resume() to succeed when runtime PM is disabled Date: Wed, 27 Oct 2021 12:55:43 +0200 [thread overview] Message-ID: <CAPDyKFpgHJA-duQSA2uqhccrDxFqWXO1R1DJxo=aOkT5FyX+Ag@mail.gmail.com> (raw) In-Reply-To: <20211027020235.GA1306582@rowland.harvard.edu> On Wed, 27 Oct 2021 at 04:02, Alan Stern <stern@rowland.harvard.edu> wrote: > > On Wed, Oct 27, 2021 at 12:26:26AM +0200, Ulf Hansson wrote: > > During system suspend, the PM core sets dev->power.is_suspended for the > > device that is being suspended. This flag is also being used in > > rpm_resume(), to allow it to succeed by returning 1, assuming that runtime > > PM has been disabled and the runtime PM status is RPM_ACTIVE, for the > > device. > > > > To make this behaviour a bit more useful, let's drop the check for the > > dev->power.is_suspended flag in rpm_resume(), as it doesn't really need to > > be limited to this anyway. > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > --- > > drivers/base/power/runtime.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c > > index ec94049442b9..fadc278e3a66 100644 > > --- a/drivers/base/power/runtime.c > > +++ b/drivers/base/power/runtime.c > > @@ -742,8 +742,8 @@ static int rpm_resume(struct device *dev, int rpmflags) > > repeat: > > if (dev->power.runtime_error) > > retval = -EINVAL; > > - else if (dev->power.disable_depth == 1 && dev->power.is_suspended > > - && dev->power.runtime_status == RPM_ACTIVE) > > + else if (dev->power.disable_depth > 0 && > > + dev->power.runtime_status == RPM_ACTIVE) > > IIRC there was a good reason why the original code checked for > disable_depth == 1 rather than > 0. But I don't remember exactly what > the reason was. Maybe it had something to do with the fact that during > a system sleep __device_suspend_late calls __pm_runtime_disable, and the > code was checking that there were no other disables in effect. The check was introduced in the below commit: Commit 6f3c77b040fc Author: Kevin Hilman <khilman@ti.com> Date: Fri Sep 21 22:47:34 2012 +0000 PM / Runtime: let rpm_resume() succeed if RPM_ACTIVE, even when disabled, v2 By reading the commit message it's pretty clear to me that the check was added to cover only one specific use case, during system suspend. That is, that a driver may want to call pm_runtime_get_sync() from a late/noirq callback (when the PM core has disabled runtime PM), to understand whether the device is still powered on and accessible. > This is > related to the documented behavior of rpm_resume (it's supposed to fail > with -EACCES if the device is disabled for runtime PM, no matter what > power state the device is in). > > That probably is also the explanation for why dev->power.is_suspended > gets checked: It's how the code tells whether a system sleep is in > progress. Yes, you are certainly correct about the current behaviour. It's there for a reason. On the other hand I would be greatly surprised if this change would cause any issues. Of course, I can't make guarantees, but I am, of course, willing to help to fix problems if those happen. As a matter of fact, I think the current behaviour looks quite inconsistent, as it depends on whether the device is being system suspended. Moreover, for syscore devices (dev->power.syscore is set for them), the PM core doesn't set the "is_suspended" flag. Those can benefit from a common behaviour. Finally, I think the "is_suspended" flag actually needs to be protected by a lock when set by the PM core, as it's being used in two separate execution paths. Although, rather than adding a lock for protection, we can just rely on the "disable_depth" in rpm_resume(). It would be easier and makes the behaviour consistent too. > > So overall, I suspect this change should not be made. But some other > improvement (like a nice comment) might be in order. > > Alan Stern Thanks for reviewing! Kind regards Uffe _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-10-27 10:56 UTC|newest] Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-10-26 22:26 [PATCH] PM: runtime: Allow rpm_resume() to succeed when runtime PM is disabled Ulf Hansson 2021-10-26 22:26 ` Ulf Hansson 2021-10-27 2:02 ` Alan Stern 2021-10-27 2:02 ` Alan Stern 2021-10-27 10:55 ` Ulf Hansson [this message] 2021-10-27 10:55 ` Ulf Hansson 2021-10-27 14:33 ` Alan Stern 2021-10-27 14:33 ` Alan Stern 2021-10-28 22:20 ` Ulf Hansson 2021-10-28 22:20 ` Ulf Hansson 2021-10-29 18:26 ` Rafael J. Wysocki 2021-10-29 18:26 ` Rafael J. Wysocki 2021-11-01 9:27 ` Ulf Hansson 2021-11-01 9:27 ` Ulf Hansson 2021-11-01 14:41 ` Grygorii Strashko 2021-11-01 14:41 ` Grygorii Strashko 2021-11-05 16:03 ` Ulf Hansson 2021-11-05 16:03 ` Ulf Hansson 2021-11-26 12:19 ` Ulf Hansson 2021-11-26 12:19 ` Ulf Hansson 2021-11-26 13:30 ` Rafael J. Wysocki 2021-11-26 13:30 ` Rafael J. Wysocki 2021-11-26 13:46 ` Ulf Hansson 2021-11-26 13:46 ` Ulf Hansson 2021-11-26 17:58 ` Rafael J. Wysocki 2021-11-26 17:58 ` Rafael J. Wysocki 2021-11-26 18:29 ` Rafael J. Wysocki 2021-11-26 18:29 ` Rafael J. Wysocki 2021-11-30 11:57 ` Ulf Hansson 2021-11-30 11:57 ` Ulf Hansson 2021-11-30 13:01 ` Rafael J. Wysocki 2021-11-30 13:01 ` Rafael J. Wysocki 2021-11-30 16:41 ` Ulf Hansson 2021-11-30 16:41 ` Ulf Hansson 2021-11-30 17:26 ` Rafael J. Wysocki 2021-11-30 17:26 ` Rafael J. Wysocki 2021-12-01 9:02 ` Ulf Hansson 2021-12-01 9:02 ` Ulf Hansson 2021-12-01 13:49 ` Rafael J. Wysocki 2021-12-01 13:49 ` Rafael J. Wysocki 2021-12-01 15:22 ` Ulf Hansson 2021-12-01 15:22 ` Ulf Hansson 2021-12-01 17:44 ` Rafael J. Wysocki 2021-12-01 17:44 ` Rafael J. Wysocki 2021-12-01 20:11 ` Rafael J. Wysocki 2021-12-01 20:11 ` Rafael J. Wysocki 2021-12-02 11:28 ` Ulf Hansson 2021-12-02 11:28 ` Ulf Hansson 2021-12-02 16:18 ` Rafael J. Wysocki 2021-12-02 16:18 ` Rafael J. Wysocki 2021-12-02 16:50 ` Alan Stern 2021-12-02 16:50 ` Alan Stern 2021-12-02 18:01 ` Rafael J. Wysocki 2021-12-02 18:01 ` 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='CAPDyKFpgHJA-duQSA2uqhccrDxFqWXO1R1DJxo=aOkT5FyX+Ag@mail.gmail.com' \ --to=ulf.hansson@linaro.org \ --cc=khilman@kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pm@vger.kernel.org \ --cc=mkshah@codeaurora.org \ --cc=rafael@kernel.org \ --cc=rjw@rjwysocki.net \ --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.