[PATCH][RFC][Update] PM / Runtime: Introduce driver runtime PM work routine
On Tuesday, August 14, 2012, Rafael J. Wysocki wrote:
> On Monday, August 13, 2012, Alan Stern wrote:
> > On Mon, 13 Aug 2012, Rafael J. Wysocki wrote:
> >
> > > > I guess the best we can say is that if you call pm_runtime_barrier()
> > > > after updating the dev_pm_ops method pointers then after the barrier
> > > > returns, the old method pointers will not be invoked and the old method
> > > > routines will not be running. So we need an equivalent guarantee with
> > > > regard to the pm_runtime_work pointer. (Yes, we could use a better
> > > > name for that pointer.)
> > > >
> > > > Which means the code in the patch isn't quite right, because it saves
> > > > the pm_runtime_work pointer before calling rpm_resume(). Maybe we
> > > > should avoid looking at the pointer until rpm_resume() returns.
> > >
> > > Yes, we can do that.
> > >
> > > Alternatively, we can set power.work_in_progress before calling
> > > rpm_resume(dev, 0) (i.e. regard the resume as a part of the work) to make
> > > the barrier wait for all of it to complete.
> >
> > Yep, that would work. In fact, I did it that way in the proposed code
> > posted earlier in this thread. (But that was just on general
> > principles, not because I had this particular race in mind.)
>
> OK
>
> I need to prepare a new patch now, but first I'll send a couple of (minor)
> fixes for the core runtime PM code.
The new patch is appended along with the "motivation" part of the changelog.
It is on top of the following three patches posted earlier:
https://patchwork.kernel.org/patch/1323641/
https://patchwork.kernel.org/patch/1323661/
https://patchwork.kernel.org/patch/1323651/
I changed the new callback's name to .pm_async_work() to avoid the name
conflict with the pm_runtime_work() function. I don't have a better idea
for its name at the moment.
Thanks,
Rafael
---
Unfortunately, pm_runtime_get() is not a very useful interface,
because if the device is not in the "active" state already, it
only queues up a work item supposed to resume the device. Then,
the caller doesn't really know when the device is going to be
resumed which makes it difficult to synchronize future device
accesses with the resume completion.
In that case, if the caller is the device's driver, the most
straightforward way for it to cope with the situation is to use its
.runtime_resume() callback for data processing unrelated to the
resume itself, but the correctness of this is questionable. Namely,
the driver's .runtime_resume() callback need not be executed directly
by the core and it may be run from within a subsystem or PM domain
callback. Then, the data processing carried out by the driver's
callback may disturb the subsystem's or PM domain's functionality
(for example, the subsystem may still be unready for the device to
process I/O when the driver's callback is about to return). Besides,
the .runtime_resume() callback is not supposed to do anything beyond
what is needed to resume the device.
For this reason, it appears to be necessary to introduce a mechanism
by which device drivers may schedule the execution of certain code
(say a procedure) to occur when the device in question is known to be
in the "active" state (preferably, as soon as it has been resumed).
Thus add a new runtime PM callback, .pm_async_work(), to struct
device_driver that will be executed along with the asynchronous
resume if pm_runtime_get() returns 0 (it may be executed once for
multiple subsequent invocations of pm_runtime_get() for the same
device, but if at least one of them returns 0, .pm_async_work() will
be executed at least once).
Additionally, define pm_runtime_get_nowork() that won't cause
the driver's .pm_async_work() callback to be executed.
This version of the patch doesn't include any documentation updates.
No sign-off yet.
---
drivers/base/power/runtime.c | 111 ++-
include/linux/device.h |2
include/linux/pm.h |2
include/linux/pm_runtime.h |6 ++
4 files changed, 88 insertions(+), 33 deletions(-)
Index: linux/include/linux/device.h
===
--- linux.orig/include/linux/device.h
+++ linux/include/linux/device.h
@@ -203,6 +203,7 @@ extern struct klist *bus_get_device_klis
* automatically.
* @pm:Power management operations of the device which matched
* this driver.
+ * @pm_async_work: Called after asynchronous runtime resume of the device.
* @p: Driver core's private data, no one other than the driver
* core can touch this.
*
@@ -232,6 +233,7 @@ struct device_driver {
const struct attribute_group **groups;
const struct dev_pm_ops *pm;
+ void (*pm_async_work) (struct device *dev);
struct driver_private *p;
};
Index: linux/include/linux/pm.h
===
---
[PATCH][RFC][Update] PM / Runtime: Introduce driver runtime PM work routine
On Tuesday, August 14, 2012, Rafael J. Wysocki wrote:
On Monday, August 13, 2012, Alan Stern wrote:
On Mon, 13 Aug 2012, Rafael J. Wysocki wrote:
I guess the best we can say is that if you call pm_runtime_barrier()
after updating the dev_pm_ops method pointers then after the barrier
returns, the old method pointers will not be invoked and the old method
routines will not be running. So we need an equivalent guarantee with
regard to the pm_runtime_work pointer. (Yes, we could use a better
name for that pointer.)
Which means the code in the patch isn't quite right, because it saves
the pm_runtime_work pointer before calling rpm_resume(). Maybe we
should avoid looking at the pointer until rpm_resume() returns.
Yes, we can do that.
Alternatively, we can set power.work_in_progress before calling
rpm_resume(dev, 0) (i.e. regard the resume as a part of the work) to make
the barrier wait for all of it to complete.
Yep, that would work. In fact, I did it that way in the proposed code
posted earlier in this thread. (But that was just on general
principles, not because I had this particular race in mind.)
OK
I need to prepare a new patch now, but first I'll send a couple of (minor)
fixes for the core runtime PM code.
The new patch is appended along with the motivation part of the changelog.
It is on top of the following three patches posted earlier:
https://patchwork.kernel.org/patch/1323641/
https://patchwork.kernel.org/patch/1323661/
https://patchwork.kernel.org/patch/1323651/
I changed the new callback's name to .pm_async_work() to avoid the name
conflict with the pm_runtime_work() function. I don't have a better idea
for its name at the moment.
Thanks,
Rafael
---
Unfortunately, pm_runtime_get() is not a very useful interface,
because if the device is not in the active state already, it
only queues up a work item supposed to resume the device. Then,
the caller doesn't really know when the device is going to be
resumed which makes it difficult to synchronize future device
accesses with the resume completion.
In that case, if the caller is the device's driver, the most
straightforward way for it to cope with the situation is to use its
.runtime_resume() callback for data processing unrelated to the
resume itself, but the correctness of this is questionable. Namely,
the driver's .runtime_resume() callback need not be executed directly
by the core and it may be run from within a subsystem or PM domain
callback. Then, the data processing carried out by the driver's
callback may disturb the subsystem's or PM domain's functionality
(for example, the subsystem may still be unready for the device to
process I/O when the driver's callback is about to return). Besides,
the .runtime_resume() callback is not supposed to do anything beyond
what is needed to resume the device.
For this reason, it appears to be necessary to introduce a mechanism
by which device drivers may schedule the execution of certain code
(say a procedure) to occur when the device in question is known to be
in the active state (preferably, as soon as it has been resumed).
Thus add a new runtime PM callback, .pm_async_work(), to struct
device_driver that will be executed along with the asynchronous
resume if pm_runtime_get() returns 0 (it may be executed once for
multiple subsequent invocations of pm_runtime_get() for the same
device, but if at least one of them returns 0, .pm_async_work() will
be executed at least once).
Additionally, define pm_runtime_get_nowork() that won't cause
the driver's .pm_async_work() callback to be executed.
This version of the patch doesn't include any documentation updates.
No sign-off yet.
---
drivers/base/power/runtime.c | 111 ++-
include/linux/device.h |2
include/linux/pm.h |2
include/linux/pm_runtime.h |6 ++
4 files changed, 88 insertions(+), 33 deletions(-)
Index: linux/include/linux/device.h
===
--- linux.orig/include/linux/device.h
+++ linux/include/linux/device.h
@@ -203,6 +203,7 @@ extern struct klist *bus_get_device_klis
* automatically.
* @pm:Power management operations of the device which matched
* this driver.
+ * @pm_async_work: Called after asynchronous runtime resume of the device.
* @p: Driver core's private data, no one other than the driver
* core can touch this.
*
@@ -232,6 +233,7 @@ struct device_driver {
const struct attribute_group **groups;
const struct dev_pm_ops *pm;
+ void (*pm_async_work) (struct device *dev);
struct driver_private *p;
};
Index: linux/include/linux/pm.h
===
--- linux.orig/include/linux/pm.h
+++ linux/include/linux/pm.h
@@ -538,6 +538,8 @@ struct dev_pm_info {
