Re: [PATCH v3 1/6] PM / core: Add LEAVE_SUSPENDED driver flag

[Ulf Hansson]

On 17 November 2017 at 15:31, Rafael J. Wysocki  wrote:
> On Fri, Nov 17, 2017 at 2:49 PM, Ulf Hansson  wrote:
>> [...]
>>

>> Second, have you considered setting the default value of
>> dev->power.may_skip_resume to true?
>
> Yes.
>
>> That would means the subsystem
>> instead need to implement an opt-out method. I am thinking that it may
>> not be an issue, since we anyway at this point, don't have drivers
>> using the LEAVE_SUSPENDED flag.
>
> Opt-out doesn't work because of the need to invoke the "noirq" callbacks.

 I am not sure I follow that.

 Whatever needs to be fixed on the subsystem level, that could be done
 before the driver starts using the LEAVE_SUSPENDED flag. No?
>>>
>>> That requires a bit of explanation, sorry for being overly concise.
>>>
>>> The core calls ->resume_noirq from the middle layer regardless of
>>> whether or not the device will be left suspended, so the
>>> ->resume_noirq cannot do arbitrary things to it.  Setting
>>> may_skip_resume by the middle layer tells the core that the middle
>>> layer is ready for that and is going to cooperate.  If may_skip_resume
>>> had been set by default, that piece of information would have been
>>> missing.
>>
>> Huh, I still don't get that. Sorry.
>>
>> If the "may_skip_resume" is default set to true by the PM core,
>> wouldn't that just mean that the middle-layer needs to implement an
>> opt-out method, rather than opt-in. In principle to opt-out the
>> middle-layer needs to set may_skip_resume to false in suspend_noirq
>> phase, no?
>
> Yes, but if the middle-layer doesn't clear it, that may mean two
> things.  First, the middle layer is ready and so on.  Good.  Second,
> the middle layer is not aware of the whole thing.  Not good.  The core
> cannot tell.
>
> In the opt-in case, however, all is clear. :-)

Okay.

>
>> Then we only need to make sure drivers don't starts use
>> LEAVE_SUSPENDED, before we make sure the middle layers is adopted. But
>> that should not be a problem.
>>
>> The benefit would be that those middle layers that can cope with
>> LEAVE_SUSPENDED as of today don't need to change.
>
> I'm not sure if that's the case.
>
> The middle layer has to evaluate dev_pm_may_skip_resume() in
> ->resume_noirq() to check if the device can be left in suspend, as it
> cannot determine that in ->suspend_noirq() yet.

Right. Okay, let's stick with the chosen method.

Kind regards
Uffe

Re: [PATCH v3 1/6] PM / core: Add LEAVE_SUSPENDED driver flag

[Ulf Hansson]

On 17 November 2017 at 15:31, Rafael J. Wysocki  wrote:
> On Fri, Nov 17, 2017 at 2:49 PM, Ulf Hansson  wrote:
>> [...]
>>

>> Second, have you considered setting the default value of
>> dev->power.may_skip_resume to true?
>
> Yes.
>
>> That would means the subsystem
>> instead need to implement an opt-out method. I am thinking that it may
>> not be an issue, since we anyway at this point, don't have drivers
>> using the LEAVE_SUSPENDED flag.
>
> Opt-out doesn't work because of the need to invoke the "noirq" callbacks.

 I am not sure I follow that.

 Whatever needs to be fixed on the subsystem level, that could be done
 before the driver starts using the LEAVE_SUSPENDED flag. No?
>>>
>>> That requires a bit of explanation, sorry for being overly concise.
>>>
>>> The core calls ->resume_noirq from the middle layer regardless of
>>> whether or not the device will be left suspended, so the
>>> ->resume_noirq cannot do arbitrary things to it.  Setting
>>> may_skip_resume by the middle layer tells the core that the middle
>>> layer is ready for that and is going to cooperate.  If may_skip_resume
>>> had been set by default, that piece of information would have been
>>> missing.
>>
>> Huh, I still don't get that. Sorry.
>>
>> If the "may_skip_resume" is default set to true by the PM core,
>> wouldn't that just mean that the middle-layer needs to implement an
>> opt-out method, rather than opt-in. In principle to opt-out the
>> middle-layer needs to set may_skip_resume to false in suspend_noirq
>> phase, no?
>
> Yes, but if the middle-layer doesn't clear it, that may mean two
> things.  First, the middle layer is ready and so on.  Good.  Second,
> the middle layer is not aware of the whole thing.  Not good.  The core
> cannot tell.
>
> In the opt-in case, however, all is clear. :-)

Okay.

>
>> Then we only need to make sure drivers don't starts use
>> LEAVE_SUSPENDED, before we make sure the middle layers is adopted. But
>> that should not be a problem.
>>
>> The benefit would be that those middle layers that can cope with
>> LEAVE_SUSPENDED as of today don't need to change.
>
> I'm not sure if that's the case.
>
> The middle layer has to evaluate dev_pm_may_skip_resume() in
> ->resume_noirq() to check if the device can be left in suspend, as it
> cannot determine that in ->suspend_noirq() yet.

Right. Okay, let's stick with the chosen method.

Kind regards
Uffe

Re: [PATCH v3 1/6] PM / core: Add LEAVE_SUSPENDED driver flag

[Rafael J. Wysocki]

On Fri, Nov 17, 2017 at 2:49 PM, Ulf Hansson  wrote:
> [...]
>
>>>
> Second, have you considered setting the default value of
> dev->power.may_skip_resume to true?

 Yes.

> That would means the subsystem
> instead need to implement an opt-out method. I am thinking that it may
> not be an issue, since we anyway at this point, don't have drivers
> using the LEAVE_SUSPENDED flag.

 Opt-out doesn't work because of the need to invoke the "noirq" callbacks.
>>>
>>> I am not sure I follow that.
>>>
>>> Whatever needs to be fixed on the subsystem level, that could be done
>>> before the driver starts using the LEAVE_SUSPENDED flag. No?
>>
>> That requires a bit of explanation, sorry for being overly concise.
>>
>> The core calls ->resume_noirq from the middle layer regardless of
>> whether or not the device will be left suspended, so the
>> ->resume_noirq cannot do arbitrary things to it.  Setting
>> may_skip_resume by the middle layer tells the core that the middle
>> layer is ready for that and is going to cooperate.  If may_skip_resume
>> had been set by default, that piece of information would have been
>> missing.
>
> Huh, I still don't get that. Sorry.
>
> If the "may_skip_resume" is default set to true by the PM core,
> wouldn't that just mean that the middle-layer needs to implement an
> opt-out method, rather than opt-in. In principle to opt-out the
> middle-layer needs to set may_skip_resume to false in suspend_noirq
> phase, no?

Yes, but if the middle-layer doesn't clear it, that may mean two
things.  First, the middle layer is ready and so on.  Good.  Second,
the middle layer is not aware of the whole thing.  Not good.  The core
cannot tell.

In the opt-in case, however, all is clear. :-)

> Then we only need to make sure drivers don't starts use
> LEAVE_SUSPENDED, before we make sure the middle layers is adopted. But
> that should not be a problem.
>
> The benefit would be that those middle layers that can cope with
> LEAVE_SUSPENDED as of today don't need to change.

I'm not sure if that's the case.

The middle layer has to evaluate dev_pm_may_skip_resume() in
->resume_noirq() to check if the device can be left in suspend, as it
cannot determine that in ->suspend_noirq() yet.

Thanks,
Rafael

Re: [PATCH v3 1/6] PM / core: Add LEAVE_SUSPENDED driver flag

[Rafael J. Wysocki]

On Fri, Nov 17, 2017 at 2:49 PM, Ulf Hansson  wrote:
> [...]
>
>>>
> Second, have you considered setting the default value of
> dev->power.may_skip_resume to true?

 Yes.

> That would means the subsystem
> instead need to implement an opt-out method. I am thinking that it may
> not be an issue, since we anyway at this point, don't have drivers
> using the LEAVE_SUSPENDED flag.

 Opt-out doesn't work because of the need to invoke the "noirq" callbacks.
>>>
>>> I am not sure I follow that.
>>>
>>> Whatever needs to be fixed on the subsystem level, that could be done
>>> before the driver starts using the LEAVE_SUSPENDED flag. No?
>>
>> That requires a bit of explanation, sorry for being overly concise.
>>
>> The core calls ->resume_noirq from the middle layer regardless of
>> whether or not the device will be left suspended, so the
>> ->resume_noirq cannot do arbitrary things to it.  Setting
>> may_skip_resume by the middle layer tells the core that the middle
>> layer is ready for that and is going to cooperate.  If may_skip_resume
>> had been set by default, that piece of information would have been
>> missing.
>
> Huh, I still don't get that. Sorry.
>
> If the "may_skip_resume" is default set to true by the PM core,
> wouldn't that just mean that the middle-layer needs to implement an
> opt-out method, rather than opt-in. In principle to opt-out the
> middle-layer needs to set may_skip_resume to false in suspend_noirq
> phase, no?

Yes, but if the middle-layer doesn't clear it, that may mean two
things.  First, the middle layer is ready and so on.  Good.  Second,
the middle layer is not aware of the whole thing.  Not good.  The core
cannot tell.

In the opt-in case, however, all is clear. :-)

> Then we only need to make sure drivers don't starts use
> LEAVE_SUSPENDED, before we make sure the middle layers is adopted. But
> that should not be a problem.
>
> The benefit would be that those middle layers that can cope with
> LEAVE_SUSPENDED as of today don't need to change.

I'm not sure if that's the case.

The middle layer has to evaluate dev_pm_may_skip_resume() in
->resume_noirq() to check if the device can be left in suspend, as it
cannot determine that in ->suspend_noirq() yet.

Thanks,
Rafael

Re: [PATCH v3 1/6] PM / core: Add LEAVE_SUSPENDED driver flag

[Ulf Hansson]

[...]

>>
 Second, have you considered setting the default value of
 dev->power.may_skip_resume to true?
>>>
>>> Yes.
>>>
 That would means the subsystem
 instead need to implement an opt-out method. I am thinking that it may
 not be an issue, since we anyway at this point, don't have drivers
 using the LEAVE_SUSPENDED flag.
>>>
>>> Opt-out doesn't work because of the need to invoke the "noirq" callbacks.
>>
>> I am not sure I follow that.
>>
>> Whatever needs to be fixed on the subsystem level, that could be done
>> before the driver starts using the LEAVE_SUSPENDED flag. No?
>
> That requires a bit of explanation, sorry for being overly concise.
>
> The core calls ->resume_noirq from the middle layer regardless of
> whether or not the device will be left suspended, so the
> ->resume_noirq cannot do arbitrary things to it.  Setting
> may_skip_resume by the middle layer tells the core that the middle
> layer is ready for that and is going to cooperate.  If may_skip_resume
> had been set by default, that piece of information would have been
> missing.

Huh, I still don't get that. Sorry.

If the "may_skip_resume" is default set to true by the PM core,
wouldn't that just mean that the middle-layer needs to implement an
opt-out method, rather than opt-in. In principle to opt-out the
middle-layer needs to set may_skip_resume to false in suspend_noirq
phase, no?

Then we only need to make sure drivers don't starts use
LEAVE_SUSPENDED, before we make sure the middle layers is adopted. But
that should not be a problem.

The benefit would be that those middle layers that can cope with
LEAVE_SUSPENDED as of today don't need to change.

Kind regards
Uffe

Re: [PATCH v3 1/6] PM / core: Add LEAVE_SUSPENDED driver flag

[Ulf Hansson]

[...]

>>
 Second, have you considered setting the default value of
 dev->power.may_skip_resume to true?
>>>
>>> Yes.
>>>
 That would means the subsystem
 instead need to implement an opt-out method. I am thinking that it may
 not be an issue, since we anyway at this point, don't have drivers
 using the LEAVE_SUSPENDED flag.
>>>
>>> Opt-out doesn't work because of the need to invoke the "noirq" callbacks.
>>
>> I am not sure I follow that.
>>
>> Whatever needs to be fixed on the subsystem level, that could be done
>> before the driver starts using the LEAVE_SUSPENDED flag. No?
>
> That requires a bit of explanation, sorry for being overly concise.
>
> The core calls ->resume_noirq from the middle layer regardless of
> whether or not the device will be left suspended, so the
> ->resume_noirq cannot do arbitrary things to it.  Setting
> may_skip_resume by the middle layer tells the core that the middle
> layer is ready for that and is going to cooperate.  If may_skip_resume
> had been set by default, that piece of information would have been
> missing.

Huh, I still don't get that. Sorry.

If the "may_skip_resume" is default set to true by the PM core,
wouldn't that just mean that the middle-layer needs to implement an
opt-out method, rather than opt-in. In principle to opt-out the
middle-layer needs to set may_skip_resume to false in suspend_noirq
phase, no?

Then we only need to make sure drivers don't starts use
LEAVE_SUSPENDED, before we make sure the middle layers is adopted. But
that should not be a problem.

The benefit would be that those middle layers that can cope with
LEAVE_SUSPENDED as of today don't need to change.

Kind regards
Uffe

Re: [PATCH v3 1/6] PM / core: Add LEAVE_SUSPENDED driver flag

[Rafael J. Wysocki]

On Fri, Nov 17, 2017 at 7:11 AM, Ulf Hansson  wrote:
> [...]
>
>>> > +++ linux-pm/include/linux/pm.h
>>> > @@ -559,6 +559,7 @@ struct pm_subsys_data {
>>> >   * NEVER_SKIP: Do not skip system suspend/resume callbacks for the 
>>> > device.
>>> >   * SMART_PREPARE: Check the return value of the driver's ->prepare 
>>> > callback.
>>> >   * SMART_SUSPEND: No need to resume the device from runtime suspend.
>>> > + * LEAVE_SUSPENDED: Avoid resuming the device during system resume if 
>>> > possible.
>>> >   *
>>> >   * Setting SMART_PREPARE instructs bus types and PM domains which may 
>>> > want
>>> >   * system suspend/resume callbacks to be skipped for the device to 
>>> > return 0 from
>>> > @@ -572,10 +573,14 @@ struct pm_subsys_data {
>>> >   * necessary from the driver's perspective.  It also may cause them to 
>>> > skip
>>> >   * invocations of the ->suspend_late and ->suspend_noirq callbacks 
>>> > provided by
>>> >   * the driver if they decide to leave the device in runtime suspend.
>>> > + *
>>> > + * Setting LEAVE_SUSPENDED informs the PM core and middle-layer code 
>>> > that the
>>> > + * driver prefers the device to be left in runtime suspend after system 
>>> > resume.
>>> >   */
>>>
>>> Question: Can LEAVE_SUSPENDED and NEVER_SKIP be valid combination? I
>>> guess not!? Should we validate for wrong combinations?
>>
>> Why not?  There's no real overlap between them.
>
> Except that NEVER_SKIP, documentation wise, tells you that your
> suspend and resume callbacks will never be skipped. :-)

You mean the comment in pm.h I suppose?  Yes, it isn't precise enough.

The proper documentation in devices.rst is less ambiguous, though. :-)

> [...]
>
>>> Second, have you considered setting the default value of
>>> dev->power.may_skip_resume to true?
>>
>> Yes.
>>
>>> That would means the subsystem
>>> instead need to implement an opt-out method. I am thinking that it may
>>> not be an issue, since we anyway at this point, don't have drivers
>>> using the LEAVE_SUSPENDED flag.
>>
>> Opt-out doesn't work because of the need to invoke the "noirq" callbacks.
>
> I am not sure I follow that.
>
> Whatever needs to be fixed on the subsystem level, that could be done
> before the driver starts using the LEAVE_SUSPENDED flag. No?

That requires a bit of explanation, sorry for being overly concise.

The core calls ->resume_noirq from the middle layer regardless of
whether or not the device will be left suspended, so the
->resume_noirq cannot do arbitrary things to it.  Setting
may_skip_resume by the middle layer tells the core that the middle
layer is ready for that and is going to cooperate.  If may_skip_resume
had been set by default, that piece of information would have been
missing.

Thanks,
Rafael

Re: [PATCH v3 1/6] PM / core: Add LEAVE_SUSPENDED driver flag

[Rafael J. Wysocki]

On Fri, Nov 17, 2017 at 7:11 AM, Ulf Hansson  wrote:
> [...]
>
>>> > +++ linux-pm/include/linux/pm.h
>>> > @@ -559,6 +559,7 @@ struct pm_subsys_data {
>>> >   * NEVER_SKIP: Do not skip system suspend/resume callbacks for the 
>>> > device.
>>> >   * SMART_PREPARE: Check the return value of the driver's ->prepare 
>>> > callback.
>>> >   * SMART_SUSPEND: No need to resume the device from runtime suspend.
>>> > + * LEAVE_SUSPENDED: Avoid resuming the device during system resume if 
>>> > possible.
>>> >   *
>>> >   * Setting SMART_PREPARE instructs bus types and PM domains which may 
>>> > want
>>> >   * system suspend/resume callbacks to be skipped for the device to 
>>> > return 0 from
>>> > @@ -572,10 +573,14 @@ struct pm_subsys_data {
>>> >   * necessary from the driver's perspective.  It also may cause them to 
>>> > skip
>>> >   * invocations of the ->suspend_late and ->suspend_noirq callbacks 
>>> > provided by
>>> >   * the driver if they decide to leave the device in runtime suspend.
>>> > + *
>>> > + * Setting LEAVE_SUSPENDED informs the PM core and middle-layer code 
>>> > that the
>>> > + * driver prefers the device to be left in runtime suspend after system 
>>> > resume.
>>> >   */
>>>
>>> Question: Can LEAVE_SUSPENDED and NEVER_SKIP be valid combination? I
>>> guess not!? Should we validate for wrong combinations?
>>
>> Why not?  There's no real overlap between them.
>
> Except that NEVER_SKIP, documentation wise, tells you that your
> suspend and resume callbacks will never be skipped. :-)

You mean the comment in pm.h I suppose?  Yes, it isn't precise enough.

The proper documentation in devices.rst is less ambiguous, though. :-)

> [...]
>
>>> Second, have you considered setting the default value of
>>> dev->power.may_skip_resume to true?
>>
>> Yes.
>>
>>> That would means the subsystem
>>> instead need to implement an opt-out method. I am thinking that it may
>>> not be an issue, since we anyway at this point, don't have drivers
>>> using the LEAVE_SUSPENDED flag.
>>
>> Opt-out doesn't work because of the need to invoke the "noirq" callbacks.
>
> I am not sure I follow that.
>
> Whatever needs to be fixed on the subsystem level, that could be done
> before the driver starts using the LEAVE_SUSPENDED flag. No?

That requires a bit of explanation, sorry for being overly concise.

The core calls ->resume_noirq from the middle layer regardless of
whether or not the device will be left suspended, so the
->resume_noirq cannot do arbitrary things to it.  Setting
may_skip_resume by the middle layer tells the core that the middle
layer is ready for that and is going to cooperate.  If may_skip_resume
had been set by default, that piece of information would have been
missing.

Thanks,
Rafael

Re: [PATCH v3 1/6] PM / core: Add LEAVE_SUSPENDED driver flag

[Rafael J. Wysocki]

On Fri, Nov 17, 2017 at 12:07 AM, Rafael J. Wysocki  wrote:
> On Thursday, November 16, 2017 4:10:16 PM CET Ulf Hansson wrote:
>> On 12 November 2017 at 01:37, Rafael J. Wysocki  wrote:
>> > From: Rafael J. Wysocki 
>> >
>> > Define and document a new driver flag, DPM_FLAG_LEAVE_SUSPENDED, to
>> > instruct the PM core and middle-layer (bus type, PM domain, etc.)
>> > code that it is desirable to leave the device in runtime suspend
>> > after system-wide transitions to the working state (for example,
>> > the device may be slow to resume and it may be better to avoid
>> > resuming it right away).
>> >
>> > Generally, the middle-layer code involved in the handling of the
>> > device is expected to indicate to the PM core whether or not the
>> > device may be left in suspend with the help of the device's
>> > power.may_skip_resume status bit.  That has to happen in the "noirq"
>> > phase of the preceding system suspend (or analogous) transition.
>> > The middle layer is then responsible for handling the device as
>> > appropriate in its "noirq" resume callback which is executed
>> > regardless of whether or not the device may be left suspended, but
>> > the other resume callbacks (except for ->complete) will be skipped
>> > automatically by the core if the device really can be left in
>> > suspend.
>> >
>> > The additional power.must_resume status bit introduced for the
>> > implementation of this mechanisn is used internally by the PM core
>> > to track the requirement to resume the device (which may depend on
>> > its children etc).
>> >
>> > Signed-off-by: Rafael J. Wysocki 
>> > Acked-by: Greg Kroah-Hartman 
>> > ---
>> >
>> > v2 -> v3: Take dev->power.usage_count when updating power.must_resume in
>> >   __device_suspend_noirq().
>> >
>> > ---

[...]

>> > +   } else {
>> > +   dev->power.must_resume = true;
>> > +   }
>> > +
>> > +   if (dev->power.must_resume)
>> > +   dpm_superior_set_must_resume(dev);
>> >
>> >  Complete:
>> > complete_all(>power.completion);
>> > @@ -1487,6 +1539,9 @@ static int __device_suspend(struct devic
>> > dev->power.direct_complete = false;
>> > }
>> >
>> > +   dev->power.may_skip_resume = false;
>> > +   dev->power.must_resume = false;
>> > +
>>
>> First, these assignment could be bypassed if the direct_complete path
>> is used. Perhaps it's more robust to reset these flags already in
>> device_prepare().
>
> In the direct-complete case may_skip_resume doesn't matter.
>
> must_resume should be set to "false", however, so that parents of
> direct-complete devices may be left in suspend (in case they don't
> fall under direct-complete themselves), so good catch.

Actually, not really.

must_resume for parents/suppliers is not updated if the device has
direct_complete set and the device's own must_resume doesn't matter
then.

So this part is good as is AFAICS.

Thanks,
Rafael

Re: [PATCH v3 1/6] PM / core: Add LEAVE_SUSPENDED driver flag

[Rafael J. Wysocki]

On Fri, Nov 17, 2017 at 12:07 AM, Rafael J. Wysocki  wrote:
> On Thursday, November 16, 2017 4:10:16 PM CET Ulf Hansson wrote:
>> On 12 November 2017 at 01:37, Rafael J. Wysocki  wrote:
>> > From: Rafael J. Wysocki 
>> >
>> > Define and document a new driver flag, DPM_FLAG_LEAVE_SUSPENDED, to
>> > instruct the PM core and middle-layer (bus type, PM domain, etc.)
>> > code that it is desirable to leave the device in runtime suspend
>> > after system-wide transitions to the working state (for example,
>> > the device may be slow to resume and it may be better to avoid
>> > resuming it right away).
>> >
>> > Generally, the middle-layer code involved in the handling of the
>> > device is expected to indicate to the PM core whether or not the
>> > device may be left in suspend with the help of the device's
>> > power.may_skip_resume status bit.  That has to happen in the "noirq"
>> > phase of the preceding system suspend (or analogous) transition.
>> > The middle layer is then responsible for handling the device as
>> > appropriate in its "noirq" resume callback which is executed
>> > regardless of whether or not the device may be left suspended, but
>> > the other resume callbacks (except for ->complete) will be skipped
>> > automatically by the core if the device really can be left in
>> > suspend.
>> >
>> > The additional power.must_resume status bit introduced for the
>> > implementation of this mechanisn is used internally by the PM core
>> > to track the requirement to resume the device (which may depend on
>> > its children etc).
>> >
>> > Signed-off-by: Rafael J. Wysocki 
>> > Acked-by: Greg Kroah-Hartman 
>> > ---
>> >
>> > v2 -> v3: Take dev->power.usage_count when updating power.must_resume in
>> >   __device_suspend_noirq().
>> >
>> > ---

[...]

>> > +   } else {
>> > +   dev->power.must_resume = true;
>> > +   }
>> > +
>> > +   if (dev->power.must_resume)
>> > +   dpm_superior_set_must_resume(dev);
>> >
>> >  Complete:
>> > complete_all(>power.completion);
>> > @@ -1487,6 +1539,9 @@ static int __device_suspend(struct devic
>> > dev->power.direct_complete = false;
>> > }
>> >
>> > +   dev->power.may_skip_resume = false;
>> > +   dev->power.must_resume = false;
>> > +
>>
>> First, these assignment could be bypassed if the direct_complete path
>> is used. Perhaps it's more robust to reset these flags already in
>> device_prepare().
>
> In the direct-complete case may_skip_resume doesn't matter.
>
> must_resume should be set to "false", however, so that parents of
> direct-complete devices may be left in suspend (in case they don't
> fall under direct-complete themselves), so good catch.

Actually, not really.

must_resume for parents/suppliers is not updated if the device has
direct_complete set and the device's own must_resume doesn't matter
then.

So this part is good as is AFAICS.

Thanks,
Rafael

Re: [PATCH v3 1/6] PM / core: Add LEAVE_SUSPENDED driver flag

[Ulf Hansson]

[...]

>> > +++ linux-pm/include/linux/pm.h
>> > @@ -559,6 +559,7 @@ struct pm_subsys_data {
>> >   * NEVER_SKIP: Do not skip system suspend/resume callbacks for the device.
>> >   * SMART_PREPARE: Check the return value of the driver's ->prepare 
>> > callback.
>> >   * SMART_SUSPEND: No need to resume the device from runtime suspend.
>> > + * LEAVE_SUSPENDED: Avoid resuming the device during system resume if 
>> > possible.
>> >   *
>> >   * Setting SMART_PREPARE instructs bus types and PM domains which may want
>> >   * system suspend/resume callbacks to be skipped for the device to return 
>> > 0 from
>> > @@ -572,10 +573,14 @@ struct pm_subsys_data {
>> >   * necessary from the driver's perspective.  It also may cause them to 
>> > skip
>> >   * invocations of the ->suspend_late and ->suspend_noirq callbacks 
>> > provided by
>> >   * the driver if they decide to leave the device in runtime suspend.
>> > + *
>> > + * Setting LEAVE_SUSPENDED informs the PM core and middle-layer code that 
>> > the
>> > + * driver prefers the device to be left in runtime suspend after system 
>> > resume.
>> >   */
>>
>> Question: Can LEAVE_SUSPENDED and NEVER_SKIP be valid combination? I
>> guess not!? Should we validate for wrong combinations?
>
> Why not?  There's no real overlap between them.

Except that NEVER_SKIP, documentation wise, tells you that your
suspend and resume callbacks will never be skipped. :-)

[...]

>> Second, have you considered setting the default value of
>> dev->power.may_skip_resume to true?
>
> Yes.
>
>> That would means the subsystem
>> instead need to implement an opt-out method. I am thinking that it may
>> not be an issue, since we anyway at this point, don't have drivers
>> using the LEAVE_SUSPENDED flag.
>
> Opt-out doesn't work because of the need to invoke the "noirq" callbacks.

I am not sure I follow that.

Whatever needs to be fixed on the subsystem level, that could be done
before the driver starts using the LEAVE_SUSPENDED flag. No?

>
>> [...]
>>
>> > +However, it may be desirable to leave some devices in runtime suspend 
>> > after
>> > +system transitions to the working state and device drivers can use the
>> > +``DPM_FLAG_LEAVE_SUSPENDED`` flag to indicate to the PM core (and 
>> > middle-layer
>> > +code) that this is the case.  Whether or not the devices will actually be 
>> > left
>> > +in suspend may depend on their state before the given system 
>> > suspend-resume
>> > +cycle and on the type of the system transition under way.  In particular,
>> > +devices are not left suspended if that transition is a restore from 
>> > hibernation,
>> > +as device states are not guaranteed to be reflected by the information 
>> > stored in
>> > +the hibernation image in that case.
>> > +
>> > +The middle-layer code involved in the handling of the device has to 
>> > indicate to
>> > +the PM core if the device may be left in suspend with the help of its
>> > +:c:member:`power.may_skip_resume` status bit.  That has to happen in the 
>> > "noirq"
>> > +phase of the preceding system-wide suspend (or analogous) transition.  The
>>
>> Does it have to be managed in the "noirq" phase? Wouldn't be perfectly
>> okay do this in the suspend and suspend_late phases as well?
>
> The wording is slightly misleading I think.
>
> In fact technically may_skip_resume may be set earlier, but the core checks it
> in the "noirq" phase only anyway.

Yeah, okay.

Kind regards
Uffe

Re: [PATCH v3 1/6] PM / core: Add LEAVE_SUSPENDED driver flag

[Ulf Hansson]

[...]

>> > +++ linux-pm/include/linux/pm.h
>> > @@ -559,6 +559,7 @@ struct pm_subsys_data {
>> >   * NEVER_SKIP: Do not skip system suspend/resume callbacks for the device.
>> >   * SMART_PREPARE: Check the return value of the driver's ->prepare 
>> > callback.
>> >   * SMART_SUSPEND: No need to resume the device from runtime suspend.
>> > + * LEAVE_SUSPENDED: Avoid resuming the device during system resume if 
>> > possible.
>> >   *
>> >   * Setting SMART_PREPARE instructs bus types and PM domains which may want
>> >   * system suspend/resume callbacks to be skipped for the device to return 
>> > 0 from
>> > @@ -572,10 +573,14 @@ struct pm_subsys_data {
>> >   * necessary from the driver's perspective.  It also may cause them to 
>> > skip
>> >   * invocations of the ->suspend_late and ->suspend_noirq callbacks 
>> > provided by
>> >   * the driver if they decide to leave the device in runtime suspend.
>> > + *
>> > + * Setting LEAVE_SUSPENDED informs the PM core and middle-layer code that 
>> > the
>> > + * driver prefers the device to be left in runtime suspend after system 
>> > resume.
>> >   */
>>
>> Question: Can LEAVE_SUSPENDED and NEVER_SKIP be valid combination? I
>> guess not!? Should we validate for wrong combinations?
>
> Why not?  There's no real overlap between them.

Except that NEVER_SKIP, documentation wise, tells you that your
suspend and resume callbacks will never be skipped. :-)

[...]

>> Second, have you considered setting the default value of
>> dev->power.may_skip_resume to true?
>
> Yes.
>
>> That would means the subsystem
>> instead need to implement an opt-out method. I am thinking that it may
>> not be an issue, since we anyway at this point, don't have drivers
>> using the LEAVE_SUSPENDED flag.
>
> Opt-out doesn't work because of the need to invoke the "noirq" callbacks.

I am not sure I follow that.

Whatever needs to be fixed on the subsystem level, that could be done
before the driver starts using the LEAVE_SUSPENDED flag. No?

>
>> [...]
>>
>> > +However, it may be desirable to leave some devices in runtime suspend 
>> > after
>> > +system transitions to the working state and device drivers can use the
>> > +``DPM_FLAG_LEAVE_SUSPENDED`` flag to indicate to the PM core (and 
>> > middle-layer
>> > +code) that this is the case.  Whether or not the devices will actually be 
>> > left
>> > +in suspend may depend on their state before the given system 
>> > suspend-resume
>> > +cycle and on the type of the system transition under way.  In particular,
>> > +devices are not left suspended if that transition is a restore from 
>> > hibernation,
>> > +as device states are not guaranteed to be reflected by the information 
>> > stored in
>> > +the hibernation image in that case.
>> > +
>> > +The middle-layer code involved in the handling of the device has to 
>> > indicate to
>> > +the PM core if the device may be left in suspend with the help of its
>> > +:c:member:`power.may_skip_resume` status bit.  That has to happen in the 
>> > "noirq"
>> > +phase of the preceding system-wide suspend (or analogous) transition.  The
>>
>> Does it have to be managed in the "noirq" phase? Wouldn't be perfectly
>> okay do this in the suspend and suspend_late phases as well?
>
> The wording is slightly misleading I think.
>
> In fact technically may_skip_resume may be set earlier, but the core checks it
> in the "noirq" phase only anyway.

Yeah, okay.

Kind regards
Uffe

Re: [PATCH v3 1/6] PM / core: Add LEAVE_SUSPENDED driver flag

[Rafael J. Wysocki]

On Thursday, November 16, 2017 4:10:16 PM CET Ulf Hansson wrote:
> On 12 November 2017 at 01:37, Rafael J. Wysocki  wrote:
> > From: Rafael J. Wysocki 
> >
> > Define and document a new driver flag, DPM_FLAG_LEAVE_SUSPENDED, to
> > instruct the PM core and middle-layer (bus type, PM domain, etc.)
> > code that it is desirable to leave the device in runtime suspend
> > after system-wide transitions to the working state (for example,
> > the device may be slow to resume and it may be better to avoid
> > resuming it right away).
> >
> > Generally, the middle-layer code involved in the handling of the
> > device is expected to indicate to the PM core whether or not the
> > device may be left in suspend with the help of the device's
> > power.may_skip_resume status bit.  That has to happen in the "noirq"
> > phase of the preceding system suspend (or analogous) transition.
> > The middle layer is then responsible for handling the device as
> > appropriate in its "noirq" resume callback which is executed
> > regardless of whether or not the device may be left suspended, but
> > the other resume callbacks (except for ->complete) will be skipped
> > automatically by the core if the device really can be left in
> > suspend.
> >
> > The additional power.must_resume status bit introduced for the
> > implementation of this mechanisn is used internally by the PM core
> > to track the requirement to resume the device (which may depend on
> > its children etc).
> >
> > Signed-off-by: Rafael J. Wysocki 
> > Acked-by: Greg Kroah-Hartman 
> > ---
> >
> > v2 -> v3: Take dev->power.usage_count when updating power.must_resume in
> >   __device_suspend_noirq().
> >
> > ---
> >  Documentation/driver-api/pm/devices.rst |   24 ++-
> >  drivers/base/power/main.c   |   66 
> > +---
> >  drivers/base/power/runtime.c|9 ++--
> >  include/linux/pm.h  |   14 +-
> >  include/linux/pm_runtime.h  |9 ++--
> >  5 files changed, 104 insertions(+), 18 deletions(-)
> >
> > Index: linux-pm/include/linux/pm.h
> > ===
> > --- linux-pm.orig/include/linux/pm.h
> > +++ linux-pm/include/linux/pm.h
> > @@ -559,6 +559,7 @@ struct pm_subsys_data {
> >   * NEVER_SKIP: Do not skip system suspend/resume callbacks for the device.
> >   * SMART_PREPARE: Check the return value of the driver's ->prepare 
> > callback.
> >   * SMART_SUSPEND: No need to resume the device from runtime suspend.
> > + * LEAVE_SUSPENDED: Avoid resuming the device during system resume if 
> > possible.
> >   *
> >   * Setting SMART_PREPARE instructs bus types and PM domains which may want
> >   * system suspend/resume callbacks to be skipped for the device to return 
> > 0 from
> > @@ -572,10 +573,14 @@ struct pm_subsys_data {
> >   * necessary from the driver's perspective.  It also may cause them to skip
> >   * invocations of the ->suspend_late and ->suspend_noirq callbacks 
> > provided by
> >   * the driver if they decide to leave the device in runtime suspend.
> > + *
> > + * Setting LEAVE_SUSPENDED informs the PM core and middle-layer code that 
> > the
> > + * driver prefers the device to be left in runtime suspend after system 
> > resume.
> >   */
> 
> Question: Can LEAVE_SUSPENDED and NEVER_SKIP be valid combination? I
> guess not!? Should we validate for wrong combinations?

Why not?  There's no real overlap between them.

> 
> [...]
> 
> >  /**
> >   * __device_suspend_noirq - Execute a "noirq suspend" callback for given 
> > device.
> >   * @dev: Device to handle.
> > @@ -1127,10 +1161,28 @@ static int __device_suspend_noirq(struct
> > }
> >
> > error = dpm_run_callback(callback, dev, state, info);
> > -   if (!error)
> > -   dev->power.is_noirq_suspended = true;
> > -   else
> > +   if (error) {
> > async_error = error;
> > +   goto Complete;
> > +   }
> > +
> > +   dev->power.is_noirq_suspended = true;
> > +
> > +   if (dev_pm_test_driver_flags(dev, DPM_FLAG_LEAVE_SUSPENDED)) {
> > +   /*
> > +* The only safe strategy here is to require that if the 
> > device
> > +* may not be left in suspend, resume callbacks must be 
> > invoked
> > +* for it.
> > +*/
> > +   dev->power.must_resume = dev->power.must_resume ||
> > +   !dev->power.may_skip_resume ||
> > +   
> > atomic_read(>power.usage_count);
> 
> dev->power.usage_count is always > 0 at this point, meaning that
> dev->power.must_resume always becomes true. :-)
> 
> You should rather use "atomic_read(>power.usage_count) > 1".

Right, thanks.  I tend to forget about that.

> > +   } else {
> > +   

Re: [PATCH v3 1/6] PM / core: Add LEAVE_SUSPENDED driver flag

[Rafael J. Wysocki]

On Thursday, November 16, 2017 4:10:16 PM CET Ulf Hansson wrote:
> On 12 November 2017 at 01:37, Rafael J. Wysocki  wrote:
> > From: Rafael J. Wysocki 
> >
> > Define and document a new driver flag, DPM_FLAG_LEAVE_SUSPENDED, to
> > instruct the PM core and middle-layer (bus type, PM domain, etc.)
> > code that it is desirable to leave the device in runtime suspend
> > after system-wide transitions to the working state (for example,
> > the device may be slow to resume and it may be better to avoid
> > resuming it right away).
> >
> > Generally, the middle-layer code involved in the handling of the
> > device is expected to indicate to the PM core whether or not the
> > device may be left in suspend with the help of the device's
> > power.may_skip_resume status bit.  That has to happen in the "noirq"
> > phase of the preceding system suspend (or analogous) transition.
> > The middle layer is then responsible for handling the device as
> > appropriate in its "noirq" resume callback which is executed
> > regardless of whether or not the device may be left suspended, but
> > the other resume callbacks (except for ->complete) will be skipped
> > automatically by the core if the device really can be left in
> > suspend.
> >
> > The additional power.must_resume status bit introduced for the
> > implementation of this mechanisn is used internally by the PM core
> > to track the requirement to resume the device (which may depend on
> > its children etc).
> >
> > Signed-off-by: Rafael J. Wysocki 
> > Acked-by: Greg Kroah-Hartman 
> > ---
> >
> > v2 -> v3: Take dev->power.usage_count when updating power.must_resume in
> >   __device_suspend_noirq().
> >
> > ---
> >  Documentation/driver-api/pm/devices.rst |   24 ++-
> >  drivers/base/power/main.c   |   66 
> > +---
> >  drivers/base/power/runtime.c|9 ++--
> >  include/linux/pm.h  |   14 +-
> >  include/linux/pm_runtime.h  |9 ++--
> >  5 files changed, 104 insertions(+), 18 deletions(-)
> >
> > Index: linux-pm/include/linux/pm.h
> > ===
> > --- linux-pm.orig/include/linux/pm.h
> > +++ linux-pm/include/linux/pm.h
> > @@ -559,6 +559,7 @@ struct pm_subsys_data {
> >   * NEVER_SKIP: Do not skip system suspend/resume callbacks for the device.
> >   * SMART_PREPARE: Check the return value of the driver's ->prepare 
> > callback.
> >   * SMART_SUSPEND: No need to resume the device from runtime suspend.
> > + * LEAVE_SUSPENDED: Avoid resuming the device during system resume if 
> > possible.
> >   *
> >   * Setting SMART_PREPARE instructs bus types and PM domains which may want
> >   * system suspend/resume callbacks to be skipped for the device to return 
> > 0 from
> > @@ -572,10 +573,14 @@ struct pm_subsys_data {
> >   * necessary from the driver's perspective.  It also may cause them to skip
> >   * invocations of the ->suspend_late and ->suspend_noirq callbacks 
> > provided by
> >   * the driver if they decide to leave the device in runtime suspend.
> > + *
> > + * Setting LEAVE_SUSPENDED informs the PM core and middle-layer code that 
> > the
> > + * driver prefers the device to be left in runtime suspend after system 
> > resume.
> >   */
> 
> Question: Can LEAVE_SUSPENDED and NEVER_SKIP be valid combination? I
> guess not!? Should we validate for wrong combinations?

Why not?  There's no real overlap between them.

> 
> [...]
> 
> >  /**
> >   * __device_suspend_noirq - Execute a "noirq suspend" callback for given 
> > device.
> >   * @dev: Device to handle.
> > @@ -1127,10 +1161,28 @@ static int __device_suspend_noirq(struct
> > }
> >
> > error = dpm_run_callback(callback, dev, state, info);
> > -   if (!error)
> > -   dev->power.is_noirq_suspended = true;
> > -   else
> > +   if (error) {
> > async_error = error;
> > +   goto Complete;
> > +   }
> > +
> > +   dev->power.is_noirq_suspended = true;
> > +
> > +   if (dev_pm_test_driver_flags(dev, DPM_FLAG_LEAVE_SUSPENDED)) {
> > +   /*
> > +* The only safe strategy here is to require that if the 
> > device
> > +* may not be left in suspend, resume callbacks must be 
> > invoked
> > +* for it.
> > +*/
> > +   dev->power.must_resume = dev->power.must_resume ||
> > +   !dev->power.may_skip_resume ||
> > +   
> > atomic_read(>power.usage_count);
> 
> dev->power.usage_count is always > 0 at this point, meaning that
> dev->power.must_resume always becomes true. :-)
> 
> You should rather use "atomic_read(>power.usage_count) > 1".

Right, thanks.  I tend to forget about that.

> > +   } else {
> > +   dev->power.must_resume = true;
> > +   }
> > +
> > +   if (dev->power.must_resume)
> > +

Re: [PATCH v3 1/6] PM / core: Add LEAVE_SUSPENDED driver flag

[Ulf Hansson]

On 12 November 2017 at 01:37, Rafael J. Wysocki  wrote:
> From: Rafael J. Wysocki 
>
> Define and document a new driver flag, DPM_FLAG_LEAVE_SUSPENDED, to
> instruct the PM core and middle-layer (bus type, PM domain, etc.)
> code that it is desirable to leave the device in runtime suspend
> after system-wide transitions to the working state (for example,
> the device may be slow to resume and it may be better to avoid
> resuming it right away).
>
> Generally, the middle-layer code involved in the handling of the
> device is expected to indicate to the PM core whether or not the
> device may be left in suspend with the help of the device's
> power.may_skip_resume status bit.  That has to happen in the "noirq"
> phase of the preceding system suspend (or analogous) transition.
> The middle layer is then responsible for handling the device as
> appropriate in its "noirq" resume callback which is executed
> regardless of whether or not the device may be left suspended, but
> the other resume callbacks (except for ->complete) will be skipped
> automatically by the core if the device really can be left in
> suspend.
>
> The additional power.must_resume status bit introduced for the
> implementation of this mechanisn is used internally by the PM core
> to track the requirement to resume the device (which may depend on
> its children etc).
>
> Signed-off-by: Rafael J. Wysocki 
> Acked-by: Greg Kroah-Hartman 
> ---
>
> v2 -> v3: Take dev->power.usage_count when updating power.must_resume in
>   __device_suspend_noirq().
>
> ---
>  Documentation/driver-api/pm/devices.rst |   24 ++-
>  drivers/base/power/main.c   |   66 
> +---
>  drivers/base/power/runtime.c|9 ++--
>  include/linux/pm.h  |   14 +-
>  include/linux/pm_runtime.h  |9 ++--
>  5 files changed, 104 insertions(+), 18 deletions(-)
>
> Index: linux-pm/include/linux/pm.h
> ===
> --- linux-pm.orig/include/linux/pm.h
> +++ linux-pm/include/linux/pm.h
> @@ -559,6 +559,7 @@ struct pm_subsys_data {
>   * NEVER_SKIP: Do not skip system suspend/resume callbacks for the device.
>   * SMART_PREPARE: Check the return value of the driver's ->prepare callback.
>   * SMART_SUSPEND: No need to resume the device from runtime suspend.
> + * LEAVE_SUSPENDED: Avoid resuming the device during system resume if 
> possible.
>   *
>   * Setting SMART_PREPARE instructs bus types and PM domains which may want
>   * system suspend/resume callbacks to be skipped for the device to return 0 
> from
> @@ -572,10 +573,14 @@ struct pm_subsys_data {
>   * necessary from the driver's perspective.  It also may cause them to skip
>   * invocations of the ->suspend_late and ->suspend_noirq callbacks provided 
> by
>   * the driver if they decide to leave the device in runtime suspend.
> + *
> + * Setting LEAVE_SUSPENDED informs the PM core and middle-layer code that the
> + * driver prefers the device to be left in runtime suspend after system 
> resume.
>   */

Question: Can LEAVE_SUSPENDED and NEVER_SKIP be valid combination? I
guess not!? Should we validate for wrong combinations?

[...]

>  /**
>   * __device_suspend_noirq - Execute a "noirq suspend" callback for given 
> device.
>   * @dev: Device to handle.
> @@ -1127,10 +1161,28 @@ static int __device_suspend_noirq(struct
> }
>
> error = dpm_run_callback(callback, dev, state, info);
> -   if (!error)
> -   dev->power.is_noirq_suspended = true;
> -   else
> +   if (error) {
> async_error = error;
> +   goto Complete;
> +   }
> +
> +   dev->power.is_noirq_suspended = true;
> +
> +   if (dev_pm_test_driver_flags(dev, DPM_FLAG_LEAVE_SUSPENDED)) {
> +   /*
> +* The only safe strategy here is to require that if the 
> device
> +* may not be left in suspend, resume callbacks must be 
> invoked
> +* for it.
> +*/
> +   dev->power.must_resume = dev->power.must_resume ||
> +   !dev->power.may_skip_resume ||
> +   atomic_read(>power.usage_count);

dev->power.usage_count is always > 0 at this point, meaning that
dev->power.must_resume always becomes true. :-)

You should rather use "atomic_read(>power.usage_count) > 1".

> +   } else {
> +   dev->power.must_resume = true;
> +   }
> +
> +   if (dev->power.must_resume)
> +   dpm_superior_set_must_resume(dev);
>
>  Complete:
> complete_all(>power.completion);
> @@ -1487,6 +1539,9 @@ static int __device_suspend(struct devic
> dev->power.direct_complete = false;
> }
>
> +   dev->power.may_skip_resume = false;
> +   

Re: [PATCH v3 1/6] PM / core: Add LEAVE_SUSPENDED driver flag

[Ulf Hansson]

On 12 November 2017 at 01:37, Rafael J. Wysocki  wrote:
> From: Rafael J. Wysocki 
>
> Define and document a new driver flag, DPM_FLAG_LEAVE_SUSPENDED, to
> instruct the PM core and middle-layer (bus type, PM domain, etc.)
> code that it is desirable to leave the device in runtime suspend
> after system-wide transitions to the working state (for example,
> the device may be slow to resume and it may be better to avoid
> resuming it right away).
>
> Generally, the middle-layer code involved in the handling of the
> device is expected to indicate to the PM core whether or not the
> device may be left in suspend with the help of the device's
> power.may_skip_resume status bit.  That has to happen in the "noirq"
> phase of the preceding system suspend (or analogous) transition.
> The middle layer is then responsible for handling the device as
> appropriate in its "noirq" resume callback which is executed
> regardless of whether or not the device may be left suspended, but
> the other resume callbacks (except for ->complete) will be skipped
> automatically by the core if the device really can be left in
> suspend.
>
> The additional power.must_resume status bit introduced for the
> implementation of this mechanisn is used internally by the PM core
> to track the requirement to resume the device (which may depend on
> its children etc).
>
> Signed-off-by: Rafael J. Wysocki 
> Acked-by: Greg Kroah-Hartman 
> ---
>
> v2 -> v3: Take dev->power.usage_count when updating power.must_resume in
>   __device_suspend_noirq().
>
> ---
>  Documentation/driver-api/pm/devices.rst |   24 ++-
>  drivers/base/power/main.c   |   66 
> +---
>  drivers/base/power/runtime.c|9 ++--
>  include/linux/pm.h  |   14 +-
>  include/linux/pm_runtime.h  |9 ++--
>  5 files changed, 104 insertions(+), 18 deletions(-)
>
> Index: linux-pm/include/linux/pm.h
> ===
> --- linux-pm.orig/include/linux/pm.h
> +++ linux-pm/include/linux/pm.h
> @@ -559,6 +559,7 @@ struct pm_subsys_data {
>   * NEVER_SKIP: Do not skip system suspend/resume callbacks for the device.
>   * SMART_PREPARE: Check the return value of the driver's ->prepare callback.
>   * SMART_SUSPEND: No need to resume the device from runtime suspend.
> + * LEAVE_SUSPENDED: Avoid resuming the device during system resume if 
> possible.
>   *
>   * Setting SMART_PREPARE instructs bus types and PM domains which may want
>   * system suspend/resume callbacks to be skipped for the device to return 0 
> from
> @@ -572,10 +573,14 @@ struct pm_subsys_data {
>   * necessary from the driver's perspective.  It also may cause them to skip
>   * invocations of the ->suspend_late and ->suspend_noirq callbacks provided 
> by
>   * the driver if they decide to leave the device in runtime suspend.
> + *
> + * Setting LEAVE_SUSPENDED informs the PM core and middle-layer code that the
> + * driver prefers the device to be left in runtime suspend after system 
> resume.
>   */

Question: Can LEAVE_SUSPENDED and NEVER_SKIP be valid combination? I
guess not!? Should we validate for wrong combinations?

[...]

>  /**
>   * __device_suspend_noirq - Execute a "noirq suspend" callback for given 
> device.
>   * @dev: Device to handle.
> @@ -1127,10 +1161,28 @@ static int __device_suspend_noirq(struct
> }
>
> error = dpm_run_callback(callback, dev, state, info);
> -   if (!error)
> -   dev->power.is_noirq_suspended = true;
> -   else
> +   if (error) {
> async_error = error;
> +   goto Complete;
> +   }
> +
> +   dev->power.is_noirq_suspended = true;
> +
> +   if (dev_pm_test_driver_flags(dev, DPM_FLAG_LEAVE_SUSPENDED)) {
> +   /*
> +* The only safe strategy here is to require that if the 
> device
> +* may not be left in suspend, resume callbacks must be 
> invoked
> +* for it.
> +*/
> +   dev->power.must_resume = dev->power.must_resume ||
> +   !dev->power.may_skip_resume ||
> +   atomic_read(>power.usage_count);

dev->power.usage_count is always > 0 at this point, meaning that
dev->power.must_resume always becomes true. :-)

You should rather use "atomic_read(>power.usage_count) > 1".

> +   } else {
> +   dev->power.must_resume = true;
> +   }
> +
> +   if (dev->power.must_resume)
> +   dpm_superior_set_must_resume(dev);
>
>  Complete:
> complete_all(>power.completion);
> @@ -1487,6 +1539,9 @@ static int __device_suspend(struct devic
> dev->power.direct_complete = false;
> }
>
> +   dev->power.may_skip_resume = false;
> +   dev->power.must_resume = false;
> +

First, these assignment could be bypassed if the direct_complete path