PM / Runtime: Introduce pm_runtime_get_and_call()

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 introduce a new runtime PM helper function, __pm_runtime_get_and_call(), and its simplified variant pm_runtime_get_and_call(), allowing the caller to increment the device's runtime PM usage counter and execute a function provided as the second argument, either synchronously, if the device is in the "active" state, or from the runtime PM workqueue, after resuming the device (in that case the execution of the function is queued up along with a request to resume the device).

The simplified helper, pm_runtime_get_and_call(), will only execute the function if the runtime PM status of the device is "active". The more complicated one, __pm_runtime_get_and_call(), has one more argument allowing the caller to specify whether or not the function should be executed even if runtime PM is disabled for the device or there has been a runtime PM error. In those cases, the function will always be executed synchronously.

This version of the patch doesn't include any documentation updates.

No sign-off yet.

There are some known concerns about this approach.

First of all, the patch at

https://patchwork.kernel.org/patch/1299781/

increases the size of struct device by the size of a pointer, which may seem to be a bit excessive to somebody, although I personally don't think it's a big problem. We don't use _that_ many struct device objects for it to matter much.

Second, which is more important to me, it seems that for a given device func() will always be the same pointer and it will be used by the device's driver only. In that case, most likely, it will be possible to determine the address of func() at the time of driver initialization, so the setting and clearing of power.func and passing the address of func() as an argument every time __pm_runtime_get_and_call() is run may turn out to be an unnecessary overhead. Thus it may be more efficient to use a function pointer in struct device_driver (it can't be located in struct dev_pm_ops, because some drivers don't use it at all, like USB drivers, and it wouldn't be useful for subsystems and PM domains) to store the address of func() permanently.

For the above reasons, the appended patch implements an alternative approach, which is to modify the way pm_runtime_get() works so that, when the device is not active, it will queue a resume request for the device _along_ _with_ the execution of a driver routine provided through a new function pointer .pm_runtime_work(). There also is pm_runtime_get_nowork() that won't do that and works in the same way as the "old" pm_runtime_get().

Of course, the disadvantage of the new patch is that it makes the change in struct device_driver, but perhaps it's not a big deal.

I wonder what you think.

drivers/base/power/runtime.c +9 -1

@@ -715,11 +736,29 @@ static void pm_runtime_work(struct work_
 		rpm_suspend(dev, RPM_NOWAIT | RPM_AUTO);
 		break;
 	case RPM_REQ_RESUME:
-		rpm_resume(dev, RPM_NOWAIT);
+		if (dev->power.run_driver_work && dev->driver->pm_runtime_work)
+			driver_work = dev->driver->pm_runtime_work;
+
+		dev->power.run_driver_work = false;
+		rpm_resume(dev, driver_work ? 0 : RPM_NOWAIT);
 		break;
 	}
 
  out:
+	if (driver_work) {
+		pm_runtime_get_noresume(dev);
+		dev->power.work_in_progress = true;
+		spin_unlock_irq(&dev->power.lock);

Quoted 31 lines · Rafael J. Wysocki

There are some known concerns about this approach.

First of all, the patch at

https://patchwork.kernel.org/patch/1299781/

increases the size of struct device by the size of a pointer, which may seem to be a bit excessive to somebody, although I personally don't think it's a big problem. We don't use _that_ many struct device objects for it to matter much.

Second, which is more important to me, it seems that for a given device func() will always be the same pointer and it will be used by the device's driver only. In that case, most likely, it will be possible to determine the address of func() at the time of driver initialization, so the setting and clearing of power.func and passing the address of func() as an argument every time __pm_runtime_get_and_call() is run may turn out to be an unnecessary overhead. Thus it may be more efficient to use a function pointer in struct device_driver (it can't be located in struct dev_pm_ops, because some drivers don't use it at all, like USB drivers, and it wouldn't be useful for subsystems and PM domains) to store the address of func() permanently.

For the above reasons, the appended patch implements an alternative approach, which is to modify the way pm_runtime_get() works so that, when the device is not active, it will queue a resume request for the device _along_ _with_ the execution of a driver routine provided through a new function pointer .pm_runtime_work(). There also is pm_runtime_get_nowork() that won't do that and works in the same way as the "old" pm_runtime_get().

Of course, the disadvantage of the new patch is that it makes the change in struct device_driver, but perhaps it's not a big deal.

Quoted 1 line

I wonder what you think.

I have some concerns about this patch.

Firstly, the patch doesn't do anything in the case where the device is already at full power.

Should we invoke the callback routine
synchronously? This loses the advantage of a workqueue's "pristine" environment, but on the other hand it is much more efficient.

(And we're talking about hot pathways, where efficiency matters.) The alternative, of course, is to have the driver invoke the callback directly if pm_runtime_get() returns 1.

Secondly, is it necessary for __pm_runtime_barrier() to wait for the callback routine?

More generally, does __pm_runtime_barrier() really
do what it's supposed to do? What happens if it runs at the same time as another thread is executing the pm_runtime_put(parent) call at the end of rpm_resume(), or the rpm_resume(parent, 0) in the middle?

Thirdly, I would reorganize the new code added to pm_runtime_work(); see below.

Quoted 4 lines

drivers/base/power/runtime.c +0 -0

@@ -533,6 +550,13 @@ static int rpm_resume(struct device *dev
 		goto out;
 	}
 

drivers/base/power/runtime.c static int rpm_resume(struct device *dev · line 559

Quoted 7 lines

drivers/base/power/runtime.c +7 -0

+	if ((rpmflags & RPM_ASYNC) && (rpmflags & RPM_RUN_WORK)) {
+		dev->power.run_driver_work = true;
+		rpm_queue_up_resume(dev);
+		retval = 0;
+		goto out;
+	}
+

The section of code just before the start of this hunk exits the routine if the device is already active. Do you want to put this new section in the preceding "if" block?

Also, it feels odd to have this code here when there is another section lower down that also tests for RPM_ASYNC and does almost the same thing. It suggests that this new section isn't in the right place.

For instance, consider what happens in the "no_callbacks" case where the parent is already active.

Quoted 18 lines

drivers/base/power/runtime.c +9 -1

@@ -715,11 +736,29 @@ static void pm_runtime_work(struct work_
 		rpm_suspend(dev, RPM_NOWAIT | RPM_AUTO);
 		break;
 	case RPM_REQ_RESUME:
-		rpm_resume(dev, RPM_NOWAIT);
+		if (dev->power.run_driver_work && dev->driver->pm_runtime_work)
+			driver_work = dev->driver->pm_runtime_work;
+
+		dev->power.run_driver_work = false;
+		rpm_resume(dev, driver_work ? 0 : RPM_NOWAIT);
 		break;
 	}
 
  out:
+	if (driver_work) {
+		pm_runtime_get_noresume(dev);
+		dev->power.work_in_progress = true;
+		spin_unlock_irq(&dev->power.lock);

drivers/base/power/runtime.c static void pm_runtime_work(struct work_ · line 761

Quoted 10 lines

drivers/base/power/runtime.c +10 -0

+
+		driver_work(dev);
+
+		spin_lock_irq(&dev->power.lock);
+		dev->power.work_in_progress = false;
+		wake_up_all(&dev->power.wait_queue);
+		pm_runtime_put_noidle(dev);
+		rpm_idle(dev, RPM_NOWAIT);
+	}
+

It seems very illgical to do all the callback stuff here, after the "switch" statement. IMO it would make more sense to put it all together, more or less as follows:

	case RPM_REQ_RESUME:
		if (dev->power.run_driver_work && dev->driver->pm_runtime_work) {
			driver_work = dev->driver->pm_runtime_work;
			dev->power.run_driver_work = false;
			dev->power.work_in_progress = true;
			pm_runtime_get_noresume(dev);
			rpm_resume(dev, 0);
			spin_unlock_irq(&dev->power.lock);

			driver_work(dev);

			spin_lock_irq(&dev->power.lock);

			dev->power.work_in_progress = false;
			wake_up_all(&dev->power.wait_queue);
			pm_runtime_put_noidle(dev);
			rpm_idle(dev, RPM_NOWAIT);
		} else {
			rpm_resume(dev, RPM_NOWAIT);
		}
		break;

Notice also that it's important to do the _get_noresume _before_ calling rpm_resume(). Otherwise the device might get suspended again before the callback gets a chance to run.

Quoted 2 lines

Firstly, the patch doesn't do anything in the case where the device is already at full power.

This is intentional, because I'm not sure that the code to be run if pm_runtime_get() returns 1 should always be pm_runtime_work().

For example, the driver may want to acquire a lock before running pm_runtime_get() and execute that code under the lock.

Quoted 2 lines

Secondly, is it necessary for __pm_runtime_barrier() to wait for the callback routine?

I believe so. At least that's what is documented about __pm_runtime_barrier().

Quoted 4 lines

More generally, does __pm_runtime_barrier() really
do what it's supposed to do? What happens if it runs at the same time as another thread is executing the pm_runtime_put(parent) call at the end of rpm_resume(), or the rpm_resume(parent, 0) in the middle?

So these are two different situations. When pm_runtime_put(parent) is executed, the device has been resumed and no runtime PM code _for_ _the_ _device_ is running (although the trace_rpm_return_int() is at a wrong place in my opinion). The second one is more interesting, but it really is equivalent to calling pm_runtime_resume() (in a different thread) after __pm_runtime_barrier() has run.

Quoted 2 lines

For instance, consider what happens in the "no_callbacks" case where the parent is already active.

The no_callbacks case is actually interesting, because I think that the function should return 1 in that case. Otherwise, the caller of pm_runtime_get() may think that it has to wait for the device to resume, which isn't the case. So, this seems to need fixing now.

Moreover, if power.no_callbacks is set, the RPM_SUSPENDING and RPM_RESUMING status values are impossible, as far as I can say, so the entire "no callbacks" section should be moved right after the check against RPM_ACTIVE. The same appears to apply to the analogous "no callbacks" check in rpm_suspend() (i.e. it should be done earlier).

Quoted 6 lines · Rafael J. Wysocki

Quoted 2 lines

Firstly, the patch doesn't do anything in the case where the device is already at full power.

This is intentional, because I'm not sure that the code to be run if pm_runtime_get() returns 1 should always be pm_runtime_work().

Quoted 2 lines

For example, the driver may want to acquire a lock before running pm_runtime_get() and execute that code under the lock.

Your reason above makes sense, but the example isn't quite right. If the driver wants to execute the callback under a lock then the callback has to take that lock -- this is necessary because of the async workqueue case.

In other words, you're saying there may well be situations where the synchronous and async cases need to run slightly different code.

True enough.

Quoted 10 lines

Quoted 2 lines

Secondly, is it necessary for __pm_runtime_barrier() to wait for the callback routine?

I believe so. At least that's what is documented about __pm_runtime_barrier().

Quoted 4 lines

More generally, does __pm_runtime_barrier() really
do what it's supposed to do? What happens if it runs at the same time as another thread is executing the pm_runtime_put(parent) call at the end of rpm_resume(), or the rpm_resume(parent, 0) in the middle?

Quoted 6 lines

So these are two different situations. When pm_runtime_put(parent) is executed, the device has been resumed and no runtime PM code _for_ _the_ _device_ is running (although the trace_rpm_return_int() is at a wrong place in my opinion). The second one is more interesting, but it really is equivalent to calling pm_runtime_resume() (in a different thread) after __pm_runtime_barrier() has run.

__pm_runtime_barrier() has never made very strong guarantees about runtime_resume callbacks. But the kerneldoc does claim that any pending callbacks will have been completed, and that claim evidently is violated in the rpm_resume(parent,0) case.

Maybe the kerneldoc needs to be changed, or maybe we need to fix the code.

Quoted 7 lines

Quoted 2 lines

For instance, consider what happens in the "no_callbacks" case where the parent is already active.

The no_callbacks case is actually interesting, because I think that the function should return 1 in that case. Otherwise, the caller of pm_runtime_get() may think that it has to wait for the device to resume, which isn't the case. So, this seems to need fixing now.

Right.

Quoted 5 lines

Moreover, if power.no_callbacks is set, the RPM_SUSPENDING and RPM_RESUMING status values are impossible, as far as I can say, so the entire "no callbacks" section should be moved right after the check against RPM_ACTIVE. The same appears to apply to the analogous "no callbacks" check in rpm_suspend() (i.e. it should be done earlier).

I'm not so sure about that. Basically we've got two conditions:

	if (A) {
		...
	}
	if (B) {
		...
	}

where A and B can never both be true. In this situation it doesn't make much difference what order you do the tests. We should use whichever order is better for adding the RPM_RUN_WORK code.

Quoted 4 lines

__pm_runtime_barrier() has never made very strong guarantees about runtime_resume callbacks. But the kerneldoc does claim that any pending callbacks will have been completed, and that claim evidently is violated in the rpm_resume(parent,0) case.

"Flush all pending requests for the device from pm_wq and wait for all runtime PM operations involving the device in progress to complete."

It doesn't mention the parent.

But I agree, it's not very clear.

Quoted 1 line

Maybe the kerneldoc needs to be changed, or maybe we need to fix the code.

If anything, I'd change the kerneldoc. The code pretty much has to be what it is in this respect.

I think that it's better to reorder the checks so that the final ordering is:

* check power.no_callbacks and parent status
* check RPM_RUN_WORK
* check RPM_RESUMING || RPM_SUSPENDING
* check RPM_ASYNC

so that we don't schedule the execution of pm_runtime_work() if power.no_callbacks is set and the parent is active and we still do the power.deferred_resume optimization if RPM_RUN_WORK is unset.

Quoted 9 lines · Rafael J. Wysocki

Quoted 4 lines

__pm_runtime_barrier() has never made very strong guarantees about runtime_resume callbacks. But the kerneldoc does claim that any pending callbacks will have been completed, and that claim evidently is violated in the rpm_resume(parent,0) case.

"Flush all pending requests for the device from pm_wq and wait for all runtime PM operations involving the device in progress to complete."

It doesn't mention the parent.

You're missing the point. Suppose you do an async resume and while the workqueue routine is executing pm_resume(parent,0), another thread calls pm_runtime_barrier() for the same device. The barrier will return more or less immediately, even though there is a runtime PM operation involving the device (that is, the async resume) still in progress. The rpm_resume() routine was running before pm_runtime_barrier() was called and will still be running afterward.

Quoted 6 lines

But I agree, it's not very clear.

Quoted 1 line

Maybe the kerneldoc needs to be changed, or maybe we need to fix the code.

If anything, I'd change the kerneldoc. The code pretty much has to be what it is in this respect.

I'm not sure what guarantees pm_runtime_barrier() _can_ make about runtime_resume callbacks. If you call that routine while the device is suspended then a runtime_resume callback could occur at any moment, because userspace can write "on" to the power/control attribute whenever it wants to.

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.

Quoted 7 lines

I think that it's better to reorder the checks so that the final ordering is:

* check power.no_callbacks and parent status
* check RPM_RUN_WORK
* check RPM_RESUMING || RPM_SUSPENDING
* check RPM_ASYNC

Quoted 3 lines

so that we don't schedule the execution of pm_runtime_work() if power.no_callbacks is set and the parent is active and we still do the power.deferred_resume optimization if RPM_RUN_WORK is unset.

That seems reasonable.

Quoted 10 lines

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.

Quoted 13 lines · Rafael J. Wysocki

Quoted 10 lines

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.

Quoted 3 lines

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

Quoted 19 lines · Alan Stern

Quoted 16 lines · Rafael J. Wysocki

Quoted 10 lines

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.

Quoted 3 lines

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.