| Message ID | 61559514.Vym8FNvob2@aspire.rjw.lan (mailing list archive) |
|---|---|
| State | Mainlined |
| Delegated to: | Rafael Wysocki |
| Headers | show |
| Series | PM / core: Clear the direct_complete flag on errors | expand |
On 4 October 2018 at 11:08, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > If __device_suspend() returns early on an error or pending wakeup > and the power.direct_complete flag has been set for the device > already, the subsequent device_resume() will be confused by it > and it will call pm_runtime_enable() incorrectly, as runtime PM > has not been disabled for the device by __device_suspend(). I think it would be fair to mention that is related to the async suspend path, in dpm_suspend(). > > To avoid that, clear power.direct_complete if __device_suspend() > is not going to disable runtime PM for the device before returning. Overall, by looking at the behavior in dpm_suspend() of async suspended devices, it does look a bit fragile to me. My worries is that we put asynced suspended devices in the dpm_suspended_list, no matter if the device was successfully suspended or not. This differs from the no-async path. In the long run, maybe we should change that instead? > > Fixes: aae4518b3124 (PM / sleep: Mechanism to avoid resuming runtime-suspended devices unnecessarily) > Reported-by: Al Cooper <alcooperx@gmail.com> > Cc: 3.16+ <stable@vger.kernel.org> # 3.16+ > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> Kind regards Uffe > --- > drivers/base/power/main.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > Index: linux-pm/drivers/base/power/main.c > =================================================================== > --- linux-pm.orig/drivers/base/power/main.c > +++ linux-pm/drivers/base/power/main.c > @@ -1713,8 +1713,10 @@ static int __device_suspend(struct devic > > dpm_wait_for_subordinate(dev, async); > > - if (async_error) > + if (async_error) { > + dev->power.direct_complete = false; > goto Complete; > + } > > /* > * If a device configured to wake up the system from sleep states > @@ -1726,6 +1728,7 @@ static int __device_suspend(struct devic > pm_wakeup_event(dev, 0); > > if (pm_wakeup_pending()) { > + dev->power.direct_complete = false; > async_error = -EBUSY; > goto Complete; > } >
> On 4 October 2018 at 11:08, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > If __device_suspend() returns early on an error or pending wakeup > > and the power.direct_complete flag has been set for the device > > already, the subsequent device_resume() will be confused by it > > and it will call pm_runtime_enable() incorrectly, as runtime PM > > has not been disabled for the device by __device_suspend(). > > I think it would be fair to mention that is related to the async > suspend path, in dpm_suspend(). This also fixed the issue and looks cleaner. > > > > > To avoid that, clear power.direct_complete if __device_suspend() > > is not going to disable runtime PM for the device before returning. > > Overall, by looking at the behavior in dpm_suspend() of async > suspended devices, it does look a bit fragile to me. > > My worries is that we put asynced suspended devices in the > dpm_suspended_list, no matter if the device was successfully suspended > or not. This differs from the no-async path. > > In the long run, maybe we should change that instead? I originally looked into this. Currently dmp_suspend moves async devices from the prepared list to the suspended list as they are queued and I looked at moving this to __device_suspend (after the checks for async_error and wake_pending) but realized that this would change normal resume ordering and was afraid that would be too disruptive. Al On Thu, Oct 4, 2018 at 9:23 AM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On 4 October 2018 at 11:08, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > If __device_suspend() returns early on an error or pending wakeup > > and the power.direct_complete flag has been set for the device > > already, the subsequent device_resume() will be confused by it > > and it will call pm_runtime_enable() incorrectly, as runtime PM > > has not been disabled for the device by __device_suspend(). > > I think it would be fair to mention that is related to the async > suspend path, in dpm_suspend(). > > > > > To avoid that, clear power.direct_complete if __device_suspend() > > is not going to disable runtime PM for the device before returning. > > Overall, by looking at the behavior in dpm_suspend() of async > suspended devices, it does look a bit fragile to me. > > My worries is that we put asynced suspended devices in the > dpm_suspended_list, no matter if the device was successfully suspended > or not. This differs from the no-async path. > > In the long run, maybe we should change that instead? > > > > > Fixes: aae4518b3124 (PM / sleep: Mechanism to avoid resuming runtime-suspended devices unnecessarily) > > Reported-by: Al Cooper <alcooperx@gmail.com> > > Cc: 3.16+ <stable@vger.kernel.org> # 3.16+ > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> > > Kind regards > Uffe > > > --- > > drivers/base/power/main.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > Index: linux-pm/drivers/base/power/main.c > > =================================================================== > > --- linux-pm.orig/drivers/base/power/main.c > > +++ linux-pm/drivers/base/power/main.c > > @@ -1713,8 +1713,10 @@ static int __device_suspend(struct devic > > > > dpm_wait_for_subordinate(dev, async); > > > > - if (async_error) > > + if (async_error) { > > + dev->power.direct_complete = false; > > goto Complete; > > + } > > > > /* > > * If a device configured to wake up the system from sleep states > > @@ -1726,6 +1728,7 @@ static int __device_suspend(struct devic > > pm_wakeup_event(dev, 0); > > > > if (pm_wakeup_pending()) { > > + dev->power.direct_complete = false; > > async_error = -EBUSY; > > goto Complete; > > } > >
On 4 October 2018 at 15:59, Alan Cooper <alcooperx@gmail.com> wrote: >> On 4 October 2018 at 11:08, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> > >> > If __device_suspend() returns early on an error or pending wakeup >> > and the power.direct_complete flag has been set for the device >> > already, the subsequent device_resume() will be confused by it >> > and it will call pm_runtime_enable() incorrectly, as runtime PM >> > has not been disabled for the device by __device_suspend(). >> >> I think it would be fair to mention that is related to the async >> suspend path, in dpm_suspend(). > > This also fixed the issue and looks cleaner. > >> >> > >> > To avoid that, clear power.direct_complete if __device_suspend() >> > is not going to disable runtime PM for the device before returning. >> >> Overall, by looking at the behavior in dpm_suspend() of async >> suspended devices, it does look a bit fragile to me. >> >> My worries is that we put asynced suspended devices in the >> dpm_suspended_list, no matter if the device was successfully suspended >> or not. This differs from the no-async path. >> >> In the long run, maybe we should change that instead? > > I originally looked into this. Currently dmp_suspend moves async > devices from the prepared list to the suspended list as they are > queued and I looked at moving this to __device_suspend (after the > checks for async_error and wake_pending) but realized that this would > change normal resume ordering and was afraid that would be too > disruptive. > I understand, however, I would be surprised if that would be a real problem. But who knows. :-) The hole async thing is opt-in, and it does also assume that drivers/subsystems to cope with devices suspend/resume ordering to be based upon device relationships, like parent/child, device_link-supplier/consumers. If it's not working, drivers should disable the async option, at least in my opinion. Kind regards Uffe
On Thu, Oct 4, 2018 at 3:23 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On 4 October 2018 at 11:08, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > If __device_suspend() returns early on an error or pending wakeup > > and the power.direct_complete flag has been set for the device > > already, the subsequent device_resume() will be confused by it > > and it will call pm_runtime_enable() incorrectly, as runtime PM > > has not been disabled for the device by __device_suspend(). > > I think it would be fair to mention that is related to the async > suspend path, in dpm_suspend(). OK, fair enough. > > > > To avoid that, clear power.direct_complete if __device_suspend() > > is not going to disable runtime PM for the device before returning. > > Overall, by looking at the behavior in dpm_suspend() of async > suspended devices, it does look a bit fragile to me. > > My worries is that we put asynced suspended devices in the > dpm_suspended_list, no matter if the device was successfully suspended > or not. This differs from the no-async path. That's because this was the most straightforward way to organize that (otherwise you need to worry about the list locking with respect to the async suspends etc and you really need to preserve the ordering there). > In the long run, maybe we should change that instead? Is there anything wrong with it really?
On 4 October 2018 at 19:47, Rafael J. Wysocki <rafael@kernel.org> wrote: > On Thu, Oct 4, 2018 at 3:23 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: >> >> On 4 October 2018 at 11:08, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> > >> > If __device_suspend() returns early on an error or pending wakeup >> > and the power.direct_complete flag has been set for the device >> > already, the subsequent device_resume() will be confused by it >> > and it will call pm_runtime_enable() incorrectly, as runtime PM >> > has not been disabled for the device by __device_suspend(). >> >> I think it would be fair to mention that is related to the async >> suspend path, in dpm_suspend(). > > OK, fair enough. > >> > >> > To avoid that, clear power.direct_complete if __device_suspend() >> > is not going to disable runtime PM for the device before returning. >> >> Overall, by looking at the behavior in dpm_suspend() of async >> suspended devices, it does look a bit fragile to me. >> >> My worries is that we put asynced suspended devices in the >> dpm_suspended_list, no matter if the device was successfully suspended >> or not. This differs from the no-async path. > > That's because this was the most straightforward way to organize that > (otherwise you need to worry about the list locking with respect to > the async suspends etc and you really need to preserve the ordering > there). I understand about the lock, but not sure if that is something to worry about, at least from contention point of view, if that is what you mean? In regards to the order, is that really problem for async enabled devices? > >> In the long run, maybe we should change that instead? > > Is there anything wrong with it really? No, besides complexity. :-) My, point was that we could potentially simplify the code in device_resume() and in __device_suspend(), as the behavior would them becomes more deterministic. device_resume() will never be called unless __device_suspend() has succeeded for the device. Anyway, as stated, I fine with your patch. And if I get some time I might send a patch to show you what I mean. Kind regards Uffe
On Thu, Oct 4, 2018 at 8:48 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On 4 October 2018 at 19:47, Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Thu, Oct 4, 2018 at 3:23 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > >> > >> On 4 October 2018 at 11:08, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > >> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > >> > > >> > If __device_suspend() returns early on an error or pending wakeup > >> > and the power.direct_complete flag has been set for the device > >> > already, the subsequent device_resume() will be confused by it > >> > and it will call pm_runtime_enable() incorrectly, as runtime PM > >> > has not been disabled for the device by __device_suspend(). > >> > >> I think it would be fair to mention that is related to the async > >> suspend path, in dpm_suspend(). > > > > OK, fair enough. > > > >> > > >> > To avoid that, clear power.direct_complete if __device_suspend() > >> > is not going to disable runtime PM for the device before returning. > >> > >> Overall, by looking at the behavior in dpm_suspend() of async > >> suspended devices, it does look a bit fragile to me. > >> > >> My worries is that we put asynced suspended devices in the > >> dpm_suspended_list, no matter if the device was successfully suspended > >> or not. This differs from the no-async path. > > > > That's because this was the most straightforward way to organize that > > (otherwise you need to worry about the list locking with respect to > > the async suspends etc and you really need to preserve the ordering > > there). > > I understand about the lock, but not sure if that is something to > worry about, at least from contention point of view, if that is what > you mean? The contention may not be a problem, but there would be some extra overhead related to the dragging of the lock cache line between CPUs. > In regards to the order, is that really problem for async enabled devices? You may be underestimating the problem somewhat. For example, say you have 4 devices, A which is the parent of B and C, and D which is a child of C. Say that D depends on B too, but it cannot be a child of it and there's no device link between B and D. [For instance, the driver of A has registered both B and C, and the driver of C has registered D, but it doesn't know about the dependency between D and B.] Also say that A, B and C are all async, but D is sync and all A, B, C are before D in the original list order. dpm_suspend() will get to A, B and C only after dealing with D. It will then start to suspend B and C almost at the same time and A will wait for both of them. So far so good. Next, A will resume first, B and C after it, and D after C. Say that B is somewhat faster to resume, but it actually adds itself back to the list after C and D (as they don't wait for it). The resume of C and D succeeds (because B is already there physically), but the ordering of the list is now A->C->D->B and there will be trouble during the next suspend, because B will be suspending in parallel with D which depends on it. In this case you have to guarantee that D and B will not be reordered, but it generally is hard without an explicit "link" between them - unless the original ordering of the list is preserved entirely. > > > >> In the long run, maybe we should change that instead? > > > > Is there anything wrong with it really? > > No, besides complexity. :-) And how exactly are you measuring the "complexity" here? > My, point was that we could potentially simplify the code in > device_resume() and in __device_suspend(), as the behavior would them > becomes more deterministic. No, it wouldn't be more deterministic. In fact, it would then become less deterministic and provably so if you take consecutive suspend-resume cycles into account. In principle, the order in which devices are handled might be different every time then and sorry, but I'm failing to see how that can be regarded as "more deterministic". > device_resume() will never be called unless __device_suspend() has succeeded for the device. Which doesn't matter. What matters is that (and which is the case already) the resume callbacks will not be invoked without invoking the suspend callbacks for the device beforehand.
On 4 October 2018 at 22:57, Rafael J. Wysocki <rafael@kernel.org> wrote: > On Thu, Oct 4, 2018 at 8:48 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: >> >> On 4 October 2018 at 19:47, Rafael J. Wysocki <rafael@kernel.org> wrote: >> > On Thu, Oct 4, 2018 at 3:23 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: >> >> >> >> On 4 October 2018 at 11:08, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >> >> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> >> > >> >> > If __device_suspend() returns early on an error or pending wakeup >> >> > and the power.direct_complete flag has been set for the device >> >> > already, the subsequent device_resume() will be confused by it >> >> > and it will call pm_runtime_enable() incorrectly, as runtime PM >> >> > has not been disabled for the device by __device_suspend(). >> >> >> >> I think it would be fair to mention that is related to the async >> >> suspend path, in dpm_suspend(). >> > >> > OK, fair enough. >> > >> >> > >> >> > To avoid that, clear power.direct_complete if __device_suspend() >> >> > is not going to disable runtime PM for the device before returning. >> >> >> >> Overall, by looking at the behavior in dpm_suspend() of async >> >> suspended devices, it does look a bit fragile to me. >> >> >> >> My worries is that we put asynced suspended devices in the >> >> dpm_suspended_list, no matter if the device was successfully suspended >> >> or not. This differs from the no-async path. >> > >> > That's because this was the most straightforward way to organize that >> > (otherwise you need to worry about the list locking with respect to >> > the async suspends etc and you really need to preserve the ordering >> > there). >> >> I understand about the lock, but not sure if that is something to >> worry about, at least from contention point of view, if that is what >> you mean? > > The contention may not be a problem, but there would be some extra > overhead related to the dragging of the lock cache line between CPUs. > >> In regards to the order, is that really problem for async enabled devices? > > You may be underestimating the problem somewhat. > > For example, say you have 4 devices, A which is the parent of B and C, > and D which is a child of C. Say that D depends on B too, but it > cannot be a child of it and there's no device link between B and D. > [For instance, the driver of A has registered both B and C, and the > driver of C has registered D, but it doesn't know about the dependency > between D and B.] Also say that A, B and C are all async, but D is > sync and all A, B, C are before D in the original list order. > > dpm_suspend() will get to A, B and C only after dealing with D. It > will then start to suspend B and C almost at the same time and A will > wait for both of them. So far so good. > > Next, A will resume first, B and C after it, and D after C. Say that > B is somewhat faster to resume, but it actually adds itself back to > the list after C and D (as they don't wait for it). The resume of C > and D succeeds (because B is already there physically), but the > ordering of the list is now A->C->D->B and there will be trouble > during the next suspend, because B will be suspending in parallel with > D which depends on it. > > In this case you have to guarantee that D and B will not be reordered, > but it generally is hard without an explicit "link" between them - > unless the original ordering of the list is preserved entirely. Thanks for the detailed description! My naive approach was simply that these cases should not exist. Those drivers that opt-in for the async method, must be very careful when doing so. However, you certainly have a point that this may not be the case. > >> > >> >> In the long run, maybe we should change that instead? >> > >> > Is there anything wrong with it really? >> >> No, besides complexity. :-) > > And how exactly are you measuring the "complexity" here? > >> My, point was that we could potentially simplify the code in >> device_resume() and in __device_suspend(), as the behavior would them >> becomes more deterministic. > > No, it wouldn't be more deterministic. In fact, it would then become > less deterministic and provably so if you take consecutive > suspend-resume cycles into account. In principle, the order in which > devices are handled might be different every time then and sorry, but > I'm failing to see how that can be regarded as "more deterministic". > >> device_resume() will never be called unless __device_suspend() has succeeded for the device. > > Which doesn't matter. What matters is that (and which is the case > already) the resume callbacks will not be invoked without invoking the > suspend callbacks for the device beforehand. Right. I rest my case - and sorry for the noise. Kind regards Uffe
Index: linux-pm/drivers/base/power/main.c =================================================================== --- linux-pm.orig/drivers/base/power/main.c +++ linux-pm/drivers/base/power/main.c @@ -1713,8 +1713,10 @@ static int __device_suspend(struct devic dpm_wait_for_subordinate(dev, async); - if (async_error) + if (async_error) { + dev->power.direct_complete = false; goto Complete; + } /* * If a device configured to wake up the system from sleep states @@ -1726,6 +1728,7 @@ static int __device_suspend(struct devic pm_wakeup_event(dev, 0); if (pm_wakeup_pending()) { + dev->power.direct_complete = false; async_error = -EBUSY; goto Complete; }