pm_runtime_suspended() can be false if RPM_SUSPENDED

Hi Rafael,

Just curious why pm_runtime_suspended() requires the device to be enabled for it to return true:

static inline bool pm_runtime_suspended(struct device *dev)

{
	return dev->power.runtime_status == RPM_SUSPENDED
		&& !dev->power.disable_depth;
}

I must be misunderstanding something, but I would consider a device that has been runtime suspended before runtime PM was disabled to still be runtime suspended.

I just noticed this when testing with your pm-domains branch. when I noticed that an 'if (pm_runtime_suspended(dev))' check in my PM domain's ->suspend_noirq() was always failing since it's after the PM core calls pm_runtime_disable(). I had to change my PM domain code to only check dev->power.runtime_status for it to work.

Hi,

Quoted 6 lines · Kevin Hilman

Hi Rafael,

Just curious why pm_runtime_suspended() requires the device to be enabled for it to return true:

static inline bool pm_runtime_suspended(struct device *dev)

Quoted 8 lines

{
	return dev->power.runtime_status == RPM_SUSPENDED
		&& !dev->power.disable_depth;
}

I must be misunderstanding something, but I would consider a device that has been runtime suspended before runtime PM was disabled to still be runtime suspended.

The problem is that while the runtime PM of the device is disabled (ie. dev->power.disable_depth > 0), its status may be switched from RPM_SUSPENDED to RPM_ACTIVE on the fly, using pm_runtime_set_active() (and the other way around) and it doesn't reflect the real status in principle.

So it was a tough choice what to do about that and I decided to go with returning false (in many cases runtime PM disabled means the device is always operational).

Quoted 5 lines

I just noticed this when testing with your pm-domains branch. when I noticed that an 'if (pm_runtime_suspended(dev))' check in my PM domain's ->suspend_noirq() was always failing since it's after the PM core calls pm_runtime_disable(). I had to change my PM domain code to only check dev->power.runtime_status for it to work.

Well, at this point I'm kind of cautious about changing pm_runtime_suspended(), so perhaps we need a separate callback returnig true in the status is RPM_SUSPENDED regardless of the value of power.disable_depth, like pm_runtime_status_suspended() or something similar.

Hi,

Quoted 6 lines · Kevin Hilman

Hi Rafael,

Just curious why pm_runtime_suspended() requires the device to be enabled for it to return true:

static inline bool pm_runtime_suspended(struct device *dev)

Quoted 8 lines

{
	return dev->power.runtime_status == RPM_SUSPENDED
		&& !dev->power.disable_depth;
}

I must be misunderstanding something, but I would consider a device that has been runtime suspended before runtime PM was disabled to still be runtime suspended.

The problem is that while the runtime PM of the device is disabled (ie. dev->power.disable_depth > 0), its status may be switched from RPM_SUSPENDED to RPM_ACTIVE on the fly, using pm_runtime_set_active() (and the other way around) and it doesn't reflect the real status in principle.

So it was a tough choice what to do about that and I decided to go with returning false (in many cases runtime PM disabled means the device is always operational).

Quoted 5 lines

I just noticed this when testing with your pm-domains branch. when I noticed that an 'if (pm_runtime_suspended(dev))' check in my PM domain's ->suspend_noirq() was always failing since it's after the PM core calls pm_runtime_disable(). I had to change my PM domain code to only check dev->power.runtime_status for it to work.

Well, at this point I'm kind of cautious about changing pm_runtime_suspended(), so perhaps we need a separate callback returnig true in the status is RPM_SUSPENDED regardless of the value of power.disable_depth, like pm_runtime_status_suspended() or something similar.

Quoted 18 lines · Rafael J. Wysocki

Hi,

Quoted 6 lines · Kevin Hilman

Hi Rafael,

Just curious why pm_runtime_suspended() requires the device to be enabled for it to return true:

static inline bool pm_runtime_suspended(struct device *dev)

Quoted 8 lines

{
	return dev->power.runtime_status == RPM_SUSPENDED
		&& !dev->power.disable_depth;
}

I must be misunderstanding something, but I would consider a device that has been runtime suspended before runtime PM was disabled to still be runtime suspended.

Quoted 5 lines

The problem is that while the runtime PM of the device is disabled (ie. dev->power.disable_depth > 0), its status may be switched from RPM_SUSPENDED to RPM_ACTIVE on the fly, using pm_runtime_set_active() (and the other way around) and it doesn't reflect the real status in principle.

OK, I guess this is the root of the confusion. Namely, that dev->power.runtime_status doesn't necessarily reflect the actual device state.

Quoted 3 lines

So it was a tough choice what to do about that and I decided to go with returning false (in many cases runtime PM disabled means the device is always operational).

And with your recent changes (pm_runtime_disable() in suspend path), it also be very common that

Quoted 10 lines

Quoted 5 lines

I just noticed this when testing with your pm-domains branch. when I noticed that an 'if (pm_runtime_suspended(dev))' check in my PM domain's ->suspend_noirq() was always failing since it's after the PM core calls pm_runtime_disable(). I had to change my PM domain code to only check dev->power.runtime_status for it to work.

Well, at this point I'm kind of cautious about changing pm_runtime_suspended(), so perhaps we need a separate callback returnig true in the status is RPM_SUSPENDED regardless of the value of power.disable_depth, like pm_runtime_status_suspended() or something similar.

OK, but documentation would probably be needed to describe why you might use one vs. the other.

However, based on the pm_runtime_set_active() problem you mentioned above, I'm not sure this will help either, since what the PM domain's noirq callback will want to do will be based on the actual device hardware state, not on the PM core's view of the device state.

Quoted 5 lines

However, based on the pm_runtime_set_active() problem you mentioned above, I'm not sure this will help either, since what the PM domain's noirq callback will want to do will be based on the actual device hardware state, not on the PM core's view of the device state.

Yes. For devices whose runtime PM is never enabled, this is quite clear (we must assume they are operational). For devices whose runtime PM is temporarily disabled and the reenabled, it's not that clear, but at least for the system suspend case we may require drivers not to use pm_runtime_set_active/suspended() in their callbacks, so that we may assume that the status hasn't changed between .suspend() and .resume().

So, I think your approach (to check power.runtime_status) is correct in this respect.

Quoted 5 lines

However, based on the pm_runtime_set_active() problem you mentioned above, I'm not sure this will help either, since what the PM domain's noirq callback will want to do will be based on the actual device hardware state, not on the PM core's view of the device state.

Yes. For devices whose runtime PM is never enabled, this is quite clear (we must assume they are operational). For devices whose runtime PM is temporarily disabled and the reenabled, it's not that clear, but at least for the system suspend case we may require drivers not to use pm_runtime_set_active/suspended() in their callbacks, so that we may assume that the status hasn't changed between .suspend() and .resume().

So, I think your approach (to check power.runtime_status) is correct in this respect.

"Rafael J. Wysocki" <[email protected]> writes:

[...]

Quoted 13 lines

Quoted 5 lines

However, based on the pm_runtime_set_active() problem you mentioned above, I'm not sure this will help either, since what the PM domain's noirq callback will want to do will be based on the actual device hardware state, not on the PM core's view of the device state.

Yes. For devices whose runtime PM is never enabled, this is quite clear (we must assume they are operational). For devices whose runtime PM is temporarily disabled and the reenabled, it's not that clear, but at least for the system suspend case we may require drivers not to use pm_runtime_set_active/suspended() in their callbacks, so that we may assume that the status hasn't changed between .suspend() and .resume().

Quoted 2 lines

So, I think your approach (to check power.runtime_status) is correct in this respect.

OK, I'll just directly check power.runtime_status in the noirq methods, since at that point I always know that disable_depth > 0.

Quoted 20 lines · Kevin Hilman

"Rafael J. Wysocki" <[email protected]> writes:

[...]

Quoted 15 lines

Quoted 5 lines

However, based on the pm_runtime_set_active() problem you mentioned above, I'm not sure this will help either, since what the PM domain's noirq callback will want to do will be based on the actual device hardware state, not on the PM core's view of the device state.

Yes.  For devices whose runtime PM is never enabled, this is quite clear (we must assume they are operational).  For devices whose runtime PM is temporarily disabled and the reenabled, it's not that clear, but at least for the system suspend case we may require drivers not to use pm_runtime_set_active/suspended() in their callbacks, so that we may assume that the status hasn't changed between .suspend() and .resume().

So, I think your approach (to check power.runtime_status) is correct in this respect.

Quoted 2 lines

OK, I'll just directly check power.runtime_status in the noirq methods, since at that point I always know that disable_depth > 0.

That's what I wanted to say. :-)  The only problem with that is if CONFIG_PM_RUNTIME is unset, power.runtime_status is not present, so I think we'll need a static inline to work around that.

Quoted 20 lines · Kevin Hilman

"Rafael J. Wysocki" <[email protected]> writes:

[...]

Quoted 15 lines

Quoted 5 lines

However, based on the pm_runtime_set_active() problem you mentioned above, I'm not sure this will help either, since what the PM domain's noirq callback will want to do will be based on the actual device hardware state, not on the PM core's view of the device state.

Yes.  For devices whose runtime PM is never enabled, this is quite clear (we must assume they are operational).  For devices whose runtime PM is temporarily disabled and the reenabled, it's not that clear, but at least for the system suspend case we may require drivers not to use pm_runtime_set_active/suspended() in their callbacks, so that we may assume that the status hasn't changed between .suspend() and .resume().

So, I think your approach (to check power.runtime_status) is correct in this respect.

Quoted 2 lines

OK, I'll just directly check power.runtime_status in the noirq methods, since at that point I always know that disable_depth > 0.

That's what I wanted to say. :-)  The only problem with that is if CONFIG_PM_RUNTIME is unset, power.runtime_status is not present, so I think we'll need a static inline to work around that.

Quoted 24 lines · Rafael J. Wysocki

Quoted 22 lines · Kevin Hilman

"Rafael J. Wysocki" <[email protected]> writes:

[...]

Quoted 13 lines

Quoted 5 lines

However, based on the pm_runtime_set_active() problem you mentioned above, I'm not sure this will help either, since what the PM domain's noirq callback will want to do will be based on the actual device hardware state, not on the PM core's view of the device state.

Yes. For devices whose runtime PM is never enabled, this is quite clear (we must assume they are operational). For devices whose runtime PM is temporarily disabled and the reenabled, it's not that clear, but at least for the system suspend case we may require drivers not to use pm_runtime_set_active/suspended() in their callbacks, so that we may assume that the status hasn't changed between .suspend() and .resume().

Quoted 2 lines

So, I think your approach (to check power.runtime_status) is correct in this respect.

OK, I'll just directly check power.runtime_status in the noirq methods, since at that point I always know that disable_depth > 0.

Quoted 4 lines

That's what I wanted to say. :-) The only problem with that is if CONFIG_PM_RUNTIME is unset, power.runtime_status is not present, so I think we'll need a static inline to work around that.

OK, I can prepare a patch for a static inline for pm_runtime_status_suspended(), unless you have one already in progress.

Quoted 30 lines · Kevin

Quoted 28 lines · Rafael J. Wysocki

Quoted 17 lines · Kevin Hilman

"Rafael J. Wysocki" <[email protected]> writes:

[...]

Quoted 13 lines

Quoted 5 lines

However, based on the pm_runtime_set_active() problem you mentioned above, I'm not sure this will help either, since what the PM domain's noirq callback will want to do will be based on the actual device hardware state, not on the PM core's view of the device state.

Yes.  For devices whose runtime PM is never enabled, this is quite clear (we must assume they are operational).  For devices whose runtime PM is temporarily disabled and the reenabled, it's not that clear, but at least for the system suspend case we may require drivers not to use pm_runtime_set_active/suspended() in their callbacks, so that we may assume that the status hasn't changed between .suspend() and .resume().

Quoted 5 lines

Quoted 2 lines

So, I think your approach (to check power.runtime_status) is correct in this respect.

OK, I'll just directly check power.runtime_status in the noirq methods, since at that point I always know that disable_depth > 0.

That's what I wanted to say. :-)  The only problem with that is if CONFIG_PM_RUNTIME is unset, power.runtime_status is not present, so I think we'll need a static inline to work around that.

Quoted 3 lines

OK, I can prepare a patch for a static inline for pm_runtime_status_suspended(), unless you have one already in progress.

No, I don't, please send me one if you can. :-)

Quoted 30 lines · Kevin

Quoted 28 lines · Rafael J. Wysocki

Quoted 17 lines · Kevin Hilman

"Rafael J. Wysocki" <[email protected]> writes:

[...]

Quoted 13 lines

Quoted 5 lines

However, based on the pm_runtime_set_active() problem you mentioned above, I'm not sure this will help either, since what the PM domain's noirq callback will want to do will be based on the actual device hardware state, not on the PM core's view of the device state.

Yes.  For devices whose runtime PM is never enabled, this is quite clear (we must assume they are operational).  For devices whose runtime PM is temporarily disabled and the reenabled, it's not that clear, but at least for the system suspend case we may require drivers not to use pm_runtime_set_active/suspended() in their callbacks, so that we may assume that the status hasn't changed between .suspend() and .resume().

Quoted 5 lines

Quoted 2 lines

So, I think your approach (to check power.runtime_status) is correct in this respect.

OK, I'll just directly check power.runtime_status in the noirq methods, since at that point I always know that disable_depth > 0.

That's what I wanted to say. :-)  The only problem with that is if CONFIG_PM_RUNTIME is unset, power.runtime_status is not present, so I think we'll need a static inline to work around that.

Quoted 3 lines

OK, I can prepare a patch for a static inline for pm_runtime_status_suspended(), unless you have one already in progress.

No, I don't, please send me one if you can. :-)

Quoted 24 lines · Rafael J. Wysocki

Quoted 22 lines · Kevin Hilman

"Rafael J. Wysocki" <[email protected]> writes:

[...]

Quoted 13 lines

Quoted 5 lines

However, based on the pm_runtime_set_active() problem you mentioned above, I'm not sure this will help either, since what the PM domain's noirq callback will want to do will be based on the actual device hardware state, not on the PM core's view of the device state.

Yes. For devices whose runtime PM is never enabled, this is quite clear (we must assume they are operational). For devices whose runtime PM is temporarily disabled and the reenabled, it's not that clear, but at least for the system suspend case we may require drivers not to use pm_runtime_set_active/suspended() in their callbacks, so that we may assume that the status hasn't changed between .suspend() and .resume().

Quoted 2 lines

So, I think your approach (to check power.runtime_status) is correct in this respect.

OK, I'll just directly check power.runtime_status in the noirq methods, since at that point I always know that disable_depth > 0.

Quoted 4 lines

That's what I wanted to say. :-) The only problem with that is if CONFIG_PM_RUNTIME is unset, power.runtime_status is not present, so I think we'll need a static inline to work around that.

OK, I can prepare a patch for a static inline for pm_runtime_status_suspended(), unless you have one already in progress.

"Rafael J. Wysocki" <[email protected]> writes:

[...]

Quoted 13 lines

Quoted 5 lines

However, based on the pm_runtime_set_active() problem you mentioned above, I'm not sure this will help either, since what the PM domain's noirq callback will want to do will be based on the actual device hardware state, not on the PM core's view of the device state.

Yes. For devices whose runtime PM is never enabled, this is quite clear (we must assume they are operational). For devices whose runtime PM is temporarily disabled and the reenabled, it's not that clear, but at least for the system suspend case we may require drivers not to use pm_runtime_set_active/suspended() in their callbacks, so that we may assume that the status hasn't changed between .suspend() and .resume().

Quoted 2 lines

So, I think your approach (to check power.runtime_status) is correct in this respect.

OK, I'll just directly check power.runtime_status in the noirq methods, since at that point I always know that disable_depth > 0.

Quoted 18 lines · Rafael J. Wysocki

Hi,

Quoted 6 lines · Kevin Hilman

Hi Rafael,

Just curious why pm_runtime_suspended() requires the device to be enabled for it to return true:

static inline bool pm_runtime_suspended(struct device *dev)

Quoted 8 lines

{
	return dev->power.runtime_status == RPM_SUSPENDED
		&& !dev->power.disable_depth;
}

I must be misunderstanding something, but I would consider a device that has been runtime suspended before runtime PM was disabled to still be runtime suspended.

Quoted 5 lines

The problem is that while the runtime PM of the device is disabled (ie. dev->power.disable_depth > 0), its status may be switched from RPM_SUSPENDED to RPM_ACTIVE on the fly, using pm_runtime_set_active() (and the other way around) and it doesn't reflect the real status in principle.

OK, I guess this is the root of the confusion. Namely, that dev->power.runtime_status doesn't necessarily reflect the actual device state.

Quoted 3 lines

So it was a tough choice what to do about that and I decided to go with returning false (in many cases runtime PM disabled means the device is always operational).

And with your recent changes (pm_runtime_disable() in suspend path), it also be very common that

Quoted 10 lines

Quoted 5 lines

I just noticed this when testing with your pm-domains branch. when I noticed that an 'if (pm_runtime_suspended(dev))' check in my PM domain's ->suspend_noirq() was always failing since it's after the PM core calls pm_runtime_disable(). I had to change my PM domain code to only check dev->power.runtime_status for it to work.

Well, at this point I'm kind of cautious about changing pm_runtime_suspended(), so perhaps we need a separate callback returnig true in the status is RPM_SUSPENDED regardless of the value of power.disable_depth, like pm_runtime_status_suspended() or something similar.

OK, but documentation would probably be needed to describe why you might use one vs. the other.

However, based on the pm_runtime_set_active() problem you mentioned above, I'm not sure this will help either, since what the PM domain's noirq callback will want to do will be based on the actual device hardware state, not on the PM core's view of the device state.

Hi Rafael,

Just curious why pm_runtime_suspended() requires the device to be enabled for it to return true:

static inline bool pm_runtime_suspended(struct device *dev)

{
	return dev->power.runtime_status == RPM_SUSPENDED
		&& !dev->power.disable_depth;
}

I must be misunderstanding something, but I would consider a device that has been runtime suspended before runtime PM was disabled to still be runtime suspended.

I just noticed this when testing with your pm-domains branch. when I noticed that an 'if (pm_runtime_suspended(dev))' check in my PM domain's ->suspend_noirq() was always failing since it's after the PM core calls pm_runtime_disable(). I had to change my PM domain code to only check dev->power.runtime_status for it to work.