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

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

[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.)

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?

Kevin

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

[Rafael J. Wysocki]

Hi,

On Saturday, July 09, 2011, Kevin Hilman wrote:
> 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?

That's correct.

> 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()?

That should be fine.

> 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?

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

Thanks,
Rafael


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

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

[Rafael J. Wysocki]

Hi,

On Saturday, July 09, 2011, Kevin Hilman wrote:
> 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?

That's correct.

> 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()?

That should be fine.

> 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?

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

Thanks,
Rafael


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

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

[Kevin Hilman]

"Rafael J. Wysocki" <rjw@sisk.pl> writes:

> Hi,
>
> On Saturday, July 09, 2011, Kevin Hilman wrote:
>> 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?
>
> That's correct.
>
>> 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()?
>
> That should be fine.
>
>> 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?
>
> I don't see anything wrong with your approach. :-)

Thanks.

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

I responded to that thread.

Kevin

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

[Kevin Hilman]

"Rafael J. Wysocki" <rjw@sisk.pl> writes:

> Hi,
>
> On Saturday, July 09, 2011, Kevin Hilman wrote:
>> 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?
>
> That's correct.
>
>> 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()?
>
> That should be fine.
>
>> 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?
>
> I don't see anything wrong with your approach. :-)

Thanks.

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

I responded to that thread.

Kevin

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

[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.)

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?

Kevin