Re: [PATCH] PM / runtime: Rework pm_runtime_force_suspend/resume()

[Rafael J. Wysocki]

On Wednesday, January 3, 2018 12:21:36 AM CET Rafael J. Wysocki wrote:
> On Tue, Jan 2, 2018 at 8:07 PM, Rafael J. Wysocki  wrote:
> > On Tuesday, January 2, 2018 2:04:04 PM CET Lukas Wunner wrote:
> >> On Tue, Jan 02, 2018 at 12:02:18PM +0100, Rafael J. Wysocki wrote:
> >> > On Tue, Jan 2, 2018 at 11:51 AM, Lukas Wunner  wrote:
> >> > > On Tue, Jan 02, 2018 at 01:56:28AM +0100, Rafael J. Wysocki wrote:
> >> > >> + if (atomic_read(>power.usage_count) <= 1 &&
> >> > >> + atomic_read(>power.child_count) == 0)
> >> > >> + pm_runtime_set_suspended(dev);
> >> > >>
> >> > >> - pm_runtime_set_suspended(dev);
> >> > >
> >> > > The ->runtime_suspend callback *has* been executed at this point.
> >> > > If the status is only updated conditionally, it may not reflect
> >> > > the device's actual power state correctly.  That doesn't seem to
> >> > > be a good idea.
> >> >
> >> > It doesn't matter, because this is done with runtime PM disabled, isn't 
> >> > it?
> >>
> >> It might not make a difference for the use case I have in mind, but
> >> pm_runtime_status_suspended() will return an incorrect result and is
> >> called from 47 files in 4.15-rc6 according to lxr.free-electrons.com.
> >
> > Generally, the runtime PM status is only meaningful for devices with 
> > runtime PM
> > enabled.
> >
> > There is an exception, which is during system suspend/resume, when runtime 
> > PM
> > is automatically disabled by the core, but that only under certain 
> > assumptions.
> >
> > Basically, you have to assume that no one else will mess up with the device
> > between the times you call pm_runtime_status_suspended() to check its 
> > runtime
> > PM status (or between the first time you do that and the last time runtime 
> > PM
> > has been enabled for the device).
> >
> > This patch doesn't change the situation in that respect.
> 
> BTW, I'm not sure why you are worrying about the "status" field alone
> and not about the usage counter that can be greater than 0 after
> pm_runtime_force_suspend() which is inconsistent with the device's
> physical state (and with the "status" field too for that matter -
> always without the patch and in some cases with it) then.  As a matter
> of fact, the information left by the runtime PM framework is messed up
> with here this way or another and so anyway the only party that can
> make sense of it after pm_runtime_force_suspend() is the subsequent
> pm_runtime_force_resume().

All of that said, I have overlooked the fact that pm_runtime_force_suspend()
itself can be called twice in a row for the same device during the same
system-wide PM transition (genpd can do that, confusingly enough).

I'll send a v2 in a moment.

Thanks,
Rafael

Re: [PATCH] PM / runtime: Rework pm_runtime_force_suspend/resume()

[Rafael J. Wysocki]

On Wednesday, January 3, 2018 12:21:36 AM CET Rafael J. Wysocki wrote:
> On Tue, Jan 2, 2018 at 8:07 PM, Rafael J. Wysocki  wrote:
> > On Tuesday, January 2, 2018 2:04:04 PM CET Lukas Wunner wrote:
> >> On Tue, Jan 02, 2018 at 12:02:18PM +0100, Rafael J. Wysocki wrote:
> >> > On Tue, Jan 2, 2018 at 11:51 AM, Lukas Wunner  wrote:
> >> > > On Tue, Jan 02, 2018 at 01:56:28AM +0100, Rafael J. Wysocki wrote:
> >> > >> + if (atomic_read(>power.usage_count) <= 1 &&
> >> > >> + atomic_read(>power.child_count) == 0)
> >> > >> + pm_runtime_set_suspended(dev);
> >> > >>
> >> > >> - pm_runtime_set_suspended(dev);
> >> > >
> >> > > The ->runtime_suspend callback *has* been executed at this point.
> >> > > If the status is only updated conditionally, it may not reflect
> >> > > the device's actual power state correctly.  That doesn't seem to
> >> > > be a good idea.
> >> >
> >> > It doesn't matter, because this is done with runtime PM disabled, isn't 
> >> > it?
> >>
> >> It might not make a difference for the use case I have in mind, but
> >> pm_runtime_status_suspended() will return an incorrect result and is
> >> called from 47 files in 4.15-rc6 according to lxr.free-electrons.com.
> >
> > Generally, the runtime PM status is only meaningful for devices with 
> > runtime PM
> > enabled.
> >
> > There is an exception, which is during system suspend/resume, when runtime 
> > PM
> > is automatically disabled by the core, but that only under certain 
> > assumptions.
> >
> > Basically, you have to assume that no one else will mess up with the device
> > between the times you call pm_runtime_status_suspended() to check its 
> > runtime
> > PM status (or between the first time you do that and the last time runtime 
> > PM
> > has been enabled for the device).
> >
> > This patch doesn't change the situation in that respect.
> 
> BTW, I'm not sure why you are worrying about the "status" field alone
> and not about the usage counter that can be greater than 0 after
> pm_runtime_force_suspend() which is inconsistent with the device's
> physical state (and with the "status" field too for that matter -
> always without the patch and in some cases with it) then.  As a matter
> of fact, the information left by the runtime PM framework is messed up
> with here this way or another and so anyway the only party that can
> make sense of it after pm_runtime_force_suspend() is the subsequent
> pm_runtime_force_resume().

All of that said, I have overlooked the fact that pm_runtime_force_suspend()
itself can be called twice in a row for the same device during the same
system-wide PM transition (genpd can do that, confusingly enough).

I'll send a v2 in a moment.

Thanks,
Rafael

Re: [PATCH] PM / runtime: Rework pm_runtime_force_suspend/resume()

[Rafael J. Wysocki]

On Tue, Jan 2, 2018 at 8:07 PM, Rafael J. Wysocki  wrote:
> On Tuesday, January 2, 2018 2:04:04 PM CET Lukas Wunner wrote:
>> On Tue, Jan 02, 2018 at 12:02:18PM +0100, Rafael J. Wysocki wrote:
>> > On Tue, Jan 2, 2018 at 11:51 AM, Lukas Wunner  wrote:
>> > > On Tue, Jan 02, 2018 at 01:56:28AM +0100, Rafael J. Wysocki wrote:
>> > >> + if (atomic_read(>power.usage_count) <= 1 &&
>> > >> + atomic_read(>power.child_count) == 0)
>> > >> + pm_runtime_set_suspended(dev);
>> > >>
>> > >> - pm_runtime_set_suspended(dev);
>> > >
>> > > The ->runtime_suspend callback *has* been executed at this point.
>> > > If the status is only updated conditionally, it may not reflect
>> > > the device's actual power state correctly.  That doesn't seem to
>> > > be a good idea.
>> >
>> > It doesn't matter, because this is done with runtime PM disabled, isn't it?
>>
>> It might not make a difference for the use case I have in mind, but
>> pm_runtime_status_suspended() will return an incorrect result and is
>> called from 47 files in 4.15-rc6 according to lxr.free-electrons.com.
>
> Generally, the runtime PM status is only meaningful for devices with runtime 
> PM
> enabled.
>
> There is an exception, which is during system suspend/resume, when runtime PM
> is automatically disabled by the core, but that only under certain 
> assumptions.
>
> Basically, you have to assume that no one else will mess up with the device
> between the times you call pm_runtime_status_suspended() to check its runtime
> PM status (or between the first time you do that and the last time runtime PM
> has been enabled for the device).
>
> This patch doesn't change the situation in that respect.

BTW, I'm not sure why you are worrying about the "status" field alone
and not about the usage counter that can be greater than 0 after
pm_runtime_force_suspend() which is inconsistent with the device's
physical state (and with the "status" field too for that matter -
always without the patch and in some cases with it) then.  As a matter
of fact, the information left by the runtime PM framework is messed up
with here this way or another and so anyway the only party that can
make sense of it after pm_runtime_force_suspend() is the subsequent
pm_runtime_force_resume().

Thanks,
Rafael

Re: [PATCH] PM / runtime: Rework pm_runtime_force_suspend/resume()

[Rafael J. Wysocki]

On Tue, Jan 2, 2018 at 8:07 PM, Rafael J. Wysocki  wrote:
> On Tuesday, January 2, 2018 2:04:04 PM CET Lukas Wunner wrote:
>> On Tue, Jan 02, 2018 at 12:02:18PM +0100, Rafael J. Wysocki wrote:
>> > On Tue, Jan 2, 2018 at 11:51 AM, Lukas Wunner  wrote:
>> > > On Tue, Jan 02, 2018 at 01:56:28AM +0100, Rafael J. Wysocki wrote:
>> > >> + if (atomic_read(>power.usage_count) <= 1 &&
>> > >> + atomic_read(>power.child_count) == 0)
>> > >> + pm_runtime_set_suspended(dev);
>> > >>
>> > >> - pm_runtime_set_suspended(dev);
>> > >
>> > > The ->runtime_suspend callback *has* been executed at this point.
>> > > If the status is only updated conditionally, it may not reflect
>> > > the device's actual power state correctly.  That doesn't seem to
>> > > be a good idea.
>> >
>> > It doesn't matter, because this is done with runtime PM disabled, isn't it?
>>
>> It might not make a difference for the use case I have in mind, but
>> pm_runtime_status_suspended() will return an incorrect result and is
>> called from 47 files in 4.15-rc6 according to lxr.free-electrons.com.
>
> Generally, the runtime PM status is only meaningful for devices with runtime 
> PM
> enabled.
>
> There is an exception, which is during system suspend/resume, when runtime PM
> is automatically disabled by the core, but that only under certain 
> assumptions.
>
> Basically, you have to assume that no one else will mess up with the device
> between the times you call pm_runtime_status_suspended() to check its runtime
> PM status (or between the first time you do that and the last time runtime PM
> has been enabled for the device).
>
> This patch doesn't change the situation in that respect.

BTW, I'm not sure why you are worrying about the "status" field alone
and not about the usage counter that can be greater than 0 after
pm_runtime_force_suspend() which is inconsistent with the device's
physical state (and with the "status" field too for that matter -
always without the patch and in some cases with it) then.  As a matter
of fact, the information left by the runtime PM framework is messed up
with here this way or another and so anyway the only party that can
make sense of it after pm_runtime_force_suspend() is the subsequent
pm_runtime_force_resume().

Thanks,
Rafael

Re: [PATCH] PM / runtime: Rework pm_runtime_force_suspend/resume()

[Rafael J. Wysocki]

On Tuesday, January 2, 2018 2:04:04 PM CET Lukas Wunner wrote:
> On Tue, Jan 02, 2018 at 12:02:18PM +0100, Rafael J. Wysocki wrote:
> > On Tue, Jan 2, 2018 at 11:51 AM, Lukas Wunner  wrote:
> > > On Tue, Jan 02, 2018 at 01:56:28AM +0100, Rafael J. Wysocki wrote:
> > >> + if (atomic_read(>power.usage_count) <= 1 &&
> > >> + atomic_read(>power.child_count) == 0)
> > >> + pm_runtime_set_suspended(dev);
> > >>
> > >> - pm_runtime_set_suspended(dev);
> > >
> > > The ->runtime_suspend callback *has* been executed at this point.
> > > If the status is only updated conditionally, it may not reflect
> > > the device's actual power state correctly.  That doesn't seem to
> > > be a good idea.
> > 
> > It doesn't matter, because this is done with runtime PM disabled, isn't it?
> 
> It might not make a difference for the use case I have in mind, but
> pm_runtime_status_suspended() will return an incorrect result and is
> called from 47 files in 4.15-rc6 according to lxr.free-electrons.com.

Generally, the runtime PM status is only meaningful for devices with runtime PM
enabled.

There is an exception, which is during system suspend/resume, when runtime PM
is automatically disabled by the core, but that only under certain assumptions.

Basically, you have to assume that no one else will mess up with the device
between the times you call pm_runtime_status_suspended() to check its runtime
PM status (or between the first time you do that and the last time runtime PM
has been enabled for the device).

This patch doesn't change the situation in that respect.

> > > The kerneldoc says:
> > >
> > > Typically this function may be invoked from a system suspend callback
> > > to make sure the device is put into low power state.
> > >
> > > That portion is not modified by your patch.
> > >
> > > "Typically" implies that it's legal to call pm_runtime_force_suspend() in
> > > *other* contexts than as a ->suspend hook.
> > 
> > It should only be used during system suspend anyway, however.
> 
> Then the kerneldoc is wrong.

It isn't wrong.  It may be incomplete, but the information in it is correct.

And making it more complete is not part of this patch IMO.

> > >> One addition that would be really helpful:  pm_runtime_force_suspend()
> > >> should also force-suspend all children and consumers of the given
> > >> device.  Likewise, those should be resumed on pm_runtime_force_resume().
> > >> Then I could just add a device link from the audio PCI device on the GPU
> > >> to the graphics PCI device and just call pm_runtime_force_*() on the
> > >> graphics device (supplier) to magically power them both off and on.
> > >
> > > Actually, the assumption is that pm_runtime_force_suspend() must be
> > > called for the children before it is called for the parent even
> > > without my patch, so it is just not going to work this way.
> > 
> > Moreover, what if those devices have nonzero usage counters?  There
> > may be other reasons for that than just dependencies, like for example
> > user space might have written "on" to their "control" files in sysfs.
> 
> In that case pm_runtime_force_suspend() should return a negative errno.

In which case it wouldn't be suitable for the system-wide PM callback role.

> I envision amending control_store() so that "off" can be written to the
> "control" file, allowing userspace to invoke pm_runtime_force_suspend()
> to force certain devices into runtime suspend.

But it isn't a good tool for the purpose which I'm trying to tell you.

You need something else.

> The user would get back
> an error if the call failed for some reason (such as an active child or
> consumer of the to be force-suspended device).  That would be a clean
> replacement for the ON/OFF options we currently have for the
> vga_switcheroo debugfs control file.

Well, it looks like you are looking for an interface to invoke
pm_runtime_suspend() for the device as it behaves exactly the way you want. :-)

Thanks,
Rafael

Re: [PATCH] PM / runtime: Rework pm_runtime_force_suspend/resume()

[Rafael J. Wysocki]

On Tuesday, January 2, 2018 2:04:04 PM CET Lukas Wunner wrote:
> On Tue, Jan 02, 2018 at 12:02:18PM +0100, Rafael J. Wysocki wrote:
> > On Tue, Jan 2, 2018 at 11:51 AM, Lukas Wunner  wrote:
> > > On Tue, Jan 02, 2018 at 01:56:28AM +0100, Rafael J. Wysocki wrote:
> > >> + if (atomic_read(>power.usage_count) <= 1 &&
> > >> + atomic_read(>power.child_count) == 0)
> > >> + pm_runtime_set_suspended(dev);
> > >>
> > >> - pm_runtime_set_suspended(dev);
> > >
> > > The ->runtime_suspend callback *has* been executed at this point.
> > > If the status is only updated conditionally, it may not reflect
> > > the device's actual power state correctly.  That doesn't seem to
> > > be a good idea.
> > 
> > It doesn't matter, because this is done with runtime PM disabled, isn't it?
> 
> It might not make a difference for the use case I have in mind, but
> pm_runtime_status_suspended() will return an incorrect result and is
> called from 47 files in 4.15-rc6 according to lxr.free-electrons.com.

Generally, the runtime PM status is only meaningful for devices with runtime PM
enabled.

There is an exception, which is during system suspend/resume, when runtime PM
is automatically disabled by the core, but that only under certain assumptions.

Basically, you have to assume that no one else will mess up with the device
between the times you call pm_runtime_status_suspended() to check its runtime
PM status (or between the first time you do that and the last time runtime PM
has been enabled for the device).

This patch doesn't change the situation in that respect.

> > > The kerneldoc says:
> > >
> > > Typically this function may be invoked from a system suspend callback
> > > to make sure the device is put into low power state.
> > >
> > > That portion is not modified by your patch.
> > >
> > > "Typically" implies that it's legal to call pm_runtime_force_suspend() in
> > > *other* contexts than as a ->suspend hook.
> > 
> > It should only be used during system suspend anyway, however.
> 
> Then the kerneldoc is wrong.

It isn't wrong.  It may be incomplete, but the information in it is correct.

And making it more complete is not part of this patch IMO.

> > >> One addition that would be really helpful:  pm_runtime_force_suspend()
> > >> should also force-suspend all children and consumers of the given
> > >> device.  Likewise, those should be resumed on pm_runtime_force_resume().
> > >> Then I could just add a device link from the audio PCI device on the GPU
> > >> to the graphics PCI device and just call pm_runtime_force_*() on the
> > >> graphics device (supplier) to magically power them both off and on.
> > >
> > > Actually, the assumption is that pm_runtime_force_suspend() must be
> > > called for the children before it is called for the parent even
> > > without my patch, so it is just not going to work this way.
> > 
> > Moreover, what if those devices have nonzero usage counters?  There
> > may be other reasons for that than just dependencies, like for example
> > user space might have written "on" to their "control" files in sysfs.
> 
> In that case pm_runtime_force_suspend() should return a negative errno.

In which case it wouldn't be suitable for the system-wide PM callback role.

> I envision amending control_store() so that "off" can be written to the
> "control" file, allowing userspace to invoke pm_runtime_force_suspend()
> to force certain devices into runtime suspend.

But it isn't a good tool for the purpose which I'm trying to tell you.

You need something else.

> The user would get back
> an error if the call failed for some reason (such as an active child or
> consumer of the to be force-suspended device).  That would be a clean
> replacement for the ON/OFF options we currently have for the
> vga_switcheroo debugfs control file.

Well, it looks like you are looking for an interface to invoke
pm_runtime_suspend() for the device as it behaves exactly the way you want. :-)

Thanks,
Rafael

Re: [PATCH] PM / runtime: Rework pm_runtime_force_suspend/resume()

[Lukas Wunner]

On Tue, Jan 02, 2018 at 12:02:18PM +0100, Rafael J. Wysocki wrote:
> On Tue, Jan 2, 2018 at 11:51 AM, Lukas Wunner  wrote:
> > On Tue, Jan 02, 2018 at 01:56:28AM +0100, Rafael J. Wysocki wrote:
> >> + if (atomic_read(>power.usage_count) <= 1 &&
> >> + atomic_read(>power.child_count) == 0)
> >> + pm_runtime_set_suspended(dev);
> >>
> >> - pm_runtime_set_suspended(dev);
> >
> > The ->runtime_suspend callback *has* been executed at this point.
> > If the status is only updated conditionally, it may not reflect
> > the device's actual power state correctly.  That doesn't seem to
> > be a good idea.
> 
> It doesn't matter, because this is done with runtime PM disabled, isn't it?

It might not make a difference for the use case I have in mind, but
pm_runtime_status_suspended() will return an incorrect result and is
called from 47 files in 4.15-rc6 according to lxr.free-electrons.com.


> > The kerneldoc says:
> >
> > Typically this function may be invoked from a system suspend callback
> > to make sure the device is put into low power state.
> >
> > That portion is not modified by your patch.
> >
> > "Typically" implies that it's legal to call pm_runtime_force_suspend() in
> > *other* contexts than as a ->suspend hook.
> 
> It should only be used during system suspend anyway, however.

Then the kerneldoc is wrong.


> >> One addition that would be really helpful:  pm_runtime_force_suspend()
> >> should also force-suspend all children and consumers of the given
> >> device.  Likewise, those should be resumed on pm_runtime_force_resume().
> >> Then I could just add a device link from the audio PCI device on the GPU
> >> to the graphics PCI device and just call pm_runtime_force_*() on the
> >> graphics device (supplier) to magically power them both off and on.
> >
> > Actually, the assumption is that pm_runtime_force_suspend() must be
> > called for the children before it is called for the parent even
> > without my patch, so it is just not going to work this way.
> 
> Moreover, what if those devices have nonzero usage counters?  There
> may be other reasons for that than just dependencies, like for example
> user space might have written "on" to their "control" files in sysfs.

In that case pm_runtime_force_suspend() should return a negative errno.

I envision amending control_store() so that "off" can be written to the
"control" file, allowing userspace to invoke pm_runtime_force_suspend()
to force certain devices into runtime suspend.  The user would get back
an error if the call failed for some reason (such as an active child or
consumer of the to be force-suspended device).  That would be a clean
replacement for the ON/OFF options we currently have for the
vga_switcheroo debugfs control file.

Thanks,

Lukas

Re: [PATCH] PM / runtime: Rework pm_runtime_force_suspend/resume()

[Lukas Wunner]

On Tue, Jan 02, 2018 at 12:02:18PM +0100, Rafael J. Wysocki wrote:
> On Tue, Jan 2, 2018 at 11:51 AM, Lukas Wunner  wrote:
> > On Tue, Jan 02, 2018 at 01:56:28AM +0100, Rafael J. Wysocki wrote:
> >> + if (atomic_read(>power.usage_count) <= 1 &&
> >> + atomic_read(>power.child_count) == 0)
> >> + pm_runtime_set_suspended(dev);
> >>
> >> - pm_runtime_set_suspended(dev);
> >
> > The ->runtime_suspend callback *has* been executed at this point.
> > If the status is only updated conditionally, it may not reflect
> > the device's actual power state correctly.  That doesn't seem to
> > be a good idea.
> 
> It doesn't matter, because this is done with runtime PM disabled, isn't it?

It might not make a difference for the use case I have in mind, but
pm_runtime_status_suspended() will return an incorrect result and is
called from 47 files in 4.15-rc6 according to lxr.free-electrons.com.


> > The kerneldoc says:
> >
> > Typically this function may be invoked from a system suspend callback
> > to make sure the device is put into low power state.
> >
> > That portion is not modified by your patch.
> >
> > "Typically" implies that it's legal to call pm_runtime_force_suspend() in
> > *other* contexts than as a ->suspend hook.
> 
> It should only be used during system suspend anyway, however.

Then the kerneldoc is wrong.


> >> One addition that would be really helpful:  pm_runtime_force_suspend()
> >> should also force-suspend all children and consumers of the given
> >> device.  Likewise, those should be resumed on pm_runtime_force_resume().
> >> Then I could just add a device link from the audio PCI device on the GPU
> >> to the graphics PCI device and just call pm_runtime_force_*() on the
> >> graphics device (supplier) to magically power them both off and on.
> >
> > Actually, the assumption is that pm_runtime_force_suspend() must be
> > called for the children before it is called for the parent even
> > without my patch, so it is just not going to work this way.
> 
> Moreover, what if those devices have nonzero usage counters?  There
> may be other reasons for that than just dependencies, like for example
> user space might have written "on" to their "control" files in sysfs.

In that case pm_runtime_force_suspend() should return a negative errno.

I envision amending control_store() so that "off" can be written to the
"control" file, allowing userspace to invoke pm_runtime_force_suspend()
to force certain devices into runtime suspend.  The user would get back
an error if the call failed for some reason (such as an active child or
consumer of the to be force-suspended device).  That would be a clean
replacement for the ON/OFF options we currently have for the
vga_switcheroo debugfs control file.

Thanks,

Lukas

Re: [PATCH] PM / runtime: Rework pm_runtime_force_suspend/resume()

[Rafael J. Wysocki]

On Tue, Jan 2, 2018 at 12:02 PM, Rafael J. Wysocki  wrote:
> On Tue, Jan 2, 2018 at 11:51 AM, Lukas Wunner  wrote:
>> On Tue, Jan 02, 2018 at 01:56:28AM +0100, Rafael J. Wysocki wrote:

[cut]

>
>> One addition that would be really helpful:  pm_runtime_force_suspend()
>> should also force-suspend all children and consumers of the given
>> device.  Likewise, those should be resumed on pm_runtime_force_resume().
>> Then I could just add a device link from the audio PCI device on the GPU
>> to the graphics PCI device and just call pm_runtime_force_*() on the
>> graphics device (supplier) to magically power them both off and on.
>
> Actually, the assumption is that pm_runtime_force_suspend() must be
> called for the children before it is called for the parent even
> without my patch, so it is just not going to work this way.

Moreover, what if those devices have nonzero usage counters?  There
may be other reasons for that than just dependencies, like for example
user space might have written "on" to their "control" files in sysfs.

What you are looking for doesn't seem to match the runtime PM
framework's assumptions, I'm afraid.

Thanks,
Rafael

Re: [PATCH] PM / runtime: Rework pm_runtime_force_suspend/resume()

[Rafael J. Wysocki]

On Tue, Jan 2, 2018 at 12:02 PM, Rafael J. Wysocki  wrote:
> On Tue, Jan 2, 2018 at 11:51 AM, Lukas Wunner  wrote:
>> On Tue, Jan 02, 2018 at 01:56:28AM +0100, Rafael J. Wysocki wrote:

[cut]

>
>> One addition that would be really helpful:  pm_runtime_force_suspend()
>> should also force-suspend all children and consumers of the given
>> device.  Likewise, those should be resumed on pm_runtime_force_resume().
>> Then I could just add a device link from the audio PCI device on the GPU
>> to the graphics PCI device and just call pm_runtime_force_*() on the
>> graphics device (supplier) to magically power them both off and on.
>
> Actually, the assumption is that pm_runtime_force_suspend() must be
> called for the children before it is called for the parent even
> without my patch, so it is just not going to work this way.

Moreover, what if those devices have nonzero usage counters?  There
may be other reasons for that than just dependencies, like for example
user space might have written "on" to their "control" files in sysfs.

What you are looking for doesn't seem to match the runtime PM
framework's assumptions, I'm afraid.

Thanks,
Rafael

Re: [PATCH] PM / runtime: Rework pm_runtime_force_suspend/resume()

[Rafael J. Wysocki]

On Tue, Jan 2, 2018 at 11:51 AM, Lukas Wunner  wrote:
> On Tue, Jan 02, 2018 at 01:56:28AM +0100, Rafael J. Wysocki wrote:
>> + if (atomic_read(>power.usage_count) <= 1 &&
>> + atomic_read(>power.child_count) == 0)
>> + pm_runtime_set_suspended(dev);
>>
>> - pm_runtime_set_suspended(dev);
>
> The ->runtime_suspend callback *has* been executed at this point.
> If the status is only updated conditionally, it may not reflect
> the device's actual power state correctly.  That doesn't seem to
> be a good idea.

It doesn't matter, because this is done with runtime PM disabled, isn't it?

So what matters is the status after the subsequent
pm_runtime_force_resume() returns, and that should reflect the actual
state of the device correctly.  Doesn't it?

> The kerneldoc says:
>
> Typically this function may be invoked from a system suspend callback
> to make sure the device is put into low power state.
>
> That portion is not modified by your patch.
>
> "Typically" implies that it's legal to call pm_runtime_force_suspend() in
> *other* contexts than as a ->suspend hook.

It should only be used during system suspend anyway, however.

> Let's say pm_runtime_force_suspend() is invoked at runtime, outside of
> a system sleep transition.

That will disable runtime PM for you until you call
pm_runtime_force_resume() for the device.

> Due to updating the power state only
> conditionally, users will see an incorrect power state in sysfs.

But that's with runtime PM disabled, so it doesn't matter.

> The reason I'm speaking up here or taking an interest in the
> pm_runtime_force_*() functions is that I would like to use them
> for manual power control in vga_switcheroo, the kernel subsystem
> for switchable Laptop graphics.  It currently supports two modes of
> operation for each GPU, automatic power control (autosuspend via
> runtime PM) or manual power control (by echoing ON and OFF to a
> debugfs file).
>
> Manual power control is currently a mess because it switches the GPU
> off behind the PM core's back, causing all sorts of issues during a
> system sleep transition.
>
> Potentially pm_runtime_force_*() could be used as a clean way to
> reimplement manual power control because it wouldn't happen behind
> the PM core's back.  However your patch seems to break this potential
> use case because:

No, it doesn't break anything.

It actually doesn't even change anything for real except for dropping
some manipulations of the device's parent usage counter.

> a) the device's power state may not be reflected correctly because
>it's only updated conditionally (see above),
>
> b) pm_runtime_force_resume(), despite its name, is no longer guaranteed
>to force the device on (it now allows the device to continue
>slumbering).

It does that without the patch too.  The name is just confusing. :-)

> One addition that would be really helpful:  pm_runtime_force_suspend()
> should also force-suspend all children and consumers of the given
> device.  Likewise, those should be resumed on pm_runtime_force_resume().
> Then I could just add a device link from the audio PCI device on the GPU
> to the graphics PCI device and just call pm_runtime_force_*() on the
> graphics device (supplier) to magically power them both off and on.

Actually, the assumption is that pm_runtime_force_suspend() must be
called for the children before it is called for the parent even
without my patch, so it is just not going to work this way.

Thanks,
Rafael

Re: [PATCH] PM / runtime: Rework pm_runtime_force_suspend/resume()

[Rafael J. Wysocki]

On Tue, Jan 2, 2018 at 11:51 AM, Lukas Wunner  wrote:
> On Tue, Jan 02, 2018 at 01:56:28AM +0100, Rafael J. Wysocki wrote:
>> + if (atomic_read(>power.usage_count) <= 1 &&
>> + atomic_read(>power.child_count) == 0)
>> + pm_runtime_set_suspended(dev);
>>
>> - pm_runtime_set_suspended(dev);
>
> The ->runtime_suspend callback *has* been executed at this point.
> If the status is only updated conditionally, it may not reflect
> the device's actual power state correctly.  That doesn't seem to
> be a good idea.

It doesn't matter, because this is done with runtime PM disabled, isn't it?

So what matters is the status after the subsequent
pm_runtime_force_resume() returns, and that should reflect the actual
state of the device correctly.  Doesn't it?

> The kerneldoc says:
>
> Typically this function may be invoked from a system suspend callback
> to make sure the device is put into low power state.
>
> That portion is not modified by your patch.
>
> "Typically" implies that it's legal to call pm_runtime_force_suspend() in
> *other* contexts than as a ->suspend hook.

It should only be used during system suspend anyway, however.

> Let's say pm_runtime_force_suspend() is invoked at runtime, outside of
> a system sleep transition.

That will disable runtime PM for you until you call
pm_runtime_force_resume() for the device.

> Due to updating the power state only
> conditionally, users will see an incorrect power state in sysfs.

But that's with runtime PM disabled, so it doesn't matter.

> The reason I'm speaking up here or taking an interest in the
> pm_runtime_force_*() functions is that I would like to use them
> for manual power control in vga_switcheroo, the kernel subsystem
> for switchable Laptop graphics.  It currently supports two modes of
> operation for each GPU, automatic power control (autosuspend via
> runtime PM) or manual power control (by echoing ON and OFF to a
> debugfs file).
>
> Manual power control is currently a mess because it switches the GPU
> off behind the PM core's back, causing all sorts of issues during a
> system sleep transition.
>
> Potentially pm_runtime_force_*() could be used as a clean way to
> reimplement manual power control because it wouldn't happen behind
> the PM core's back.  However your patch seems to break this potential
> use case because:

No, it doesn't break anything.

It actually doesn't even change anything for real except for dropping
some manipulations of the device's parent usage counter.

> a) the device's power state may not be reflected correctly because
>it's only updated conditionally (see above),
>
> b) pm_runtime_force_resume(), despite its name, is no longer guaranteed
>to force the device on (it now allows the device to continue
>slumbering).

It does that without the patch too.  The name is just confusing. :-)

> One addition that would be really helpful:  pm_runtime_force_suspend()
> should also force-suspend all children and consumers of the given
> device.  Likewise, those should be resumed on pm_runtime_force_resume().
> Then I could just add a device link from the audio PCI device on the GPU
> to the graphics PCI device and just call pm_runtime_force_*() on the
> graphics device (supplier) to magically power them both off and on.

Actually, the assumption is that pm_runtime_force_suspend() must be
called for the children before it is called for the parent even
without my patch, so it is just not going to work this way.

Thanks,
Rafael

Re: [PATCH] PM / runtime: Rework pm_runtime_force_suspend/resume()

[Lukas Wunner]

On Tue, Jan 02, 2018 at 01:56:28AM +0100, Rafael J. Wysocki wrote:
> + if (atomic_read(>power.usage_count) <= 1 &&
> + atomic_read(>power.child_count) == 0)
> + pm_runtime_set_suspended(dev);
>  
> - pm_runtime_set_suspended(dev);

The ->runtime_suspend callback *has* been executed at this point.
If the status is only updated conditionally, it may not reflect
the device's actual power state correctly.  That doesn't seem to
be a good idea.

The kerneldoc says:

Typically this function may be invoked from a system suspend callback
to make sure the device is put into low power state.

That portion is not modified by your patch.

"Typically" implies that it's legal to call pm_runtime_force_suspend() in
*other* contexts than as a ->suspend hook.

Let's say pm_runtime_force_suspend() is invoked at runtime, outside of
a system sleep transition.  Due to updating the power state only
conditionally, users will see an incorrect power state in sysfs.

The reason I'm speaking up here or taking an interest in the
pm_runtime_force_*() functions is that I would like to use them
for manual power control in vga_switcheroo, the kernel subsystem
for switchable Laptop graphics.  It currently supports two modes of
operation for each GPU, automatic power control (autosuspend via
runtime PM) or manual power control (by echoing ON and OFF to a
debugfs file).

Manual power control is currently a mess because it switches the GPU
off behind the PM core's back, causing all sorts of issues during a
system sleep transition.

Potentially pm_runtime_force_*() could be used as a clean way to
reimplement manual power control because it wouldn't happen behind
the PM core's back.  However your patch seems to break this potential
use case because:

a) the device's power state may not be reflected correctly because
   it's only updated conditionally (see above),

b) pm_runtime_force_resume(), despite its name, is no longer guaranteed
   to force the device on (it now allows the device to continue
   slumbering).

One addition that would be really helpful:  pm_runtime_force_suspend()
should also force-suspend all children and consumers of the given
device.  Likewise, those should be resumed on pm_runtime_force_resume().
Then I could just add a device link from the audio PCI device on the GPU
to the graphics PCI device and just call pm_runtime_force_*() on the
graphics device (supplier) to magically power them both off and on.

Thanks,

Lukas

Re: [PATCH] PM / runtime: Rework pm_runtime_force_suspend/resume()

[Lukas Wunner]

On Tue, Jan 02, 2018 at 01:56:28AM +0100, Rafael J. Wysocki wrote:
> + if (atomic_read(>power.usage_count) <= 1 &&
> + atomic_read(>power.child_count) == 0)
> + pm_runtime_set_suspended(dev);
>  
> - pm_runtime_set_suspended(dev);

The ->runtime_suspend callback *has* been executed at this point.
If the status is only updated conditionally, it may not reflect
the device's actual power state correctly.  That doesn't seem to
be a good idea.

The kerneldoc says:

Typically this function may be invoked from a system suspend callback
to make sure the device is put into low power state.

That portion is not modified by your patch.

"Typically" implies that it's legal to call pm_runtime_force_suspend() in
*other* contexts than as a ->suspend hook.

Let's say pm_runtime_force_suspend() is invoked at runtime, outside of
a system sleep transition.  Due to updating the power state only
conditionally, users will see an incorrect power state in sysfs.

The reason I'm speaking up here or taking an interest in the
pm_runtime_force_*() functions is that I would like to use them
for manual power control in vga_switcheroo, the kernel subsystem
for switchable Laptop graphics.  It currently supports two modes of
operation for each GPU, automatic power control (autosuspend via
runtime PM) or manual power control (by echoing ON and OFF to a
debugfs file).

Manual power control is currently a mess because it switches the GPU
off behind the PM core's back, causing all sorts of issues during a
system sleep transition.

Potentially pm_runtime_force_*() could be used as a clean way to
reimplement manual power control because it wouldn't happen behind
the PM core's back.  However your patch seems to break this potential
use case because:

a) the device's power state may not be reflected correctly because
   it's only updated conditionally (see above),

b) pm_runtime_force_resume(), despite its name, is no longer guaranteed
   to force the device on (it now allows the device to continue
   slumbering).

One addition that would be really helpful:  pm_runtime_force_suspend()
should also force-suspend all children and consumers of the given
device.  Likewise, those should be resumed on pm_runtime_force_resume().
Then I could just add a device link from the audio PCI device on the GPU
to the graphics PCI device and just call pm_runtime_force_*() on the
graphics device (supplier) to magically power them both off and on.

Thanks,

Lukas

[PATCH] PM / runtime: Rework pm_runtime_force_suspend/resume()

[Rafael J. Wysocki]

From: Rafael J. Wysocki 

One of the limitations of pm_runtime_force_suspend/resume() is that
if a parent driver wants to use these functions, all of its child
drivers have to do that too because of the parent usage counter
manipulations necessary to get the correct state of the parent during
system-wide transitions to the working state (system resume).
However, that limitation turns out to be artificial, so remove it.

First, notice that it is not really necessary to change the device's
runtime PM status in pm_runtime_force_suspend().  Moreover, if
pm_runtime_set_suspended() is not called for a device, the children
counter of its parent (if there is a parent) will not drop, so it
is not necessary to increment the parent's usage counter in that
case, as long as the children counters of devices are checked
along with their usage counters in order to decide whether or not
the devices may be left in suspend on the way out (that is, during
the subsequent system-wide transition to the working state).

Accordingly, modify pm_runtime_force_suspend() to only call
pm_runtime_set_suspended() for devices whose usage and children
counters are at the "no references" level and drop the parent usage
counter incrementation from it.

Second, notice that with the above change in pm_runtime_force_suspend(),
if the runtime PM status of the device in pm_runtime_force_resume() is
"suspended", then either the device had remained in runtime suspend
before pm_runtime_force_suspend() was called for it (in which case it
is safe to leave it in suspend), or its runtime PM status has been
changed by pm_runtime_force_suspend() itself.  Of course, the latter
case is only possible if the usage and children counters of the device
are at the "no reference" levels, so the device may be left in suspend
then.

In turn, if the runtime PM status of the device in
pm_runtime_force_resume() is "active", pm_runtime_force_suspend()
called for it previously found active references to it, so it needs
to be resumed.

Hence, with the above change in pm_runtime_force_suspend(),
pm_runtime_force_resume() only needs to check the device's runtime PM
status to decide whether or not to resume it and it doesn't need to
manipulate the device parent's usage counter any more, so modify it
accordingly.

Signed-off-by: Rafael J. Wysocki 
---
 drivers/base/power/runtime.c |   61 +--
 1 file changed, 19 insertions(+), 42 deletions(-)

Index: linux-pm/drivers/base/power/runtime.c
===
--- linux-pm.orig/drivers/base/power/runtime.c
+++ linux-pm/drivers/base/power/runtime.c
@@ -1618,12 +1618,15 @@ void pm_runtime_drop_link(struct device
  * @dev: Device to suspend.
  *
  * Disable runtime PM so we safely can check the device's runtime PM status and
- * if it is active, invoke it's .runtime_suspend callback to bring it into
- * suspend state. Keep runtime PM disabled to preserve the state unless we
- * encounter errors.
+ * if it is active, invoke it's ->runtime_suspend callback to suspend it.  
Also,
+ * if the device's usage and children counters don't indicate that the device
+ * was in use before the system-wide transition under way, change its runtime 
PM
+ * status to suspended after suspending it.  Keep runtime PM disabled to
+ * preserve the state unless we encounter errors.
  *
  * Typically this function may be invoked from a system suspend callback to 
make
- * sure the device is put into low power state.
+ * sure the device is put into low power state.  It assumes that the analogous
+ * pm_runtime_force_resume() will be used to resume the device.
  */
 int pm_runtime_force_suspend(struct device *dev)
 {
@@ -1645,18 +1648,12 @@ int pm_runtime_force_suspend(struct devi
if (ret)
goto err;
 
-   /*
-* Increase the runtime PM usage count for the device's parent, in case
-* when we find the device being used when system suspend was invoked.
-* This informs pm_runtime_force_resume() to resume the parent
-* immediately, which is needed to be able to resume its children,
-* when not deferring the resume to be managed via runtime PM.
-*/
-   if (dev->parent && atomic_read(>power.usage_count) > 1)
-   pm_runtime_get_noresume(dev->parent);
+   if (atomic_read(>power.usage_count) <= 1 &&
+   atomic_read(>power.child_count) == 0)
+   pm_runtime_set_suspended(dev);
 
-   pm_runtime_set_suspended(dev);
return 0;
+
 err:
pm_runtime_enable(dev);
return ret;
@@ -1669,13 +1666,9 @@ EXPORT_SYMBOL_GPL(pm_runtime_force_suspe
  *
  * Prior invoking this function we expect the user to have brought the device
  * into low power state by a call to pm_runtime_force_suspend(). Here we 
reverse
- * those actions and brings the device into full power, if it is expected to be
- * used on 

[PATCH] PM / runtime: Rework pm_runtime_force_suspend/resume()

[Rafael J. Wysocki]

From: Rafael J. Wysocki 

One of the limitations of pm_runtime_force_suspend/resume() is that
if a parent driver wants to use these functions, all of its child
drivers have to do that too because of the parent usage counter
manipulations necessary to get the correct state of the parent during
system-wide transitions to the working state (system resume).
However, that limitation turns out to be artificial, so remove it.

First, notice that it is not really necessary to change the device's
runtime PM status in pm_runtime_force_suspend().  Moreover, if
pm_runtime_set_suspended() is not called for a device, the children
counter of its parent (if there is a parent) will not drop, so it
is not necessary to increment the parent's usage counter in that
case, as long as the children counters of devices are checked
along with their usage counters in order to decide whether or not
the devices may be left in suspend on the way out (that is, during
the subsequent system-wide transition to the working state).

Accordingly, modify pm_runtime_force_suspend() to only call
pm_runtime_set_suspended() for devices whose usage and children
counters are at the "no references" level and drop the parent usage
counter incrementation from it.

Second, notice that with the above change in pm_runtime_force_suspend(),
if the runtime PM status of the device in pm_runtime_force_resume() is
"suspended", then either the device had remained in runtime suspend
before pm_runtime_force_suspend() was called for it (in which case it
is safe to leave it in suspend), or its runtime PM status has been
changed by pm_runtime_force_suspend() itself.  Of course, the latter
case is only possible if the usage and children counters of the device
are at the "no reference" levels, so the device may be left in suspend
then.

In turn, if the runtime PM status of the device in
pm_runtime_force_resume() is "active", pm_runtime_force_suspend()
called for it previously found active references to it, so it needs
to be resumed.

Hence, with the above change in pm_runtime_force_suspend(),
pm_runtime_force_resume() only needs to check the device's runtime PM
status to decide whether or not to resume it and it doesn't need to
manipulate the device parent's usage counter any more, so modify it
accordingly.

Signed-off-by: Rafael J. Wysocki 
---
 drivers/base/power/runtime.c |   61 +--
 1 file changed, 19 insertions(+), 42 deletions(-)

Index: linux-pm/drivers/base/power/runtime.c
===
--- linux-pm.orig/drivers/base/power/runtime.c
+++ linux-pm/drivers/base/power/runtime.c
@@ -1618,12 +1618,15 @@ void pm_runtime_drop_link(struct device
  * @dev: Device to suspend.
  *
  * Disable runtime PM so we safely can check the device's runtime PM status and
- * if it is active, invoke it's .runtime_suspend callback to bring it into
- * suspend state. Keep runtime PM disabled to preserve the state unless we
- * encounter errors.
+ * if it is active, invoke it's ->runtime_suspend callback to suspend it.  
Also,
+ * if the device's usage and children counters don't indicate that the device
+ * was in use before the system-wide transition under way, change its runtime 
PM
+ * status to suspended after suspending it.  Keep runtime PM disabled to
+ * preserve the state unless we encounter errors.
  *
  * Typically this function may be invoked from a system suspend callback to 
make
- * sure the device is put into low power state.
+ * sure the device is put into low power state.  It assumes that the analogous
+ * pm_runtime_force_resume() will be used to resume the device.
  */
 int pm_runtime_force_suspend(struct device *dev)
 {
@@ -1645,18 +1648,12 @@ int pm_runtime_force_suspend(struct devi
if (ret)
goto err;
 
-   /*
-* Increase the runtime PM usage count for the device's parent, in case
-* when we find the device being used when system suspend was invoked.
-* This informs pm_runtime_force_resume() to resume the parent
-* immediately, which is needed to be able to resume its children,
-* when not deferring the resume to be managed via runtime PM.
-*/
-   if (dev->parent && atomic_read(>power.usage_count) > 1)
-   pm_runtime_get_noresume(dev->parent);
+   if (atomic_read(>power.usage_count) <= 1 &&
+   atomic_read(>power.child_count) == 0)
+   pm_runtime_set_suspended(dev);
 
-   pm_runtime_set_suspended(dev);
return 0;
+
 err:
pm_runtime_enable(dev);
return ret;
@@ -1669,13 +1666,9 @@ EXPORT_SYMBOL_GPL(pm_runtime_force_suspe
  *
  * Prior invoking this function we expect the user to have brought the device
  * into low power state by a call to pm_runtime_force_suspend(). Here we 
reverse
- * those actions and brings the device into full power, if it is expected to be
- * used on system resume. To distinguish that, we check whether the