Question about expected behavior when PM runtime is disabled

Hi Rafael,

Sorry if this question has been raised before. I actually have two questions here. These questions are related to PM runtime being disabled at runtime (i.e. call to pm_runtime_disable() )

If I call pm_runtime_enabled() to first determine if PM runtime is enabled followed conditionally by a call to pm_runtime_get_sync(), it would be possible for PM runtime to be disabled between these two calls and the get_sync() will fail. Is there any reason to even use the enabled() call?

My goal here was to use the enabled() call to determine
if PM runtime was configured/enabled in the kernel and then to manage my resources, clocks etc, in a different way if PM runtime is not present.

My second question then is what if PM runtime is enabled in the kernel and then gets disabled at runtime. What is the expected behavior for a driver? Should it fail all requests with EGAIN until PM runtime is enabled again? (in suspend state, PM runtime gets disable, new i/o request is made, power and clocks need to be turned on).

What about delayed autosuspend? I believe that if PM runtime is disabled while there is a delayed autosuspend pending, the suspend will fail without notification (clocks and power will be left on).

Will PM
runtime still be in the idle state once PM runtime is re-enabled?

Quoted 11 lines · Kenneth Heitke

Hi Rafael,

Sorry if this question has been raised before. I actually have two questions here. These questions are related to PM runtime being disabled at runtime (i.e. call to pm_runtime_disable() )

If I call pm_runtime_enabled() to first determine if PM runtime is enabled followed conditionally by a call to pm_runtime_get_sync(), it would be possible for PM runtime to be disabled between these two calls and the get_sync() will fail. Is there any reason to even use the enabled() call?

As a general rule, a device won't be enabled or disabled for runtime PM unless its driver or subsystem enables or disables it. Since you should already know what the driver and subsystem are doing, there usually isn't any reason for using pm_runtime_enabled().

Quoted 3 lines

My goal here was to use the enabled() call to determine
if PM runtime was configured/enabled in the kernel and then to manage my resources, clocks etc, in a different way if PM runtime is not present.

If runtime PM isn't present, why do you want to manage your clocks etc. at all? The fact that it's not in the kernel means the system manager doesn't care about power usage.

Quoted 5 lines

My second question then is what if PM runtime is enabled in the kernel and then gets disabled at runtime. What is the expected behavior for a driver? Should it fail all requests with EGAIN until PM runtime is enabled again? (in suspend state, PM runtime gets disable, new i/o request is made, power and clocks need to be turned on).

It's up to the driver and the subsystem, since they are the entities that are responsible for disabling runtime PM. If you think disabling runtime PM will cause problems, then don't do it.

Quoted 4 lines

What about delayed autosuspend? I believe that if PM runtime is disabled while there is a delayed autosuspend pending, the suspend will fail without notification (clocks and power will be left on).

That's right.

Quoted 2 lines

Will PM
runtime still be in the idle state once PM runtime is re-enabled?

The device will be in the same state as it was when it was disabled, unless you explicitly call pm_runtime_set_active() or pm_runtime_set_suspended().

Alan Stern

Quoted 3 lines

If runtime PM isn't present, why do you want to manage your clocks etc. at all? The fact that it's not in the kernel means the system manager doesn't care about power usage.

I am trying to be backwards compatible. There is likely a period of time from when the runtime PM feature was added to when it was turned on by default. If the feature happens to be disabled, I think it makes sense for the driver to still do what it can to manage its resources. The power guys aren't going to let me off the hook that easily :)

Quoted 3 lines

It's up to the driver and the subsystem, since they are the entities that are responsible for disabling runtime PM. If you think disabling runtime PM will cause problems, then don't do it.

I'm thinking about within runtime PM itself. I believe during system suspend, disable() followed by enable() can be called. If that happens, are there any scenarios that I need to be concerned about? Can my autosuspend timer just happen to fire during that window between disable and enable resulting in a failure to suspend? My driver is part of the i2c subsystem, do I know for a fact that disable() won't be used?

Quoted 9 lines · Kenneth Heitke

Quoted 3 lines

If runtime PM isn't present, why do you want to manage your clocks etc. at all? The fact that it's not in the kernel means the system manager doesn't care about power usage.

I am trying to be backwards compatible. There is likely a period of time from when the runtime PM feature was added to when it was turned on by default. If the feature happens to be disabled, I think it makes sense for the driver to still do what it can to manage its resources. The power guys aren't going to let me off the hook that easily :)

Maybe not... but then you can point out that it's somebody else's fault! :-) More to the point, runtime PM should be enabled at the same time it is added. The code for both belongs in the subsystem and driver, so there's no reason not to do them together.

IMO it's silly to bypass the runtime PM core's notion of when power management should or should not be available. If you're going to do that, why bother using the runtime PM framework in the first place?

Quoted 10 lines

Quoted 3 lines

It's up to the driver and the subsystem, since they are the entities that are responsible for disabling runtime PM. If you think disabling runtime PM will cause problems, then don't do it.

I'm thinking about within runtime PM itself. I believe during system suspend, disable() followed by enable() can be called. If that happens, are there any scenarios that I need to be concerned about? Can my autosuspend timer just happen to fire during that window between disable and enable resulting in a failure to suspend? My driver is part of the i2c subsystem, do I know for a fact that disable() won't be used?

Ah -- that's a very good question.

The PM core doesn't call disable followed by enable, but subsystems do as part of system resume. In fact, I'm not at all sure that the current implementation is correct in this regard. The failure scenario you bring up seems entirely possible.

I think we need to have the PM core call pm_runtime_get_noresume() before invoking the resume_noirq (or thaw_noirq or restore_noirq) callback, and then call pm_runtime_put_sync() after invoking the complete callback. This would solve your race: The put_sync would invoke the runtime_idle method, which would start another runtime suspend or autosuspend.

(It used to be that the PM core called pm_runtime_get_noresume() earlier on, before the prepare callback. This also solved your race, but it caused other problems and so was changed.)

It's true that subsystems could do this for themselves, but then they'd _all_ have to do it. So we might as well put it in the PM core.

Rafael, what do you think?

Quoted 7 lines

I think we need to have the PM core call pm_runtime_get_noresume() before invoking the resume_noirq (or thaw_noirq or restore_noirq) callback, and then call pm_runtime_put_sync() after invoking the complete callback. This would solve your race: The put_sync would invoke the runtime_idle method, which would start another runtime suspend or autosuspend.

Quoted 8 lines

(It used to be that the PM core called pm_runtime_get_noresume() earlier on, before the prepare callback. This also solved your race, but it caused other problems and so was changed.)

It's true that subsystems could do this for themselves, but then they'd _all_ have to do it. So we might as well put it in the PM core.

Rafael, what do you think?

Yes, we can do that.

I even suspect that all subsystems will end up calling pm_runtime_disable() somewhere in the system suspend code path and pm_runtime_enable() during system resume. It might be a good idea to do that in the core too, after calling the subsystem's .suspend() and before calling its .resume(), respectively.

Quoted 16 lines · Rafael J. Wysocki

Quoted 7 lines

I think we need to have the PM core call pm_runtime_get_noresume() before invoking the resume_noirq (or thaw_noirq or restore_noirq) callback, and then call pm_runtime_put_sync() after invoking the complete callback. This would solve your race: The put_sync would invoke the runtime_idle method, which would start another runtime suspend or autosuspend.

Quoted 8 lines

(It used to be that the PM core called pm_runtime_get_noresume() earlier on, before the prepare callback. This also solved your race, but it caused other problems and so was changed.)

It's true that subsystems could do this for themselves, but then they'd _all_ have to do it. So we might as well put it in the PM core.

Rafael, what do you think?

Quoted 7 lines

Yes, we can do that.

I even suspect that all subsystems will end up calling pm_runtime_disable() somewhere in the system suspend code path and pm_runtime_enable() during system resume. It might be a good idea to do that in the core too, after calling the subsystem's .suspend() and before calling its .resume(), respectively.

Will that bring back Kevin's problems? There was a specific commit:
"PM: Allow pm_runtime_suspend() to succeed during system suspend". If the core disables runtime PM, won't he be right back where he was before?

Quoted 26 lines · Alan Stern

Quoted 16 lines · Rafael J. Wysocki

Quoted 7 lines

I think we need to have the PM core call pm_runtime_get_noresume() before invoking the resume_noirq (or thaw_noirq or restore_noirq) callback, and then call pm_runtime_put_sync() after invoking the complete callback. This would solve your race: The put_sync would invoke the runtime_idle method, which would start another runtime suspend or autosuspend.

Quoted 8 lines

(It used to be that the PM core called pm_runtime_get_noresume() earlier on, before the prepare callback. This also solved your race, but it caused other problems and so was changed.)

It's true that subsystems could do this for themselves, but then they'd _all_ have to do it. So we might as well put it in the PM core.

Rafael, what do you think?

Quoted 7 lines

Yes, we can do that.

I even suspect that all subsystems will end up calling pm_runtime_disable() somewhere in the system suspend code path and pm_runtime_enable() during system resume. It might be a good idea to do that in the core too, after calling the subsystem's .suspend() and before calling its .resume(), respectively.

Quoted 4 lines

Will that bring back Kevin's problems? There was a specific commit:
"PM: Allow pm_runtime_suspend() to succeed during system suspend". If the core disables runtime PM, won't he be right back where he was before?

Not exactly, because that commit removed the pm_runtime_get_noresume() done before .prepare(), which was too early. As I said before, I don't see anything wrong with running pm_runtime_ helpers from .prepare() or .complete(). However, to me, it is highly doubtful if there is any valid reason for calling them after .suspend() has been executed. In fact, I think that .suspend() should ensure that they won't be executed for the given device after it has returned, so doing pm_runtime_disable() in the core at this point makes sense.

We really shouldn't allow any runtime PM callbacks to race with .suspend_noirq() and .resume_noirq(), because allowing that to happen would be asking for breakage.

Quoted 41 lines · Rafael J. Wysocki

Quoted 26 lines · Alan Stern

Quoted 16 lines · Rafael J. Wysocki

Quoted 7 lines

I think we need to have the PM core call pm_runtime_get_noresume() before invoking the resume_noirq (or thaw_noirq or restore_noirq) callback, and then call pm_runtime_put_sync() after invoking the complete callback. This would solve your race: The put_sync would invoke the runtime_idle method, which would start another runtime suspend or autosuspend.

Quoted 8 lines

(It used to be that the PM core called pm_runtime_get_noresume() earlier on, before the prepare callback. This also solved your race, but it caused other problems and so was changed.)

It's true that subsystems could do this for themselves, but then they'd _all_ have to do it. So we might as well put it in the PM core.

Rafael, what do you think?

Quoted 7 lines

Yes, we can do that.

I even suspect that all subsystems will end up calling pm_runtime_disable() somewhere in the system suspend code path and pm_runtime_enable() during system resume. It might be a good idea to do that in the core too, after calling the subsystem's .suspend() and before calling its .resume(), respectively.

Quoted 4 lines

Will that bring back Kevin's problems? There was a specific commit:
"PM: Allow pm_runtime_suspend() to succeed during system suspend". If the core disables runtime PM, won't he be right back where he was before?

Not exactly, because that commit removed the pm_runtime_get_noresume() done before .prepare(), which was too early. As I said before, I don't see anything wrong with running pm_runtime_ helpers from .prepare() or .complete(). However, to me, it is highly doubtful if there is any valid reason for calling them after .suspend() has been executed. In fact, I think that .suspend() should ensure that they won't be executed for the given device after it has returned, so doing pm_runtime_disable() in the core at this point makes sense.

Quoted 3 lines

We really shouldn't allow any runtime PM callbacks to race with .suspend_noirq() and .resume_noirq(), because allowing that to happen would be asking for breakage.

Then you suggest:

	Call pm_runtime_disable after .suspend;

	Call pm_runtime_get_noresume and pm_runtime_enable before
	.resume;

	Call pm_runtime_put_sync after .complete.

Right?

Quoted 4 lines

Then you suggest:

	Call pm_runtime_disable after .suspend;

Quoted 6 lines

Call pm_runtime_get_noresume and pm_runtime_enable before .resume;

Call pm_runtime_put_sync after .complete.

Right?

Yes, that would be resonable IMO.

Quoted 4 lines · Rafael J. Wysocki

Quoted 4 lines

Then you suggest:

	Call pm_runtime_disable after .suspend;

Quoted 8 lines

Quoted 6 lines

Call pm_runtime_get_noresume and pm_runtime_enable before .resume;

Call pm_runtime_put_sync after .complete.

Right?

Yes, that would be resonable IMO.

This turns out to be harder than it looks. If an error occurs, we may run the complete callback for devices that never were suspended or resumed and hence never had their usage_count incremented. How can we tell that we need to skip the pm_runtime_put_sync for these devices?

Would it be okay to call pm_runtime_put_sync immediately after the resume callback instead of after complete?

Quoted 20 lines · Alan Stern

Quoted 4 lines · Rafael J. Wysocki

Quoted 4 lines

Then you suggest:

	Call pm_runtime_disable after .suspend;

Quoted 8 lines

Quoted 6 lines

Call pm_runtime_get_noresume and pm_runtime_enable before .resume;

Call pm_runtime_put_sync after .complete.

Right?

Yes, that would be resonable IMO.

This turns out to be harder than it looks. If an error occurs, we may run the complete callback for devices that never were suspended or resumed and hence never had their usage_count incremented. How can we tell that we need to skip the pm_runtime_put_sync for these devices?

Quoted 2 lines

Would it be okay to call pm_runtime_put_sync immediately after the resume callback instead of after complete?

Yes, it would.

That said we may be better off by simply reverting commit e8665002477f0278f84f898145b1f141ba26ee26 (PM: Allow pm_runtime_suspend() to succeed during system suspend).

Quoted 26 lines · Rafael J. Wysocki

Quoted 15 lines · Alan Stern

Quoted 4 lines · Rafael J. Wysocki

Quoted 4 lines

Then you suggest:

	Call pm_runtime_disable after .suspend;

Quoted 8 lines

Quoted 6 lines

Call pm_runtime_get_noresume and pm_runtime_enable before .resume;

Call pm_runtime_put_sync after .complete.

Right?

Yes, that would be resonable IMO.

Quoted 7 lines

This turns out to be harder than it looks. If an error occurs, we may run the complete callback for devices that never were suspended or resumed and hence never had their usage_count incremented. How can we tell that we need to skip the pm_runtime_put_sync for these devices?

Would it be okay to call pm_runtime_put_sync immediately after the resume callback instead of after complete?

Yes, it would.

Quoted 3 lines

That said we may be better off by simply reverting commit e8665002477f0278f84f898145b1f141ba26ee26 (PM: Allow pm_runtime_suspend() to succeed during system suspend).

I'm OK with blocking runtime PM requests after .suspend (and unblocking before .resume) e.g. protecting the _noirq callbacks, but I hope we don't have to go back to blocking them before .prepare (and unblocking after .complete)