| Message ID | 1432044679-10256-3-git-send-email-tomeu.vizoso@collabora.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
On Tue, 19 May 2015, Tomeu Vizoso wrote: > This function makes less cumbersome to enable runtime PM in a device and > its descendants. > > Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com> I don't see the point of this. In the scenario you have in mind, are the device and all its descendants registered by the same subsystem/driver? If they are, can't the subsystem/driver code enable runtime PM for each of them when they are registered, by adding a single call in the right spot? If they don't all belong to the same subsystem/driver, who is going to call your pm_runtime_enable_recursive routine? No single caller will have the right to enable runtime PM for all these devices. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, May 19, 2015 01:49:15 PM Alan Stern wrote: > On Tue, 19 May 2015, Tomeu Vizoso wrote: > > > This function makes less cumbersome to enable runtime PM in a device and > > its descendants. > > > > Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com> > > I don't see the point of this. In the scenario you have in mind, are > the device and all its descendants registered by the same > subsystem/driver? If they are, can't the subsystem/driver code enable > runtime PM for each of them when they are registered, by adding a > single call in the right spot? > > If they don't all belong to the same subsystem/driver, who is going to > call your pm_runtime_enable_recursive routine? No single caller will > have the right to enable runtime PM for all these devices. Agreed.
On 19 May 2015 at 19:49, Alan Stern <stern@rowland.harvard.edu> wrote: > On Tue, 19 May 2015, Tomeu Vizoso wrote: > >> This function makes less cumbersome to enable runtime PM in a device and >> its descendants. >> >> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com> > > I don't see the point of this. In the scenario you have in mind, are > the device and all its descendants registered by the same > subsystem/driver? Not quite, the scenario here is a driver (uvcvideo) that deals with a specific piece of hardware and knows that all the descendants of the device it's bound to are virtual. The subtree is: 1-1:1.0 (bound to uvcvideo) ep_87 input4 event4 media0 video0 I liked how the force_direct_complete flag played out here, but I agree with Rafael that it can be abused as the PM domain or the bus type weren't able to prevent going directly to complete. This is my testing branch, btw: https://git.collabora.com/cgit/user/tomeu/linux.git/log/?h=fast-resume-v5 > If they are, can't the subsystem/driver code enable > runtime PM for each of them when they are registered, by adding a > single call in the right spot? > > If they don't all belong to the same subsystem/driver, who is going to > call your pm_runtime_enable_recursive routine? No single caller will > have the right to enable runtime PM for all these devices. Yeah, I was thinking that uvcvideo might be able to decide that, but I'm not really sure about it. Thanks, Tomeu > Alan Stern > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 20 May 2015, Tomeu Vizoso wrote: > On 19 May 2015 at 19:49, Alan Stern <stern@rowland.harvard.edu> wrote: > > On Tue, 19 May 2015, Tomeu Vizoso wrote: > > > >> This function makes less cumbersome to enable runtime PM in a device and > >> its descendants. > >> > >> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com> > > > > I don't see the point of this. In the scenario you have in mind, are > > the device and all its descendants registered by the same > > subsystem/driver? > > Not quite, the scenario here is a driver (uvcvideo) that deals with a > specific piece of hardware and knows that all the descendants of the > device it's bound to are virtual. > > The subtree is: > > 1-1:1.0 (bound to uvcvideo) > ep_87 > input4 > event4 > media0 > video0 Just because these sub-devices are virtual, it doesn't mean you can ignore the way they interact with runtime PM. In the case of ep_87 this doesn't matter. Endpoint devices (like all devices) are in the SUSPENDED state by default when they are created, and they never leave that state. Does the uvcvideo interface go into runtime suspend at the right times? If it does then you shouldn't have anything more to worry about. > I liked how the force_direct_complete flag played out here, but I > agree with Rafael that it can be abused as the PM domain or the bus > type weren't able to prevent going directly to complete. > > This is my testing branch, btw: > > https://git.collabora.com/cgit/user/tomeu/linux.git/log/?h=fast-resume-v5 > > > If they are, can't the subsystem/driver code enable > > runtime PM for each of them when they are registered, by adding a > > single call in the right spot? > > > > If they don't all belong to the same subsystem/driver, who is going to > > call your pm_runtime_enable_recursive routine? No single caller will > > have the right to enable runtime PM for all these devices. > > Yeah, I was thinking that uvcvideo might be able to decide that, but > I'm not really sure about it. A possible way around the problem is to use pm_suspend_ignore_children on the uvcvideo interface. But I'm not sure that would be the right thing to do. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 20 May 2015 at 16:24, Alan Stern <stern@rowland.harvard.edu> wrote: > On Wed, 20 May 2015, Tomeu Vizoso wrote: > >> On 19 May 2015 at 19:49, Alan Stern <stern@rowland.harvard.edu> wrote: >> > On Tue, 19 May 2015, Tomeu Vizoso wrote: >> > >> >> This function makes less cumbersome to enable runtime PM in a device and >> >> its descendants. >> >> >> >> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com> >> > >> > I don't see the point of this. In the scenario you have in mind, are >> > the device and all its descendants registered by the same >> > subsystem/driver? >> >> Not quite, the scenario here is a driver (uvcvideo) that deals with a >> specific piece of hardware and knows that all the descendants of the >> device it's bound to are virtual. >> >> The subtree is: >> >> 1-1:1.0 (bound to uvcvideo) >> ep_87 >> input4 >> event4 >> media0 >> video0 > > Just because these sub-devices are virtual, it doesn't mean you can > ignore the way they interact with runtime PM. Fair enough, but then, how are we expected to be able to use the direct_complete facility if the core bails out if a descendant doesn't have runtime PM enabled? > In the case of ep_87 this doesn't matter. Endpoint devices (like all > devices) are in the SUSPENDED state by default when they are created, > and they never leave that state. I don't see why it doesn't matter for endpoints or the others. They don't have runtime PM enabled, so no ancestor will be able to do direct_complete. > Does the uvcvideo interface go into runtime suspend at the right times? > If it does then you shouldn't have anything more to worry about. It goes in and out of runtime suspend just fine, but when the system goes to sleep, it's runtime resumed and then suspended. And a full resume takes typically very long for USB cameras. >> I liked how the force_direct_complete flag played out here, but I >> agree with Rafael that it can be abused as the PM domain or the bus >> type weren't able to prevent going directly to complete. >> >> This is my testing branch, btw: >> >> https://git.collabora.com/cgit/user/tomeu/linux.git/log/?h=fast-resume-v5 >> >> > If they are, can't the subsystem/driver code enable >> > runtime PM for each of them when they are registered, by adding a >> > single call in the right spot? >> > >> > If they don't all belong to the same subsystem/driver, who is going to >> > call your pm_runtime_enable_recursive routine? No single caller will >> > have the right to enable runtime PM for all these devices. >> >> Yeah, I was thinking that uvcvideo might be able to decide that, but >> I'm not really sure about it. > > A possible way around the problem is to use pm_suspend_ignore_children > on the uvcvideo interface. But I'm not sure that would be the right > thing to do. Would that mean that if a device has ignore_children then it could still do direct_complete even if its descendants weren't able to? Thanks, Tomeu > Alan Stern > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2 Jul 2015, Tomeu Vizoso wrote: > > Just because these sub-devices are virtual, it doesn't mean you can > > ignore the way they interact with runtime PM. > > Fair enough, but then, how are we expected to be able to use the > direct_complete facility if the core bails out if a descendant doesn't > have runtime PM enabled? > > > In the case of ep_87 this doesn't matter. Endpoint devices (like all > > devices) are in the SUSPENDED state by default when they are created, > > and they never leave that state. > > I don't see why it doesn't matter for endpoints or the others. They > don't have runtime PM enabled, so no ancestor will be able to do > direct_complete. Ah, you're concerned about these lines near the start of __device_suspend(): if (dev->power.direct_complete) { if (pm_runtime_status_suspended(dev)) { pm_runtime_disable(dev); if (pm_runtime_suspended_if_enabled(dev)) goto Complete; pm_runtime_enable(dev); } dev->power.direct_complete = false; } Perhaps the pm_runtime_suspended_if_enabled() test should be changed to pm_runtime_status_suspended(). Then it won't matter whether the descendant devices are enabled for runtime PM. > > A possible way around the problem is to use pm_suspend_ignore_children > > on the uvcvideo interface. But I'm not sure that would be the right > > thing to do. > > Would that mean that if a device has ignore_children then it could > still do direct_complete even if its descendants weren't able to? I think we could justify that. The ignore_children flag means we can communicate with the children even when the device is in runtime suspend, so there's no reason to force the device to leave runtime suspend during a system sleep. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2 July 2015 at 17:21, Alan Stern <stern@rowland.harvard.edu> wrote: > On Thu, 2 Jul 2015, Tomeu Vizoso wrote: > >> > Just because these sub-devices are virtual, it doesn't mean you can >> > ignore the way they interact with runtime PM. >> >> Fair enough, but then, how are we expected to be able to use the >> direct_complete facility if the core bails out if a descendant doesn't >> have runtime PM enabled? >> >> > In the case of ep_87 this doesn't matter. Endpoint devices (like all >> > devices) are in the SUSPENDED state by default when they are created, >> > and they never leave that state. >> >> I don't see why it doesn't matter for endpoints or the others. They >> don't have runtime PM enabled, so no ancestor will be able to do >> direct_complete. > > Ah, you're concerned about these lines near the start of > __device_suspend(): > > if (dev->power.direct_complete) { > if (pm_runtime_status_suspended(dev)) { > pm_runtime_disable(dev); > if (pm_runtime_suspended_if_enabled(dev)) > goto Complete; > > pm_runtime_enable(dev); > } > dev->power.direct_complete = false; > } > > Perhaps the pm_runtime_suspended_if_enabled() test should be changed to > pm_runtime_status_suspended(). Then it won't matter whether the > descendant devices are enabled for runtime PM. Yeah, that would remove the need for messing with the runtime PM enable status of descendant devices, but I wonder why Rafael went that way initially. >> > A possible way around the problem is to use pm_suspend_ignore_children >> > on the uvcvideo interface. But I'm not sure that would be the right >> > thing to do. >> >> Would that mean that if a device has ignore_children then it could >> still do direct_complete even if its descendants weren't able to? > > I think we could justify that. The ignore_children flag means we can > communicate with the children even when the device is in runtime > suspend, so there's no reason to force the device to leave runtime > suspend during a system sleep. IIUIC, what you are proposing is to use ignore_children in a way similar to how force_direct_complete was used in this patch? http://thread.gmane.org/gmane.linux.power-management.general/60198/focus=60292 That should work as well, but Rafael raised some objections and thus I went with the present direct_complete_default, which should work if we can relax the check as discussed above. Thanks, Tomeu > Alan Stern > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 3 Jul 2015, Tomeu Vizoso wrote: > On 2 July 2015 at 17:21, Alan Stern <stern@rowland.harvard.edu> wrote: > > On Thu, 2 Jul 2015, Tomeu Vizoso wrote: > > > >> > Just because these sub-devices are virtual, it doesn't mean you can > >> > ignore the way they interact with runtime PM. > >> > >> Fair enough, but then, how are we expected to be able to use the > >> direct_complete facility if the core bails out if a descendant doesn't > >> have runtime PM enabled? > >> > >> > In the case of ep_87 this doesn't matter. Endpoint devices (like all > >> > devices) are in the SUSPENDED state by default when they are created, > >> > and they never leave that state. > >> > >> I don't see why it doesn't matter for endpoints or the others. They > >> don't have runtime PM enabled, so no ancestor will be able to do > >> direct_complete. > > > > Ah, you're concerned about these lines near the start of > > __device_suspend(): > > > > if (dev->power.direct_complete) { > > if (pm_runtime_status_suspended(dev)) { > > pm_runtime_disable(dev); > > if (pm_runtime_suspended_if_enabled(dev)) > > goto Complete; > > > > pm_runtime_enable(dev); > > } > > dev->power.direct_complete = false; > > } > > > > Perhaps the pm_runtime_suspended_if_enabled() test should be changed to > > pm_runtime_status_suspended(). Then it won't matter whether the > > descendant devices are enabled for runtime PM. > > Yeah, that would remove the need for messing with the runtime PM > enable status of descendant devices, but I wonder why Rafael went that > way initially. I forget the details. Probably it was just to be safe. We probably thought that if a device was disabled for runtime PM then its runtime PM status might not be accurate. But if direct_complete is set then it may be reasonable to assume that the runtime PM status _is_ accurate. > >> > A possible way around the problem is to use pm_suspend_ignore_children > >> > on the uvcvideo interface. But I'm not sure that would be the right > >> > thing to do. > >> > >> Would that mean that if a device has ignore_children then it could > >> still do direct_complete even if its descendants weren't able to? > > > > I think we could justify that. The ignore_children flag means we can > > communicate with the children even when the device is in runtime > > suspend, so there's no reason to force the device to leave runtime > > suspend during a system sleep. > > IIUIC, what you are proposing is to use ignore_children in a way > similar to how force_direct_complete was used in this patch? > > http://thread.gmane.org/gmane.linux.power-management.general/60198/focus=60292 That message doesn't contain a patch. > That should work as well, but Rafael raised some objections and thus I > went with the present direct_complete_default, which should work if we > can relax the check as discussed above. Rafael and I briefly discussed ignore_children while the original direct_complete patch was being designed. We didn't come to any definite conclusion and decided to forget about it for the time being. Maybe now would be a good time to reconsider it. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 3 July 2015 at 16:16, Alan Stern <stern@rowland.harvard.edu> wrote: > On Fri, 3 Jul 2015, Tomeu Vizoso wrote: > >> On 2 July 2015 at 17:21, Alan Stern <stern@rowland.harvard.edu> wrote: >> > On Thu, 2 Jul 2015, Tomeu Vizoso wrote: >> > >> >> > Just because these sub-devices are virtual, it doesn't mean you can >> >> > ignore the way they interact with runtime PM. >> >> >> >> Fair enough, but then, how are we expected to be able to use the >> >> direct_complete facility if the core bails out if a descendant doesn't >> >> have runtime PM enabled? >> >> >> >> > In the case of ep_87 this doesn't matter. Endpoint devices (like all >> >> > devices) are in the SUSPENDED state by default when they are created, >> >> > and they never leave that state. >> >> >> >> I don't see why it doesn't matter for endpoints or the others. They >> >> don't have runtime PM enabled, so no ancestor will be able to do >> >> direct_complete. >> > >> > Ah, you're concerned about these lines near the start of >> > __device_suspend(): >> > >> > if (dev->power.direct_complete) { >> > if (pm_runtime_status_suspended(dev)) { >> > pm_runtime_disable(dev); >> > if (pm_runtime_suspended_if_enabled(dev)) >> > goto Complete; >> > >> > pm_runtime_enable(dev); >> > } >> > dev->power.direct_complete = false; >> > } >> > >> > Perhaps the pm_runtime_suspended_if_enabled() test should be changed to >> > pm_runtime_status_suspended(). Then it won't matter whether the >> > descendant devices are enabled for runtime PM. >> >> Yeah, that would remove the need for messing with the runtime PM >> enable status of descendant devices, but I wonder why Rafael went that >> way initially. > > I forget the details. Probably it was just to be safe. We probably > thought that if a device was disabled for runtime PM then its runtime > PM status might not be accurate. But if direct_complete is set then it > may be reasonable to assume that the runtime PM status _is_ accurate. Cool. >> >> > A possible way around the problem is to use pm_suspend_ignore_children >> >> > on the uvcvideo interface. But I'm not sure that would be the right >> >> > thing to do. >> >> >> >> Would that mean that if a device has ignore_children then it could >> >> still do direct_complete even if its descendants weren't able to? >> > >> > I think we could justify that. The ignore_children flag means we can >> > communicate with the children even when the device is in runtime >> > suspend, so there's no reason to force the device to leave runtime >> > suspend during a system sleep. >> >> IIUIC, what you are proposing is to use ignore_children in a way >> similar to how force_direct_complete was used in this patch? >> >> http://thread.gmane.org/gmane.linux.power-management.general/60198/focus=60292 > > That message doesn't contain a patch. The patch is at the top of the thread: http://thread.gmane.org/gmane.linux.power-management.general/60198/focus=60292 >> That should work as well, but Rafael raised some objections and thus I >> went with the present direct_complete_default, which should work if we >> can relax the check as discussed above. > > Rafael and I briefly discussed ignore_children while the original > direct_complete patch was being designed. We didn't come to any > definite conclusion and decided to forget about it for the time being. > Maybe now would be a good time to reconsider it. I would prefer to have ignore_children ignore whether the children of a device were able to do direct_complete, rather than having a direct_complete_default flag (plus not requiring that all its descendants have runtime PM enabled). Thanks, Tomeu > Alan Stern > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 3 Jul 2015, Tomeu Vizoso wrote: > >> Yeah, that would remove the need for messing with the runtime PM > >> enable status of descendant devices, but I wonder why Rafael went that > >> way initially. > > > > I forget the details. Probably it was just to be safe. We probably > > thought that if a device was disabled for runtime PM then its runtime > > PM status might not be accurate. But if direct_complete is set then it > > may be reasonable to assume that the runtime PM status _is_ accurate. > > Cool. > > Rafael and I briefly discussed ignore_children while the original > > direct_complete patch was being designed. We didn't come to any > > definite conclusion and decided to forget about it for the time being. > > Maybe now would be a good time to reconsider it. > > I would prefer to have ignore_children ignore whether the children of > a device were able to do direct_complete, rather than having a > direct_complete_default flag (plus not requiring that all its > descendants have runtime PM enabled). Okay, but remember that sometimes these "virtual" devices will exist beneath a device that needs to have ignore_children off. So this won't be a complete solution to your problem. Let's see what Rafael thinks about these two issues. It seems to me that the hardest part is dealing with drivers/subsystems that have no runtime PM support. In such cases, we have to be very careful not to use direct_complete unless we know that the device does no power management at all. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday, July 03, 2015 04:22:02 PM Tomeu Vizoso wrote: > On 3 July 2015 at 16:16, Alan Stern <stern@rowland.harvard.edu> wrote: > > On Fri, 3 Jul 2015, Tomeu Vizoso wrote: > > > >> On 2 July 2015 at 17:21, Alan Stern <stern@rowland.harvard.edu> wrote: > >> > On Thu, 2 Jul 2015, Tomeu Vizoso wrote: > >> > > >> >> > Just because these sub-devices are virtual, it doesn't mean you can > >> >> > ignore the way they interact with runtime PM. > >> >> > >> >> Fair enough, but then, how are we expected to be able to use the > >> >> direct_complete facility if the core bails out if a descendant doesn't > >> >> have runtime PM enabled? > >> >> > >> >> > In the case of ep_87 this doesn't matter. Endpoint devices (like all > >> >> > devices) are in the SUSPENDED state by default when they are created, > >> >> > and they never leave that state. > >> >> > >> >> I don't see why it doesn't matter for endpoints or the others. They > >> >> don't have runtime PM enabled, so no ancestor will be able to do > >> >> direct_complete. > >> > > >> > Ah, you're concerned about these lines near the start of > >> > __device_suspend(): > >> > > >> > if (dev->power.direct_complete) { > >> > if (pm_runtime_status_suspended(dev)) { > >> > pm_runtime_disable(dev); > >> > if (pm_runtime_suspended_if_enabled(dev)) > >> > goto Complete; > >> > > >> > pm_runtime_enable(dev); > >> > } > >> > dev->power.direct_complete = false; > >> > } > >> > > >> > Perhaps the pm_runtime_suspended_if_enabled() test should be changed to > >> > pm_runtime_status_suspended(). Then it won't matter whether the > >> > descendant devices are enabled for runtime PM. > >> > >> Yeah, that would remove the need for messing with the runtime PM > >> enable status of descendant devices, but I wonder why Rafael went that > >> way initially. > > > > I forget the details. Probably it was just to be safe. We probably > > thought that if a device was disabled for runtime PM then its runtime > > PM status might not be accurate. But if direct_complete is set then it > > may be reasonable to assume that the runtime PM status _is_ accurate. > > Cool. We're walking a grey area here. What exactly does power.direct_complete mean for devices whose runtime PM is disabled? > >> >> > A possible way around the problem is to use pm_suspend_ignore_children > >> >> > on the uvcvideo interface. But I'm not sure that would be the right > >> >> > thing to do. > >> >> > >> >> Would that mean that if a device has ignore_children then it could > >> >> still do direct_complete even if its descendants weren't able to? > >> > > >> > I think we could justify that. The ignore_children flag means we can > >> > communicate with the children even when the device is in runtime > >> > suspend, so there's no reason to force the device to leave runtime > >> > suspend during a system sleep. > >> > >> IIUIC, what you are proposing is to use ignore_children in a way > >> similar to how force_direct_complete was used in this patch? > >> > >> http://thread.gmane.org/gmane.linux.power-management.general/60198/focus=60292 > > > > That message doesn't contain a patch. > > The patch is at the top of the thread: > > http://thread.gmane.org/gmane.linux.power-management.general/60198/focus=60292 > > >> That should work as well, but Rafael raised some objections and thus I > >> went with the present direct_complete_default, which should work if we > >> can relax the check as discussed above. > > > > Rafael and I briefly discussed ignore_children while the original > > direct_complete patch was being designed. We didn't come to any > > definite conclusion and decided to forget about it for the time being. > > Maybe now would be a good time to reconsider it. > > I would prefer to have ignore_children ignore whether the children of > a device were able to do direct_complete, rather than having a > direct_complete_default flag (plus not requiring that all its > descendants have runtime PM enabled). Why? Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday, July 03, 2015 11:11:19 AM Alan Stern wrote: > On Fri, 3 Jul 2015, Tomeu Vizoso wrote: > > > >> Yeah, that would remove the need for messing with the runtime PM > > >> enable status of descendant devices, but I wonder why Rafael went that > > >> way initially. > > > > > > I forget the details. Probably it was just to be safe. We probably > > > thought that if a device was disabled for runtime PM then its runtime > > > PM status might not be accurate. But if direct_complete is set then it > > > may be reasonable to assume that the runtime PM status _is_ accurate. > > > > Cool. > > > > Rafael and I briefly discussed ignore_children while the original > > > direct_complete patch was being designed. We didn't come to any > > > definite conclusion and decided to forget about it for the time being. > > > Maybe now would be a good time to reconsider it. > > > > I would prefer to have ignore_children ignore whether the children of > > a device were able to do direct_complete, rather than having a > > direct_complete_default flag (plus not requiring that all its > > descendants have runtime PM enabled). > > Okay, but remember that sometimes these "virtual" devices will exist > beneath a device that needs to have ignore_children off. So this won't > be a complete solution to your problem. > > Let's see what Rafael thinks about these two issues. It seems to me > that the hardest part is dealing with drivers/subsystems that have no > runtime PM support. In such cases, we have to be very careful not to > use direct_complete unless we know that the device does no power > management at all. Precisely. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 4 Jul 2015, Rafael J. Wysocki wrote: > > >> > Perhaps the pm_runtime_suspended_if_enabled() test should be changed to > > >> > pm_runtime_status_suspended(). Then it won't matter whether the > > >> > descendant devices are enabled for runtime PM. > > >> > > >> Yeah, that would remove the need for messing with the runtime PM > > >> enable status of descendant devices, but I wonder why Rafael went that > > >> way initially. > > > > > > I forget the details. Probably it was just to be safe. We probably > > > thought that if a device was disabled for runtime PM then its runtime > > > PM status might not be accurate. But if direct_complete is set then it > > > may be reasonable to assume that the runtime PM status _is_ accurate. > > > > Cool. > > We're walking a grey area here. What exactly does power.direct_complete mean > for devices whose runtime PM is disabled? > > Let's see what Rafael thinks about these two issues. It seems to me > > that the hardest part is dealing with drivers/subsystems that have no > > runtime PM support. In such cases, we have to be very careful not to > > use direct_complete unless we know that the device does no power > > management at all. > > Precisely. All right, we can make a decision and document it. The following seems reasonable to me: If dev->power.direct_complete is set then the PM core will assume that dev->power.rpm_status is accurate even when dev->power.disable_depth > 0. The core will obey the .direct_complete setting regardless of .disable_depth. As a consequence, devices that support system sleep but don't support runtime PM must _never_ have .direct_complete set. On the other hand, if a device (such as a "virtual" device) requires no callbacks for either system sleep or runtime PM, then there is no harm in setting .direct_complete. Indeed, doing so may help speed up an ancestor device's sleep transition. How does that sound? Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday, July 04, 2015 10:37:55 AM Alan Stern wrote: > On Sat, 4 Jul 2015, Rafael J. Wysocki wrote: > > > > >> > Perhaps the pm_runtime_suspended_if_enabled() test should be changed to > > > >> > pm_runtime_status_suspended(). Then it won't matter whether the > > > >> > descendant devices are enabled for runtime PM. > > > >> > > > >> Yeah, that would remove the need for messing with the runtime PM > > > >> enable status of descendant devices, but I wonder why Rafael went that > > > >> way initially. > > > > > > > > I forget the details. Probably it was just to be safe. We probably > > > > thought that if a device was disabled for runtime PM then its runtime > > > > PM status might not be accurate. But if direct_complete is set then it > > > > may be reasonable to assume that the runtime PM status _is_ accurate. > > > > > > Cool. > > > > We're walking a grey area here. What exactly does power.direct_complete mean > > for devices whose runtime PM is disabled? > > > > Let's see what Rafael thinks about these two issues. It seems to me > > > that the hardest part is dealing with drivers/subsystems that have no > > > runtime PM support. In such cases, we have to be very careful not to > > > use direct_complete unless we know that the device does no power > > > management at all. > > > > Precisely. > > All right, we can make a decision and document it. The following seems > reasonable to me: > > If dev->power.direct_complete is set then the PM core will > assume that dev->power.rpm_status is accurate even when > dev->power.disable_depth > 0. The core will obey the > .direct_complete setting regardless of .disable_depth. > > As a consequence, devices that support system sleep but don't > support runtime PM must _never_ have .direct_complete set. > > On the other hand, if a device (such as a "virtual" device) > requires no callbacks for either system sleep or runtime PM, > then there is no harm in setting .direct_complete. Indeed, > doing so may help speed up an ancestor device's sleep > transition. > > How does that sound? It would be workable I think, but I'd prefer the core to be told directly about devices whose runtime PM status doesn't matter (because nothing changes between "suspended" and "active"), so they may be treated in a special way safely. If we had that information, no special rules other than "that is a device whose runtime PM status doesn't matter, so treat it accordingly" would be necessary. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday, July 06, 2015 01:36:46 AM Rafael J. Wysocki wrote: > On Saturday, July 04, 2015 10:37:55 AM Alan Stern wrote: > > On Sat, 4 Jul 2015, Rafael J. Wysocki wrote: > > > > > > >> > Perhaps the pm_runtime_suspended_if_enabled() test should be changed to > > > > >> > pm_runtime_status_suspended(). Then it won't matter whether the > > > > >> > descendant devices are enabled for runtime PM. > > > > >> > > > > >> Yeah, that would remove the need for messing with the runtime PM > > > > >> enable status of descendant devices, but I wonder why Rafael went that > > > > >> way initially. > > > > > > > > > > I forget the details. Probably it was just to be safe. We probably > > > > > thought that if a device was disabled for runtime PM then its runtime > > > > > PM status might not be accurate. But if direct_complete is set then it > > > > > may be reasonable to assume that the runtime PM status _is_ accurate. > > > > > > > > Cool. > > > > > > We're walking a grey area here. What exactly does power.direct_complete mean > > > for devices whose runtime PM is disabled? > > > > > > Let's see what Rafael thinks about these two issues. It seems to me > > > > that the hardest part is dealing with drivers/subsystems that have no > > > > runtime PM support. In such cases, we have to be very careful not to > > > > use direct_complete unless we know that the device does no power > > > > management at all. > > > > > > Precisely. > > > > All right, we can make a decision and document it. The following seems > > reasonable to me: > > > > If dev->power.direct_complete is set then the PM core will > > assume that dev->power.rpm_status is accurate even when > > dev->power.disable_depth > 0. The core will obey the > > .direct_complete setting regardless of .disable_depth. > > > > As a consequence, devices that support system sleep but don't > > support runtime PM must _never_ have .direct_complete set. > > > > On the other hand, if a device (such as a "virtual" device) > > requires no callbacks for either system sleep or runtime PM, > > then there is no harm in setting .direct_complete. Indeed, > > doing so may help speed up an ancestor device's sleep > > transition. > > > > How does that sound? > > It would be workable I think, but I'd prefer the core to be told directly > about devices whose runtime PM status doesn't matter (because nothing changes > between "suspended" and "active"), so they may be treated in a special way > safely. > > If we had that information, no special rules other than "that is a device > whose runtime PM status doesn't matter, so treat it accordingly" would be > necessary. That said, a situation to consider is when device X is just a software device, but it has children that correspond to physical hardware. If that is the case, the usual parent-children rules should apply to X and its children (ie. X should only be marked as "suspended" if all of its children are suspended) and I see no reason why the parent-children rules for direct_resume should not apply here. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 7 Jul 2015, Rafael J. Wysocki wrote: > > > All right, we can make a decision and document it. The following seems > > > reasonable to me: > > > > > > If dev->power.direct_complete is set then the PM core will > > > assume that dev->power.rpm_status is accurate even when > > > dev->power.disable_depth > 0. The core will obey the > > > .direct_complete setting regardless of .disable_depth. > > > > > > As a consequence, devices that support system sleep but don't > > > support runtime PM must _never_ have .direct_complete set. > > > > > > On the other hand, if a device (such as a "virtual" device) > > > requires no callbacks for either system sleep or runtime PM, > > > then there is no harm in setting .direct_complete. Indeed, > > > doing so may help speed up an ancestor device's sleep > > > transition. > > > > > > How does that sound? > > > > It would be workable I think, but I'd prefer the core to be told directly > > about devices whose runtime PM status doesn't matter (because nothing changes > > between "suspended" and "active"), so they may be treated in a special way > > safely. > > > > If we had that information, no special rules other than "that is a device > > whose runtime PM status doesn't matter, so treat it accordingly" would be > > necessary. > > That said, a situation to consider is when device X is just a software device, > but it has children that correspond to physical hardware. If that is the case, > the usual parent-children rules should apply to X and its children (ie. X should > only be marked as "suspended" if all of its children are suspended) and I see > no reason why the parent-children rules for direct_resume should not apply here. Yes, this illustrates that in some ways we must not treat "virtual" or "software" devices specially. Being "virtual" is not the same as having the ignore_children flag set. The change I'm proposing is not related to whether a device is "virtual". I'm just suggesting that the normal direct_complete rules should apply even when devices are runtime-PM-disabled. This doesn't mean that their runtime PM status doesn't matter. Just the opposite, in fact -- it means that the PM core should pay attention to the runtime PM status during a sleep transition even though disabled_depth > 0. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, July 07, 2015 10:55:59 AM Alan Stern wrote: > On Tue, 7 Jul 2015, Rafael J. Wysocki wrote: > > > > > All right, we can make a decision and document it. The following seems > > > > reasonable to me: > > > > > > > > If dev->power.direct_complete is set then the PM core will > > > > assume that dev->power.rpm_status is accurate even when > > > > dev->power.disable_depth > 0. The core will obey the > > > > .direct_complete setting regardless of .disable_depth. > > > > > > > > As a consequence, devices that support system sleep but don't > > > > support runtime PM must _never_ have .direct_complete set. > > > > > > > > On the other hand, if a device (such as a "virtual" device) > > > > requires no callbacks for either system sleep or runtime PM, > > > > then there is no harm in setting .direct_complete. Indeed, > > > > doing so may help speed up an ancestor device's sleep > > > > transition. > > > > > > > > How does that sound? > > > > > > It would be workable I think, but I'd prefer the core to be told directly > > > about devices whose runtime PM status doesn't matter (because nothing changes > > > between "suspended" and "active"), so they may be treated in a special way > > > safely. > > > > > > If we had that information, no special rules other than "that is a device > > > whose runtime PM status doesn't matter, so treat it accordingly" would be > > > necessary. > > > > That said, a situation to consider is when device X is just a software device, > > but it has children that correspond to physical hardware. If that is the case, > > the usual parent-children rules should apply to X and its children (ie. X should > > only be marked as "suspended" if all of its children are suspended) and I see > > no reason why the parent-children rules for direct_resume should not apply here. > > Yes, this illustrates that in some ways we must not treat "virtual" or > "software" devices specially. Being "virtual" is not the same as > having the ignore_children flag set. > > The change I'm proposing is not related to whether a device is > "virtual". I'm just suggesting that the normal direct_complete rules > should apply even when devices are runtime-PM-disabled. > > This doesn't mean that their runtime PM status doesn't matter. Just > the opposite, in fact -- it means that the PM core should pay attention > to the runtime PM status during a sleep transition even though > disabled_depth > 0. I seem to have lost the context here, sorry about that. The idea seems to be to rely on the fact that the RPM status for all devices is initially RPM_SUSPENDED and that never changes if runtime PM is never enabled for the device, so in that particular case it would be OK to treat the "power.direct_complete set + RPM status == RPM_SUSPENDED" combination as valid even though runtime PM has never been enabled for the device in question (provided that power.direct_complete will never be set for "real" devices that don't support runtime PM). Is that correct? That seems to be fragile, but I have no strong opinion. Let's do that change if it allows us to make forward progress here. Please feel free to submit a documentation patch along the lines you've suggested. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c index 5070c4f..2c96f3f 100644 --- a/drivers/base/power/runtime.c +++ b/drivers/base/power/runtime.c @@ -1189,6 +1189,21 @@ void pm_runtime_enable(struct device *dev) } EXPORT_SYMBOL_GPL(pm_runtime_enable); +static int __pm_runtime_enable_recursive(struct device *dev, void *data) +{ + pm_runtime_enable(dev); + + device_for_each_child(dev, NULL, __pm_runtime_enable_recursive); + + return 0; +} + +void pm_runtime_enable_recursive(struct device *dev) +{ + __pm_runtime_enable_recursive(dev, NULL); +} +EXPORT_SYMBOL_GPL(pm_runtime_enable_recursive); + /** * pm_runtime_forbid - Block runtime PM of a device. * @dev: Device to handle. diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h index 30e84d4..d393ac3 100644 --- a/include/linux/pm_runtime.h +++ b/include/linux/pm_runtime.h @@ -43,6 +43,7 @@ extern int pm_schedule_suspend(struct device *dev, unsigned int delay); extern int __pm_runtime_set_status(struct device *dev, unsigned int status); extern int pm_runtime_barrier(struct device *dev); extern void pm_runtime_enable(struct device *dev); +extern void pm_runtime_enable_recursive(struct device *dev); extern void __pm_runtime_disable(struct device *dev, bool check_resume); extern void pm_runtime_allow(struct device *dev); extern void pm_runtime_forbid(struct device *dev);
This function makes less cumbersome to enable runtime PM in a device and its descendants. Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com> --- drivers/base/power/runtime.c | 15 +++++++++++++++ include/linux/pm_runtime.h | 1 + 2 files changed, 16 insertions(+)