PM domain using _noirq methods to "finish" pending runtime PM transistions

Hi Rafael,

So I'm now experimenting with your suggestion of using the noirq callbacks of the PM domain to ensure device low-power state transitions for the cases where runtime PM has been disabled from userspace (or a driver has used runtime PM calls in it's suspend/resume path but they have no effect since runtime PM is disabled.)

Before I get too far, I want to be sure I understood your suggestion correctly, and run my current implemtation past you.

For starters, I just set the driver's noirq callbacks to point to the runtime PM callbacks.

Then, in the PM domain's suspend_noirq, I basically do

	ret = pm_generic_suspend_noirq(dev);
	if (!ret && !(dev->power.runtime_status == RPM_SUSPENDED))
		magic_device_power_down(dev); /* shared with pm_domain runtime PM methods */
                priv->flags |= MAGIC_DEVICE_SUSPENDED;
	}
	return ret;

and in the PM domain's resume_norq, I basically do:

	if ((priv->flags & MAGIC_DEVICE_SUSPENDED) &&
	    !(dev->power.runtime_status == RPM_SUSPENDED)) {
		priv->flags &= ~OMAP_DEVICE_SUSPENDED;
		magic_device_power_up(dev);
	}
	return pm_generic_resume_noirq(dev);

Note: The MAGIC_DEVICE_SUSPENDED flag is used so resume_noirq only reenables devices that were disabled by suspend_noirq so that devices which were disabled by runtime PM are left untouched (similar to how you've implmented generic PM domains.)

Since the driver's noirq callbacks point to the runtime PM callbacks this all works quite well.

I believe this was basically the suggestion you were recommending, right?

However, what I need is for the driver's runtime PM callbacks not to be called unconditionally, but only if the PM domain's noirq methods are actually going to disable/power-down the device (e.g. it's not already runtime suspended.)

The first obvious solution is to create new driver noirq methods that check if the device is not already RPM_SUSPENDED and only then call the
runtime PM callbacks. That works, but hey, it's a Friday night so I
decided to take it to the next level...

Instead of making all the drivers add new noirq methods that just conditionally call the runtime PM methods, what if I just handle it in the PM domain? IOW, what if I just call pm_generic_runtime_* from the PM domain's noirq methods? e.g. call pm_generic_runtime_suspend() just before magic_device_power_down() and call pm_generic_runtime_resume() just after magic_device_power_up()?

I tested this today with a handful of our drivers that do all their PM in terms of the runtime PM API (including get_sync/put_sync in their suspend path) and they all work as expected without modification.

I know it's not much to add a couple new callbacks to each of these drivers, but if I can handle it at the PM domain level, I'd rather allow the drivers to stay simple.

What do you think?

Hi,

Quoted 37 lines · Kevin Hilman

Hi Rafael,

So I'm now experimenting with your suggestion of using the noirq callbacks of the PM domain to ensure device low-power state transitions for the cases where runtime PM has been disabled from userspace (or a driver has used runtime PM calls in it's suspend/resume path but they have no effect since runtime PM is disabled.)

Before I get too far, I want to be sure I understood your suggestion correctly, and run my current implemtation past you.

For starters, I just set the driver's noirq callbacks to point to the runtime PM callbacks.

Then, in the PM domain's suspend_noirq, I basically do

	ret = pm_generic_suspend_noirq(dev);
	if (!ret && !(dev->power.runtime_status == RPM_SUSPENDED))
		magic_device_power_down(dev); /* shared with pm_domain runtime PM methods */
                priv->flags |= MAGIC_DEVICE_SUSPENDED;
	}
	return ret;

and in the PM domain's resume_norq, I basically do:

	if ((priv->flags & MAGIC_DEVICE_SUSPENDED) &&
	    !(dev->power.runtime_status == RPM_SUSPENDED)) {
		priv->flags &= ~OMAP_DEVICE_SUSPENDED;
		magic_device_power_up(dev);
	}
	return pm_generic_resume_noirq(dev);

Note: The MAGIC_DEVICE_SUSPENDED flag is used so resume_noirq only reenables devices that were disabled by suspend_noirq so that devices which were disabled by runtime PM are left untouched (similar to how you've implmented generic PM domains.)

Quoted 4 lines

Since the driver's noirq callbacks point to the runtime PM callbacks this all works quite well.

I believe this was basically the suggestion you were recommending, right?

That's correct.

Quoted 10 lines

However, what I need is for the driver's runtime PM callbacks not to be called unconditionally, but only if the PM domain's noirq methods are actually going to disable/power-down the device (e.g. it's not already runtime suspended.)

The first obvious solution is to create new driver noirq methods that check if the device is not already RPM_SUSPENDED and only then call the
runtime PM callbacks. That works, but hey, it's a Friday night so I
decided to take it to the next level...

Quoted 6 lines

Instead of making all the drivers add new noirq methods that just conditionally call the runtime PM methods, what if I just handle it in the PM domain? IOW, what if I just call pm_generic_runtime_* from the PM domain's noirq methods? e.g. call pm_generic_runtime_suspend() just before magic_device_power_down() and call pm_generic_runtime_resume() just after magic_device_power_up()?

That should be fine.

Quoted 4 lines

I tested this today with a handful of our drivers that do all their PM in terms of the runtime PM API (including get_sync/put_sync in their suspend path) and they all work as expected without modification.

Quoted 5 lines

I know it's not much to add a couple new callbacks to each of these drivers, but if I can handle it at the PM domain level, I'd rather allow the drivers to stay simple.

What do you think?

I don't see anything wrong with your approach. :-)

PS
I wonder what you think about this patch:
https://patchwork.kernel.org/patch/959672/

Hi,

Quoted 37 lines · Kevin Hilman

Hi Rafael,

So I'm now experimenting with your suggestion of using the noirq callbacks of the PM domain to ensure device low-power state transitions for the cases where runtime PM has been disabled from userspace (or a driver has used runtime PM calls in it's suspend/resume path but they have no effect since runtime PM is disabled.)

Before I get too far, I want to be sure I understood your suggestion correctly, and run my current implemtation past you.

For starters, I just set the driver's noirq callbacks to point to the runtime PM callbacks.

Then, in the PM domain's suspend_noirq, I basically do

	ret = pm_generic_suspend_noirq(dev);
	if (!ret && !(dev->power.runtime_status == RPM_SUSPENDED))
		magic_device_power_down(dev); /* shared with pm_domain runtime PM methods */
                priv->flags |= MAGIC_DEVICE_SUSPENDED;
	}
	return ret;

and in the PM domain's resume_norq, I basically do:

	if ((priv->flags & MAGIC_DEVICE_SUSPENDED) &&
	    !(dev->power.runtime_status == RPM_SUSPENDED)) {
		priv->flags &= ~OMAP_DEVICE_SUSPENDED;
		magic_device_power_up(dev);
	}
	return pm_generic_resume_noirq(dev);

Note: The MAGIC_DEVICE_SUSPENDED flag is used so resume_noirq only reenables devices that were disabled by suspend_noirq so that devices which were disabled by runtime PM are left untouched (similar to how you've implmented generic PM domains.)

Quoted 4 lines

Since the driver's noirq callbacks point to the runtime PM callbacks this all works quite well.

I believe this was basically the suggestion you were recommending, right?

That's correct.

Quoted 10 lines

However, what I need is for the driver's runtime PM callbacks not to be called unconditionally, but only if the PM domain's noirq methods are actually going to disable/power-down the device (e.g. it's not already runtime suspended.)

The first obvious solution is to create new driver noirq methods that check if the device is not already RPM_SUSPENDED and only then call the
runtime PM callbacks. That works, but hey, it's a Friday night so I
decided to take it to the next level...

Quoted 6 lines

Instead of making all the drivers add new noirq methods that just conditionally call the runtime PM methods, what if I just handle it in the PM domain? IOW, what if I just call pm_generic_runtime_* from the PM domain's noirq methods? e.g. call pm_generic_runtime_suspend() just before magic_device_power_down() and call pm_generic_runtime_resume() just after magic_device_power_up()?

That should be fine.

Quoted 4 lines

I tested this today with a handful of our drivers that do all their PM in terms of the runtime PM API (including get_sync/put_sync in their suspend path) and they all work as expected without modification.

Quoted 5 lines

I know it's not much to add a couple new callbacks to each of these drivers, but if I can handle it at the PM domain level, I'd rather allow the drivers to stay simple.

What do you think?

I don't see anything wrong with your approach. :-)

PS
I wonder what you think about this patch:
https://patchwork.kernel.org/patch/959672/

Hi Rafael,

So I'm now experimenting with your suggestion of using the noirq callbacks of the PM domain to ensure device low-power state transitions for the cases where runtime PM has been disabled from userspace (or a driver has used runtime PM calls in it's suspend/resume path but they have no effect since runtime PM is disabled.)

Before I get too far, I want to be sure I understood your suggestion correctly, and run my current implemtation past you.

For starters, I just set the driver's noirq callbacks to point to the runtime PM callbacks.

Then, in the PM domain's suspend_noirq, I basically do

	ret = pm_generic_suspend_noirq(dev);
	if (!ret && !(dev->power.runtime_status == RPM_SUSPENDED))
		magic_device_power_down(dev); /* shared with pm_domain runtime PM methods */
                priv->flags |= MAGIC_DEVICE_SUSPENDED;
	}
	return ret;

and in the PM domain's resume_norq, I basically do:

	if ((priv->flags & MAGIC_DEVICE_SUSPENDED) &&
	    !(dev->power.runtime_status == RPM_SUSPENDED)) {
		priv->flags &= ~OMAP_DEVICE_SUSPENDED;
		magic_device_power_up(dev);
	}
	return pm_generic_resume_noirq(dev);

Note: The MAGIC_DEVICE_SUSPENDED flag is used so resume_noirq only reenables devices that were disabled by suspend_noirq so that devices which were disabled by runtime PM are left untouched (similar to how you've implmented generic PM domains.)

Since the driver's noirq callbacks point to the runtime PM callbacks this all works quite well.

I believe this was basically the suggestion you were recommending, right?

However, what I need is for the driver's runtime PM callbacks not to be called unconditionally, but only if the PM domain's noirq methods are actually going to disable/power-down the device (e.g. it's not already runtime suspended.)

The first obvious solution is to create new driver noirq methods that check if the device is not already RPM_SUSPENDED and only then call the
runtime PM callbacks. That works, but hey, it's a Friday night so I
decided to take it to the next level...

Instead of making all the drivers add new noirq methods that just conditionally call the runtime PM methods, what if I just handle it in the PM domain? IOW, what if I just call pm_generic_runtime_* from the PM domain's noirq methods? e.g. call pm_generic_runtime_suspend() just before magic_device_power_down() and call pm_generic_runtime_resume() just after magic_device_power_up()?

I tested this today with a handful of our drivers that do all their PM in terms of the runtime PM API (including get_sync/put_sync in their suspend path) and they all work as expected without modification.

I know it's not much to add a couple new callbacks to each of these drivers, but if I can handle it at the PM domain level, I'd rather allow the drivers to stay simple.

What do you think?