Re: [PATCH] PM / core: Clear the direct_complete flag on errors

[Ulf Hansson]

On 4 October 2018 at 22:57, Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
> On Thu, Oct 4, 2018 at 8:48 PM Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
>>
>> On 4 October 2018 at 19:47, Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
>> > On Thu, Oct 4, 2018 at 3:23 PM Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
>> >>
>> >> On 4 October 2018 at 11:08, Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> wrote:
>> >> > From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>> >> >
>> >> > 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