Re: Do we need asynchronous pm_runtime_get()? (was: Re: bisected regression ...)
On Friday, August 10, 2012, Ming Lei wrote: On Fri, Aug 10, 2012 at 3:41 AM, Rafael J. Wysocki r...@sisk.pl wrote: It just isn't guaranteed that the subsystem callback won't do anything after driver-runtime_resume completion. I agree that it isn't likely to happen. In fact, the subsystem callback should make sure that don't happen, see below comments on .runtime_resume: * @runtime_resume: Put the device into the fully active state in response to a * wakeup event generated by hardware or at the request of software. If * necessary, put the device into the full-power state and restore its * registers, so that it is fully operational. So once driver-runtime_resume completes, the device should be fully operational from the view of driver. This comment only applies literally to drivers whose callbacks are run directly by the PM core. If subsystems and/or PM domains are involved, the interactions between different layers of callbacks obviously have to be taken into account. Please note, however, that this comment doesn't say anything about processing I/O by the callback (hint: the callback is _not_ _supposed_ to do that). Firstly, introduce one extra pointer in device may increase memory consume for device allocation, Yes, it does, which may or may not matter depending on the actual size of struct device and the CPU cache line size on the given machine, right? It may double memory allocation size in some cases. And it is very possible since there are so many device objects in system. Numbers, please? If you don't have them, it's just waving your hands. It is easily observed and proved. Suppose sizeof(struct foo_dev) is 508bytes, it will become 516bytes after your patch applies on 64bit arch, so ksize(foo_dev_ptr) will become 1024 and the memory consumption of the object is doubled. I meant real numbers, not made-up ones. I have explained it before, it is enough to keep the pointer read only since driver can maintain its internal state in its specific device instance (for example, usb_interface objects) and decide what to do in 'func' for situations, right? Yes, it is. I actually have a patch that does something similar (I'll post it shortly). I have seen your patch which moves the 'func' from device into device_driver. It is much better than before. Oh, thanks for letting me know. Of course, it is based on the assumption that func() will always be the same pointer for the given driver, which doesn't seem to be proven, but perhaps it is sufficient. At least I'm not aware of use cases where it wouldn't be. Since you have moved 'func' into device_driver, and you still thought the pointer can't be changed after it is set, so why not implement it as callback? I don't understand what you mean. It _is_ a callback now in fact. Thanks, Rafael -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Do we need asynchronous pm_runtime_get()? (was: Re: bisected regression ...)
On Thursday, August 09, 2012, Ming Lei wrote: On Thu, Aug 9, 2012 at 4:16 AM, Rafael J. Wysocki r...@sisk.pl wrote: On Wednesday, August 08, 2012, Alan Stern wrote: To be honest, I agree on the idea: The runtime-resume method does nothing but bring the device back to full power; it doesn't do any other processing, which should be done in 'func' or some kind of callback. Good. :-) I just think it is not good to introduce one extra field of 'func' in dev_pm_info which is embedded into struct device in the patch, see the reasons in the last part of the reply. That was my original suggestion. Rafael pointed out that having a single function call to do this would make it easier for driver writers to get it right. Not only would it be easier to get it right, in my opinion, but also in the example above func() may be called in some places where the driver may not want it to be called and which are very difficult to detect (like a resume from __device_suspend() during system suspend). Moreover, if the driver IMO, func() does some driver specific things, which PM core shouldn't pay special attention to in theory. But also it shouldn't execute that code, right? callback is not executed directly by the PM core, but instead it is executed by a subsystem or PM domain callback, there's no guarantee that the device _can_ be used for processing regular I/O before the driver callback returns (the subsystem callback may still need to do something _after_ that happens). driver-runtime_resume should be allowed to do I/O things after the device has been woken up inside driver callback, shouldn't it? If it isn't allowed, something wrong should have be reported before. Well, the lack of reports doesn't mean there are no bugs. :-) People may actually see those bugs, but they don't report them or they report them as system suspend/resume bugs, for example. So, this appears to be a matter of correctness too. If you've got a system with 1 device instances, you can probably spare the memory needed to store these function pointers. But you're right that this is a disadvantage. Yes, it is in grand general, but that also is a matter of alignment and of the way the slab allocator works. For example, if every struct device object were stored at the beginning of a new memory page, then its size wouldn't matter a lot as long as it were smaller than PAGE_SIZE. I haven't checked the details, but I'm pretty sure that focusing on the size alone doesn't give us the whole picture here. The allocation by kmalloc is not page aligned, and it is 2-power aligned, for example 16, 32, 64,..., also it is at least hardware L1 cache size aligned. Sure, that's why I used the conditional above. And it doesn't mean I didn't have the point. Firstly, introduce one extra pointer in device may increase memory consume for device allocation, Yes, it does, which may or may not matter depending on the actual size of struct device and the CPU cache line size on the given machine, right? also it reflects one design drawback in the patch, see below. More importantly, the implementation violates some software design principle in object oriented design. It doesn't violate anything and you're just ignoring what you've been told. That makes discussing with you rather difficult, but I'll try again nevertheless. If you look at the actual patch I've just posted: https://patchwork.kernel.org/patch/1299781/ you can see that power.func is never run directly. Moreover, the pointer it contains is only used to run a function in pm_runtime_work() and note that pm_runtime_work() reads that pointer _twice_, because it may be changed in the meantime by a concurrent thread. All of this means what I've told you already at least once: power.func is _not_ _a_ _member_ _function_. IOW, if it were C++, power.func would still be a function pointer, not a method, because it is _data_, although its data type happens to be void function taking a struct device pointer argument. It is a data field used to pass information to a work function, pm_runtime_work(), from a piece of code that schedules its execution, __pm_runtime_get_and_call(). See now? The driver core takes much object oriented idea in its design and implementation, and introduces device / driver / bus class. One class is an abstraction of one kind of objects or instances with same attributes, so one class may include many objects, for example, usb_device(class) is abstraction for all usb devices, and there may have many many usb devices in a system, but only one usb_device structure defined. One specific driver class is a special class since it may only have one driver object , which is basically read only. In OO world, it might be called static class. Generally, one driver object serves one specific device class, instead of one device object. For example,
Re: Do we need asynchronous pm_runtime_get()? (was: Re: bisected regression ...)
On Thu, Aug 9, 2012 at 6:46 PM, Rafael J. Wysocki r...@sisk.pl wrote: On Thursday, August 09, 2012, Ming Lei wrote: driver-runtime_resume should be allowed to do I/O things after the device has been woken up inside driver callback, shouldn't it? If it isn't allowed, something wrong should have be reported before. Well, the lack of reports doesn't mean there are no bugs. :-) People may actually see those bugs, but they don't report them or they report them as system suspend/resume bugs, for example. Also, I am still wondering why subsystem PM callback need to do something after driver-runtime_resume completes, could you explain it? Firstly, introduce one extra pointer in device may increase memory consume for device allocation, Yes, it does, which may or may not matter depending on the actual size of struct device and the CPU cache line size on the given machine, right? It may double memory allocation size in some cases. And it is very possible since there are so many device objects in system. also it reflects one design drawback in the patch, see below. More importantly, the implementation violates some software design principle in object oriented design. It doesn't violate anything and you're just ignoring what you've been told. That makes discussing with you rather difficult, but I'll try again nevertheless. If you look at the actual patch I've just posted: https://patchwork.kernel.org/patch/1299781/ you can see that power.func is never run directly. Moreover, the pointer it contains is only used to run a function in pm_runtime_work() and note that pm_runtime_work() reads that pointer _twice_, because it may be changed in the meantime by a concurrent thread. I have explained it before, it is enough to keep the pointer read only since driver can maintain its internal state in its specific device instance (for example, usb_interface objects) and decide what to do in 'func' for situations, right? Thanks, -- Ming Lei -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Do we need asynchronous pm_runtime_get()? (was: Re: bisected regression ...)
On Thursday, August 09, 2012, Ming Lei wrote: On Thu, Aug 9, 2012 at 6:46 PM, Rafael J. Wysocki r...@sisk.pl wrote: On Thursday, August 09, 2012, Ming Lei wrote: driver-runtime_resume should be allowed to do I/O things after the device has been woken up inside driver callback, shouldn't it? If it isn't allowed, something wrong should have be reported before. Well, the lack of reports doesn't mean there are no bugs. :-) People may actually see those bugs, but they don't report them or they report them as system suspend/resume bugs, for example. Also, I am still wondering why subsystem PM callback need to do something after driver-runtime_resume completes, could you explain it? It just isn't guaranteed that the subsystem callback won't do anything after driver-runtime_resume completion. I agree that it isn't likely to happen. Firstly, introduce one extra pointer in device may increase memory consume for device allocation, Yes, it does, which may or may not matter depending on the actual size of struct device and the CPU cache line size on the given machine, right? It may double memory allocation size in some cases. And it is very possible since there are so many device objects in system. Numbers, please? If you don't have them, it's just waving your hands. also it reflects one design drawback in the patch, see below. More importantly, the implementation violates some software design principle in object oriented design. It doesn't violate anything and you're just ignoring what you've been told. That makes discussing with you rather difficult, but I'll try again nevertheless. If you look at the actual patch I've just posted: https://patchwork.kernel.org/patch/1299781/ you can see that power.func is never run directly. Moreover, the pointer it contains is only used to run a function in pm_runtime_work() and note that pm_runtime_work() reads that pointer _twice_, because it may be changed in the meantime by a concurrent thread. I have explained it before, it is enough to keep the pointer read only since driver can maintain its internal state in its specific device instance (for example, usb_interface objects) and decide what to do in 'func' for situations, right? Yes, it is. I actually have a patch that does something similar (I'll post it shortly). Of course, it is based on the assumption that func() will always be the same pointer for the given driver, which doesn't seem to be proven, but perhaps it is sufficient. At least I'm not aware of use cases where it wouldn't be. Thanks, Rafael -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Do we need asynchronous pm_runtime_get()? (was: Re: bisected regression ...)
On Fri, Aug 10, 2012 at 3:41 AM, Rafael J. Wysocki r...@sisk.pl wrote: It just isn't guaranteed that the subsystem callback won't do anything after driver-runtime_resume completion. I agree that it isn't likely to happen. In fact, the subsystem callback should make sure that don't happen, see below comments on .runtime_resume: * @runtime_resume: Put the device into the fully active state in response to a * wakeup event generated by hardware or at the request of software. If * necessary, put the device into the full-power state and restore its * registers, so that it is fully operational. So once driver-runtime_resume completes, the device should be fully operational from the view of driver. Firstly, introduce one extra pointer in device may increase memory consume for device allocation, Yes, it does, which may or may not matter depending on the actual size of struct device and the CPU cache line size on the given machine, right? It may double memory allocation size in some cases. And it is very possible since there are so many device objects in system. Numbers, please? If you don't have them, it's just waving your hands. It is easily observed and proved. Suppose sizeof(struct foo_dev) is 508bytes, it will become 516bytes after your patch applies on 64bit arch, so ksize(foo_dev_ptr) will become 1024 and the memory consumption of the object is doubled. I have explained it before, it is enough to keep the pointer read only since driver can maintain its internal state in its specific device instance (for example, usb_interface objects) and decide what to do in 'func' for situations, right? Yes, it is. I actually have a patch that does something similar (I'll post it shortly). I have seen your patch which moves the 'func' from device into device_driver. It is much better than before. Of course, it is based on the assumption that func() will always be the same pointer for the given driver, which doesn't seem to be proven, but perhaps it is sufficient. At least I'm not aware of use cases where it wouldn't be. Since you have moved 'func' into device_driver, and you still thought the pointer can't be changed after it is set, so why not implement it as callback? IMO, it is a bit weird to just store a function pointer data(not for callback) in driver object, but anyway, it is better than before, :-) Thanks, -- Ming Lei -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Do we need asynchronous pm_runtime_get()? (was: Re: bisected regression ...)
On Wednesday, August 08, 2012, Ming Lei wrote: On Wed, Aug 8, 2012 at 4:45 AM, Rafael J. Wysocki r...@sisk.pl wrote: On Tuesday, August 07, 2012, Ming Lei wrote: On Tue, Aug 7, 2012 at 7:23 PM, Rafael J. Wysocki r...@sisk.pl wrote: [...] and you want to move something out of previous .runtime_resume No, I don't. Where did you get that idea from? If so, I am wondering why the 'func' can't be called in .runtime_resume directly, and follow the below inside caller at the same time? if (device is active or disabled) call func(device). This was covered in my last reply to Alan. and do it in another new callback to speedup resume, No, not to speed up resume. The idea is to allow drivers to run something when the resume is complete, so that they don't have to implement a resume detection logic or use .runtime_resume() to run things that don't belong there. Looks it was said by you, :-) Unless your _driver_ callback is actually executed from within a PM domain callback, for example, and something else may be waiting for it to complete, so your data processing is adding latencies to some other threads. I'm not making that up, by the way, that really can happen. See http://marc.info/?l=linux-pmm=134394271517527w=2 We were discussing specific pseudo-code in the documentation and you conveniently took the above out of context. Never mind. :-) I was trying to illustrate my point with a convincing example and I admit I could do better. Anyway the point was that the purpose of .runtime_resume() was not to process random I/O. Its purpose is to _resume_ a suspended device, no less, no more. Which the so that they don't have to [...] use .runtime_resume() to run things that don't belong there. sentence above is about. So I've been saying the same thing all the time and it's never been specifically about speedup (or rather about latencies added by random I/O processing in drivers' runtime resume callbacks). Alan also said Okay, those are valid reasons for the idea. Except for this one, I don't see other obvious advantages about the patch. so it should be reasonable to introduce the .runtime_post_resume callback in logic. No. This doesn't have anything to do with callbacks! If you want a new callback, you should specify what the role of this callback is, otherwise it is not well defined. I this case, though, what the role of func() is depends on the caller and most likely every driver would use it for something different. So no, I don't see how it can be a callback. Also, the 'func' should be per driver, not per device since only one 'func' is enough for all same kind of devices driven by one same driver. It isn't per device! It is per _caller_. The fact that the pointer is stored _temporarily_ in struct device doesn't mean that it is per device and that it is a callback. From the struct device point of view it is _data_, not a member function. The fact is that it will become per-device one you store it in 'struct device'. Suppose one driver may drive 1 same devices, Do you have any specific example of that? If not, then please don't make up arguments. the same data will be stored inside all the 1 device instances, it is a good way to do it? Not mention 90% devices mayn't use the _temporarily_ data at all. It may be unused just as well as an additional callback pointer in a driver object. [...] So now please count how many struct dev_pm_ops objects there are on that system and compute the differece. And please note that drivers that don't use struct dev_pm_ops for power management will do that in the future. Most of dev_pm_ops stays inside module image, and not in ram. Care to explain? I'm not sure I understand the above correctly. It is a bit difficult to get the count of all dev_pm_ops objects in ram since it is defined statically. Still, they are occupying memory, aren't they? So you really can't tell the difference between storing pointers in device driver objects and struct device objects. For example, in USB subsystem, there are only 2 dev_pm_ops objects in RAM for a normal system, but there may have hundreds of usb devices in the system(usb_device, usb_interface, ep_device, ...). Yes, USB is kind of exceptional, but also this means that your let's put that pointer into struct dev_pm_ops idea won't work for USB drivers, precisely because they don't use struct dev_pm_ops objects. Thanks, Rafael -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Do we need asynchronous pm_runtime_get()? (was: Re: bisected regression ...)
On Thu, Aug 9, 2012 at 4:16 AM, Rafael J. Wysocki r...@sisk.pl wrote: On Wednesday, August 08, 2012, Alan Stern wrote: To be honest, I agree on the idea: The runtime-resume method does nothing but bring the device back to full power; it doesn't do any other processing, which should be done in 'func' or some kind of callback. I just think it is not good to introduce one extra field of 'func' in dev_pm_info which is embedded into struct device in the patch, see the reasons in the last part of the reply. That was my original suggestion. Rafael pointed out that having a single function call to do this would make it easier for driver writers to get it right. Not only would it be easier to get it right, in my opinion, but also in the example above func() may be called in some places where the driver may not want it to be called and which are very difficult to detect (like a resume from __device_suspend() during system suspend). Moreover, if the driver IMO, func() does some driver specific things, which PM core shouldn't pay special attention to in theory. callback is not executed directly by the PM core, but instead it is executed by a subsystem or PM domain callback, there's no guarantee that the device _can_ be used for processing regular I/O before the driver callback returns (the subsystem callback may still need to do something _after_ that happens). driver-runtime_resume should be allowed to do I/O things after the device has been woken up inside driver callback, shouldn't it? If it isn't allowed, something wrong should have be reported before. So, this appears to be a matter of correctness too. If you've got a system with 1 device instances, you can probably spare the memory needed to store these function pointers. But you're right that this is a disadvantage. Yes, it is in grand general, but that also is a matter of alignment and of the way the slab allocator works. For example, if every struct device object were stored at the beginning of a new memory page, then its size wouldn't matter a lot as long as it were smaller than PAGE_SIZE. I haven't checked the details, but I'm pretty sure that focusing on the size alone doesn't give us the whole picture here. The allocation by kmalloc is not page aligned, and it is 2-power aligned, for example 16, 32, 64,..., also it is at least hardware L1 cache size aligned. Firstly, introduce one extra pointer in device may increase memory consume for device allocation, also it reflects one design drawback in the patch, see below. More importantly, the implementation violates some software design principle in object oriented design. The driver core takes much object oriented idea in its design and implementation, and introduces device / driver / bus class. One class is an abstraction of one kind of objects or instances with same attributes, so one class may include many objects, for example, usb_device(class) is abstraction for all usb devices, and there may have many many usb devices in a system, but only one usb_device structure defined. One specific driver class is a special class since it may only have one driver object , which is basically read only. In OO world, it might be called static class. Generally, one driver object serves one specific device class, instead of one device object. For example, usb_storage_driver is a driver object, which serves all usb mass storage devices which all belongs to usb mass storage class). The 'func' to be introduced is a function pointer, which should be driver related thing and should serve one specific device class in theory, and it shouldn't serve only one concrete device object, so it is not good to include it into 'struct device'. The only function pointer in struct device: void(*release)(struct device *dev) should be removed. All its users should convert to use device_type to define release handler for its 'device class', instead of device object. So suggest to improve it. Thanks, -- Ming Lei -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Do we need asynchronous pm_runtime_get()? (was: Re: bisected regression ...)
On Tuesday, August 07, 2012, Ming Lei wrote: On Mon, Aug 6, 2012 at 10:47 PM, Alan Stern st...@rowland.harvard.edu wrote: No, no, you have completely misunderstood the whole point of this change. Sorry, you are right. And the callback should be renamed as '.runtime_post_resume', which should do something I/O related in theory just after device becomes active. The idea is for func to be called at a time when it is known that the device is at full power. That means it _has_ to be called after the runtime_resume callback returns. Yes, I agree, but I don't think it may make .runtime_post_resume not doable, do I? No more device PM callbacks, please. Besides, callbacks in struct dev_pm_ops are not only for drivers. Also, func should not be stored in dev_pm_ops because it won't be a read-only value. Sorry, could you explain it in detail that why the 'func' has to switch to multiple functions dynamically? I understand one function is enough and sometimes it can be bypassed with one flag(such as, ignore_post_resume is introduced in dev_pm_info) set. Also, the driver can store the device specific states in its own device instance to deal with different situations in the callback. If the idea is doable, we can save one pointer in 'struct device', since the 'func' may not be used by more than 90% devices, also have document benefit, even may simplify implementation of the mechanism. And how many struct device objects there are for the one extra pointer to matter, let alone the fact that you want to replace it by one extra pointer somewhere else? Rafael -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Do we need asynchronous pm_runtime_get()? (was: Re: bisected regression ...)
On Monday, August 06, 2012, Rafael J. Wysocki wrote:
On Monday, August 06, 2012, Alan Stern wrote:
On Sun, 5 Aug 2012, Rafael J. Wysocki wrote:
[...]
What about the appended experimental patch?
For now, I think it would be best to start with a single func argument.
If it turns out that anyone really needs to have two separate
arguments, the single-argument form can be reimplemented on top of the
two-argument form easily enough.
All right.
@@ -484,6 +484,15 @@ static int rpm_suspend(struct device *de
goto out;
}
+void rpm_queue_up_resume(struct device *dev)
+{
+ dev-power.request = RPM_REQ_RESUME;
+ if (!dev-power.request_pending) {
+ dev-power.request_pending = true;
+ queue_work(pm_wq, dev-power.work);
+ }
+}
+
/**
* rpm_resume - Carry out runtime resume of given device.
* @dev: Device to resume.
@@ -524,7 +533,9 @@ static int rpm_resume(struct device *dev
* rather than cancelling it now only to restart it again in the near
* future.
*/
- dev-power.request = RPM_REQ_NONE;
+ if (dev-power.request != RPM_REQ_RESUME || !dev-power.func)
+ dev-power.request = RPM_REQ_NONE;
+
if (!dev-power.timer_autosuspends)
pm_runtime_deactivate_timer(dev);
@@ -533,6 +544,12 @@ static int rpm_resume(struct device *dev
goto out;
}
+ if (dev-power.func (rpmflags RPM_ASYNC)) {
+ rpm_queue_up_resume(dev);
+ retval = 0;
+ goto out;
+ }
+
if (dev-power.runtime_status == RPM_RESUMING
|| dev-power.runtime_status == RPM_SUSPENDING) {
DEFINE_WAIT(wait);
All those changes (and some of the following ones) are symptoms of a
basic mistake in this approach.
Every time you say something like this (i.e. liks someone who knows better)
s/liks/like/
I kind of feel like being under attach, which I hope is not your intention.
s/attach/attack/
Two typos in one sentence, I guess it could have been worse ...
Never mind. :-)
Those changes are there for different reasons rather unrelated to the way
func() is being called, so let me explain.
First, rpm_queue_up_resume() is introduced, because the code it contains will
have to be called in two different places. At least I don't see how to avoid
that without increasing the code complexity too much.
Second, the following change in rpm_resume()
- dev-power.request = RPM_REQ_NONE;
+ if (dev-power.request != RPM_REQ_RESUME || !dev-power.func)
+ dev-power.request = RPM_REQ_NONE;
is made to prevent rpm_resume() from canceling the execution of func() queued
up by an earlier pm_runtime_get_and_call(). I believe it is necessary.
Finally, this change in rpm_resume():
+ if (dev-power.func (rpmflags RPM_ASYNC)) {
+ rpm_queue_up_resume(dev);
+ retval = 0;
+ goto out;
+ }
is not strictly necessary if pm_runtime_get_and_call() is modified to run
rpm_queue_up_resume() directly, like in the new version of the patch which is
appended. This reduces the number of checks overall, so perhaps it's better.
The idea of this new feature is to
call func as soon as we know the device is at full power, no matter
how it got there.
Yes, it is so.
That means we should call it near the end of
rpm_resume() (just before the rpm_idle() call), not from within
pm_runtime_work().
Doing it this way will be more efficient and (I think) will remove
some races.
Except that func() shouldn't be executed under dev-power.lock, which makes it
rather difficult to call it from rpm_resume(). Or at least it seems so.
Moreover, it should be called after we've changed the status to RPM_ACTIVE
_and_ dropped the lock.
So we could drop the lock right before returning, execute func() and acquire
the lock once again, but then func() might be executed by any thread that
happened to resume the device. In that case the caller of
pm_runtime_get_and_call() would have to worry about locks that such threads
might acquire and it would have to make sure that func() didn't try to acquire
them too. That may not be a big deal, but if func() is executed by
pm_runtime_work(), that issue simply goes away.
Then, however, there's another issue: what should happen if
pm_runtime_get_and_call() finds that it cannot execute func() right away,
so it queues up resume and the execution of it, in the meantime some other
thread resumes the device synchronously and pm_runtime_get_and_call() is
run again. I think in that case func() should be executed synchronously
and the one waiting for execution should be canceled. The alternative
would be to return -EAGAIN from pm_runtime_get_and_call() and expect the
caller to cope with that, which isn't too attractive.
This actually is analogous to the case when pm_runtime_get_and_call()
sees that power.func is not NULL. In my
Re: Do we need asynchronous pm_runtime_get()? (was: Re: bisected regression ...)
On Tue, 7 Aug 2012, Ming Lei wrote: On Tue, Aug 7, 2012 at 7:23 PM, Rafael J. Wysocki r...@sisk.pl wrote: Yes, I agree, but I don't think it may make .runtime_post_resume not doable, do I? No more device PM callbacks, please. IMO, what the patch is doing is to introduce one callback which is just called after .runtime_resume is completed, and you want to move something out of previous .runtime_resume and do it in another new callback to speedup resume, so it should be reasonable to introduce the .runtime_post_resume callback in logic. No, that's really not what the patch is doing. The idea behind the new API is that func will be called as soon as we know the device is at full power. That could be after the next runtime resume or it could be right away. This is a one-time call; it should not be made _every_ time the device resumes. Also, the 'func' should be per driver, not per device since only one 'func' is enough for all same kind of devices driven by one same driver. But what if the subsystem defines its own dev_pm_info structure? Then the driver's dev_pm_info will be ignored by the runtime PM core. All the subsystems would have to be changed. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Do we need asynchronous pm_runtime_get()? (was: Re: bisected regression ...)
On Tue, Aug 7, 2012 at 11:42 PM, Alan Stern st...@rowland.harvard.edu wrote: No, that's really not what the patch is doing. The idea behind the new API is that func will be called as soon as we know the device is at full power. That could be after the next runtime resume or it could be right away. This is a one-time call; it should IMO, in the both two cases, the 'func' should be very similar with .runtime_post_resume from view of the caller because the caller don't know what the power state of the device is now, so they may always think the 'func' should do something similar done in .runtime_post_resume. Also .runtime_post_resume always knows the device's power state is active, which is same with 'func'. In fact, it doesn't matter if the active state is the 1st time or other times, does it? not be made _every_ time the device resumes. Suppose the device is always resumed in the path(such as irq context), the callback is still called every time. If the .runtime_post_resume is to be a one-time call, that is easy to do it. Also I am wondering why the callback shouldn't be called after resume in sync context, and it may simplify implementation if the two contexts (sync vs. async) are kept consistent. Also, the 'func' should be per driver, not per device since only one 'func' is enough for all same kind of devices driven by one same driver. But what if the subsystem defines its own dev_pm_info structure? Then the driver's dev_pm_info will be ignored by the runtime PM core. All the subsystems would have to be changed. Suppose .runtime_post_resume is introduced, the priority of dev_pm_info for .runtime_post_resume callback can be changed to adapt to the situation. Thanks, -- Ming Lei -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Do we need asynchronous pm_runtime_get()? (was: Re: bisected regression ...)
On Tue, 7 Aug 2012, Rafael J. Wysocki wrote: All those changes (and some of the following ones) are symptoms of a basic mistake in this approach. Every time you say something like this (i.e. liks someone who knows better) s/liks/like/ I kind of feel like being under attach, which I hope is not your intention. s/attach/attack/ Sorry; you're right. It's all too easy to get very arrogant in email messages. I'll try not to attack so strongly in the future. The idea of this new feature is to call func as soon as we know the device is at full power, no matter how it got there. Yes, it is so. Incidentally, that sentence is the justification for the invariance condition mentioned later. power.func should be called as soon as we know the device is at full power; therefore when the status changes to RPM_ACTIVE it should be called and then cleared (if it was set), and it should never get set while the status is RPM_ACTIVE. Therefore it should never be true that power.func is set _and_ the status is RPM_ACTIVE. That means we should call it near the end of rpm_resume() (just before the rpm_idle() call), not from within pm_runtime_work(). Doing it this way will be more efficient and (I think) will remove some races. Except that func() shouldn't be executed under dev-power.lock, which makes it rather difficult to call it from rpm_resume(). Or at least it seems so. Moreover, it should be called after we've changed the status to RPM_ACTIVE _and_ dropped the lock. So we could drop the lock right before returning, execute func() and acquire the lock once again, Yes; that's what I had in mind. We already do something similar when calling pm_runtime_put(parent). but then func() might be executed by any thread that happened to resume the device. In that case the caller of pm_runtime_get_and_call() would have to worry about locks that such threads might acquire and it would have to make sure that func() didn't try to acquire them too. That may not be a big deal, but if func() is executed by pm_runtime_work(), that issue simply goes away. But then you have to worry about races between pm_runtime_resume() and the workqueue. If the device is resumed by some other thread, it could be suspended again before func is called. Then, however, there's another issue: what should happen if pm_runtime_get_and_call() finds that it cannot execute func() right away, so it queues up resume and the execution of it, in the meantime some other thread resumes the device synchronously and pm_runtime_get_and_call() is run again. I think in that case func() should be executed synchronously and the one waiting for execution should be canceled. The alternative would be to return -EAGAIN from pm_runtime_get_and_call() and expect the caller to cope with that, which isn't too attractive. This actually is analogous to the case when pm_runtime_get_and_call() sees that power.func is not NULL. In my experimental patches it returned -EAGAIN in that case, but perhaps it's better to replace the existing power.func with the new one. Then, by doing pm_runtime_get_and_call(dev, NULL) we can ensure that either the previous func() has run already or it will never run, which may be useful. A good point. I agree that pm_runtime_get_and_call() should always overwrite the existing power.func value. There are a couple of other issues remaining. What's the best approach when disable_count 0? My feeling is that we should still rely on power.runtime_status as the best approximation to the device's state, so we shouldn't call func directly unless the status is already RPM_ACTIVE. If the status is something else, we can't queue an async resume request. So we just set power.func and return. Eventually the driver will either call pm_runtime_set_active() or pm_runtime_enable() followed by pm_runtime_resume(), at which time we would call power.func. Also, what should happen when power.runtime_error is set? The same as when disable_depth 0? You mentioned that pm_runtime_disable() does a resume if there's a pending resume request. I had forgotten about this. It worries me, because subsystems use code sequences like this: pm_runtime_disable(dev); pm_runtime_set_active(dev); pm_runtime_enable(dev); in their system resume routines (in fact, we advise them to do so in the Documentation file). Now, it is unlikely for a resume request to be pending during system sleep, but it doesn't seem to be impossible. When there is such a pending request, the pm_runtime_disable() call will try to do a runtime resume at a time when the device has just been restored to full power. That's not good. Probably this pattern occurs in few enough places that we could go through and fix them all. But how? Should there be a new function: pm_adjust_runtime_status_after_system_resume()? Alan Stern -- To unsubscribe from this list: send the
Re: Do we need asynchronous pm_runtime_get()? (was: Re: bisected regression ...)
On Tuesday, August 07, 2012, Ming Lei wrote: On Tue, Aug 7, 2012 at 7:23 PM, Rafael J. Wysocki r...@sisk.pl wrote: Yes, I agree, but I don't think it may make .runtime_post_resume not doable, do I? No more device PM callbacks, please. IMO, what the patch is doing is to introduce one callback which is just called after .runtime_resume is completed, No, it is not a callback. It is a function to be run _once_ when the device is known to be active. It is not a member of a data type or anything like this. It's kind of disappointing that you don't see a difference between that and a callback. and you want to move something out of previous .runtime_resume No, I don't. Where did you get that idea from? and do it in another new callback to speedup resume, No, not to speed up resume. The idea is to allow drivers to run something when the resume is complete, so that they don't have to implement a resume detection logic or use .runtime_resume() to run things that don't belong there. so it should be reasonable to introduce the .runtime_post_resume callback in logic. No. This doesn't have anything to do with callbacks! If you want a new callback, you should specify what the role of this callback is, otherwise it is not well defined. I this case, though, what the role of func() is depends on the caller and most likely every driver would use it for something different. So no, I don't see how it can be a callback. Also, the 'func' should be per driver, not per device since only one 'func' is enough for all same kind of devices driven by one same driver. It isn't per device! It is per _caller_. The fact that the pointer is stored _temporarily_ in struct device doesn't mean that it is per device and that it is a callback. From the struct device point of view it is _data_, not a member function. Besides, callbacks in struct dev_pm_ops are not only for drivers. All the current 3 runtime callbacks are for devices. If you mean they can be defined in bus/power_domain/device_type, .runtime_post_resume still can be defined there too. No, it wouldn't make _any_ _sense_ there, because its role there cannot be defined in any sane way. Also, func should not be stored in dev_pm_ops because it won't be a read-only value. Sorry, could you explain it in detail that why the 'func' has to switch to multiple functions dynamically? I understand one function is enough and sometimes it can be bypassed with one flag(such as, ignore_post_resume is introduced in dev_pm_info) set. Also, the driver can store the device specific states in its own device instance to deal with different situations in the callback. If the idea is doable, we can save one pointer in 'struct device', since the 'func' may not be used by more than 90% devices, also have document benefit, even may simplify implementation of the mechanism. And how many struct device objects there are for the one extra pointer to matter, let alone the fact that you want to replace it by one extra pointer somewhere else? For example, in the pandaboard(one omap4 based small system), follows the count of device instances: [root@root]#dmesg | grep device_add | wc -l 471 The above is just a simple configuration(no graphics, no video/video, only console enabled) on Pandaboard. If the callback may be defined in dev_pm_info, I guess you mean struct dev_pm_ops, right? not only memory can be saved, also there are other advantages described before. So now please count how many struct dev_pm_ops objects there are on that system and compute the differece. And please note that drivers that don't use struct dev_pm_ops for power management will do that in the future. Also please note that the caller of pm_runtime_get_and_call() need not be a driver, at least in theory. Thanks, Rafael -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Do we need asynchronous pm_runtime_get()? (was: Re: bisected regression ...)
On Tuesday, August 07, 2012, Ming Lei wrote: On Tue, Aug 7, 2012 at 11:42 PM, Alan Stern st...@rowland.harvard.edu wrote: No, that's really not what the patch is doing. The idea behind the new API is that func will be called as soon as we know the device is at full power. That could be after the next runtime resume or it could be right away. This is a one-time call; it should IMO, in the both two cases, the 'func' should be very similar with .runtime_post_resume from view of the caller because the caller don't know what the power state of the device is now, so they may always think the 'func' should do something similar done in .runtime_post_resume. Also .runtime_post_resume always knows the device's power state is active, which is same with 'func'. In fact, it doesn't matter if the active state is the 1st time or other times, does it? What Alan wanted to say, I think, was that .runtime_post_resume() would have to be always identical, where func() need not be always the same function. Moreover, .runtime_post_resume() would _always_ be run after a device resume, while func() is run only _once_. not be made _every_ time the device resumes. Suppose the device is always resumed in the path(such as irq context), the callback is still called every time. Yes, but what if you have _two_ code paths and you want to call different code as func() in each of them? If the .runtime_post_resume is to be a one-time call, that is easy to do it. No, it isn't. Also I am wondering why the callback shouldn't be called after resume in sync context, and it may simplify implementation if the two contexts (sync vs. async) are kept consistent. I have no idea what you're talking about. We actually have a callback that is run every time a device is resumed. It is called .runtime_idle(). Does it help, though? No, it doesn't. Also, the 'func' should be per driver, not per device since only one 'func' is enough for all same kind of devices driven by one same driver. But what if the subsystem defines its own dev_pm_info structure? Then the driver's dev_pm_info will be ignored by the runtime PM core. All the subsystems would have to be changed. Suppose .runtime_post_resume is introduced, It is not going to be introduced, period. Thanks, Rafael -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Do we need asynchronous pm_runtime_get()? (was: Re: bisected regression ...)
On Tuesday, August 07, 2012, Alan Stern wrote: On Tue, 7 Aug 2012, Rafael J. Wysocki wrote: All those changes (and some of the following ones) are symptoms of a basic mistake in this approach. Every time you say something like this (i.e. liks someone who knows better) s/liks/like/ I kind of feel like being under attach, which I hope is not your intention. s/attach/attack/ Sorry; you're right. It's all too easy to get very arrogant in email messages. I'll try not to attack so strongly in the future. Thanks! The idea of this new feature is to call func as soon as we know the device is at full power, no matter how it got there. Yes, it is so. Incidentally, that sentence is the justification for the invariance condition mentioned later. :-) power.func should be called as soon as we know the device is at full power; therefore when the status changes to RPM_ACTIVE it should be called and then cleared (if it was set), and it should never get set while the status is RPM_ACTIVE. Therefore it should never be true that power.func is set _and_ the status is RPM_ACTIVE. I guess with the patch I've just sent: http://marc.info/?l=linux-pmm=134437366811066w=4 it's almost the case, except when a synchronous resume happens before the work item scheduled by __pm_runtime_get_and_call() is run. However, I don't think it is a problem in that case, because the device won't be suspended before the execution of that work item starts (rpm_check_suspend_allowed() will see that power.request_pending is set and that power.request is RPM_REQ_RESUME, so it will return -EAGAIN). That means we should call it near the end of rpm_resume() (just before the rpm_idle() call), not from within pm_runtime_work(). Doing it this way will be more efficient and (I think) will remove some races. Except that func() shouldn't be executed under dev-power.lock, which makes it rather difficult to call it from rpm_resume(). Or at least it seems so. Moreover, it should be called after we've changed the status to RPM_ACTIVE _and_ dropped the lock. So we could drop the lock right before returning, execute func() and acquire the lock once again, Yes; that's what I had in mind. We already do something similar when calling pm_runtime_put(parent). Yes, we do. However, I still don't think it's really safe to call func() from rpm_resume(), because it may be run synchronously from a context quite unrelated to the caller of __pm_runtime_get_and_call() (for example, from the pm_runtime_barrier() in __device_suspend()). but then func() might be executed by any thread that happened to resume the device. In that case the caller of pm_runtime_get_and_call() would have to worry about locks that such threads might acquire and it would have to make sure that func() didn't try to acquire them too. That may not be a big deal, but if func() is executed by pm_runtime_work(), that issue simply goes away. But then you have to worry about races between pm_runtime_resume() and the workqueue. If the device is resumed by some other thread, it could be suspended again before func is called. No, it can't, if the device's usage count is incremented before dropping power.lock after rpm_resume(dev, 0) has returned. Then, however, there's another issue: what should happen if pm_runtime_get_and_call() finds that it cannot execute func() right away, so it queues up resume and the execution of it, in the meantime some other thread resumes the device synchronously and pm_runtime_get_and_call() is run again. I think in that case func() should be executed synchronously and the one waiting for execution should be canceled. The alternative would be to return -EAGAIN from pm_runtime_get_and_call() and expect the caller to cope with that, which isn't too attractive. This actually is analogous to the case when pm_runtime_get_and_call() sees that power.func is not NULL. In my experimental patches it returned -EAGAIN in that case, but perhaps it's better to replace the existing power.func with the new one. Then, by doing pm_runtime_get_and_call(dev, NULL) we can ensure that either the previous func() has run already or it will never run, which may be useful. A good point. I agree that pm_runtime_get_and_call() should always overwrite the existing power.func value. There are a couple of other issues remaining. What's the best approach when disable_count 0? My feeling is that we should still rely on power.runtime_status as the best approximation to the device's state, so we shouldn't call func directly unless the status is already RPM_ACTIVE. Well, that's one possibility. In that case, though, the caller may want to run func() regardless of whether or not runtime PM is enabled for the given device and that would require some serious trickery. For this reason, in the
Re: Do we need asynchronous pm_runtime_get()? (was: Re: bisected regression ...)
On Wed, Aug 8, 2012 at 4:45 AM, Rafael J. Wysocki r...@sisk.pl wrote: On Tuesday, August 07, 2012, Ming Lei wrote: On Tue, Aug 7, 2012 at 7:23 PM, Rafael J. Wysocki r...@sisk.pl wrote: Yes, I agree, but I don't think it may make .runtime_post_resume not doable, do I? No more device PM callbacks, please. IMO, what the patch is doing is to introduce one callback which is just called after .runtime_resume is completed, No, it is not a callback. It is a function to be run _once_ when the device is known to be active. It is not a member of a data type or anything like this. Looks it was said by Alan, not me, :-) The documentation should explain that in some ways, func is like a workqueue callback routine:. See http://marc.info/?l=linux-usbm=134426838507799w=2 It's kind of disappointing that you don't see a difference between that and a callback. and you want to move something out of previous .runtime_resume No, I don't. Where did you get that idea from? If so, I am wondering why the 'func' can't be called in .runtime_resume directly, and follow the below inside caller at the same time? if (device is active or disabled) call func(device). and do it in another new callback to speedup resume, No, not to speed up resume. The idea is to allow drivers to run something when the resume is complete, so that they don't have to implement a resume detection logic or use .runtime_resume() to run things that don't belong there. Looks it was said by you, :-) Unless your _driver_ callback is actually executed from within a PM domain callback, for example, and something else may be waiting for it to complete, so your data processing is adding latencies to some other threads. I'm not making that up, by the way, that really can happen. See http://marc.info/?l=linux-pmm=134394271517527w=2 Alan also said Okay, those are valid reasons for the idea. Except for this one, I don't see other obvious advantages about the patch. so it should be reasonable to introduce the .runtime_post_resume callback in logic. No. This doesn't have anything to do with callbacks! If you want a new callback, you should specify what the role of this callback is, otherwise it is not well defined. I this case, though, what the role of func() is depends on the caller and most likely every driver would use it for something different. So no, I don't see how it can be a callback. Also, the 'func' should be per driver, not per device since only one 'func' is enough for all same kind of devices driven by one same driver. It isn't per device! It is per _caller_. The fact that the pointer is stored _temporarily_ in struct device doesn't mean that it is per device and that it is a callback. From the struct device point of view it is _data_, not a member function. The fact is that it will become per-device one you store it in 'struct device'. Suppose one driver may drive 1 same devices, the same data will be stored inside all the 1 device instances, it is a good way to do it? Not mention 90% devices mayn't use the _temporarily_ data at all. Besides, callbacks in struct dev_pm_ops are not only for drivers. All the current 3 runtime callbacks are for devices. If you mean they can be defined in bus/power_domain/device_type, .runtime_post_resume still can be defined there too. No, it wouldn't make _any_ _sense_ there, because its role there cannot be defined in any sane way. Also, func should not be stored in dev_pm_ops because it won't be a read-only value. Sorry, could you explain it in detail that why the 'func' has to switch to multiple functions dynamically? I understand one function is enough and sometimes it can be bypassed with one flag(such as, ignore_post_resume is introduced in dev_pm_info) set. Also, the driver can store the device specific states in its own device instance to deal with different situations in the callback. If the idea is doable, we can save one pointer in 'struct device', since the 'func' may not be used by more than 90% devices, also have document benefit, even may simplify implementation of the mechanism. And how many struct device objects there are for the one extra pointer to matter, let alone the fact that you want to replace it by one extra pointer somewhere else? For example, in the pandaboard(one omap4 based small system), follows the count of device instances: [root@root]#dmesg | grep device_add | wc -l 471 The above is just a simple configuration(no graphics, no video/video, only console enabled) on Pandaboard. If the callback may be defined in dev_pm_info, I guess you mean struct dev_pm_ops, right? Sorry, it is a typo. not only memory can be saved, also there are other advantages described before. So now please count how many struct dev_pm_ops objects there are on that system and compute the differece. And please note
Re: Do we need asynchronous pm_runtime_get()? (was: Re: bisected regression ...)
On Sun, Aug 5, 2012 at 11:26 PM, Rafael J. Wysocki r...@sisk.pl wrote:
---
drivers/base/power/runtime.c | 82
+++
include/linux/pm.h |1
include/linux/pm_runtime.h | 14 +++
3 files changed, 90 insertions(+), 7 deletions(-)
Index: linux/drivers/base/power/runtime.c
===
--- linux.orig/drivers/base/power/runtime.c
+++ linux/drivers/base/power/runtime.c
@@ -484,6 +484,15 @@ static int rpm_suspend(struct device *de
goto out;
}
+void rpm_queue_up_resume(struct device *dev)
+{
+ dev-power.request = RPM_REQ_RESUME;
+ if (!dev-power.request_pending) {
+ dev-power.request_pending = true;
+ queue_work(pm_wq, dev-power.work);
+ }
+}
+
/**
* rpm_resume - Carry out runtime resume of given device.
* @dev: Device to resume.
@@ -524,7 +533,9 @@ static int rpm_resume(struct device *dev
* rather than cancelling it now only to restart it again in the near
* future.
*/
- dev-power.request = RPM_REQ_NONE;
+ if (dev-power.request != RPM_REQ_RESUME || !dev-power.func)
+ dev-power.request = RPM_REQ_NONE;
+
if (!dev-power.timer_autosuspends)
pm_runtime_deactivate_timer(dev);
@@ -533,6 +544,12 @@ static int rpm_resume(struct device *dev
goto out;
}
+ if (dev-power.func (rpmflags RPM_ASYNC)) {
+ rpm_queue_up_resume(dev);
+ retval = 0;
+ goto out;
+ }
+
if (dev-power.runtime_status == RPM_RESUMING
|| dev-power.runtime_status == RPM_SUSPENDING) {
DEFINE_WAIT(wait);
@@ -591,11 +608,7 @@ static int rpm_resume(struct device *dev
/* Carry out an asynchronous or a synchronous resume. */
if (rpmflags RPM_ASYNC) {
- dev-power.request = RPM_REQ_RESUME;
- if (!dev-power.request_pending) {
- dev-power.request_pending = true;
- queue_work(pm_wq, dev-power.work);
- }
+ rpm_queue_up_resume(dev);
retval = 0;
goto out;
}
@@ -691,6 +704,7 @@ static int rpm_resume(struct device *dev
static void pm_runtime_work(struct work_struct *work)
{
struct device *dev = container_of(work, struct device, power.work);
+ void (*func)(struct device *) = NULL;
enum rpm_request req;
spin_lock_irq(dev-power.lock);
@@ -715,12 +729,24 @@ static void pm_runtime_work(struct work_
rpm_suspend(dev, RPM_NOWAIT | RPM_AUTO);
break;
case RPM_REQ_RESUME:
- rpm_resume(dev, RPM_NOWAIT);
+ func = dev-power.func;
+ if (func) {
+ dev-power.func = NULL;
+ pm_runtime_get_noresume(dev);
+ rpm_resume(dev, 0);
+ } else {
+ rpm_resume(dev, RPM_NOWAIT);
+ }
break;
}
out:
spin_unlock_irq(dev-power.lock);
+
+ if (func) {
+ func(dev);
+ pm_runtime_put(dev);
+ }
}
/**
@@ -877,6 +903,48 @@ int __pm_runtime_resume(struct device *d
}
EXPORT_SYMBOL_GPL(__pm_runtime_resume);
+int pm_runtime_get_and_call(struct device *dev, void (*func)(struct device
*),
+void (*func_async)(struct device *))
+{
+ unsigned long flags;
+ int ret;
+
+ atomic_inc(dev-power.usage_count);
+
+ spin_lock_irqsave(dev-power.lock, flags);
+
+ ret = dev-power.runtime_error;
+ if (ret) {
+ func = NULL;
+ goto out;
+ }
+
+ if (dev-power.runtime_status != RPM_ACTIVE
+dev-power.disable_depth == 0)
+ func = NULL;
Looks the above is a bit odd, and your motivation is to call 'func'
for a suspended and runtime-PM enabled device in irq context, isn't it?
+
+ if (!func func_async) {
+ if (dev-power.func) {
+ ret = -EAGAIN;
+ goto out;
+ } else {
+ dev-power.func = func_async;
+ }
+ }
+
+ rpm_resume(dev, RPM_ASYNC);
+
+ out:
+ spin_unlock_irqrestore(dev-power.lock, flags);
+
+ if (func) {
+ func(dev);
Maybe the return value should be passed to caller. Also the race between
'func' and its .runtime_resume callback should be stated in comment.
In fact, maybe it is better to call 'func' always first, then call
' rpm_resume(dev, RPM_ASYNC);', otherwise the driver may
be confused about the order between 'func' and its .runtime_resume
callback.
+ return 1;
+ }
+
+
Re: Do we need asynchronous pm_runtime_get()? (was: Re: bisected regression ...)
On Sun, 5 Aug 2012, Rafael J. Wysocki wrote:
I don't really think we'll need to store func_sync in dev-power. At least
I don't see a reason to do that at the moment.
You're right; I wasn't thinking straight.
I also think that func_sync() would have to be called if runtime PM is
disabled for the given device, so that callers don't have to check that
condition themselves.
Yes.
What about the appended experimental patch?
For now, I think it would be best to start with a single func argument.
If it turns out that anyone really needs to have two separate
arguments, the single-argument form can be reimplemented on top of the
two-argument form easily enough.
@@ -484,6 +484,15 @@ static int rpm_suspend(struct device *de
goto out;
}
+void rpm_queue_up_resume(struct device *dev)
+{
+ dev-power.request = RPM_REQ_RESUME;
+ if (!dev-power.request_pending) {
+ dev-power.request_pending = true;
+ queue_work(pm_wq, dev-power.work);
+ }
+}
+
/**
* rpm_resume - Carry out runtime resume of given device.
* @dev: Device to resume.
@@ -524,7 +533,9 @@ static int rpm_resume(struct device *dev
* rather than cancelling it now only to restart it again in the near
* future.
*/
- dev-power.request = RPM_REQ_NONE;
+ if (dev-power.request != RPM_REQ_RESUME || !dev-power.func)
+ dev-power.request = RPM_REQ_NONE;
+
if (!dev-power.timer_autosuspends)
pm_runtime_deactivate_timer(dev);
@@ -533,6 +544,12 @@ static int rpm_resume(struct device *dev
goto out;
}
+ if (dev-power.func (rpmflags RPM_ASYNC)) {
+ rpm_queue_up_resume(dev);
+ retval = 0;
+ goto out;
+ }
+
if (dev-power.runtime_status == RPM_RESUMING
|| dev-power.runtime_status == RPM_SUSPENDING) {
DEFINE_WAIT(wait);
All those changes (and some of the following ones) are symptoms of a
basic mistake in this approach. The idea of this new feature is to
call func as soon as we know the device is at full power, no matter
how it got there. That means we should call it near the end of
rpm_resume() (just before the rpm_idle() call), not from within
pm_runtime_work().
Doing it this way will be more efficient and (I think) will remove
some races.
@@ -877,6 +903,48 @@ int __pm_runtime_resume(struct device *d
}
EXPORT_SYMBOL_GPL(__pm_runtime_resume);
+int pm_runtime_get_and_call(struct device *dev, void (*func)(struct device
*),
+ void (*func_async)(struct device *))
+{
+ unsigned long flags;
+ int ret;
+
+ atomic_inc(dev-power.usage_count);
+
+ spin_lock_irqsave(dev-power.lock, flags);
+
+ ret = dev-power.runtime_error;
+ if (ret) {
+ func = NULL;
+ goto out;
+ }
+
+ if (dev-power.runtime_status != RPM_ACTIVE
+ dev-power.disable_depth == 0)
+ func = NULL;
+
+ if (!func func_async) {
+ if (dev-power.func) {
+ ret = -EAGAIN;
+ goto out;
+ } else {
+ dev-power.func = func_async;
+ }
+ }
The logic here is kind of hard to follow. It will be simpler when
there's only one func:
If the status is RPM_ACTIVE or disable_depth 0 then
call func directly.
Otherwise save func in dev.power and do an async resume.
Some more things:
In __pm_runtime_set_status(), if power.func is set then I think we
should call it if the new status is ACTIVE. Likwise at the end of
rpm_suspend(), if the suspend fails. There should be an invariant:
Whenever the status is RPM_ACTIVE, power.func must be NULL.
pm_runtime_barrier() should always clear power.func, even if the
rpm_resume() call fails.
The documentation should explain that in some ways, func is like a
workqueue callback routine: Subsystems and drivers have to be careful
to make sure that it can't be invoked after the device is unbound. A
safe way to do this is to call pm_runtime_barrier() from within the
driver's .remove() routine, after making sure that
pm_runtime_get_and_call() won't be invoked any more.
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Do we need asynchronous pm_runtime_get()? (was: Re: bisected regression ...)
On Monday, August 06, 2012, Alan Stern wrote:
On Sun, 5 Aug 2012, Rafael J. Wysocki wrote:
[...]
What about the appended experimental patch?
For now, I think it would be best to start with a single func argument.
If it turns out that anyone really needs to have two separate
arguments, the single-argument form can be reimplemented on top of the
two-argument form easily enough.
All right.
@@ -484,6 +484,15 @@ static int rpm_suspend(struct device *de
goto out;
}
+void rpm_queue_up_resume(struct device *dev)
+{
+ dev-power.request = RPM_REQ_RESUME;
+ if (!dev-power.request_pending) {
+ dev-power.request_pending = true;
+ queue_work(pm_wq, dev-power.work);
+ }
+}
+
/**
* rpm_resume - Carry out runtime resume of given device.
* @dev: Device to resume.
@@ -524,7 +533,9 @@ static int rpm_resume(struct device *dev
* rather than cancelling it now only to restart it again in the near
* future.
*/
- dev-power.request = RPM_REQ_NONE;
+ if (dev-power.request != RPM_REQ_RESUME || !dev-power.func)
+ dev-power.request = RPM_REQ_NONE;
+
if (!dev-power.timer_autosuspends)
pm_runtime_deactivate_timer(dev);
@@ -533,6 +544,12 @@ static int rpm_resume(struct device *dev
goto out;
}
+ if (dev-power.func (rpmflags RPM_ASYNC)) {
+ rpm_queue_up_resume(dev);
+ retval = 0;
+ goto out;
+ }
+
if (dev-power.runtime_status == RPM_RESUMING
|| dev-power.runtime_status == RPM_SUSPENDING) {
DEFINE_WAIT(wait);
All those changes (and some of the following ones) are symptoms of a
basic mistake in this approach.
Every time you say something like this (i.e. liks someone who knows better)
I kind of feel like being under attach, which I hope is not your intention.
Never mind. :-)
Those changes are there for different reasons rather unrelated to the way
func() is being called, so let me explain.
First, rpm_queue_up_resume() is introduced, because the code it contains will
have to be called in two different places. At least I don't see how to avoid
that without increasing the code complexity too much.
Second, the following change in rpm_resume()
- dev-power.request = RPM_REQ_NONE;
+ if (dev-power.request != RPM_REQ_RESUME || !dev-power.func)
+ dev-power.request = RPM_REQ_NONE;
is made to prevent rpm_resume() from canceling the execution of func() queued
up by an earlier pm_runtime_get_and_call(). I believe it is necessary.
Finally, this change in rpm_resume():
+ if (dev-power.func (rpmflags RPM_ASYNC)) {
+ rpm_queue_up_resume(dev);
+ retval = 0;
+ goto out;
+ }
is not strictly necessary if pm_runtime_get_and_call() is modified to run
rpm_queue_up_resume() directly, like in the new version of the patch which is
appended. This reduces the number of checks overall, so perhaps it's better.
The idea of this new feature is to
call func as soon as we know the device is at full power, no matter
how it got there.
Yes, it is so.
That means we should call it near the end of
rpm_resume() (just before the rpm_idle() call), not from within
pm_runtime_work().
Doing it this way will be more efficient and (I think) will remove
some races.
Except that func() shouldn't be executed under dev-power.lock, which makes it
rather difficult to call it from rpm_resume(). Or at least it seems so.
Moreover, it should be called after we've changed the status to RPM_ACTIVE
_and_ dropped the lock.
Besides, I'd like to know what races you're referring to (perhaps you're seeing
some more of them than I am).
@@ -877,6 +903,48 @@ int __pm_runtime_resume(struct device *d
}
EXPORT_SYMBOL_GPL(__pm_runtime_resume);
+int pm_runtime_get_and_call(struct device *dev, void (*func)(struct device
*),
+void (*func_async)(struct device *))
+{
+ unsigned long flags;
+ int ret;
+
+ atomic_inc(dev-power.usage_count);
+
+ spin_lock_irqsave(dev-power.lock, flags);
+
+ ret = dev-power.runtime_error;
+ if (ret) {
+ func = NULL;
+ goto out;
+ }
+
+ if (dev-power.runtime_status != RPM_ACTIVE
+dev-power.disable_depth == 0)
+ func = NULL;
+
+ if (!func func_async) {
+ if (dev-power.func) {
+ ret = -EAGAIN;
+ goto out;
+ } else {
+ dev-power.func = func_async;
+ }
+ }
The logic here is kind of hard to follow. It will be simpler when
there's only one func:
If the status is RPM_ACTIVE or disable_depth 0 then
call func directly.
Otherwise save func in dev.power and do an async resume.
Yeah. But do not run func() under power.lock, right?
Some more things:
In
Re: Do we need asynchronous pm_runtime_get()? (was: Re: bisected regression ...)
On Mon, Aug 6, 2012 at 10:47 PM, Alan Stern st...@rowland.harvard.edu wrote: No, no, you have completely misunderstood the whole point of this change. Sorry, you are right. And the callback should be renamed as '.runtime_post_resume', which should do something I/O related in theory just after device becomes active. The idea is for func to be called at a time when it is known that the device is at full power. That means it _has_ to be called after the runtime_resume callback returns. Yes, I agree, but I don't think it may make .runtime_post_resume not doable, do I? Also, func should not be stored in dev_pm_ops because it won't be a read-only value. Sorry, could you explain it in detail that why the 'func' has to switch to multiple functions dynamically? I understand one function is enough and sometimes it can be bypassed with one flag(such as, ignore_post_resume is introduced in dev_pm_info) set. Also, the driver can store the device specific states in its own device instance to deal with different situations in the callback. If the idea is doable, we can save one pointer in 'struct device', since the 'func' may not be used by more than 90% devices, also have document benefit, even may simplify implementation of the mechanism. Thanks, -- Ming Lei -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Do we need asynchronous pm_runtime_get()? (was: Re: bisected regression ...)
On Friday, August 03, 2012, Alan Stern wrote: On Thu, 2 Aug 2012, Rafael J. Wysocki wrote: I don't know about that -- the logic involved in doing the processing within the resume callback isn't terribly complicated. At least, not much more complicated than the logic involved in setting up a custom work routine as you suggest. That's why I had the idea of pm_request_resume_and_call(dev, func) described in this message: http://marc.info/?l=linux-usbm=134377126804170w=4 although perheps it would be better to call it something like pm_runtime_async_resume_and_call(dev, func). Hmmm. You'd probably want a version that does a get at the same time. I suppose you would call func directly if the device was already resumed, without going through the workqueue? Yes. Yes, I agree this would be a better interface then pm_runtime_get. OK Well, that shouldn't need the is_suspended flag at all, methinks, and the reason it does need it is because it uses pm_runtime_get(). Not so. Consider your scheme. When starting an I/O request, you call pm_runtime_get_noresume() and then check to see if the device is active, say by calling pm_runtime_suspended(). Suppose at that moment the suspend callback has just finished and has released the private spinlock. The device's status is still RPM_SUSPENDING, so pm_runtime_suspended() returns 0 and you try to carry out the I/O. To fix this problem you have to synchronize the status checking with the suspend/resume operations. This means the status changes have to occur under the protection of the private lock, which means a private flag is needed. What about checking if the status is RPM_ACTIVE under dev-power.lock? That's what rpm_resume() does, more or less. That wouldn't solve the race described above. Moreover, processing requests in the resume callback is not something I'd recommend to anyone, because that's going to happen when the status of the device is RPM_RESUMING, we're holding a reference on the parent (if there's one) etc. I don't see any problem with that. The parent's child_count will be incremented while the requests are being processed regardless. And if you've been waiting for the device to resume in order to carry out some processing, within the resume callback is the logical place to do the work. Unless your _driver_ callback is actually executed from within a PM domain callback, for example, and something else may be waiting for it to complete, so your data processing is adding latencies to some other threads. I'm not making that up, by the way, that really can happen. And what if you are a parent waited for by a child to resume so that _it_ can process its data? Would you still want to process your data in the resume callback in that case? Okay, those are valid reasons. (Although a device handling I/O requests isn't likely to have a child with its own independent I/O handling.) I agree that this isn't very likely in practice. I suppose we could keep pm_runtime_get_sync as is, and just change pm_runtime_get to pm_runtime_get_async (and likewise for _put). That could reduce the confusion during the changeover. Changing pm_runtime_get() to pm_runtime_get_async() would be an improvement, although pm_runtime_get_but_do_not_resume_immediately() might even be better. Or even pm_runtime_get_but_do_not_access_hardware_when_it_returns(). ;-) I see no reason to have any of those things, though. Yes, they _may_ be useful to someone knowing the runtime PM core internals to save a few lines of code in some places, but generally they lead people to make serious mistakes that tend to be difficult to debug. For this very reason pm_runtime_get() is a bad interface and I wish we hadn't introduced it at all. Even if we give it a more descriptive name, it won't be much better. And note how that doesn't apply to the pm_runtime_put*() family. After all, doing pm_runtime_put() instead of pm_runtime_put_sync() will never lead to accessing registers of a suspended device. All right. But I still think pm_runtime_put_async is better than pm_runtime_put. At least it forces people to think about what they're doing. I agree. My current plan is to provide a better alternative interface, then to change the name of pm_runtime_put to pm_runtime_put_async and to document that it's going to be deprecated in future. Thanks, Rafael -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Do we need asynchronous pm_runtime_get()? (was: Re: bisected regression ...)
On Friday, August 03, 2012, Alan Stern wrote: On Thu, 2 Aug 2012, Alan Stern wrote: On Thu, 2 Aug 2012, Rafael J. Wysocki wrote: I don't know about that -- the logic involved in doing the processing within the resume callback isn't terribly complicated. At least, not much more complicated than the logic involved in setting up a custom work routine as you suggest. That's why I had the idea of pm_request_resume_and_call(dev, func) described in this message: http://marc.info/?l=linux-usbm=134377126804170w=4 although perheps it would be better to call it something like pm_runtime_async_resume_and_call(dev, func). Hmmm. You'd probably want a version that does a get at the same time. I suppose you would call func directly if the device was already resumed, without going through the workqueue? Yes, I agree this would be a better interface then pm_runtime_get. A few problems: Which are good points. :-) What happens if a system sleep begins after pm_runtime_async_resume_and_call() but before the runtime resume? When the system wakes up, the device will be back at full power without ever executing a runtime PM call. Then how will func get invoked? The pm_runtime_barrier() at the beginning of __device_suspend() guarantees that if there's a resume request pending at this point, the resume will be carried out. I think we can modify rpm_resume() to return only after func() is executed, so there shouldn't be a problem with that particular case. What happens if the driver is unbound after pm_runtime_async_resume_and_call() but before the runtime resume? The When the runtime resume occurs, func will be invoked -- and the driver might not even be in memory any more. We have no protection against that at the moment, but then there is no guarantee that the driver's runtime PM callbacks won't be run after it's been unbound. Perhaps we should run pm_runtime_barrier() before executing .remove() in __device_release_driver() (and perhaps run pm_runtime_idle() afterwards in case the subsystem or PM domain knows how to power down the device even if there's no driver) and expect .remove() to clean up? Rafael -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Do we need asynchronous pm_runtime_get()? (was: Re: bisected regression ...)
On Sat, 4 Aug 2012, Rafael J. Wysocki wrote: On Friday, August 03, 2012, Ming Lei wrote: On Fri, Aug 3, 2012 at 10:20 AM, Alan Stern st...@rowland.harvard.edu wrote: On Thu, 2 Aug 2012, Rafael J. Wysocki wrote: Hmmm. You'd probably want a version that does a get at the same time. I suppose you would call func directly if the device was already resumed, without going through the workqueue? Maybe it isn't good, the 'func' might not be run in the current context (irq context or some spinlock is held). Then I'd say don't use this interface. If you have code that needs to be run in a different context, then you have to use a work item (or equivalent) anyway and you can do a synchronous runtime resume from there. That wasn't what he meant. What if the code needs to run in the _same_ context as the caller? For example, with a spinlock held. func would need to know whether it should acquire the lock, which means it needs to know whether it was called directly or through the workqueue. I suppose we could pass it an extra argument with this information... but that's a little ugly. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Do we need asynchronous pm_runtime_get()? (was: Re: bisected regression ...)
On Sat, 4 Aug 2012, Rafael J. Wysocki wrote: That wasn't what he meant. What if the code needs to run in the _same_ context as the caller? For example, with a spinlock held. I see. I think it wouldn't be unreasonable to require that func should take all of the necessary locks by itself. But then if func was called directly, because the device was at full power, it would deadlock trying to acquire a lock the caller already holds. However, I believe that if func is going to be called through the workqueue, the usage counter should be incremented before the preceding resume and decremented after func returns by the workqueue thread. Okay. func would need to know whether it should acquire the lock, which means it needs to know whether it was called directly or through the workqueue. I suppose we could pass it an extra argument with this information... but that's a little ugly. I agree. I'd prefer to avoid that, if possible. Do you have any better suggestions? Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Do we need asynchronous pm_runtime_get()? (was: Re: bisected regression ...)
On Saturday, August 04, 2012, Alan Stern wrote: On Sat, 4 Aug 2012, Rafael J. Wysocki wrote: That wasn't what he meant. What if the code needs to run in the _same_ context as the caller? For example, with a spinlock held. I see. I think it wouldn't be unreasonable to require that func should take all of the necessary locks by itself. But then if func was called directly, because the device was at full power, it would deadlock trying to acquire a lock the caller already holds. I wonder why the caller may want to take any locks beforehand? However, I believe that if func is going to be called through the workqueue, the usage counter should be incremented before the preceding resume and decremented after func returns by the workqueue thread. Okay. func would need to know whether it should acquire the lock, which means it needs to know whether it was called directly or through the workqueue. I suppose we could pass it an extra argument with this information... but that's a little ugly. I agree. I'd prefer to avoid that, if possible. Do you have any better suggestions? Hmm. What about pm_runtime_get_and_call(dev, func_sync, func_async), where func_sync() is to be called if the device is already active and func_async() is to be called if it has to be resumed from the workqueue? Rafael -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Do we need asynchronous pm_runtime_get()? (was: Re: bisected regression ...)
On Thu, 2 Aug 2012, Alan Stern wrote: On Thu, 2 Aug 2012, Rafael J. Wysocki wrote: I don't know about that -- the logic involved in doing the processing within the resume callback isn't terribly complicated. At least, not much more complicated than the logic involved in setting up a custom work routine as you suggest. That's why I had the idea of pm_request_resume_and_call(dev, func) described in this message: http://marc.info/?l=linux-usbm=134377126804170w=4 although perheps it would be better to call it something like pm_runtime_async_resume_and_call(dev, func). Hmmm. You'd probably want a version that does a get at the same time. I suppose you would call func directly if the device was already resumed, without going through the workqueue? Yes, I agree this would be a better interface then pm_runtime_get. A few problems: What happens if a system sleep begins after pm_runtime_async_resume_and_call() but before the runtime resume? When the system wakes up, the device will be back at full power without ever executing a runtime PM call. Then how will func get invoked? What happens if the driver is unbound after pm_runtime_async_resume_and_call() but before the runtime resume? The When the runtime resume occurs, func will be invoked -- and the driver might not even be in memory any more. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Do we need asynchronous pm_runtime_get()? (was: Re: bisected regression ...)
On Fri, 3 Aug 2012, Ming Lei wrote: On Fri, Aug 3, 2012 at 10:20 AM, Alan Stern st...@rowland.harvard.edu wrote: On Thu, 2 Aug 2012, Rafael J. Wysocki wrote: Hmmm. You'd probably want a version that does a get at the same time. I suppose you would call func directly if the device was already resumed, without going through the workqueue? Maybe it isn't good, the 'func' might not be run in the current context (irq context or some spinlock is held). True. But we don't want to wait for the workqueue if the device is already at full power. Suggestions? And what if you are a parent waited for by a child to resume so that _it_ can process its data? Would you still want to process your data in the resume callback in that case? Looks the child is always resumed after its parent completes the resuming, and current pm_runtime_get doesn't delay the resume of the parent, and just make the .runtime_resume run in another context. I don't understand. Rafael's point was that if the parent's resume callback did a bunch of real work, it would delay resuming the child because the child can't be resumed until the parent's resume callback returns. Also are there actual examples about the above situation? I don't know of any. I suspect there aren't many examples. It wouldn't make much sense. IMO, the .runtime_resume callback can handle the IO things easily via scheduling worker function if the driver don't want to delay its children's resume. That was one of Rafael's other suggestions. But calling pm_request_resume_and_call() is easier than scheduling a worker function. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Do we need asynchronous pm_runtime_get()? (was: Re: bisected regression ...)
On Thu, 2 Aug 2012, Rafael J. Wysocki wrote: I don't know about that -- the logic involved in doing the processing within the resume callback isn't terribly complicated. At least, not much more complicated than the logic involved in setting up a custom work routine as you suggest. That's why I had the idea of pm_request_resume_and_call(dev, func) described in this message: http://marc.info/?l=linux-usbm=134377126804170w=4 although perheps it would be better to call it something like pm_runtime_async_resume_and_call(dev, func). Hmmm. You'd probably want a version that does a get at the same time. I suppose you would call func directly if the device was already resumed, without going through the workqueue? Yes, I agree this would be a better interface then pm_runtime_get. Well, that shouldn't need the is_suspended flag at all, methinks, and the reason it does need it is because it uses pm_runtime_get(). Not so. Consider your scheme. When starting an I/O request, you call pm_runtime_get_noresume() and then check to see if the device is active, say by calling pm_runtime_suspended(). Suppose at that moment the suspend callback has just finished and has released the private spinlock. The device's status is still RPM_SUSPENDING, so pm_runtime_suspended() returns 0 and you try to carry out the I/O. To fix this problem you have to synchronize the status checking with the suspend/resume operations. This means the status changes have to occur under the protection of the private lock, which means a private flag is needed. What about checking if the status is RPM_ACTIVE under dev-power.lock? That's what rpm_resume() does, more or less. That wouldn't solve the race described above. Moreover, processing requests in the resume callback is not something I'd recommend to anyone, because that's going to happen when the status of the device is RPM_RESUMING, we're holding a reference on the parent (if there's one) etc. I don't see any problem with that. The parent's child_count will be incremented while the requests are being processed regardless. And if you've been waiting for the device to resume in order to carry out some processing, within the resume callback is the logical place to do the work. Unless your _driver_ callback is actually executed from within a PM domain callback, for example, and something else may be waiting for it to complete, so your data processing is adding latencies to some other threads. I'm not making that up, by the way, that really can happen. And what if you are a parent waited for by a child to resume so that _it_ can process its data? Would you still want to process your data in the resume callback in that case? Okay, those are valid reasons. (Although a device handling I/O requests isn't likely to have a child with its own independent I/O handling.) I suppose we could keep pm_runtime_get_sync as is, and just change pm_runtime_get to pm_runtime_get_async (and likewise for _put). That could reduce the confusion during the changeover. Changing pm_runtime_get() to pm_runtime_get_async() would be an improvement, although pm_runtime_get_but_do_not_resume_immediately() might even be better. Or even pm_runtime_get_but_do_not_access_hardware_when_it_returns(). ;-) I see no reason to have any of those things, though. Yes, they _may_ be useful to someone knowing the runtime PM core internals to save a few lines of code in some places, but generally they lead people to make serious mistakes that tend to be difficult to debug. For this very reason pm_runtime_get() is a bad interface and I wish we hadn't introduced it at all. Even if we give it a more descriptive name, it won't be much better. And note how that doesn't apply to the pm_runtime_put*() family. After all, doing pm_runtime_put() instead of pm_runtime_put_sync() will never lead to accessing registers of a suspended device. All right. But I still think pm_runtime_put_async is better than pm_runtime_put. At least it forces people to think about what they're doing. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Do we need asynchronous pm_runtime_get()? (was: Re: bisected regression ...)
On Fri, Aug 3, 2012 at 10:20 AM, Alan Stern st...@rowland.harvard.edu wrote: On Thu, 2 Aug 2012, Rafael J. Wysocki wrote: Hmmm. You'd probably want a version that does a get at the same time. I suppose you would call func directly if the device was already resumed, without going through the workqueue? Maybe it isn't good, the 'func' might not be run in the current context (irq context or some spinlock is held). And what if you are a parent waited for by a child to resume so that _it_ can process its data? Would you still want to process your data in the resume callback in that case? Looks the child is always resumed after its parent completes the resuming, and current pm_runtime_get doesn't delay the resume of the parent, and just make the .runtime_resume run in another context. Also are there actual examples about the above situation? Okay, those are valid reasons. (Although a device handling I/O requests isn't likely to have a child with its own independent I/O handling.) IMO, the .runtime_resume callback can handle the IO things easily via scheduling worker function if the driver don't want to delay its children's resume. Thanks, -- Ming Lei -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Do we need asynchronous pm_runtime_get()? (was: Re: bisected regression ...)
On Tue, 31 Jul 2012, Rafael J. Wysocki wrote: On Tuesday, July 31, 2012, Alan Stern wrote: [CC: list trimmed] On Tue, 31 Jul 2012, Rafael J. Wysocki wrote: Now it occured to me that perhaps we don't need the current asynchronous pm_runtime_get() at all. The usefulness of it is quite questionable, because either we want to resume the device immediately, for which pm_runtime_get_sync() should be used, or we just want to bump up the usage counter, in which cases pm_runtime_get_noresume() should always be sufficient. I fail to see any particularly compelling use case for pm_runtime_get() doing an asynchronous resume at the moment, but perhaps that's just me. There are indeed valid uses for pm_runtime_get(). We are forced to use it in non-sleepable contexts when we want to resume the device as quickly as possible. Example: a driver receives an I/O request from an interrupt handler. Is it actually suitable to be used in such contexts? It doesn't give any guarantee that the device will be active when it returns, so the caller can't really rely on it. Of course not. When you're in interrupt context, you can't wait around to see if the device will actually resume. (Unless you're using irq-safe runtime PM, but we can ignore that case.) The caller always has to check if the device has been resumed before accessing it anyway, so it may as well do a _get_noresume(), do the check and do pm_request_resume() if needed directly. This is exactly equivalent to doing pm_runtime_get() followed by a check, except that it's longer. Yes, I agree that pm_runtime_get(dev) is nothing more than a shorthand way of doing pm_runtime_get_noresume(dev) followed by pm_request_resume(dev). That doesn't mean it should be eliminated. Shorthands are convenient. As another example, pm_runtime_get_sync(dev) is nothing more than a shorthand for pm_runtime_get_noresume(dev) followed by pm_runtime_resume(dev). IMO, having these joint functions that do a get/put combined with a suspend/resume/idle is very useful. Now, as far as interrupt handlers are concerned, there are two cases: either the device has to be active to generate an interrupt (like PCI), or the interrupt is generated by something else on behalf of it. We don't seem to handle the first case very well right now, because in that case the interrupt handler will always know that the device is active, so it should do a _get_noresume() and then change the device's status to active without calling any kind of resume, but we don't provide a helper for that. I disagree. Even if the device has to be active to generate an IRQ, it's possible for the interrupt handler to be delayed until after the device is suspended (unless the suspend routine explicitly calls synchronize_irq(), which would be pretty foolish). Hence the handler can't make any assumptions about the device's state. In the second case calling pm_runtime_get() doesn't really help either. I think it's better to do _get_noresume(), check the status and if suspended, set a flag for the resume callback to do something specific before returning successfully, then call pm_request_resume() and return. This is exactly equivalent to: pm_runtime_get(), check the status, if suspended then set a flag, otherwise do the work. Except that again, it's longer. Check out the sample driver code in section 9 of Documentation/power/runtime_pm.txt, especially the foo_read_or_write() routine. Well, IMO the naming should have been the other way around from the start. That is, we should have made pm_runtime_get be the synchronous routine and pm_runtime_get_async be the asynchronous one. But it's too late to change now. I'm not sure it is too late. If we first change all the instances of pm_runtime_get() to pm_runtime_get_async() and then all of the instances of pm_runtime_get_sync() to pm_runtime_get(), it should be technically possible. Of course, it would be confusing, but that's a different matter. :-) If you're willing to risk a certain amount of confusion, count me in. :-) While you're changing names around, consider also adding a _runtime somewhere to: pm_children_suspended, pm_schedule_suspend, pm_request_idle, pm_request_resume, pm_request_autosuspend. For example, we could have pm_runtime_idle_async instead of pm_request_idle. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Do we need asynchronous pm_runtime_get()? (was: Re: bisected regression ...)
On Wednesday, August 01, 2012, Alan Stern wrote: On Tue, 31 Jul 2012, Rafael J. Wysocki wrote: On Tuesday, July 31, 2012, Alan Stern wrote: [CC: list trimmed] On Tue, 31 Jul 2012, Rafael J. Wysocki wrote: Now it occured to me that perhaps we don't need the current asynchronous pm_runtime_get() at all. The usefulness of it is quite questionable, because either we want to resume the device immediately, for which pm_runtime_get_sync() should be used, or we just want to bump up the usage counter, in which cases pm_runtime_get_noresume() should always be sufficient. I fail to see any particularly compelling use case for pm_runtime_get() doing an asynchronous resume at the moment, but perhaps that's just me. There are indeed valid uses for pm_runtime_get(). We are forced to use it in non-sleepable contexts when we want to resume the device as quickly as possible. Example: a driver receives an I/O request from an interrupt handler. Is it actually suitable to be used in such contexts? It doesn't give any guarantee that the device will be active when it returns, so the caller can't really rely on it. Of course not. When you're in interrupt context, you can't wait around to see if the device will actually resume. (Unless you're using irq-safe runtime PM, but we can ignore that case.) The caller always has to check if the device has been resumed before accessing it anyway, so it may as well do a _get_noresume(), do the check and do pm_request_resume() if needed directly. This is exactly equivalent to doing pm_runtime_get() followed by a check, except that it's longer. Except that I meant something different from what I wrote, sorry about that (must be too much heat). What I really thought about was to do _get_noresume(), then check if the device is active and if not, queue up a work item (or another delayed execution in process context) that will do pm_runtime_resume() and then access the hardware. Why do I think it's better than plain pm_runtime_get()? Because the code path calling pm_runtime_resume() will know that it is safe to access the hardware after it has returned (unless it returns an error code, but that's exceptional). In contrast, with pm_runtime_get() there is no way to predict when the device is going to be resumed, so if the device happens to be suspended when pm_runtime_get() returns, the driver kind of has to poll it until it becomes active, or use a wait queue woken up from the resume callback, or do all of the processing in the resume callback itself (like in the example you mentioned). I'm not sure if the expectation that all driver writers will be able to implement any of these options correctly is a realistic one. Yes, I agree that pm_runtime_get(dev) is nothing more than a shorthand way of doing pm_runtime_get_noresume(dev) followed by pm_request_resume(dev). That doesn't mean it should be eliminated. Shorthands are convenient. As another example, pm_runtime_get_sync(dev) is nothing more than a shorthand for pm_runtime_get_noresume(dev) followed by pm_runtime_resume(dev). IMO, having these joint functions that do a get/put combined with a suspend/resume/idle is very useful. Now, as far as interrupt handlers are concerned, there are two cases: either the device has to be active to generate an interrupt (like PCI), or the interrupt is generated by something else on behalf of it. We don't seem to handle the first case very well right now, because in that case the interrupt handler will always know that the device is active, so it should do a _get_noresume() and then change the device's status to active without calling any kind of resume, but we don't provide a helper for that. I disagree. Even if the device has to be active to generate an IRQ, it's possible for the interrupt handler to be delayed until after the device is suspended (unless the suspend routine explicitly calls synchronize_irq(), which would be pretty foolish). Hence the handler can't make any assumptions about the device's state. In the second case calling pm_runtime_get() doesn't really help either. I think it's better to do _get_noresume(), check the status and if suspended, set a flag for the resume callback to do something specific before returning successfully, then call pm_request_resume() and return. This is exactly equivalent to: pm_runtime_get(), check the status, if suspended then set a flag, otherwise do the work. Except that again, it's longer. Check out the sample driver code in section 9 of Documentation/power/runtime_pm.txt, especially the foo_read_or_write() routine. Well, that shouldn't need the is_suspended flag at all, methinks, and the reason it does need it is because it uses pm_runtime_get(). Moreover, processing requests in the resume callback is not something I'd recommend to anyone,
Do we need asynchronous pm_runtime_get()? (was: Re: bisected regression ...)
On Sunday, July 29, 2012, Rafael J. Wysocki wrote: On Sunday, July 29, 2012, Alan Stern wrote: On Sun, 29 Jul 2012, Rafael J. Wysocki wrote: The difference is, if you use _put_sync(), you need to wait the extra 10 ms for local_pci_probe() to return (if the parent is actually suspended), although you might not need to wait for it if you used _put(), right? Yes, that's the difference. But who waits for local_pci_probe() to return? :-) pci_register_driver() might, but that's not a big deal. Hot-plug might as well, though. Which, to me, means that using _put_sync() may not be always better. It probably doesn't matter a lot, but then the workqueue overhead shouldn't matter a lot either. It's that in the end, the extra overhead is pretty small. For me there's also an issue of style: If you do a synchronous get then it looks odd not to do a synchronous put. My feeling has always been that the async routines are for use in non-process contexts, where the sync routines can't be used. Using them just to return a little more quickly is a foreign idea. I see. :-) The reason for using sync get is quite obvious: we want to be able to access the device later on. For sync put that's not so clear, though. There are cases when we definitely want to do it, like the failing .probe() in which we want to disable runtime PM next to the put, but usually it doesn't hurt (too much) to defer it IMO. Now it occured to me that perhaps we don't need the current asynchronous pm_runtime_get() at all. The usefulness of it is quite questionable, because either we want to resume the device immediately, for which pm_runtime_get_sync() should be used, or we just want to bump up the usage counter, in which cases pm_runtime_get_noresume() should always be sufficient. I fail to see any particularly compelling use case for pm_runtime_get() doing an asynchronous resume at the moment, but perhaps that's just me. However, I receive reports of people using pm_runtime_get() where they really should use pm_runtime_get_sync(), so I wonder if we can simply rename pm_runtime_get_sync() as pm_runtime_get() and drop the asynchronous version altogether? Rafael -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Do we need asynchronous pm_runtime_get()? (was: Re: bisected regression ...)
[CC: list trimmed] On Tue, 31 Jul 2012, Rafael J. Wysocki wrote: Now it occured to me that perhaps we don't need the current asynchronous pm_runtime_get() at all. The usefulness of it is quite questionable, because either we want to resume the device immediately, for which pm_runtime_get_sync() should be used, or we just want to bump up the usage counter, in which cases pm_runtime_get_noresume() should always be sufficient. I fail to see any particularly compelling use case for pm_runtime_get() doing an asynchronous resume at the moment, but perhaps that's just me. There are indeed valid uses for pm_runtime_get(). We are forced to use it in non-sleepable contexts when we want to resume the device as quickly as possible. Example: a driver receives an I/O request from an interrupt handler. However, I receive reports of people using pm_runtime_get() where they really should use pm_runtime_get_sync(), so I wonder if we can simply rename pm_runtime_get_sync() as pm_runtime_get() and drop the asynchronous version altogether? Well, IMO the naming should have been the other way around from the start. That is, we should have made pm_runtime_get be the synchronous routine and pm_runtime_get_async be the asynchronous one. But it's too late to change now. And no, we can't get rid of the async version. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Do we need asynchronous pm_runtime_get()? (was: Re: bisected regression ...)
On Tuesday, July 31, 2012, Alan Stern wrote: [CC: list trimmed] On Tue, 31 Jul 2012, Rafael J. Wysocki wrote: Now it occured to me that perhaps we don't need the current asynchronous pm_runtime_get() at all. The usefulness of it is quite questionable, because either we want to resume the device immediately, for which pm_runtime_get_sync() should be used, or we just want to bump up the usage counter, in which cases pm_runtime_get_noresume() should always be sufficient. I fail to see any particularly compelling use case for pm_runtime_get() doing an asynchronous resume at the moment, but perhaps that's just me. There are indeed valid uses for pm_runtime_get(). We are forced to use it in non-sleepable contexts when we want to resume the device as quickly as possible. Example: a driver receives an I/O request from an interrupt handler. Is it actually suitable to be used in such contexts? It doesn't give any guarantee that the device will be active when it returns, so the caller can't really rely on it. The caller always has to check if the device has been resumed before accessing it anyway, so it may as well do a _get_noresume(), do the check and do pm_request_resume() if needed directly. Now, as far as interrupt handlers are concerned, there are two cases: either the device has to be active to generate an interrupt (like PCI), or the interrupt is generated by something else on behalf of it. We don't seem to handle the first case very well right now, because in that case the interrupt handler will always know that the device is active, so it should do a _get_noresume() and then change the device's status to active without calling any kind of resume, but we don't provide a helper for that. In the second case calling pm_runtime_get() doesn't really help either. I think it's better to do _get_noresume(), check the status and if suspended, set a flag for the resume callback to do something specific before returning successfully, then call pm_request_resume() and return. However, I receive reports of people using pm_runtime_get() where they really should use pm_runtime_get_sync(), so I wonder if we can simply rename pm_runtime_get_sync() as pm_runtime_get() and drop the asynchronous version altogether? Well, IMO the naming should have been the other way around from the start. That is, we should have made pm_runtime_get be the synchronous routine and pm_runtime_get_async be the asynchronous one. But it's too late to change now. I'm not sure it is too late. If we first change all the instances of pm_runtime_get() to pm_runtime_get_async() and then all of the instances of pm_runtime_get_sync() to pm_runtime_get(), it should be technically possible. Of course, it would be confusing, but that's a different matter. :-) And no, we can't get rid of the async version. I'm still not sure of that. Thanks, Rafael -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Do we need asynchronous pm_runtime_get()? (was: Re: bisected regression ...)
On Tuesday, July 31, 2012, Rafael J. Wysocki wrote: On Tuesday, July 31, 2012, Alan Stern wrote: [CC: list trimmed] On Tue, 31 Jul 2012, Rafael J. Wysocki wrote: Now it occured to me that perhaps we don't need the current asynchronous pm_runtime_get() at all. The usefulness of it is quite questionable, because either we want to resume the device immediately, for which pm_runtime_get_sync() should be used, or we just want to bump up the usage counter, in which cases pm_runtime_get_noresume() should always be sufficient. I fail to see any particularly compelling use case for pm_runtime_get() doing an asynchronous resume at the moment, but perhaps that's just me. There are indeed valid uses for pm_runtime_get(). We are forced to use it in non-sleepable contexts when we want to resume the device as quickly as possible. Example: a driver receives an I/O request from an interrupt handler. Is it actually suitable to be used in such contexts? It doesn't give any guarantee that the device will be active when it returns, so the caller can't really rely on it. The caller always has to check if the device has been resumed before accessing it anyway, so it may as well do a _get_noresume(), do the check and do pm_request_resume() if needed directly. Now, as far as interrupt handlers are concerned, there are two cases: either the device has to be active to generate an interrupt (like PCI), or the interrupt is generated by something else on behalf of it. We don't seem to handle the first case very well right now, because in that case the interrupt handler will always know that the device is active, so it should do a _get_noresume() and then change the device's status to active without calling any kind of resume, but we don't provide a helper for that. Unless, of course, this is a shared interrupt, in which case the handler may be invoked, because _another_ device has generated an interrupt, so the active check will have to be done anyway. In the second case calling pm_runtime_get() doesn't really help either. I think it's better to do _get_noresume(), check the status and if suspended, set a flag for the resume callback to do something specific before returning successfully, then call pm_request_resume() and return. I realize that this may be somewhat racy, so perhaps we need something like pm_request_resume_and_call(dev, func) that will execute the given function once the device has been resumed (or just happens to be active when the resume work item is run for it). However, I receive reports of people using pm_runtime_get() where they really should use pm_runtime_get_sync(), so I wonder if we can simply rename pm_runtime_get_sync() as pm_runtime_get() and drop the asynchronous version altogether? Well, IMO the naming should have been the other way around from the start. That is, we should have made pm_runtime_get be the synchronous routine and pm_runtime_get_async be the asynchronous one. But it's too late to change now. I'm not sure it is too late. If we first change all the instances of pm_runtime_get() to pm_runtime_get_async() and then all of the instances of pm_runtime_get_sync() to pm_runtime_get(), it should be technically possible. Of course, it would be confusing, but that's a different matter. :-) And no, we can't get rid of the async version. I'm still not sure of that. Thanks, Rafael -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
