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

[Ulf Hansson]

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?

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