| Message ID | Pine.LNX.4.44L0.1406271417120.875-100000@iolanthe.rowland.org (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
On Fri, Jun 27, 2014 at 02:27:28PM -0400, Alan Stern wrote: > On Wed, 25 Jun 2014, Rafael J. Wysocki wrote: > > > On Sunday, June 22, 2014 12:45:42 PM Alan Stern wrote: > > > On Sun, 22 Jun 2014, Rafael J. Wysocki wrote: > > > > > > > > How would you treat them specially? Add a "runtime_pm_not_supported" > > > > > flag? > > > > > > > > I thought about a "runtime PM has been enabled at least once" flag rather > > > > that would be set by pm_runtime_enable() every time it is called and never > > > > cleared. That would allow the core to distinguish between "runtime PM > > > > disabled temporarily" and "runtime PM not used" which turn out to be > > > > sufficiently different cases. > > > > > > Interesting idea, but it can't tell the difference between "runtime PM > > > not supported" and "runtime PM not enabled yet". I think a simple "not > > > supported" flag will be more straightforward. > > > > The question is who will set the "unsupported" flag (think devices without > > drivers etc.). Or perhaps the idea is that it will be set to start with? > > Drivers or subsystems will set the flag. It should not be set for > devices without drivers or subsystems, because the flag means that the > hardware doesn't support runtime power management, and the kernel > wouldn't know this if there was no driver or subsystem. > > The flag will not be set to start with. The idea is that you set it > when you know for certain that the device cannot be power-managed, but > you still want the Runtime PM API to work with the device. In > particular, calls to pm_runtime_resume() will succeed. > > > > > Yes. The core definitely needs to be able to distinguish between the > > > > "runtime PM disabled temporarily" and "runtime PM not supported/not used" > > > > situations. > > > > > > Let me work out a patch, and we'll see what you think. For the time > > > being we can stick with our "runtime PM must be disabled (or in error) > > > when the status is changed" approach. > > > > OK > > The patch is below. I haven't tested it with anything meaningful, but > it seems straightforward enough. > > One side point: The patch changes the string displayed for the > power/runtime_status attribute file when disable_depth > 0. Instead of > "unsupported", it will now say "disabled". The attribute will contain > "not supported" when the new flag is set. > > Is this acceptable? Why change the "unsupported" string? Can't we just leave that one alone? I'd prefer to not break userspace tools... thanks, greg k-h -- 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, 27 Jun 2014, Greg Kroah-Hartman wrote: > > One side point: The patch changes the string displayed for the > > power/runtime_status attribute file when disable_depth > 0. Instead of > > "unsupported", it will now say "disabled". The attribute will contain > > "not supported" when the new flag is set. > > > > Is this acceptable? > > Why change the "unsupported" string? Can't we just leave that one > alone? I'd prefer to not break userspace tools... I changed it because it's wrong. disable_depth > 0 means that runtime PM has temporarily been disabled, or was never enabled in the first place. It doesn't mean that runtime PM is unsupported. In fact, the word "unsupported" is ambiguous. Does it mean unsupported by the hardware or unsupported by the kernel? The hardware can't change, but the kernel can be altered by loading a module. If that change is too intrusive, I can remove it from the patch. 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 Fri, Jun 27, 2014 at 04:11:35PM -0400, Alan Stern wrote: > On Fri, 27 Jun 2014, Greg Kroah-Hartman wrote: > > > > One side point: The patch changes the string displayed for the > > > power/runtime_status attribute file when disable_depth > 0. Instead of > > > "unsupported", it will now say "disabled". The attribute will contain > > > "not supported" when the new flag is set. > > > > > > Is this acceptable? > > > > Why change the "unsupported" string? Can't we just leave that one > > alone? I'd prefer to not break userspace tools... > > I changed it because it's wrong. disable_depth > 0 means that runtime > PM has temporarily been disabled, or was never enabled in the first > place. It doesn't mean that runtime PM is unsupported. > > In fact, the word "unsupported" is ambiguous. Does it mean unsupported > by the hardware or unsupported by the kernel? The hardware can't > change, but the kernel can be altered by loading a module. > > If that change is too intrusive, I can remove it from the patch. Do you know of any tools that actually look at these files? If there isn't any, then we can try to change it and see who screams :) greg k-h -- 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, 27 Jun 2014, Greg Kroah-Hartman wrote: > On Fri, Jun 27, 2014 at 04:11:35PM -0400, Alan Stern wrote: > > On Fri, 27 Jun 2014, Greg Kroah-Hartman wrote: > > > > > > One side point: The patch changes the string displayed for the > > > > power/runtime_status attribute file when disable_depth > 0. Instead of > > > > "unsupported", it will now say "disabled". The attribute will contain > > > > "not supported" when the new flag is set. > > > > > > > > Is this acceptable? > > > > > > Why change the "unsupported" string? Can't we just leave that one > > > alone? I'd prefer to not break userspace tools... > > > > I changed it because it's wrong. disable_depth > 0 means that runtime > > PM has temporarily been disabled, or was never enabled in the first > > place. It doesn't mean that runtime PM is unsupported. > > > > In fact, the word "unsupported" is ambiguous. Does it mean unsupported > > by the hardware or unsupported by the kernel? The hardware can't > > change, but the kernel can be altered by loading a module. > > > > If that change is too intrusive, I can remove it from the patch. > > Do you know of any tools that actually look at these files? I don't. Of course, that doesn't mean much. > If there isn't any, then we can try to change it and see who screams :) It'll be a learning experience... 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, June 28, 2014 11:32:21 AM Alan Stern wrote: > On Fri, 27 Jun 2014, Greg Kroah-Hartman wrote: > > > On Fri, Jun 27, 2014 at 04:11:35PM -0400, Alan Stern wrote: > > > On Fri, 27 Jun 2014, Greg Kroah-Hartman wrote: > > > > > > > > One side point: The patch changes the string displayed for the > > > > > power/runtime_status attribute file when disable_depth > 0. Instead of > > > > > "unsupported", it will now say "disabled". The attribute will contain > > > > > "not supported" when the new flag is set. > > > > > > > > > > Is this acceptable? > > > > > > > > Why change the "unsupported" string? Can't we just leave that one > > > > alone? I'd prefer to not break userspace tools... > > > > > > I changed it because it's wrong. disable_depth > 0 means that runtime > > > PM has temporarily been disabled, or was never enabled in the first > > > place. It doesn't mean that runtime PM is unsupported. > > > > > > In fact, the word "unsupported" is ambiguous. Does it mean unsupported > > > by the hardware or unsupported by the kernel? The hardware can't > > > change, but the kernel can be altered by loading a module. > > > > > > If that change is too intrusive, I can remove it from the patch. > > > > Do you know of any tools that actually look at these files? > > I don't. Of course, that doesn't mean much. The only tool I'm aware of that may be looking at them is powertop, so if the change doesn't affect powertop adversely, it should be generally safe. > > If there isn't any, then we can try to change it and see who screams :) > > It'll be a learning experience... Yes, it will. :-) 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 Mon, 30 Jun 2014, Rafael J. Wysocki wrote: > > > Do you know of any tools that actually look at these files? > > > > I don't. Of course, that doesn't mean much. > > The only tool I'm aware of that may be looking at them is powertop, so > if the change doesn't affect powertop adversely, it should be generally > safe. > > > > If there isn't any, then we can try to change it and see who screams :) > > > > It'll be a learning experience... > > Yes, it will. :-) Then you have no other objections to the patch? 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 Monday, June 30, 2014 10:42:19 AM Alan Stern wrote: > On Mon, 30 Jun 2014, Rafael J. Wysocki wrote: > > > > > Do you know of any tools that actually look at these files? > > > > > > I don't. Of course, that doesn't mean much. > > > > The only tool I'm aware of that may be looking at them is powertop, so > > if the change doesn't affect powertop adversely, it should be generally > > safe. > > > > > > If there isn't any, then we can try to change it and see who screams :) > > > > > > It'll be a learning experience... > > > > Yes, it will. :-) > > Then you have no other objections to the patch? My concern still is that it will be confusing, because people won't read the documentation carefully enough and will confuse "runtime PM never used" with "hardware can't do PM". I'm not sure how to make that more clear, though. Also we have the no_callbacks flag and I wonder if/how it is related to the new one. Do we still need both? In addition to that, I think that "hardware can't do PM" should apply to the handling of system suspend resume too. 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 Wed, 2 Jul 2014, Rafael J. Wysocki wrote: > > Then you have no other objections to the patch? > > My concern still is that it will be confusing, because people won't read the > documentation carefully enough and will confuse "runtime PM never used" with > "hardware can't do PM". I'm not sure how to make that more clear, though. I could emphasize that distinction a little more strongly in the documentation. > Also we have the no_callbacks flag and I wonder if/how it is related to the > new one. Do we still need both? They mean different things. The no_callbacks flag is used when we want the PM core to think the device can be in RPM_SUSPENDED at times (it is "logically suspended"). rpm_not_supported is used when we want the PM core to think the device must always be in RPM_ACTIVE. > In addition to that, I think that "hardware can't do PM" should apply to the > handling of system suspend resume too. Maybe. For the use case Dan Williams and I are working on, it doesn't matter; for other cases it might matter. That's why I named the flag "rpm_not_supported" -- it applies specifically to runtime PM, not system PM. Here's a brief summary of the story behind this patch... At one point, I suggested to Dan that instead of doing something special for these devices, we could simply have the runtime_suspend() routine always return -EBUSY. He didn't like that idea because then the user would see the device was never powering down but would have no idea why. The rpm_not_supported flag provides this information to the user by causing the power/runtime_status attribute to say "not supported". (Although to be entirely fair, we could just put a message in the kernel log during probe if the hardware doesn't support runtime suspend.) Instead, Dan introduced a messy PM QoS mechanism in commit e3d105055525. I didn't like that approach, but Greg merged it before I objected. Do you have any suggestions? 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 Wed, Jul 02, 2014 at 10:27:06AM -0400, Alan Stern wrote: > On Wed, 2 Jul 2014, Rafael J. Wysocki wrote: > > > > Then you have no other objections to the patch? > > > > My concern still is that it will be confusing, because people won't read the > > documentation carefully enough and will confuse "runtime PM never used" with > > "hardware can't do PM". I'm not sure how to make that more clear, though. > > I could emphasize that distinction a little more strongly in the > documentation. > > > Also we have the no_callbacks flag and I wonder if/how it is related to the > > new one. Do we still need both? > > They mean different things. The no_callbacks flag is used when we want > the PM core to think the device can be in RPM_SUSPENDED at times (it is > "logically suspended"). rpm_not_supported is used when we want the PM > core to think the device must always be in RPM_ACTIVE. > > > In addition to that, I think that "hardware can't do PM" should apply to the > > handling of system suspend resume too. > > Maybe. For the use case Dan Williams and I are working on, it doesn't > matter; for other cases it might matter. That's why I named the flag > "rpm_not_supported" -- it applies specifically to runtime PM, not > system PM. > > Here's a brief summary of the story behind this patch... > > At one point, I suggested to Dan that instead of doing something > special for these devices, we could simply have the runtime_suspend() > routine always return -EBUSY. He didn't like that idea because then > the user would see the device was never powering down but would have no > idea why. The rpm_not_supported flag provides this information to the > user by causing the power/runtime_status attribute to say "not > supported". (Although to be entirely fair, we could just put a message > in the kernel log during probe if the hardware doesn't support runtime > suspend.) > > Instead, Dan introduced a messy PM QoS mechanism in commit > e3d105055525. I didn't like that approach, but Greg merged it before I > objected. Sorry about that, we can always revert it :) greg k-h -- 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 Wednesday, July 02, 2014 10:27:06 AM Alan Stern wrote: > On Wed, 2 Jul 2014, Rafael J. Wysocki wrote: > > > > Then you have no other objections to the patch? > > > > My concern still is that it will be confusing, because people won't read the > > documentation carefully enough and will confuse "runtime PM never used" with > > "hardware can't do PM". I'm not sure how to make that more clear, though. > > I could emphasize that distinction a little more strongly in the > documentation. > > > Also we have the no_callbacks flag and I wonder if/how it is related to the > > new one. Do we still need both? > > They mean different things. The no_callbacks flag is used when we want > the PM core to think the device can be in RPM_SUSPENDED at times (it is > "logically suspended"). rpm_not_supported is used when we want the PM > core to think the device must always be in RPM_ACTIVE. > > > In addition to that, I think that "hardware can't do PM" should apply to the > > handling of system suspend resume too. > > Maybe. For the use case Dan Williams and I are working on, it doesn't > matter; for other cases it might matter. That's why I named the flag > "rpm_not_supported" -- it applies specifically to runtime PM, not > system PM. > > Here's a brief summary of the story behind this patch... > > At one point, I suggested to Dan that instead of doing something > special for these devices, we could simply have the runtime_suspend() > routine always return -EBUSY. He didn't like that idea because then > the user would see the device was never powering down but would have no > idea why. The rpm_not_supported flag provides this information to the > user by causing the power/runtime_status attribute to say "not > supported". (Although to be entirely fair, we could just put a message > in the kernel log during probe if the hardware doesn't support runtime > suspend.) > > Instead, Dan introduced a messy PM QoS mechanism in commit > e3d105055525. I didn't like that approach, but Greg merged it before I > objected. > > Do you have any suggestions? I need some more time to think about that. I'm on vacation till Monday, I should be able to get to this by then. I hope that's not a problem. 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 Thu, 3 Jul 2014, Rafael J. Wysocki wrote: > > Do you have any suggestions? > > I need some more time to think about that. I'm on vacation till Monday, > I should be able to get to this by then. I hope that's not a problem. Tomorrow is a national holiday in the US, so I'm taking some time off too. Don't hurry. 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
That took me much more time than I had hoped, sorry about that. On Wednesday, July 02, 2014 10:27:06 AM Alan Stern wrote: > On Wed, 2 Jul 2014, Rafael J. Wysocki wrote: > > > > Then you have no other objections to the patch? > > > > My concern still is that it will be confusing, because people won't read the > > documentation carefully enough and will confuse "runtime PM never used" with > > "hardware can't do PM". I'm not sure how to make that more clear, though. > > I could emphasize that distinction a little more strongly in the > documentation. So it looks like we'd like to cover the "enabled but always active" case for runtime PM of a device? > > Also we have the no_callbacks flag and I wonder if/how it is related to the > > new one. Do we still need both? > > They mean different things. The no_callbacks flag is used when we want > the PM core to think the device can be in RPM_SUSPENDED at times (it is > "logically suspended"). rpm_not_supported is used when we want the PM > core to think the device must always be in RPM_ACTIVE. > > > In addition to that, I think that "hardware can't do PM" should apply to the > > handling of system suspend resume too. > > Maybe. For the use case Dan Williams and I are working on, it doesn't > matter; for other cases it might matter. That's why I named the flag > "rpm_not_supported" -- it applies specifically to runtime PM, not > system PM. Maybe it'd be better to call the flag always_active and make rpm_suspend() and rpm_resume() react to it accordingly? So before enabling runtime PM the driver will set always_active and that will mean the device cannot be suspended. And we could add a separate sysfs attribute for that instead of re-purposing the status thing, if needed (I'm not sure about that, however). > Here's a brief summary of the story behind this patch... > > At one point, I suggested to Dan that instead of doing something > special for these devices, we could simply have the runtime_suspend() > routine always return -EBUSY. He didn't like that idea because then > the user would see the device was never powering down but would have no > idea why. The rpm_not_supported flag provides this information to the > user by causing the power/runtime_status attribute to say "not > supported". (Although to be entirely fair, we could just put a message > in the kernel log during probe if the hardware doesn't support runtime > suspend.) > > Instead, Dan introduced a messy PM QoS mechanism in commit > e3d105055525. I didn't like that approach, but Greg merged it before I > objected. That really looks a bit like a hack to me to be honest. Greg, what's your plan toward this? > Do you have any suggestions? We could use an "always_active" type of flag for that, but I'm not sure how much value to the user that will bring. 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 Thu, Jul 17, 2014 at 12:40:23AM +0200, Rafael J. Wysocki wrote: > On Wednesday, July 02, 2014 10:27:06 AM Alan Stern wrote: > > Here's a brief summary of the story behind this patch... > > > > At one point, I suggested to Dan that instead of doing something > > special for these devices, we could simply have the runtime_suspend() > > routine always return -EBUSY. He didn't like that idea because then > > the user would see the device was never powering down but would have no > > idea why. The rpm_not_supported flag provides this information to the > > user by causing the power/runtime_status attribute to say "not > > supported". (Although to be entirely fair, we could just put a message > > in the kernel log during probe if the hardware doesn't support runtime > > suspend.) > > > > Instead, Dan introduced a messy PM QoS mechanism in commit > > e3d105055525. I didn't like that approach, but Greg merged it before I > > objected. > > That really looks a bit like a hack to me to be honest. > > Greg, what's your plan toward this? If I need to revert something that you all find was wrong, I'll be glad to do so, sorry for merging something too early. thanks, greg k-h -- 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 Wednesday, July 16, 2014 04:03:45 PM Greg Kroah-Hartman wrote: > On Thu, Jul 17, 2014 at 12:40:23AM +0200, Rafael J. Wysocki wrote: > > On Wednesday, July 02, 2014 10:27:06 AM Alan Stern wrote: > > > Here's a brief summary of the story behind this patch... > > > > > > At one point, I suggested to Dan that instead of doing something > > > special for these devices, we could simply have the runtime_suspend() > > > routine always return -EBUSY. He didn't like that idea because then > > > the user would see the device was never powering down but would have no > > > idea why. The rpm_not_supported flag provides this information to the > > > user by causing the power/runtime_status attribute to say "not > > > supported". (Although to be entirely fair, we could just put a message > > > in the kernel log during probe if the hardware doesn't support runtime > > > suspend.) > > > > > > Instead, Dan introduced a messy PM QoS mechanism in commit > > > e3d105055525. I didn't like that approach, but Greg merged it before I > > > objected. > > > > That really looks a bit like a hack to me to be honest. > > > > Greg, what's your plan toward this? > > If I need to revert something that you all find was wrong, I'll be glad > to do so, sorry for merging something too early. Alan, what do you think? I think we're still unsure if the approach taken by that commit is correct, but then I suppose we don't need to revert it at this point and we can fix it later. Is that correct, or would fixing it be difficult for some reason? 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 Thu, 17 Jul 2014, Rafael J. Wysocki wrote: > On Wednesday, July 16, 2014 04:03:45 PM Greg Kroah-Hartman wrote: > > On Thu, Jul 17, 2014 at 12:40:23AM +0200, Rafael J. Wysocki wrote: > > > On Wednesday, July 02, 2014 10:27:06 AM Alan Stern wrote: > > > > Here's a brief summary of the story behind this patch... > > > > > > > > At one point, I suggested to Dan that instead of doing something > > > > special for these devices, we could simply have the runtime_suspend() > > > > routine always return -EBUSY. He didn't like that idea because then > > > > the user would see the device was never powering down but would have no > > > > idea why. The rpm_not_supported flag provides this information to the > > > > user by causing the power/runtime_status attribute to say "not > > > > supported". (Although to be entirely fair, we could just put a message > > > > in the kernel log during probe if the hardware doesn't support runtime > > > > suspend.) > > > > > > > > Instead, Dan introduced a messy PM QoS mechanism in commit > > > > e3d105055525. I didn't like that approach, but Greg merged it before I > > > > objected. > > > > > > That really looks a bit like a hack to me to be honest. My thought exactly. > > > Greg, what's your plan toward this? > > > > If I need to revert something that you all find was wrong, I'll be glad > > to do so, sorry for merging something too early. > > Alan, what do you think? I'm in favor of reverting that commit and putting something else in its place. But first we have to figure out an approach that will satisfy everyone. I've been hoping that Dan would contribute to this thread. He has spear-headed this whole line of development, and he wrote the commit in question. Maybe he would be happy with the simplest approach: If the device doesn't support runtime PM, write a message in the kernel log and make ->runtime_suspend() always return -EBUSY. > I think we're still unsure if the approach taken by that commit is correct, > but then I suppose we don't need to revert it at this point and we can fix > it later. Is that correct, or would fixing it be difficult for some reason? As far as I can see, it would be okay to wait a while before making any changes. 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 Thursday, July 17, 2014 10:27:44 AM Alan Stern wrote: > On Thu, 17 Jul 2014, Rafael J. Wysocki wrote: > > > On Wednesday, July 16, 2014 04:03:45 PM Greg Kroah-Hartman wrote: > > > On Thu, Jul 17, 2014 at 12:40:23AM +0200, Rafael J. Wysocki wrote: > > > > On Wednesday, July 02, 2014 10:27:06 AM Alan Stern wrote: > > > > > Here's a brief summary of the story behind this patch... > > > > > > > > > > At one point, I suggested to Dan that instead of doing something > > > > > special for these devices, we could simply have the runtime_suspend() > > > > > routine always return -EBUSY. He didn't like that idea because then > > > > > the user would see the device was never powering down but would have no > > > > > idea why. The rpm_not_supported flag provides this information to the > > > > > user by causing the power/runtime_status attribute to say "not > > > > > supported". (Although to be entirely fair, we could just put a message > > > > > in the kernel log during probe if the hardware doesn't support runtime > > > > > suspend.) > > > > > > > > > > Instead, Dan introduced a messy PM QoS mechanism in commit > > > > > e3d105055525. I didn't like that approach, but Greg merged it before I > > > > > objected. > > > > > > > > That really looks a bit like a hack to me to be honest. > > My thought exactly. I'm also wondering why it unregisters the device if dev_pm_qos_add_request() fails instead simply avoiding to enable runtime PM in that case (just a side note). > > > > Greg, what's your plan toward this? > > > > > > If I need to revert something that you all find was wrong, I'll be glad > > > to do so, sorry for merging something too early. > > > > Alan, what do you think? > > I'm in favor of reverting that commit and putting something else in its > place. But first we have to figure out an approach that will satisfy > everyone. > > I've been hoping that Dan would contribute to this thread. He has > spear-headed this whole line of development, and he wrote the commit in > question. > > Maybe he would be happy with the simplest approach: If the device > doesn't support runtime PM, write a message in the kernel log and make > ->runtime_suspend() always return -EBUSY. And I wonder how commit e3d105055525 allows the user to check that the device is in the "always active" mode. I guess by the fact that the PM QoS flags are not visible in sysfs then, but that's far from obvious in my view. 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
Index: usb-3.16/Documentation/power/runtime_pm.txt =================================================================== --- usb-3.16.orig/Documentation/power/runtime_pm.txt +++ usb-3.16/Documentation/power/runtime_pm.txt @@ -233,6 +233,10 @@ defined in include/linux/pm.h: equal to zero); the initial value of it is 1 (i.e. runtime PM is initially disabled for all devices) + int rpm_not_supported; + - if set, the device does not support runtime power management; attempts to + suspend the device will fail with -EOPNOTSUPP + int runtime_error; - if set, there was a fatal error (one of the callbacks returned error code as described in Section 2), so the helper funtions will not work until @@ -419,6 +423,11 @@ drivers/base/power/runtime.c and include void pm_suspend_ignore_children(struct device *dev, bool enable); - set/unset the power.ignore_children flag of the device + void pm_runtime_not_supported(struct device *dev); + - set the device's 'power.rpm_not_supported' flag, set the device's runtime + status to 'active', and increment the device's usage counter; this action + is irreversible + int pm_runtime_set_active(struct device *dev); - clear the device's 'power.runtime_error' flag, set the device's runtime PM status to 'active' and update its parent's counter of 'active' @@ -432,7 +441,7 @@ drivers/base/power/runtime.c and include PM status to 'suspended' and update its parent's counter of 'active' children as appropriate (it is only valid to use this function if 'power.runtime_error' is set or 'power.disable_depth' is greater than - zero) + zero, and 'power.rpm_not_supported' must be zero) bool pm_runtime_active(struct device *dev); - return true if the device's runtime PM status is 'active' or its @@ -586,6 +595,15 @@ value of /sys/devices/.../power/control manage the device at run time, the driver may confuse it by using pm_runtime_forbid() this way. +If a driver supports runtime PM but one of its devices does not, the driver +can call pm_runtime_not_supported() before calling pm_runtime_enable(). After +that, calls to pm_runtime_resume() and related functions will succeed, but +attempts to suspend the device will fail (much like what happens after calling +pm_runtime_forbid(), except that pm_runtime_not_supported() cannot be undone by +either the kernel or the user). It is not necessary to call +pm_runtime_set_active() for the device; pm_runtime_not_supported() sets the +runtime PM status to 'active'. + 6. Runtime PM and System Sleep Runtime PM and system sleep (i.e., system suspend and hibernation, also known Index: usb-3.16/include/linux/pm.h =================================================================== --- usb-3.16.orig/include/linux/pm.h +++ usb-3.16/include/linux/pm.h @@ -584,6 +584,7 @@ struct dev_pm_info { atomic_t usage_count; atomic_t child_count; unsigned int disable_depth:3; + unsigned int rpm_not_supported:1; unsigned int idle_notification:1; unsigned int request_pending:1; unsigned int deferred_resume:1; Index: usb-3.16/include/linux/pm_runtime.h =================================================================== --- usb-3.16.orig/include/linux/pm_runtime.h +++ usb-3.16/include/linux/pm_runtime.h @@ -47,6 +47,7 @@ extern int __pm_runtime_set_status(struc extern int pm_runtime_barrier(struct device *dev); extern void pm_runtime_enable(struct device *dev); extern void __pm_runtime_disable(struct device *dev, bool check_resume); +extern void pm_runtime_not_supported(struct device *dev); extern void pm_runtime_allow(struct device *dev); extern void pm_runtime_forbid(struct device *dev); extern void pm_runtime_no_callbacks(struct device *dev); @@ -144,6 +145,7 @@ static inline int __pm_runtime_set_statu static inline int pm_runtime_barrier(struct device *dev) { return 0; } static inline void pm_runtime_enable(struct device *dev) {} static inline void __pm_runtime_disable(struct device *dev, bool c) {} +static inline void pm_runtime_not_supported(struct device *dev) {} static inline void pm_runtime_allow(struct device *dev) {} static inline void pm_runtime_forbid(struct device *dev) {} Index: usb-3.16/drivers/base/power/runtime.c =================================================================== --- usb-3.16.orig/drivers/base/power/runtime.c +++ usb-3.16/drivers/base/power/runtime.c @@ -239,7 +239,9 @@ static int rpm_check_suspend_allowed(str { int retval = 0; - if (dev->power.runtime_error) + if (dev->power.rpm_not_supported) + retval = -EOPNOTSUPP; + else if (dev->power.runtime_error) retval = -EINVAL; else if (dev->power.disable_depth > 0) retval = -EACCES; @@ -974,7 +976,9 @@ EXPORT_SYMBOL_GPL(__pm_runtime_resume); * RPM_SUSPENDED, as long as that reflects the actual state of the device. * However, if the device has a parent and the parent is not active, and the * parent's power.ignore_children flag is unset, the device's status cannot be - * set to RPM_ACTIVE, so -EBUSY is returned in that case. + * set to RPM_ACTIVE, so -EBUSY is returned in that case. If the device's + * power.rpm_not_supported flag is set then the device's status cannot be set + * to RPM_SUSPENDED, so -EOPNOTSUPP is returned in that case. * * If successful, __pm_runtime_set_status() clears the power.runtime_error field * and the device parent's counter of unsuspended children is modified to @@ -1002,6 +1006,11 @@ int __pm_runtime_set_status(struct devic goto out_set; if (status == RPM_SUSPENDED) { + if (dev->power.rpm_not_supported) { + error = -EOPNOTSUPP; + goto out; + } + /* It always is possible to set the status to 'suspended'. */ if (parent) { atomic_add_unless(&parent->power.child_count, -1, 0); @@ -1195,6 +1204,26 @@ void pm_runtime_enable(struct device *de EXPORT_SYMBOL_GPL(pm_runtime_enable); /** + * pm_runtime_not_supported - A device doesn't support runtime PM + * @dev: Device to handle. + * + * Set @dev->power.rpm_not_supported, indicating the @dev doesn't support + * runtime PM and consequently must always be in the RPM_ACTIVE state. + * + * For good measure, also set runtime_status to RPM_ACTIVE and increment + * usage_count. + */ +void pm_runtime_not_supported(struct device *dev) +{ + spin_lock_irq(&dev->power.lock); + dev->power.rpm_not_supported = 1; + dev->power.runtime_status = RPM_ACTIVE; + atomic_inc(&dev->power.usage_count); + spin_unlock_irq(&dev->power.lock); +} +EXPORT_SYMBOL_GPL(pm_runtime_not_supported); + +/** * pm_runtime_forbid - Block runtime PM of a device. * @dev: Device to handle. * @@ -1415,6 +1444,9 @@ void pm_runtime_remove(struct device *de * * Typically this function may be invoked from a system suspend callback to make * sure the device is put into low power state. + * + * If the device's power.rpm_not_supported flag is set, the result of calling + * this function is undefined. */ int pm_runtime_force_suspend(struct device *dev) { Index: usb-3.16/drivers/base/power/sysfs.c =================================================================== --- usb-3.16.orig/drivers/base/power/sysfs.c +++ usb-3.16/drivers/base/power/sysfs.c @@ -163,10 +163,12 @@ static ssize_t rtpm_status_show(struct d { const char *p; - if (dev->power.runtime_error) { + if (dev->power.rpm_not_supported) { + p = "not supported\n"; + } else if (dev->power.runtime_error) { p = "error\n"; } else if (dev->power.disable_depth) { - p = "unsupported\n"; + p = "disabled\n"; } else { switch (dev->power.runtime_status) { case RPM_SUSPENDED: