PM / Domains: Fix race conditions during boot

To sucessfully probe some devices their corresponding PM domains may need to be powered.

Quoted 2 lines · Ulf Hansson

To sucessfully probe some devices their corresponding PM domains may need to be powered.

Isn't that what pm_runtime_get*() is supposed to be doing? Why isn't that working?

Quoted 5 lines · Kevin Hilman

Quoted 2 lines · Ulf Hansson

To sucessfully probe some devices their corresponding PM domains may need to be powered.

Quoted 2 lines

Isn't that what pm_runtime_get*() is supposed to be doing? Why isn't that working?

Also, I do not understand why we placing device into a power domain only when we probe it. Why if I unbind device from its driver (or do not have a driver for it) it disappears from its power domain? To me power domain and having driver bound to a device are 2 orthogonal concepts.

Quoted 9 lines · Dmitry Torokhov

Quoted 7 lines · Kevin Hilman

Quoted 2 lines · Ulf Hansson

To sucessfully probe some devices their corresponding PM domains may need to be powered.

Isn't that what pm_runtime_get*() is supposed to be doing? Why isn't that working?

Quoted 4 lines

Also, I do not understand why we placing device into a power domain only when we probe it. Why if I unbind device from its driver (or do not have a driver for it) it disappears from its power domain? To me power domain and having driver bound to a device are 2 orthogonal concepts.

That's a different discussion, I don't think we want to go there within this context.

Quoted 2 lines · Ulf Hansson

To sucessfully probe some devices their corresponding PM domains may need to be powered.

Isn't that what pm_runtime_get*() is supposed to be doing? Why isn't that working?

Changes in v3:
-Rework the entire intermediate step which was suggested in v2.
That means solving the race condition, but also cope with PM domains that are initialized in powered off state.

Changes in v2:
-Added some acks.
-Updated commit messages.
-Included a wider audience of the patchset. V1 was lacking SoC
maintainers.

Here are link to the first patchset, were some discussion started.
http://marc.info/?l=linux-pm&m=141208104729597&w=2

There may be more than one device in a PM domain which then will be probed at different points in time.

Depending on timing and runtime PM support, in for the device related driver/subsystem, a PM domain may be advised to power off after a successful probe sequence.

A general requirement for a device within a PM domain, is that the PM domain must stay powered during the probe sequence. To cope with such requirement, let's add two new APIs, dev_pm_domain_get|put().

These APIs are intended to be invoked from subsystem-level code and the calls between get/put needs to be balanced.

dev_pm_domain_get(), tells the PM domain that it needs to increase a usage count and to keep supplying power. dev_pm_domain_put(), does the opposite.

Quoted 6 lines · Ulf Hansson

There may be more than one device in a PM domain which then will be probed at different points in time.

Depending on timing and runtime PM support, in for the device related driver/subsystem, a PM domain may be advised to power off after a successful probe sequence.

Quoted 3 lines

A general requirement for a device within a PM domain, is that the PM domain must stay powered during the probe sequence. To cope with such requirement, let's add two new APIs, dev_pm_domain_get|put().

I'm confused. Why arent' pm_runtime_get*() and pm_runtime_put*() working?

What's not explained here (or what I'm not understanding) is why a PM domain is powering off if it has active devices.

Quoted 14 lines · Kevin Hilman

Quoted 10 lines · Ulf Hansson

There may be more than one device in a PM domain which then will be probed at different points in time.

Depending on timing and runtime PM support, in for the device related driver/subsystem, a PM domain may be advised to power off after a successful probe sequence.

A general requirement for a device within a PM domain, is that the PM domain must stay powered during the probe sequence. To cope with such requirement, let's add two new APIs, dev_pm_domain_get|put().

I'm confused. Why arent' pm_runtime_get*() and pm_runtime_put*() working?

Quoted 2 lines

What's not explained here (or what I'm not understanding) is why a PM domain is powering off if it has active devices.

The issue AIUI is what happens during system boot - if one device in a domain probes and marks itself runtime idle then that will trigger domain powerdown even if there is another device in the domain that hasn't yet been probed. This can cause undesirable glitches (or worse) during boot depending on what's getting powered down.

Quoted 4 lines · Mark Brown

Quoted 1 line · Kevin Hilman

I'm confused. Why arent' pm_runtime_get*() and pm_runtime_put*() working?

Quoted 8 lines

Quoted 2 lines

What's not explained here (or what I'm not understanding) is why a PM domain is powering off if it has active devices.

The issue AIUI is what happens during system boot - if one device in a domain probes and marks itself runtime idle then that will trigger domain powerdown even if there is another device in the domain that hasn't yet been probed. This can cause undesirable glitches (or worse) during boot depending on what's getting powered down.

I'm not quite seeing how this series fixes that problem.

Looking at platform devices in PATCH 4/9, the new _get() and _put() are still happening around ->probe(), so if a platform device runtime suspends after probe, don't we still have a PM domain that can turn off?

Quoted 25 lines · Ulf Hansson

Changes in v3:
-Rework the entire intermediate step which was suggested in v2.
That means solving the race condition, but also cope with PM domains that are initialized in powered off state.

Changes in v2:
-Added some acks.
-Updated commit messages.
-Included a wider audience of the patchset. V1 was lacking SoC
maintainers.

Here are link to the first patchset, were some discussion started.
http://marc.info/?l=linux-pm&m=141208104729597&w=2

There may be more than one device in a PM domain which then will be probed at different points in time.

Depending on timing and runtime PM support, in for the device related driver/subsystem, a PM domain may be advised to power off after a successful probe sequence.

A general requirement for a device within a PM domain, is that the PM domain must stay powered during the probe sequence. To cope with such requirement, let's add two new APIs, dev_pm_domain_get|put().

Quoted 6 lines

These APIs are intended to be invoked from subsystem-level code and the calls between get/put needs to be balanced.

dev_pm_domain_get(), tells the PM domain that it needs to increase a usage count and to keep supplying power. dev_pm_domain_put(), does the opposite.

I'm confused. Why arent' pm_runtime_get*() and pm_runtime_put*() working?

What's not explained here (or what I'm not understanding) is why a PM domain is powering off if it has active devices.

Quoted 27 lines · Kevin Hilman

Quoted 25 lines · Ulf Hansson

Changes in v3:
-Rework the entire intermediate step which was suggested in v2.
That means solving the race condition, but also cope with PM domains that are initialized in powered off state.

Changes in v2:
-Added some acks.
-Updated commit messages.
-Included a wider audience of the patchset. V1 was lacking SoC
maintainers.

Here are link to the first patchset, were some discussion started.
http://marc.info/?l=linux-pm&m=141208104729597&w=2

There may be more than one device in a PM domain which then will be probed at different points in time.

Depending on timing and runtime PM support, in for the device related driver/subsystem, a PM domain may be advised to power off after a successful probe sequence.

A general requirement for a device within a PM domain, is that the PM domain must stay powered during the probe sequence. To cope with such requirement, let's add two new APIs, dev_pm_domain_get|put().

Quoted 8 lines

Quoted 6 lines

These APIs are intended to be invoked from subsystem-level code and the calls between get/put needs to be balanced.

dev_pm_domain_get(), tells the PM domain that it needs to increase a usage count and to keep supplying power. dev_pm_domain_put(), does the opposite.

I'm confused. Why arent' pm_runtime_get*() and pm_runtime_put*() working?

See, below.

Quoted 3 lines

What's not explained here (or what I'm not understanding) is why a PM domain is powering off if it has active devices.

It doesn't. The problem is that using pm_runtime_get_sync() in this path is not working.

Now, I failed to include some of the important information from previous discussions around this patchset. Let me iterate the patchset with better commit messages, but let's first continue this thread.

Here are some of the previous discussion:

http://marc.info/?l=linux-pm&m=141270897014653&w=2
http://marc.info/?l=linux-pm&m=141208104729597&w=2

Below is a summary of why I think "pm_runtime_get_sync()" isn't working for us.

1)
It's bad practice to use pm_runtime_get_sync() in the ->probe() path,

to bring your resources to full power. The consequence would be a driver that requires CONFIG_PM_RUNTIME to be even functional, which just isn't acceptable.

Drivers that behaves well within this context, follows the runtime PM documentation/recommendation.

They use pm_runtime_set_active() during ->probe() to reflect that their devices are fully powered and capable of handling I/O.

You may also have a look at these discussions which also touches this topic, but within a context of another patchset.
https://lkml.org/lkml/2014/10/23/95

2)
Another good example why pm_runtime_get_sync() is a bad solution to our problem, is the amba bus. Before it invokes the driver's ->probe() callback it does the following.
- "enable bus clock"
- pm_runtime_get_noresume()
- pm_runtime_set_active()
- pm_runtime_enable()

For these scenarios a pm_runtime_get_sync() from any of amba driver's ->probe() callback wouldn't have any effect, since the device is already active. In other words, the resources needs to be "manually" enabled.

Quoted 1 line · Ulf Hansson

On 24 October 2014 18:12, Kevin Hilman <[email protected]> wrote:

Quoted 3 lines

1)
It's bad practice to use pm_runtime_get_sync() in the ->probe() path,

Honestly, I'm no longer amused.

Quoted 3 lines

to bring your resources to full power. The consequence would be a driver that requires CONFIG_PM_RUNTIME to be even functional, which just isn't acceptable.

Sorry, but this is utter nonsense.

CONFIG_PM_RUNTIME unset means "no runtime PM at all", so all drivers can expect everything they need to be always on. If that is not the case, then someone is doing runtime PM behind the scenes and therefore cheating. Or in different words, for CONFIG_PM_RUNTIME unset bus types, platforms etc must ensure that everything is on from the drivers' perspective.

If that is the case, then calling pm_runtime_get_sync() from ->probe for CONFIG_PM_RUNTIME unset simply doesn't matter.

Now, for CONFIG_PM_RUNTIME enabled, if power domains are in use, doing pm_runtime_get_sync() from ->probe is the only way the driver can ensure in a non-racy way that the device will be accessible going forward.

Why? Simply because the probing need not happen during system initialization. It very well may take places when the other devices in the same domain have beein in use for quite a while and have been using runtime PM (in which case the domain may go off at any time unless it is explicityly prevented from doing that).

One thing that you may be missing is that, for CONFIG_PM_RUNTIME set, runtime PM has to be either enabled or disabled for all devices in one domain (and if it is disabled, then the domain needs to be always on for all practical purposes). Otherwise you can't just make all of them happy at the same time. The documentation doesn't cover this, because it had been written before we even started to consider power domains.

Quoted 2 lines

Drivers that behaves well within this context, follows the runtime PM documentation/recommendation.

So please go to Documentation/power/runtime_pm.txt and read it again. Quote:

"If the default initial runtime PM status of the device (i.e. 'suspended') reflects the actual state of the device, its bus type's or its driver's ->probe() callback will likely need to wake it up using one of the PM core's helper functions described in Section 4. In that case, pm_runtime_resume() should be used. Of course, for this purpose the device's runtime PM has to be enabled earlier by calling pm_runtime_enable()."

Quoted 2 lines · Rafael J. Wysocki

Quoted 1 line · Ulf Hansson

On 24 October 2014 18:12, Kevin Hilman <[email protected]> wrote:

[cut]

Quoted 41 lines

Quoted 3 lines

1)
It's bad practice to use pm_runtime_get_sync() in the ->probe() path,

Honestly, I'm no longer amused.

Quoted 3 lines

to bring your resources to full power. The consequence would be a driver that requires CONFIG_PM_RUNTIME to be even functional, which just isn't acceptable.

Sorry, but this is utter nonsense.

CONFIG_PM_RUNTIME unset means "no runtime PM at all", so all drivers can expect everything they need to be always on. If that is not the case, then someone is doing runtime PM behind the scenes and therefore cheating. Or in different words, for CONFIG_PM_RUNTIME unset bus types, platforms etc must ensure that everything is on from the drivers' perspective.

If that is the case, then calling pm_runtime_get_sync() from ->probe for CONFIG_PM_RUNTIME unset simply doesn't matter.

Now, for CONFIG_PM_RUNTIME enabled, if power domains are in use, doing pm_runtime_get_sync() from ->probe is the only way the driver can ensure in a non-racy way that the device will be accessible going forward.

Why? Simply because the probing need not happen during system initialization. It very well may take places when the other devices in the same domain have beein in use for quite a while and have been using runtime PM (in which case the domain may go off at any time unless it is explicityly prevented from doing that).

One thing that you may be missing is that, for CONFIG_PM_RUNTIME set, runtime PM has to be either enabled or disabled for all devices in one domain (and if it is disabled, then the domain needs to be always on for all practical purposes). Otherwise you can't just make all of them happy at the same time. The documentation doesn't cover this, because it had been written before we even started to consider power domains.

Quoted 2 lines

Drivers that behaves well within this context, follows the runtime PM documentation/recommendation.

Quoted 8 lines

So please go to Documentation/power/runtime_pm.txt and read it again. Quote:

"If the default initial runtime PM status of the device (i.e. 'suspended') reflects the actual state of the device, its bus type's or its driver's ->probe() callback will likely need to wake it up using one of the PM core's helper functions described in Section 4. In that case, pm_runtime_resume() should be used. Of course, for this purpose the device's runtime PM has to be enabled earlier by calling pm_runtime_enable()."

All that said there is a logical error related to calling pm_runtime_enable() and its derivatives from ->probe() that I've been overlooking pretty much from the start.

Namely, really_probe() sets dev->driver to drv before calling ->probe() for either the bus type or the driver itself, so if the driver's probe calls pm_runtime_enable(), it will execute the driver's own runtime PM resume callback before the driver can check whether or not it wants to handle the device in the first place. That doesn't sound quite right to me.

This means we need a different mechanism to ensure that the device will be accessible to the driver and/or the bus type in ->probe().

At one point we had pm_runtime_get_sync() before really_probe() in driver_proble_device() IIRC, but people complained about it, so we dropped it and put a barrier in there instead, but that's not sufficient.

We need to explicitly ensure that the device won't be powered off while ->probe() is in progress *but* we need to avoid calling the driver's runtime PM resume callback before ->probe() returns.

Quoted 66 lines · Rafael J. Wysocki

Quoted 2 lines · Rafael J. Wysocki

Quoted 1 line · Ulf Hansson

On 24 October 2014 18:12, Kevin Hilman <[email protected]> wrote:

[cut]

Quoted 41 lines

Quoted 3 lines

1)
It's bad practice to use pm_runtime_get_sync() in the ->probe() path,

Honestly, I'm no longer amused.

Quoted 3 lines

to bring your resources to full power. The consequence would be a driver that requires CONFIG_PM_RUNTIME to be even functional, which just isn't acceptable.

Sorry, but this is utter nonsense.

CONFIG_PM_RUNTIME unset means "no runtime PM at all", so all drivers can expect everything they need to be always on. If that is not the case, then someone is doing runtime PM behind the scenes and therefore cheating. Or in different words, for CONFIG_PM_RUNTIME unset bus types, platforms etc must ensure that everything is on from the drivers' perspective.

If that is the case, then calling pm_runtime_get_sync() from ->probe for CONFIG_PM_RUNTIME unset simply doesn't matter.

Now, for CONFIG_PM_RUNTIME enabled, if power domains are in use, doing pm_runtime_get_sync() from ->probe is the only way the driver can ensure in a non-racy way that the device will be accessible going forward.

Why? Simply because the probing need not happen during system initialization. It very well may take places when the other devices in the same domain have beein in use for quite a while and have been using runtime PM (in which case the domain may go off at any time unless it is explicityly prevented from doing that).

One thing that you may be missing is that, for CONFIG_PM_RUNTIME set, runtime PM has to be either enabled or disabled for all devices in one domain (and if it is disabled, then the domain needs to be always on for all practical purposes). Otherwise you can't just make all of them happy at the same time. The documentation doesn't cover this, because it had been written before we even started to consider power domains.

Quoted 2 lines

Drivers that behaves well within this context, follows the runtime PM documentation/recommendation.

Quoted 8 lines

So please go to Documentation/power/runtime_pm.txt and read it again. Quote:

"If the default initial runtime PM status of the device (i.e. 'suspended') reflects the actual state of the device, its bus type's or its driver's ->probe() callback will likely need to wake it up using one of the PM core's helper functions described in Section 4. In that case, pm_runtime_resume() should be used. Of course, for this purpose the device's runtime PM has to be enabled earlier by calling pm_runtime_enable()."

All that said there is a logical error related to calling pm_runtime_enable() and its derivatives from ->probe() that I've been overlooking pretty much from the start.

Namely, really_probe() sets dev->driver to drv before calling ->probe() for either the bus type or the driver itself, so if the driver's probe calls pm_runtime_enable(), it will execute the driver's own runtime PM resume callback before the driver can check whether or not it wants to handle the device in the first place. That doesn't sound quite right to me.

Quoted 6 lines

This means we need a different mechanism to ensure that the device will be accessible to the driver and/or the bus type in ->probe().

At one point we had pm_runtime_get_sync() before really_probe() in driver_proble_device() IIRC, but people complained about it, so we dropped it and put a barrier in there instead, but that's not sufficient.

So we actually had pm_runtime_get_noresume() before the barrier, but then we dropped it due to complaints.

Quoted 3 lines

We need to explicitly ensure that the device won't be powered off while ->probe() is in progress *but* we need to avoid calling the driver's runtime PM resume callback before ->probe() returns.

I wonder, then, if we just need to do pm_runtime_get_sync() instead of the barrier and then pm_runtime_put() instead of pm_request_idle() in driver_probe_device()?

Quoted 20 lines

Quoted 10 lines

All that said there is a logical error related to calling pm_runtime_enable() and its derivatives from ->probe() that I've been overlooking pretty much from the start.

Namely, really_probe() sets dev->driver to drv before calling ->probe() for either the bus type or the driver itself, so if the driver's probe calls pm_runtime_enable(), it will execute the driver's own runtime PM resume callback before the driver can check whether or not it wants to handle the device in the first place. That doesn't sound quite right to me.

Quoted 6 lines

This means we need a different mechanism to ensure that the device will be accessible to the driver and/or the bus type in ->probe().

At one point we had pm_runtime_get_sync() before really_probe() in driver_proble_device() IIRC, but people complained about it, so we dropped it and put a barrier in there instead, but that's not sufficient.

So we actually had pm_runtime_get_noresume() before the barrier, but then we dropped it due to complaints.

Quoted 7 lines

Quoted 3 lines

We need to explicitly ensure that the device won't be powered off while ->probe() is in progress *but* we need to avoid calling the driver's runtime PM resume callback before ->probe() returns.

I wonder, then, if we just need to do pm_runtime_get_sync() instead of the barrier and then pm_runtime_put() instead of pm_request_idle() in driver_probe_device()?

Won't we still have the same problems in the case of ->probe failure that were fixed by removing those calls[1]?

IOW, after the driver's ->probe returns failure, it's no longer safe to call the driver's runtime PM callbacks, since they may be accessing resources that no longer exits.

Hmm, thinking a little more, maybe this kind of failure is a good time for the driver to use pm_runtime_force_suspend(), then later when the PM core does the _put(), it will see the device aleady suspended and there shouldn't be any problems.

So I think I'm OK with this approach, in theory.

[CC: list drastically trimmed]

Quoted 40 lines · Kevin Hilman

Quoted 20 lines

Quoted 10 lines

All that said there is a logical error related to calling pm_runtime_enable() and its derivatives from ->probe() that I've been overlooking pretty much from the start.

Namely, really_probe() sets dev->driver to drv before calling ->probe() for either the bus type or the driver itself, so if the driver's probe calls pm_runtime_enable(), it will execute the driver's own runtime PM resume callback before the driver can check whether or not it wants to handle the device in the first place. That doesn't sound quite right to me.

Quoted 6 lines

This means we need a different mechanism to ensure that the device will be accessible to the driver and/or the bus type in ->probe().

At one point we had pm_runtime_get_sync() before really_probe() in driver_proble_device() IIRC, but people complained about it, so we dropped it and put a barrier in there instead, but that's not sufficient.

So we actually had pm_runtime_get_noresume() before the barrier, but then we dropped it due to complaints.

Quoted 7 lines

Quoted 3 lines

We need to explicitly ensure that the device won't be powered off while ->probe() is in progress *but* we need to avoid calling the driver's runtime PM resume callback before ->probe() returns.

I wonder, then, if we just need to do pm_runtime_get_sync() instead of the barrier and then pm_runtime_put() instead of pm_request_idle() in driver_probe_device()?

Won't we still have the same problems in the case of ->probe failure that were fixed by removing those calls[1]?

IOW, after the driver's ->probe returns failure, it's no longer safe to call the driver's runtime PM callbacks, since they may be accessing resources that no longer exits.

Hmm, thinking a little more, maybe this kind of failure is a good time for the driver to use pm_runtime_force_suspend(), then later when the PM core does the _put(), it will see the device aleady suspended and there shouldn't be any problems.

Quoted 1 line

So I think I'm OK with this approach, in theory.

I still think this needs to be handled at the level of the subsystem and device driver, not at the level of the PM core. If somebody needs to insure that the device won't be powered off while the probe routine is running, the subsystem/driver should take care of it.

After all, as Kevin points out, sometimes we really _do_ want the device to end up powered-down when the probing is finished. There's no way the PM core can guess what the driver layer will want in the end.

Quoted 4 lines

I still think this needs to be handled at the level of the subsystem and device driver, not at the level of the PM core. If somebody needs to insure that the device won't be powered off while the probe routine is running, the subsystem/driver should take care of it.

The problem with that is that doing pm_runtime_resume() (or equivalent) anywhere in the bus type's or driver's ->probe() will trigger the driver's PM callbacks (for the majority of bus types).

PM domains complicate that even further, because they may override bus type callbacks.

Quoted 4 lines

After all, as Kevin points out, sometimes we really _do_ want the device to end up powered-down when the probing is finished. There's no way the PM core can guess what the driver layer will want in the end.

And as I said in a reply to Kevin, if we do

	pm_runtime_get_sync(dev);
	ret = really_probe(dev, drv);
	pm_runtime_put(dev);

in driver_probe_device(), then the last statement will power down the device properly, unless the driver's ->probe() bumped up the reference counter.

Suppose that the device is in a power domain that was turned off before probing the driver, whatever the reason. It needs to but turned on for the driver to be able to access the device in ->probe() and now the question is how to do that.

The natural way would be to call pm_runtime_get_sync(dev) at the beginning of ->probe(), but that would trigger the driver's own ->runtime_resume() to run and it would require the driver to do some extra checks in that routine. Those checks wouldn't be necessary at any other time.

However, if the core does pm_runtime_get_sync(dev) before calling really_probe(), it may turn on the power domain without invoking runtime PM callbacks from the driver.

It looks like we need an agreement on what to do and what the expectations should be for each driver core operation at the high level. That is, what ->probe() should expect, what ->remove() should expect, what the state after executing each of them should be for CONFIG_PM_RUNTIME set and unset etc. Otherwise we'll always have various bus types doing what *they* think is appropriate and not agreeing with each other.

Quoted 81 lines · Rafael J. Wysocki

Quoted 66 lines · Rafael J. Wysocki

Quoted 2 lines · Rafael J. Wysocki

Quoted 1 line · Ulf Hansson

On 24 October 2014 18:12, Kevin Hilman <[email protected]> wrote:

[cut]

Quoted 41 lines

Quoted 3 lines

1)
It's bad practice to use pm_runtime_get_sync() in the ->probe() path,

Honestly, I'm no longer amused.

Quoted 3 lines

to bring your resources to full power. The consequence would be a driver that requires CONFIG_PM_RUNTIME to be even functional, which just isn't acceptable.

Sorry, but this is utter nonsense.

CONFIG_PM_RUNTIME unset means "no runtime PM at all", so all drivers can expect everything they need to be always on. If that is not the case, then someone is doing runtime PM behind the scenes and therefore cheating. Or in different words, for CONFIG_PM_RUNTIME unset bus types, platforms etc must ensure that everything is on from the drivers' perspective.

If that is the case, then calling pm_runtime_get_sync() from ->probe for CONFIG_PM_RUNTIME unset simply doesn't matter.

Now, for CONFIG_PM_RUNTIME enabled, if power domains are in use, doing pm_runtime_get_sync() from ->probe is the only way the driver can ensure in a non-racy way that the device will be accessible going forward.

Why? Simply because the probing need not happen during system initialization. It very well may take places when the other devices in the same domain have beein in use for quite a while and have been using runtime PM (in which case the domain may go off at any time unless it is explicityly prevented from doing that).

One thing that you may be missing is that, for CONFIG_PM_RUNTIME set, runtime PM has to be either enabled or disabled for all devices in one domain (and if it is disabled, then the domain needs to be always on for all practical purposes). Otherwise you can't just make all of them happy at the same time. The documentation doesn't cover this, because it had been written before we even started to consider power domains.

Quoted 2 lines

Drivers that behaves well within this context, follows the runtime PM documentation/recommendation.

Quoted 8 lines

So please go to Documentation/power/runtime_pm.txt and read it again. Quote:

"If the default initial runtime PM status of the device (i.e. 'suspended') reflects the actual state of the device, its bus type's or its driver's ->probe() callback will likely need to wake it up using one of the PM core's helper functions described in Section 4. In that case, pm_runtime_resume() should be used. Of course, for this purpose the device's runtime PM has to be enabled earlier by calling pm_runtime_enable()."

All that said there is a logical error related to calling pm_runtime_enable() and its derivatives from ->probe() that I've been overlooking pretty much from the start.

Namely, really_probe() sets dev->driver to drv before calling ->probe() for either the bus type or the driver itself, so if the driver's probe calls pm_runtime_enable(), it will execute the driver's own runtime PM resume callback before the driver can check whether or not it wants to handle the device in the first place. That doesn't sound quite right to me.

Quoted 6 lines

This means we need a different mechanism to ensure that the device will be accessible to the driver and/or the bus type in ->probe().

At one point we had pm_runtime_get_sync() before really_probe() in driver_proble_device() IIRC, but people complained about it, so we dropped it and put a barrier in there instead, but that's not sufficient.

So we actually had pm_runtime_get_noresume() before the barrier, but then we dropped it due to complaints.

Quoted 3 lines

We need to explicitly ensure that the device won't be powered off while ->probe() is in progress *but* we need to avoid calling the driver's runtime PM resume callback before ->probe() returns.

Quoted 3 lines

I wonder, then, if we just need to do pm_runtime_get_sync() instead of the barrier and then pm_runtime_put() instead of pm_request_idle() in driver_probe_device()?

Won't we still have the same problems in the case of ->probe failure that were fixed by removing those calls[1]?

IOW, after the driver's ->probe returns failure, it's no longer safe to call the driver's runtime PM callbacks, since they may be accessing resources that no longer exits.

Hmm, thinking a little more, maybe this kind of failure is a good time for the driver to use pm_runtime_force_suspend(), then later when the PM core does the _put(), it will see the device aleady suspended and there shouldn't be any problems.

All that said there is a logical error related to calling pm_runtime_enable() and its derivatives from ->probe() that I've been overlooking pretty much from the start.

Namely, really_probe() sets dev->driver to drv before calling ->probe() for either the bus type or the driver itself, so if the driver's probe calls pm_runtime_enable(), it will execute the driver's own runtime PM resume callback before the driver can check whether or not it wants to handle the device in the first place. That doesn't sound quite right to me.

This means we need a different mechanism to ensure that the device will be accessible to the driver and/or the bus type in ->probe().

At one point we had pm_runtime_get_sync() before really_probe() in driver_proble_device() IIRC, but people complained about it, so we dropped it and put a barrier in there instead, but that's not sufficient. We need to explicitly ensure that the device won't be powered off while ->probe() is in progress *but* we need to avoid calling the driver's runtime PM resume callback before ->probe() returns.

Alan, Kevin, ideas?

[CC: list drastically trimmed]

Quoted 13 lines · Rafael J. Wysocki

All that said there is a logical error related to calling pm_runtime_enable() and its derivatives from ->probe() that I've been overlooking pretty much from the start.

Namely, really_probe() sets dev->driver to drv before calling ->probe() for either the bus type or the driver itself, so if the driver's probe calls pm_runtime_enable(), it will execute the driver's own runtime PM resume callback before the driver can check whether or not it wants to handle the device in the first place. That doesn't sound quite right to me.

This means we need a different mechanism to ensure that the device will be accessible to the driver and/or the bus type in ->probe().

Quoted 8 lines

At one point we had pm_runtime_get_sync() before really_probe() in driver_proble_device() IIRC, but people complained about it, so we dropped it and put a barrier in there instead, but that's not sufficient. We need to explicitly ensure that the device won't be powered off while ->probe() is in progress *but* we need to avoid calling the driver's runtime PM resume callback before ->probe() returns.

Alan, Kevin, ideas?

I suspect this cannot be handled solely the driver core or the PM core. The bus subsystem has to manage things properly.

For example, take a look at drivers/pci/pci-driver.c:local_pci_probe(). That routine doesn't set pci_dev->driver until _after_ doing the pm_runtime_get_sync(). Therefore, if a runtime_resume callback does occur, the PCI core's runtime_resume handler will see that pci_dev->driver isn't set and won't invoke the driver's runtime_resume method.

Things become a little more complicated if the driver has to handle everything by itself and the subsystem doesn't support runtime PM. In that case, the driver's runtime_resume routine would have to check and see whether the device has successfully been bound to the driver (for example, by checking whether dev_get_drvdata() returns a non-NULL pointer) before doing anything else.

Note that a mirror version of this problem exists during unbinding, because dev->driver doesn't get cleared until _after_ the driver's remove routine has been called. A runtime suspend callback occurring in between could mistakenly get sent to the driver's runtime_suspend routine.

Quoted 29 lines · Alan Stern

[CC: list drastically trimmed]

Quoted 13 lines · Rafael J. Wysocki

All that said there is a logical error related to calling pm_runtime_enable() and its derivatives from ->probe() that I've been overlooking pretty much from the start.

Namely, really_probe() sets dev->driver to drv before calling ->probe() for either the bus type or the driver itself, so if the driver's probe calls pm_runtime_enable(), it will execute the driver's own runtime PM resume callback before the driver can check whether or not it wants to handle the device in the first place. That doesn't sound quite right to me.

This means we need a different mechanism to ensure that the device will be accessible to the driver and/or the bus type in ->probe().

Quoted 8 lines

At one point we had pm_runtime_get_sync() before really_probe() in driver_proble_device() IIRC, but people complained about it, so we dropped it and put a barrier in there instead, but that's not sufficient. We need to explicitly ensure that the device won't be powered off while ->probe() is in progress *but* we need to avoid calling the driver's runtime PM resume callback before ->probe() returns.

Alan, Kevin, ideas?

I suspect this cannot be handled solely the driver core or the PM core. The bus subsystem has to manage things properly.

Quoted 6 lines

For example, take a look at drivers/pci/pci-driver.c:local_pci_probe(). That routine doesn't set pci_dev->driver until _after_ doing the pm_runtime_get_sync(). Therefore, if a runtime_resume callback does occur, the PCI core's runtime_resume handler will see that pci_dev->driver isn't set and won't invoke the driver's runtime_resume method.

Which means that the PCI bus type wouldn't be affected by the change I'm talking about to my eyes.

Quoted 6 lines

Things become a little more complicated if the driver has to handle everything by itself and the subsystem doesn't support runtime PM. In that case, the driver's runtime_resume routine would have to check and see whether the device has successfully been bound to the driver (for example, by checking whether dev_get_drvdata() returns a non-NULL pointer) before doing anything else.

Well, I'd rather say that the driver has to initialize runtime PM properly in that case before trying to use it. :-)

Quoted 52 lines · Ulf Hansson

Quoted 27 lines · Kevin Hilman

Quoted 25 lines · Ulf Hansson

Changes in v3:
-Rework the entire intermediate step which was suggested in v2.
That means solving the race condition, but also cope with PM domains that are initialized in powered off state.

Changes in v2:
-Added some acks.
-Updated commit messages.
-Included a wider audience of the patchset. V1 was lacking SoC
maintainers.

Here are link to the first patchset, were some discussion started.
http://marc.info/?l=linux-pm&m=141208104729597&w=2

There may be more than one device in a PM domain which then will be probed at different points in time.

Depending on timing and runtime PM support, in for the device related driver/subsystem, a PM domain may be advised to power off after a successful probe sequence.

A general requirement for a device within a PM domain, is that the PM domain must stay powered during the probe sequence. To cope with such requirement, let's add two new APIs, dev_pm_domain_get|put().

Quoted 8 lines

Quoted 6 lines

These APIs are intended to be invoked from subsystem-level code and the calls between get/put needs to be balanced.

dev_pm_domain_get(), tells the PM domain that it needs to increase a usage count and to keep supplying power. dev_pm_domain_put(), does the opposite.

I'm confused. Why arent' pm_runtime_get*() and pm_runtime_put*() working?

See, below.

Quoted 3 lines

What's not explained here (or what I'm not understanding) is why a PM domain is powering off if it has active devices.

It doesn't. The problem is that using pm_runtime_get_sync() in this path is not working.

Now, I failed to include some of the important information from previous discussions around this patchset. Let me iterate the patchset with better commit messages, but let's first continue this thread.

Here are some of the previous discussion:

Quoted 7 lines

http://marc.info/?l=linux-pm&m=141270897014653&w=2
http://marc.info/?l=linux-pm&m=141208104729597&w=2

Below is a summary of why I think "pm_runtime_get_sync()" isn't working for us.

1)
It's bad practice to use pm_runtime_get_sync() in the ->probe() path,

Honestly, I'm no longer amused.

Quoted 3 lines

to bring your resources to full power. The consequence would be a driver that requires CONFIG_PM_RUNTIME to be even functional, which just isn't acceptable.

Sorry, but this is utter nonsense.

CONFIG_PM_RUNTIME unset means "no runtime PM at all", so all drivers can expect everything they need to be always on. If that is not the case, then someone is doing runtime PM behind the scenes and therefore cheating. Or in different words, for CONFIG_PM_RUNTIME unset bus types, platforms etc must ensure that everything is on from the drivers' perspective.

If that is the case, then calling pm_runtime_get_sync() from ->probe for CONFIG_PM_RUNTIME unset simply doesn't matter.

Now, for CONFIG_PM_RUNTIME enabled, if power domains are in use, doing pm_runtime_get_sync() from ->probe is the only way the driver can ensure in a non-racy way that the device will be accessible going forward.

Why? Simply because the probing need not happen during system initialization. It very well may take places when the other devices in the same domain have beein in use for quite a while and have been using runtime PM (in which case the domain may go off at any time unless it is explicityly prevented from doing that).

One thing that you may be missing is that, for CONFIG_PM_RUNTIME set, runtime PM has to be either enabled or disabled for all devices in one domain (and if it is disabled, then the domain needs to be always on for all practical purposes). Otherwise you can't just make all of them happy at the same time. The documentation doesn't cover this, because it had been written before we even started to consider power domains.

Quoted 2 lines

Drivers that behaves well within this context, follows the runtime PM documentation/recommendation.

So please go to Documentation/power/runtime_pm.txt and read it again. Quote:

"If the default initial runtime PM status of the device (i.e. 'suspended') reflects the actual state of the device, its bus type's or its driver's ->probe() callback will likely need to wake it up using one of the PM core's helper functions described in Section 4. In that case, pm_runtime_resume() should be used. Of course, for this purpose the device's runtime PM has to be enabled earlier by calling pm_runtime_enable()."

So how is this in agreement with what you're saying, I wonder?

Quoted 2 lines

They use pm_runtime_set_active() during ->probe() to reflect that their devices are fully powered and capable of handling I/O.

And how the heck can a driver (whose device belongs to a power domain) be sure that the device is "fully powered and capable of handling I/O" duing ->probe()?

Quoted 4 lines

You may also have a look at these discussions which also touches this topic, but within a context of another patchset.
https://lkml.org/lkml/2014/10/23/95

Which looks like reiterating the same incorrect arguments.

Quoted 8 lines

2)
Another good example why pm_runtime_get_sync() is a bad solution to our problem, is the amba bus. Before it invokes the driver's ->probe() callback it does the following.
- "enable bus clock"
- pm_runtime_get_noresume()
- pm_runtime_set_active()
- pm_runtime_enable()

Which may not work if power domains are involved.

Quoted 4 lines

For these scenarios a pm_runtime_get_sync() from any of amba driver's ->probe() callback wouldn't have any effect, since the device is already active. In other words, the resources needs to be "manually" enabled.

There seems to be a lot of confusion around this, so let me summarize:

- For CONFIG_PM_RUNTIME unsed drivers should be able to *safely* assume that they can access devices at any time. All of the runtime PM helpers don't matter then (up to the error codes returned).

- For CONFIG_PM_RUNTIME set, if power domains are in use, then runtime PM has to be either enabled or disabled for all devices in one domain (and if disabled, the expected behavior is like the above).

- For CONFIG_PM_RUNTIME set, if power domains are in use, drivers (or bus types etc) should not make any assumptions about devices being fully powered without ensuring that this is the case, usually by holding an active PM runtime reference to the device before accessing it.

Now, there may be places in the core/bus type code that aren't aligned with the above, so they need to be fixed this way or another.

HTH,
Rafael

Quoted 61 lines · Rafael J. Wysocki

Quoted 52 lines · Ulf Hansson

Quoted 27 lines · Kevin Hilman

Quoted 25 lines · Ulf Hansson

Changes in v3:
-Rework the entire intermediate step which was suggested in v2.
That means solving the race condition, but also cope with PM domains that are initialized in powered off state.

Changes in v2:
-Added some acks.
-Updated commit messages.
-Included a wider audience of the patchset. V1 was lacking SoC
maintainers.

Here are link to the first patchset, were some discussion started.
http://marc.info/?l=linux-pm&m=141208104729597&w=2

There may be more than one device in a PM domain which then will be probed at different points in time.

Depending on timing and runtime PM support, in for the device related driver/subsystem, a PM domain may be advised to power off after a successful probe sequence.

A general requirement for a device within a PM domain, is that the PM domain must stay powered during the probe sequence. To cope with such requirement, let's add two new APIs, dev_pm_domain_get|put().

Quoted 8 lines

Quoted 6 lines

These APIs are intended to be invoked from subsystem-level code and the calls between get/put needs to be balanced.

dev_pm_domain_get(), tells the PM domain that it needs to increase a usage count and to keep supplying power. dev_pm_domain_put(), does the opposite.

I'm confused. Why arent' pm_runtime_get*() and pm_runtime_put*() working?

See, below.

Quoted 3 lines

What's not explained here (or what I'm not understanding) is why a PM domain is powering off if it has active devices.

It doesn't. The problem is that using pm_runtime_get_sync() in this path is not working.

Now, I failed to include some of the important information from previous discussions around this patchset. Let me iterate the patchset with better commit messages, but let's first continue this thread.

Here are some of the previous discussion:

Quoted 7 lines

http://marc.info/?l=linux-pm&m=141270897014653&w=2
http://marc.info/?l=linux-pm&m=141208104729597&w=2

Below is a summary of why I think "pm_runtime_get_sync()" isn't working for us.

1)
It's bad practice to use pm_runtime_get_sync() in the ->probe() path,

Quoted 7 lines

Honestly, I'm no longer amused.

Quoted 3 lines

to bring your resources to full power. The consequence would be a driver that requires CONFIG_PM_RUNTIME to be even functional, which just isn't acceptable.

Sorry, but this is utter nonsense.

I admit, I was too vague while stating this. Looking at the big picture you are obviously right.

I should have referred to those SOCs/buses/drivers that I am working/looking at.

Quoted 6 lines

CONFIG_PM_RUNTIME unset means "no runtime PM at all", so all drivers can expect everything they need to be always on. If that is not the case, then someone is doing runtime PM behind the scenes and therefore cheating. Or in different words, for CONFIG_PM_RUNTIME unset bus types, platforms etc must ensure that everything is on from the drivers' perspective.

I don't think I have stated anything that isn't in agreement with the above?

While I am struggling in making my points clearer, it seems like we look a bit differently upon how runtime PM are being deployed.

For those drivers I am working on, it's common that these handles runtime PM resources, like for example clocks. The clocks needs to be enabled for the driver to handle I/O, but those may also be gated at request inactivity to save power. That means, the clocks may be considered as both functional clocks and runtime PM resources.

Therefore, the driver must enable its clocks during ->probe() and without relying on CONFIG_PM_RUNTIME to be set. Similar to what you stated for the buses above.

To also cope with the scenario where CONFIG_PM_RUNTIME is set, drivers must update the device's runtime PM status using pm_runtime_set_active(), to synchronize the state with the runtime PM core. Otherwise we will get clock unbalance issues while gating/ungating the clocks from the runtime PM callbacks.

The AMBA bus and some of its drivers a good example of how this has been implemented:
driver/amba/bus.c
drivers/mmc/host/mmci.c
drivers/spi/spi-pl022.c

This conclusion I have made from this is:
- Using pm_runtime_get_sync() during the ->probe() path to explicitly power up a PM domain, is not suitable as the _common_ solution to solve the race condition. It certainly may work for some scenarios, but not for those I am looking at.

Quoted 8 lines

If that is the case, then calling pm_runtime_get_sync() from ->probe for CONFIG_PM_RUNTIME unset simply doesn't matter.

Now, for CONFIG_PM_RUNTIME enabled, if power domains are in use, doing pm_runtime_get_sync() from ->probe is the only way the driver can ensure in a non-racy way that the device will be accessible going forward.

Quoted 5 lines

Why? Simply because the probing need not happen during system initialization. It very well may take places when the other devices in the same domain have beein in use for quite a while and have been using runtime PM (in which case the domain may go off at any time unless it is explicityly prevented from doing that).

For PM domains that are initialized in powered off state, we can't rely on CONFIG_PM_RUNTIME and thus not on pm_runtime_get_sync() to power on these PM domains. We need a different mechanism, which is suggested in this v3 patchset.

The requirement of being able to initialize PM domains in powered off state, was raised during review of v2 of this patchset. I do realize that's not easy for you to keep track and remember of all discussions. I apologize for not providing this as the topmost important argument to why pm_runtime_get_sync() can't be used, in my reply to Kevin.

That is quite simple to address, though. You can register a bus type notifier that will power up the domain on BUS_NOTIFY_ADD_DEVICE events (where the target device belongs to the domain), and do that only for CONFIG_PM_RUNTIME unset (otherwise runtime PM should take care of this).

Quoted 4 lines · Rafael J. Wysocki

That is quite simple to address, though. You can register a bus type notifier that will power up the domain on BUS_NOTIFY_ADD_DEVICE events (where the target device belongs to the domain), and do that only for CONFIG_PM_RUNTIME unset (otherwise runtime PM should take care of this).

Registering a bus type notifier (that will be a platform bus notifier, right?) will affect
all platform devices, not only the on-SoC devices.

Generally, there are two or even three levels of runtime PM handling, driver, (possibly) bus type and (possibly) PM domain (and multiple levels of these are possible in principle). All of them have to be initialized at different times.

Quite arguably, the PM domain and/or bus type runtime PM handling should be initialized even before registerind the device or during device registration. Doing that later may be too late. When the device has been registered, runtime PM should work to an extent allowing the driver to access the device and configure it further after calling pm_runtime_resume().

Of course, if ->probe() is to call pm_runtime_resume() for this purpose, it must take the fact that the driver's own ->runtime_resume() may be called as a result of this into account. That's why I'm asking whether or not the core should call pm_runtime_resume() before calling really_probe() in a followup branch of this thread.

The driver's own runtime PM handling must be initialized in the driver and the only place suitable for that is ->probe(). However, it needs to be done *before* the driver's own ->runtime_resume() or ->runtime_suspend() callback is executed. If that is done properly, it should be possible to cover both the CONFIG_PM_RUNTIME set/unset cases in that code.

And I wouldn't recommend anyone to do the runtime PM initialization in ->runtime_resume() (when it is called for the first time), as that would be error prone and fragile.

Quoted 7 lines

The AMBA bus and some of its drivers a good example of how this has been implemented:
driver/amba/bus.c
drivers/mmc/host/mmci.c
drivers/spi/spi-pl022.c

Quoted 5 lines

This conclusion I have made from this is:
- Using pm_runtime_get_sync() during the ->probe() path to explicitly power up a PM domain, is not suitable as the _common_ solution to solve the race condition. It certainly may work for some scenarios, but not for those I am looking at.

I think, however, that it might work if the core calls pm_runtime_get_sync() from driver_probe_device().

Quoted 4 lines

For PM domains that are initialized in powered off state, we can't rely on CONFIG_PM_RUNTIME and thus not on pm_runtime_get_sync() to power on these PM domains. We need a different mechanism, which is suggested in this v3 patchset.

That is quite simple to address, though. You can register a bus type notifier that will power up the domain on BUS_NOTIFY_ADD_DEVICE events (where the target device belongs to the domain), and do that only for CONFIG_PM_RUNTIME unset (otherwise runtime PM should take care of this).

Quoted 6 lines

The requirement of being able to initialize PM domains in powered off state, was raised during review of v2 of this patchset. I do realize that's not easy for you to keep track and remember of all discussions. I apologize for not providing this as the topmost important argument to why pm_runtime_get_sync() can't be used, in my reply to Kevin.

OK

It looks like we need an agreement on what to do and what the expectations should be for each driver core operation at the high level. That is, what ->probe() should expect, what ->remove() should expect, what the state after executing each of them should be for CONFIG_PM_RUNTIME set and unset etc. Otherwise we'll always have various bus types doing what *they* think is appropriate and not agreeing with each other.

--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[...]

Quoted 11 lines

Generally, there are two or even three levels of runtime PM handling, driver, (possibly) bus type and (possibly) PM domain (and multiple levels of these are possible in principle). All of them have to be initialized at different times.

Quite arguably, the PM domain and/or bus type runtime PM handling should be initialized even before registerind the device or during device registration. Doing that later may be too late. When the device has been registered, runtime PM should work to an extent allowing the driver to access the device and configure it further after calling pm_runtime_resume().

Quoted 5 lines

Of course, if ->probe() is to call pm_runtime_resume() for this purpose, it must take the fact that the driver's own ->runtime_resume() may be called as a result of this into account. That's why I'm asking whether or not the core should call pm_runtime_resume() before calling really_probe() in a followup branch of this thread.

I am reading the other thread, let's see.

Quoted 10 lines

The driver's own runtime PM handling must be initialized in the driver and the only place suitable for that is ->probe(). However, it needs to be done *before* the driver's own ->runtime_resume() or ->runtime_suspend() callback is executed. If that is done properly, it should be possible to cover both the CONFIG_PM_RUNTIME set/unset cases in that code.

And I wouldn't recommend anyone to do the runtime PM initialization in ->runtime_resume() (when it is called for the first time), as that would be error prone and fragile.

Great! That's means we are at least aligned on this topic. :-)

Quoted 7 lines

Quoted 7 lines

The AMBA bus and some of its drivers a good example of how this has been implemented:
driver/amba/bus.c
drivers/mmc/host/mmci.c
drivers/spi/spi-pl022.c

Quoted 8 lines

Quoted 5 lines

This conclusion I have made from this is:
- Using pm_runtime_get_sync() during the ->probe() path to explicitly power up a PM domain, is not suitable as the _common_ solution to solve the race condition. It certainly may work for some scenarios, but not for those I am looking at.

I think, however, that it might work if the core calls pm_runtime_get_sync() from driver_probe_device().

Currently this won't work.

That's because the buses' ->probe() are invoked in this path and they are doing the attachment of the device to its PM domain.

In other words, we can't power up the PM domain using pm_runtime_get_sync(), until the device has been attached to its PM domain. Right?

Quoted 50 lines · Ulf Hansson

[...]

Quoted 11 lines

Generally, there are two or even three levels of runtime PM handling, driver, (possibly) bus type and (possibly) PM domain (and multiple levels of these are possible in principle). All of them have to be initialized at different times.

Quite arguably, the PM domain and/or bus type runtime PM handling should be initialized even before registerind the device or during device registration. Doing that later may be too late. When the device has been registered, runtime PM should work to an extent allowing the driver to access the device and configure it further after calling pm_runtime_resume().

Quoted 5 lines

Of course, if ->probe() is to call pm_runtime_resume() for this purpose, it must take the fact that the driver's own ->runtime_resume() may be called as a result of this into account. That's why I'm asking whether or not the core should call pm_runtime_resume() before calling really_probe() in a followup branch of this thread.

I am reading the other thread, let's see.

Quoted 10 lines

The driver's own runtime PM handling must be initialized in the driver and the only place suitable for that is ->probe(). However, it needs to be done *before* the driver's own ->runtime_resume() or ->runtime_suspend() callback is executed. If that is done properly, it should be possible to cover both the CONFIG_PM_RUNTIME set/unset cases in that code.

And I wouldn't recommend anyone to do the runtime PM initialization in ->runtime_resume() (when it is called for the first time), as that would be error prone and fragile.

Great! That's means we are at least aligned on this topic. :-)

Quoted 7 lines

Quoted 7 lines

The AMBA bus and some of its drivers a good example of how this has been implemented:
driver/amba/bus.c
drivers/mmc/host/mmci.c
drivers/spi/spi-pl022.c

Quoted 8 lines

Quoted 5 lines

This conclusion I have made from this is:
- Using pm_runtime_get_sync() during the ->probe() path to explicitly power up a PM domain, is not suitable as the _common_ solution to solve the race condition. It certainly may work for some scenarios, but not for those I am looking at.

I think, however, that it might work if the core calls pm_runtime_get_sync() from driver_probe_device().

Quoted 8 lines

Currently this won't work.

That's because the buses' ->probe() are invoked in this path and they are doing the attachment of the device to its PM domain.

In other words, we can't power up the PM domain using pm_runtime_get_sync(), until the device has been attached to its PM domain. Right?

I think this is one of the issues that we have there. Why do we conflate probing and placing the device into a power domain? The latter should happen when we register the device. The fact that a device was probed and has a driver bound or not bound to it should have no bearing on whether the device is member of power domain or not.

Quoted 61 lines · Dmitry Torokhov

Quoted 50 lines · Ulf Hansson

[...]

Quoted 11 lines

Generally, there are two or even three levels of runtime PM handling, driver, (possibly) bus type and (possibly) PM domain (and multiple levels of these are possible in principle). All of them have to be initialized at different times.

Quite arguably, the PM domain and/or bus type runtime PM handling should be initialized even before registerind the device or during device registration. Doing that later may be too late. When the device has been registered, runtime PM should work to an extent allowing the driver to access the device and configure it further after calling pm_runtime_resume().

Quoted 5 lines

Of course, if ->probe() is to call pm_runtime_resume() for this purpose, it must take the fact that the driver's own ->runtime_resume() may be called as a result of this into account. That's why I'm asking whether or not the core should call pm_runtime_resume() before calling really_probe() in a followup branch of this thread.

I am reading the other thread, let's see.

Quoted 10 lines

The driver's own runtime PM handling must be initialized in the driver and the only place suitable for that is ->probe(). However, it needs to be done *before* the driver's own ->runtime_resume() or ->runtime_suspend() callback is executed. If that is done properly, it should be possible to cover both the CONFIG_PM_RUNTIME set/unset cases in that code.

And I wouldn't recommend anyone to do the runtime PM initialization in ->runtime_resume() (when it is called for the first time), as that would be error prone and fragile.

Great! That's means we are at least aligned on this topic. :-)

Quoted 7 lines

Quoted 7 lines

The AMBA bus and some of its drivers a good example of how this has been implemented:
driver/amba/bus.c
drivers/mmc/host/mmci.c
drivers/spi/spi-pl022.c

Quoted 8 lines

Quoted 5 lines

This conclusion I have made from this is:
- Using pm_runtime_get_sync() during the ->probe() path to explicitly power up a PM domain, is not suitable as the _common_ solution to solve the race condition. It certainly may work for some scenarios, but not for those I am looking at.

I think, however, that it might work if the core calls pm_runtime_get_sync() from driver_probe_device().

Quoted 8 lines

Currently this won't work.

That's because the buses' ->probe() are invoked in this path and they are doing the attachment of the device to its PM domain.

In other words, we can't power up the PM domain using pm_runtime_get_sync(), until the device has been attached to its PM domain. Right?

Quoted 5 lines

I think this is one of the issues that we have there. Why do we conflate probing and placing the device into a power domain? The latter should happen when we register the device. The fact that a device was probed and has a driver bound or not bound to it should have no bearing on whether the device is member of power domain or not.

Devices that are created while "discoverable buses" are being probed can't be attached to a PM domain before the probing is done, because those simply doesn't exist.

Now, I haven't yet seen a demand for such a cases, but it seems wrong to not consider them. The current solution cover these.

Quoted 4 lines

Devices that are created while "discoverable buses" are being probed can't be attached to a PM domain before the probing is done, because those simply doesn't exist.

Honestly, I'm not sure what you're talking about.

Devices on a "discoverable* bus (say PCI) are added when the *controller* is probed, not when *they* are probed.

You very much need to have a struct device registered to be able to call really_probe() for it.

Quoted 3 lines

Now, I haven't yet seen a demand for such a cases, but it seems wrong to not consider them. The current solution cover these.

Oh dear. Please rethink this.

Quoted 9 lines

Quoted 4 lines

Devices that are created while "discoverable buses" are being probed can't be attached to a PM domain before the probing is done, because those simply doesn't exist.

Honestly, I'm not sure what you're talking about.

Devices on a "discoverable* bus (say PCI) are added when the *controller* is probed, not when *they* are probed.

Okay, so maybe "discoverable buses" isn't the proper term.

Quoted 3 lines

You very much need to have a struct device registered to be able to call really_probe() for it.

Yes. But my point is that the struct device may be created dynamically at some point in time.

This is how mmc/sd/sdio cards are handled. We don't have the information about the card and thus not the struct device of it, until we have detected it. Maybe we could at that point try to add the device to its PM domain?

Quoted 6 lines

Quoted 4 lines

Now, I haven't yet seen a demand for such a cases, but it seems wrong to not consider them. The current solution cover these.

Oh dear. Please rethink this.

Currently, dev_pm_domain_attach() is being invoked at the point when a SDIO card has been found. From sdio_add_func(). How would that be solved?

Quoted 10 lines · Ulf Hansson

Quoted 9 lines

Quoted 4 lines

Devices that are created while "discoverable buses" are being probed can't be attached to a PM domain before the probing is done, because those simply doesn't exist.

Honestly, I'm not sure what you're talking about.

Devices on a "discoverable* bus (say PCI) are added when the *controller* is probed, not when *they* are probed.

Quoted 8 lines

Okay, so maybe "discoverable buses" isn't the proper term.

Quoted 3 lines

You very much need to have a struct device registered to be able to call really_probe() for it.

Yes. But my point is that the struct device may be created dynamically at some point in time.

And that is fine.

Quoted 5 lines

This is how mmc/sd/sdio cards are handled. We don't have the information about the card and thus not the struct device of it, until we have detected it. Maybe we could at that point try to add the device to its PM domain?

Well, I think we need to first define what PM domain they will fall into. Do they have to be in one? The concept of power domain, as far as I understand it, is needed when we need to form relations going outside the standard parent/child relationship. Here we have a controller and then one or more cards in it. To be able to use card you need to power up parent and you can not power down parent until all children are powered down.

But in any case, device discovery and binding them to drivers are 2 separate steps. You have a PCI bus. You enumerate it - new PCI devices are created. Some of them may be put in a certain power domain. You then bind drivers to PCI devices (not strictly 'then' but we could implement driver core to postpone binding until current round of enumeration is complete) - and you discover USB controller and maybe i2c controller. Then you bind drivers to them which causes enumeration of the new bus and new devices are created. But in all these cases enumeration and creation of new 'struct device's, and driver binding are logically separate steps.

Quoted 11 lines

Quoted 6 lines

Quoted 4 lines

Now, I haven't yet seen a demand for such a cases, but it seems wrong to not consider them. The current solution cover these.

Oh dear. Please rethink this.

Currently, dev_pm_domain_attach() is being invoked at the point when a SDIO card has been found. From sdio_add_func(). How would that be solved?

It is probably the one place where we currently doing it correctly (form logical point of view, not implementation wise - implementation wise in sdio we add device to power domain after calling device_add() which means that probe() could have been called before we added the new device to any power domain).

Quoted 59 lines · Dmitry Torokhov

Quoted 10 lines · Ulf Hansson

Quoted 9 lines

Quoted 4 lines

Devices that are created while "discoverable buses" are being probed can't be attached to a PM domain before the probing is done, because those simply doesn't exist.

Honestly, I'm not sure what you're talking about.

Devices on a "discoverable* bus (say PCI) are added when the *controller* is probed, not when *they* are probed.

Quoted 8 lines

Okay, so maybe "discoverable buses" isn't the proper term.

Quoted 3 lines

You very much need to have a struct device registered to be able to call really_probe() for it.

Yes. But my point is that the struct device may be created dynamically at some point in time.

And that is fine.

Quoted 5 lines

This is how mmc/sd/sdio cards are handled. We don't have the information about the card and thus not the struct device of it, until we have detected it. Maybe we could at that point try to add the device to its PM domain?

Well, I think we need to first define what PM domain they will fall into. Do they have to be in one? The concept of power domain, as far as I understand it, is needed when we need to form relations going outside the standard parent/child relationship. Here we have a controller and then one or more cards in it. To be able to use card you need to power up parent and you can not power down parent until all children are powered down.

But in any case, device discovery and binding them to drivers are 2 separate steps. You have a PCI bus. You enumerate it - new PCI devices are created. Some of them may be put in a certain power domain. You then bind drivers to PCI devices (not strictly 'then' but we could implement driver core to postpone binding until current round of enumeration is complete) - and you discover USB controller and maybe i2c controller. Then you bind drivers to them which causes enumeration of the new bus and new devices are created. But in all these cases enumeration and creation of new 'struct device's, and driver binding are logically separate steps.

Quoted 11 lines

Quoted 6 lines

Quoted 4 lines

Now, I haven't yet seen a demand for such a cases, but it seems wrong to not consider them. The current solution cover these.

Oh dear. Please rethink this.

Currently, dev_pm_domain_attach() is being invoked at the point when a SDIO card has been found. From sdio_add_func(). How would that be solved?

Quoted 5 lines

It is probably the one place where we currently doing it correctly (form logical point of view, not implementation wise - implementation wise in sdio we add device to power domain after calling device_add() which means that probe() could have been called before we added the new device to any power domain).

I agree. Ideally, devices should be added to power (or generally PM) domains before registration. That is, before calling device_add() for them.

That's the first thing to fix here.

Quoted 50 lines · Ulf Hansson

[...]

Quoted 11 lines

Generally, there are two or even three levels of runtime PM handling, driver, (possibly) bus type and (possibly) PM domain (and multiple levels of these are possible in principle). All of them have to be initialized at different times.

Quite arguably, the PM domain and/or bus type runtime PM handling should be initialized even before registerind the device or during device registration. Doing that later may be too late. When the device has been registered, runtime PM should work to an extent allowing the driver to access the device and configure it further after calling pm_runtime_resume().

Quoted 5 lines

Of course, if ->probe() is to call pm_runtime_resume() for this purpose, it must take the fact that the driver's own ->runtime_resume() may be called as a result of this into account. That's why I'm asking whether or not the core should call pm_runtime_resume() before calling really_probe() in a followup branch of this thread.

I am reading the other thread, let's see.

Quoted 10 lines

The driver's own runtime PM handling must be initialized in the driver and the only place suitable for that is ->probe(). However, it needs to be done *before* the driver's own ->runtime_resume() or ->runtime_suspend() callback is executed. If that is done properly, it should be possible to cover both the CONFIG_PM_RUNTIME set/unset cases in that code.

And I wouldn't recommend anyone to do the runtime PM initialization in ->runtime_resume() (when it is called for the first time), as that would be error prone and fragile.

Great! That's means we are at least aligned on this topic. :-)

Quoted 7 lines

Quoted 7 lines

The AMBA bus and some of its drivers a good example of how this has been implemented:
driver/amba/bus.c
drivers/mmc/host/mmci.c
drivers/spi/spi-pl022.c

Quoted 8 lines

Quoted 5 lines

This conclusion I have made from this is:
- Using pm_runtime_get_sync() during the ->probe() path to explicitly power up a PM domain, is not suitable as the _common_ solution to solve the race condition. It certainly may work for some scenarios, but not for those I am looking at.

I think, however, that it might work if the core calls pm_runtime_get_sync() from driver_probe_device().

Quoted 8 lines

Currently this won't work.

That's because the buses' ->probe() are invoked in this path and they are doing the attachment of the device to its PM domain.

In other words, we can't power up the PM domain using pm_runtime_get_sync(), until the device has been attached to its PM domain. Right?

I think this is one of the issues that we have there. Why do we conflate probing and placing the device into a power domain? The latter should happen when we register the device. The fact that a device was probed and has a driver bound or not bound to it should have no bearing on whether the device is member of power domain or not.

[...]

Quoted 11 lines

Generally, there are two or even three levels of runtime PM handling, driver, (possibly) bus type and (possibly) PM domain (and multiple levels of these are possible in principle). All of them have to be initialized at different times.

Quite arguably, the PM domain and/or bus type runtime PM handling should be initialized even before registerind the device or during device registration. Doing that later may be too late. When the device has been registered, runtime PM should work to an extent allowing the driver to access the device and configure it further after calling pm_runtime_resume().

Quoted 5 lines

Of course, if ->probe() is to call pm_runtime_resume() for this purpose, it must take the fact that the driver's own ->runtime_resume() may be called as a result of this into account. That's why I'm asking whether or not the core should call pm_runtime_resume() before calling really_probe() in a followup branch of this thread.

I am reading the other thread, let's see.

Quoted 10 lines

The driver's own runtime PM handling must be initialized in the driver and the only place suitable for that is ->probe(). However, it needs to be done *before* the driver's own ->runtime_resume() or ->runtime_suspend() callback is executed. If that is done properly, it should be possible to cover both the CONFIG_PM_RUNTIME set/unset cases in that code.

And I wouldn't recommend anyone to do the runtime PM initialization in ->runtime_resume() (when it is called for the first time), as that would be error prone and fragile.

Great! That's means we are at least aligned on this topic. :-)

Quoted 7 lines

Quoted 7 lines

The AMBA bus and some of its drivers a good example of how this has been implemented:
driver/amba/bus.c
drivers/mmc/host/mmci.c
drivers/spi/spi-pl022.c

Quoted 8 lines

Quoted 5 lines

This conclusion I have made from this is:
- Using pm_runtime_get_sync() during the ->probe() path to explicitly power up a PM domain, is not suitable as the _common_ solution to solve the race condition. It certainly may work for some scenarios, but not for those I am looking at.

I think, however, that it might work if the core calls pm_runtime_get_sync() from driver_probe_device().

Currently this won't work.

That's because the buses' ->probe() are invoked in this path and they are doing the attachment of the device to its PM domain.

In other words, we can't power up the PM domain using pm_runtime_get_sync(), until the device has been attached to its PM domain. Right?

[...]

Quoted 5 lines

Quoted 4 lines

For PM domains that are initialized in powered off state, we can't rely on CONFIG_PM_RUNTIME and thus not on pm_runtime_get_sync() to power on these PM domains. We need a different mechanism, which is suggested in this v3 patchset.

Quoted 4 lines

That is quite simple to address, though. You can register a bus type notifier that will power up the domain on BUS_NOTIFY_ADD_DEVICE events (where the target device belongs to the domain), and do that only for CONFIG_PM_RUNTIME unset (otherwise runtime PM should take care of this).

I guess we could use notifiers, but I am not sure I see any benefit. The code will be more complex and we need error handling as well.

Quoted 55 lines · Ulf Hansson

[...]

Quoted 11 lines

Generally, there are two or even three levels of runtime PM handling, driver, (possibly) bus type and (possibly) PM domain (and multiple levels of these are possible in principle). All of them have to be initialized at different times.

Quite arguably, the PM domain and/or bus type runtime PM handling should be initialized even before registerind the device or during device registration. Doing that later may be too late. When the device has been registered, runtime PM should work to an extent allowing the driver to access the device and configure it further after calling pm_runtime_resume().

Quoted 5 lines

Of course, if ->probe() is to call pm_runtime_resume() for this purpose, it must take the fact that the driver's own ->runtime_resume() may be called as a result of this into account. That's why I'm asking whether or not the core should call pm_runtime_resume() before calling really_probe() in a followup branch of this thread.

I am reading the other thread, let's see.

Quoted 10 lines

The driver's own runtime PM handling must be initialized in the driver and the only place suitable for that is ->probe(). However, it needs to be done *before* the driver's own ->runtime_resume() or ->runtime_suspend() callback is executed. If that is done properly, it should be possible to cover both the CONFIG_PM_RUNTIME set/unset cases in that code.

And I wouldn't recommend anyone to do the runtime PM initialization in ->runtime_resume() (when it is called for the first time), as that would be error prone and fragile.

Great! That's means we are at least aligned on this topic. :-)

Quoted 7 lines

Quoted 7 lines

The AMBA bus and some of its drivers a good example of how this has been implemented:
driver/amba/bus.c
drivers/mmc/host/mmci.c
drivers/spi/spi-pl022.c

Quoted 8 lines

Quoted 5 lines

This conclusion I have made from this is:
- Using pm_runtime_get_sync() during the ->probe() path to explicitly power up a PM domain, is not suitable as the _common_ solution to solve the race condition. It certainly may work for some scenarios, but not for those I am looking at.

I think, however, that it might work if the core calls pm_runtime_get_sync() from driver_probe_device().

Currently this won't work.

That's because the buses' ->probe() are invoked in this path and they are doing the attachment of the device to its PM domain.

Quoted 3 lines

In other words, we can't power up the PM domain using pm_runtime_get_sync(), until the device has been attached to its PM domain. Right?

Yes, but my point was that those bus types might need to be changed.

We can't make everyone happy at the same time if their ideas about what to do are different.

Quoted 9 lines

[...]

Quoted 5 lines

Quoted 4 lines

For PM domains that are initialized in powered off state, we can't rely on CONFIG_PM_RUNTIME and thus not on pm_runtime_get_sync() to power on these PM domains. We need a different mechanism, which is suggested in this v3 patchset.

Quoted 7 lines

Quoted 4 lines

That is quite simple to address, though. You can register a bus type notifier that will power up the domain on BUS_NOTIFY_ADD_DEVICE events (where the target device belongs to the domain), and do that only for CONFIG_PM_RUNTIME unset (otherwise runtime PM should take care of this).

I guess we could use notifiers, but I am not sure I see any benefit. The code will be more complex and we need error handling as well.

Like "oh, I can't power up this thing, so I should fail ->probe()"?

Then your driver would need to depend on the specific knowledge about the given PM domain, I'm afraid.

If you want error handling like that, it needs to be handled by the core, so as to avoid calling the bus type's ->probe() as well in that case.

So to summarize:

- Devices need to be added to power domains before really_probe() is called for them. Otherwise we'll have ordering problems all over.

- Runtime PM (if compiled in) needs to be enabled for all devices in power domains by default. Otherwise devices may lose power as a result for power management of the other devices in the same domain.

- The core should try to power up domains before calling really_probe() both for CONFIG_PM_RUNTIME set and unset, so ->probe() can always make the "device is accessible" assumption.

- Bus types may need to do more on top of that in their ->probe(), so the driver's ->probe() can make that assumption too in all cases.

Does that make sense to you?

Quoted 42 lines · Rafael J. Wysocki

Quoted 41 lines · Ulf Hansson

[...]

Quoted 11 lines

Generally, there are two or even three levels of runtime PM handling, driver, (possibly) bus type and (possibly) PM domain (and multiple levels of these are possible in principle). All of them have to be initialized at different times.

Quite arguably, the PM domain and/or bus type runtime PM handling should be initialized even before registerind the device or during device registration. Doing that later may be too late. When the device has been registered, runtime PM should work to an extent allowing the driver to access the device and configure it further after calling pm_runtime_resume().

Quoted 5 lines

Of course, if ->probe() is to call pm_runtime_resume() for this purpose, it must take the fact that the driver's own ->runtime_resume() may be called as a result of this into account. That's why I'm asking whether or not the core should call pm_runtime_resume() before calling really_probe() in a followup branch of this thread.

I am reading the other thread, let's see.

Quoted 10 lines

The driver's own runtime PM handling must be initialized in the driver and the only place suitable for that is ->probe(). However, it needs to be done *before* the driver's own ->runtime_resume() or ->runtime_suspend() callback is executed. If that is done properly, it should be possible to cover both the CONFIG_PM_RUNTIME set/unset cases in that code.

And I wouldn't recommend anyone to do the runtime PM initialization in ->runtime_resume() (when it is called for the first time), as that would be error prone and fragile.

Great! That's means we are at least aligned on this topic. :-)

Quoted 7 lines

Quoted 7 lines

The AMBA bus and some of its drivers a good example of how this has been implemented:
driver/amba/bus.c
drivers/mmc/host/mmci.c
drivers/spi/spi-pl022.c

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

This conclusion I have made from this is:
- Using pm_runtime_get_sync() during the ->probe() path to explicitly power up a PM domain, is not suitable as the _common_ solution to solve the race condition. It certainly may work for some scenarios, but not for those I am looking at.

Quoted 9 lines

Quoted 9 lines

Quoted 3 lines

I think, however, that it might work if the core calls pm_runtime_get_sync() from driver_probe_device().

Currently this won't work.

That's because the buses' ->probe() are invoked in this path and they are doing the attachment of the device to its PM domain.

Quoted 8 lines

Quoted 3 lines

In other words, we can't power up the PM domain using pm_runtime_get_sync(), until the device has been attached to its PM domain. Right?

Yes, but my point was that those bus types might need to be changed.

We can't make everyone happy at the same time if their ideas about what to do are different.

Urgh. I fail to understand this comment.

Why do we prefer the pm_runtime_get_sync() solution in favour of this pathset's approach?

What are the benefit do we get with pm_runtime_get_sync()?

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Of course, if ->probe() is to call pm_runtime_resume() for this purpose, it must take the fact that the driver's own ->runtime_resume() may be called as a result of this into account.

Quoted 22 lines

Quoted 9 lines

[...]

Quoted 5 lines

Quoted 4 lines

For PM domains that are initialized in powered off state, we can't rely on CONFIG_PM_RUNTIME and thus not on pm_runtime_get_sync() to power on these PM domains. We need a different mechanism, which is suggested in this v3 patchset.

Quoted 7 lines

Quoted 4 lines

That is quite simple to address, though. You can register a bus type notifier that will power up the domain on BUS_NOTIFY_ADD_DEVICE events (where the target device belongs to the domain), and do that only for CONFIG_PM_RUNTIME unset (otherwise runtime PM should take care of this).

I guess we could use notifiers, but I am not sure I see any benefit. The code will be more complex and we need error handling as well.

Like "oh, I can't power up this thing, so I should fail ->probe()"?

Then your driver would need to depend on the specific knowledge about the given PM domain, I'm afraid.

Quoted 2 lines

If you want error handling like that, it needs to be handled by the core, so as to avoid calling the bus type's ->probe() as well in that case.

Yes, I want this error handling - but I fail to understand why the bus can't handle the errors.

If works perfectly in this patchset's approach.

Quoted 5 lines

So to summarize:

- Devices need to be added to power domains before really_probe() is called for them. Otherwise we'll have ordering problems all over.

What ordering problems?

Quoted 4 lines

- Runtime PM (if compiled in) needs to be enabled for all devices in power domains by default. Otherwise devices may lose power as a result for power management of the other devices in the same domain.

Quoted 4 lines

- The core should try to power up domains before calling really_probe() both for CONFIG_PM_RUNTIME set and unset, so ->probe() can always make the "device is accessible" assumption.

And how exactly will you then power up the PM domain when CONFIG_PM_RUNTIME is unset?

Quoted 5 lines

- Bus types may need to do more on top of that in their ->probe(), so the driver's ->probe() can make that assumption too in all cases.

Does that make sense to you?

2) There are no requirements for arch/platforms/drivers to work in both cases CONFIG_PM_RUNTIME=y/n. But they must be built without errors/warning for both cases.

For example, for Keystone 2 only CONFIG_PM_RUNTIME=y is going to be supported and if some one decided to disable it - it can be perfectly done from sys_fs/ user space.

3) pm_runtime_get_sync()(or similar) is good not only because i's waking up device, but also because:
- it can wake up chain of devices (dev->parent->parent->...)

- it can wake up power domain

- it connects device to domain/class/type/bus and so allow to add additional PM layer on top of Platform bus (for example arch/arm/mach-omap2/omap_device.c).

So, it will do all needed things, and if it doesn't that problem is in platfrom/bus/driver code and not in Runtime PM.
if pm_runtime_get_sync() will be dropped - than all above will need to be implemented around the ->probe().

4) I've copied here comment from Rafael:

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Of course, if ->probe() is to call pm_runtime_resume() for this purpose, it must take the fact that the driver's own ->runtime_resume() may be called as a result of this into account.

Agree, that's a little bit annoying, but we are living with that for more then 5 years already (I'm 3 years) - so, I am, as driver developer, expecting above behavior (just walk through the drivers and you will see how many drivers expecting the same).

So, any volunteers to check and fix ~500 drivers.

[...]

Quoted 3 lines

2) There are no requirements for arch/platforms/drivers to work in both cases CONFIG_PM_RUNTIME=y/n. But they must be built without errors/warning for both cases.

For cross SoC drivers this statement is not correct! Driver _must_ support the various combinations of CONFIG_PM*.

Therefore, I think it's better to strive towards a common solution and to get the building blocks in place.

Quoted 5 lines

For example, for Keystone 2 only CONFIG_PM_RUNTIME=y is going to be supported and if some one decided to disable it - it can be perfectly done from sys_fs/ user space.

I can't see why the sysfs option to disable runtime PM should affect this discussion. That's done at runtime and after the device has been probed.

Quoted 4 lines

3) pm_runtime_get_sync()(or similar) is good not only because i's waking up device, but also because:
- it can wake up chain of devices (dev->parent->parent->...)

That's right. But that's not what this patchset aims to do.

I realize that the header of the cover letter isn't describing the problem I am trying to solve very well. I guess the below header would have been better:

"PM / Domains: Power up PM domains prior drivers starts to probe their devices"

Quoted 2 lines

- it can wake up power domain

Yes, but it requires CONFIG_PM_RUNTIME to be set.

Thus, to solve the problem during driver ->probe() we need another solution which don't require CONFIG_PM_RUNTIME to be set. As this patchset proposes.

Quoted 3 lines

- it connects device to domain/class/type/bus and so allow to add additional PM layer on top of Platform bus (for example arch/arm/mach-omap2/omap_device.c).

Quoted 4 lines

So, it will do all needed things, and if it doesn't that problem is in platfrom/bus/driver code and not in Runtime PM.
if pm_runtime_get_sync() will be dropped - than all above will need to be implemented around the ->probe().

I am not sure what you mean about dropping pm_runtime_get_sync()? All I am saying is that we can't use it to power on PM domains during the probe sequence.

Of course, you may still use pm_runtime_get_sync() from anywhere it's needed, to for example handle device's parent/child relationships.

Quoted 9 lines

4) I've copied here comment from Rafael:

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Of course, if ->probe() is to call pm_runtime_resume() for this purpose, it must take the fact that the driver's own ->runtime_resume() may be called as a result of this into account.

Agree, that's a little bit annoying, but we are living with that for more then 5 years already (I'm 3 years) - so, I am, as driver developer, expecting above behavior (just walk through the drivers and you will see how many drivers expecting the same).

Quoted 1 line

So, any volunteers to check and fix ~500 drivers.

We don't have to change these drivers. An certainly they are not 500. :-)

They will still work as is!

Though we need this fix to comply with them, which is supposed to go in for 3.18 rc[n].
"PM / Domains: Fix initial default state of the need_restore flag"

4) I've copied here comment from Rafael:

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Of course, if ->probe() is to call pm_runtime_resume() for this purpose, it must take the fact that the driver's own ->runtime_resume() may be called as a result of this into account.

Agree, that's a little bit annoying, but we are living with that for more then 5 years already (I'm 3 years) - so, I am, as driver developer, expecting above behavior (just walk through the drivers and you will see how many drivers expecting the same).

So, any volunteers to check and fix ~500 drivers.

Quoted 5 lines

Quoted 5 lines

- Runtime PM (if compiled in) needs to be enabled for all devices in power domains by default. Otherwise devices may lose power as a result for power management of the other devices in the same domain.

It should prevent enabling/disabling of RPM from sys_fs too.

Quoted 4 lines

Quoted 4 lines

- The core should try to power up domains before calling really_probe() both for CONFIG_PM_RUNTIME set and unset, so ->probe() can always make the "device is accessible" assumption.

Here I'm still think that pm_runtime_get_sync() (or similar) API should work. Another way, PM domain should decide what to do when the first device attached to it
- power up and stay powered on for example
(It will work if devices are attached before ->probe();
it will not work if devices will be attached once they've been created, but this is different question)

Quoted 8 lines

And how exactly will you then power up the PM domain when CONFIG_PM_RUNTIME is unset?

Quoted 4 lines

- Bus types may need to do more on top of that in their ->probe(), so the driver's ->probe() can make that assumption too in all cases.

Quoted 2 lines

Quoted 1 line

Does that make sense to you?

I would like to take the liberty to add a couple of points from me:
- it seems reasonable to have ability to disable Runtime PM globally from sys_fs:
once disabled - "get" should power on device, "put" should do nothing all the time except when ->remove() is called.

- It might be reasonable to add API like pm_runtime_probe_getXXX() which will do everything what standard pm_runtime_get_sync() is doing with one exception:
it will not call driver's .runtime_resume() callback.

- Just opinion. For GPD, It looks like integration/design problem and may be it shouldn't be integrated with Runtime PM through dev_pm_ops. Instead, it might be better to invoke it directly from RPM core.

On Friday, November 07, 2014 07:25:08 PM Grygorii Strashko wrote:

[cut]

Quoted 9 lines

4) I've copied here comment from Rafael:

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Of course, if ->probe() is to call pm_runtime_resume() for this purpose, it must take the fact that the driver's own ->runtime_resume() may be called as a result of this into account.

Agree, that's a little bit annoying, but we are living with that for more then 5 years already (I'm 3 years) - so, I am, as driver developer, expecting above behavior (just walk through the drivers and you will see how many drivers expecting the same).

Quoted 1 line

So, any volunteers to check and fix ~500 drivers.

Where did you get that number from?

Also please note that some bus types don't have this problem by design (e.g. PCI, as pointed to by Alan).

[cut]

Quoted 7 lines

Quoted 5 lines

Quoted 5 lines

- Runtime PM (if compiled in) needs to be enabled for all devices in power domains by default. Otherwise devices may lose power as a result for power management of the other devices in the same domain.

It should prevent enabling/disabling of RPM from sys_fs too.

It looks like you're confusing disable/enable with auto/on. These are different things.

Quoted 11 lines

Quoted 4 lines

Quoted 4 lines

- The core should try to power up domains before calling really_probe() both for CONFIG_PM_RUNTIME set and unset, so ->probe() can always make the "device is accessible" assumption.

Here I'm still think that pm_runtime_get_sync() (or similar) API should work. Another way, PM domain should decide what to do when the first device attached to it
- power up and stay powered on for example
(It will work if devices are attached before ->probe();
it will not work if devices will be attached once they've been created, but this is different question)

The PM domain itself can't do that. The only place that has enough knowledge is the code that enumerates devices (DT, ACPI or board-specific).

Quoted 8 lines

Quoted 8 lines

And how exactly will you then power up the PM domain when CONFIG_PM_RUNTIME is unset?

Quoted 4 lines

- Bus types may need to do more on top of that in their ->probe(), so the

    driver's ->probe() can make that assumption too in all cases.

Quoted 7 lines

Quoted 2 lines

Quoted 1 line

Does that make sense to you?

I would like to take the liberty to add a couple of points from me:
- it seems reasonable to have ability to disable Runtime PM globally from sys_fs:
once disabled - "get" should power on device, "put" should do nothing all the time except when ->remove() is called.

This is how the "power/control" file works and you can easily have this by writing "on" to that file for every device.

Quoted 1 line

Quoted 3 lines

- It might be reasonable to add API like pm_runtime_probe_getXXX() which will do everything what standard pm_runtime_get_sync() is doing with one exception:
it will not call driver's .runtime_resume() callback.

Use case?

2) There are no requirements for arch/platforms/drivers to work in both cases CONFIG_PM_RUNTIME=y/n. But they must be built without errors/warning for both cases.