Re: [PATCH] mmc: rtsx_usb_sdmmc: Handle runtime PM while changing led
On Thu, 22 Sep 2016, Oliver Neukum wrote: > On Wed, 2016-09-21 at 10:35 -0400, Alan Stern wrote: > > On Wed, 21 Sep 2016, Oliver Neukum wrote: > > > > Yes, but this is not the point. A heuristic with a timeout makes > > > sense only if the uses are unpredictable. If you know with a high > > > degree of probability when the next activity comes, you ought to either > > > suspend now or not all until the next activity. > > > > > > Likewise the heuristic is appropriate for leaf nodes. You get nothing > > > from a delay on inner nodes. > > > > Almost true, but not quite. When an inner node has more than one leaf > > beneath it, enabling an autosuspend delay for the inner node can make > > sense -- particularly if the leaf activities are uncorrelated. > > > Well, it is true that an inner node is likelier to be woken up > depending on the number of children. That is a reason to have a longer > timeout for an inner node. But it should start when the first node goes > idle. It makes no sense to start yet another timeout when the last node > goes idle. > > In terms of mathematics I think we would need to multiply the timeout > with the square root of busy children and restart it whenever a child > goes to idle. > But it seems to me that this is impractical. > > So I would suggest that we are missing an API for drivers to tell the > core that they become idle for a known period of time and to propagate > that immediately up if that is the last leaf to become idle. This seems like over-engineering. In many cases the driver does not know how long the idle period will last. And it may happen that the leaf drivers do not understand the energy tradeoffs involved in suspending the ancestor device. > > > Any storage (generic sense) device > > > is an inner node. It should suspend immediately after the block > > > device which is the leaf node. > > > > Yes. In this case, however, the USB device has two platform devices > > beneath it: one for SDMMC and one for MemoryStick cards. > > Indeed, we can hope that the power efficient work queue used will > join the polling of both devices. Ideally we could model the mutual > exclusion. Ulf is going to work on it. 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: [PATCH] mmc: rtsx_usb_sdmmc: Handle runtime PM while changing led
On Thu, 22 Sep 2016, Ulf Hansson wrote: > >> > > An observation I made, is when the sdmmc device gets runtime resumed > >> > > (pm_runtime_get_sync()), the parent device (the usb device) may also > >> > > become runtime resumed (unless it's already). In this sequence, but > >> > > *only* when actually runtime resuming the usb device, the runtime PM > >> > > core decides to update the last busy mark for the usb device. Should > >> > > it really do that? > >> > > >> > Yes, that's deliberate. The whole idea of autosuspend is to prevent > >> > the device from being runtime-suspended too soon after it was used, and > >> > the PM core considers runtime-resume to be a form of usage. > >> > >> I understand it's deliberate, but I was more question whether we > >> actually should have the runtime PM core to behaves like this. > >> > >> I don't think its behaviour is consistent, as in such case all calls > >> to __pm_runtime_resume() should trigger the runtime PM core to update > >> the last busy mark. > > > > Not a bad idea... > > Yes, it is. :-) > > Although, I am still concerned about he inconsistent behaviour. > > > > >> Don't get me wrong, I am *not* suggesting we should do that change, as > >> it would mean the last busy mark would be updated way too often. > > > > The updates aren't very expensive. Just one atomic write. It probably > > takes less time than acquiring the runtime-PM spinlock. > > > >> Instead, perhaps it's better to leave the responsibility of updating > >> the last busy mark to the runtime PM users solely. > > > > Maybe, but I think doing it once in the core, like this, can remove the > > need for a lot of function calls in drivers. > > Unfortunate not. Most updates of the last busy mark happens when a > device is no longer required to be runtime resumed. As when a driver > has completed to serve a request and is about to call pm_runtime_put() > (or similar API). > > So, I still believe doing it in the runtime PM core is just a waste. > > I think it's better to leave the update to the users entirely, it > would become consistent but also more flexible, as one could easily > think of situations where you may in some cases want to update the > last busy mark and in some other not. You can try making this change if you want. I'd be afraid of regressions. 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: [PATCH] mmc: rtsx_usb_sdmmc: Handle runtime PM while changing led
On Wed, 2016-09-21 at 10:35 -0400, Alan Stern wrote: > On Wed, 21 Sep 2016, Oliver Neukum wrote: > > Yes, but this is not the point. A heuristic with a timeout makes > > sense only if the uses are unpredictable. If you know with a high > > degree of probability when the next activity comes, you ought to either > > suspend now or not all until the next activity. > > > > Likewise the heuristic is appropriate for leaf nodes. You get nothing > > from a delay on inner nodes. > > Almost true, but not quite. When an inner node has more than one leaf > beneath it, enabling an autosuspend delay for the inner node can make > sense -- particularly if the leaf activities are uncorrelated. Well, it is true that an inner node is likelier to be woken up depending on the number of children. That is a reason to have a longer timeout for an inner node. But it should start when the first node goes idle. It makes no sense to start yet another timeout when the last node goes idle. In terms of mathematics I think we would need to multiply the timeout with the square root of busy children and restart it whenever a child goes to idle. But it seems to me that this is impractical. So I would suggest that we are missing an API for drivers to tell the core that they become idle for a known period of time and to propagate that immediately up if that is the last leaf to become idle. > > Any storage (generic sense) device > > is an inner node. It should suspend immediately after the block > > device which is the leaf node. > > Yes. In this case, however, the USB device has two platform devices > beneath it: one for SDMMC and one for MemoryStick cards. Indeed, we can hope that the power efficient work queue used will join the polling of both devices. Ideally we could model the mutual exclusion. Regards Oliver -- 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: [PATCH] mmc: rtsx_usb_sdmmc: Handle runtime PM while changing led
On 21 September 2016 at 16:45, Alan Stern wrote: > On Wed, 21 Sep 2016, Ulf Hansson wrote: > >> > > My concern is also 2s autosuspend timeout which is set for the usb >> > > device. Somehow I feel we need to be able "share" more information >> > > between a parent-child relationship, in this case between the sdmmc >> > > device and the usb device. >> > >> > I agree, but it's not clear how this should be done. One easy solution >> > would be to turn off USB autosuspend and do all the runtime-PM >> > management in the sdmmc and memstick drivers. >> >> Hmm, this could be a very good option. In the end the sdmmc/memstick >> drivers knows best about which autosuspend timeout to use. > > I tend to agree, and so does Oliver. Okay. > >> > > An observation I made, is when the sdmmc device gets runtime resumed >> > > (pm_runtime_get_sync()), the parent device (the usb device) may also >> > > become runtime resumed (unless it's already). In this sequence, but >> > > *only* when actually runtime resuming the usb device, the runtime PM >> > > core decides to update the last busy mark for the usb device. Should >> > > it really do that? >> > >> > Yes, that's deliberate. The whole idea of autosuspend is to prevent >> > the device from being runtime-suspended too soon after it was used, and >> > the PM core considers runtime-resume to be a form of usage. >> >> I understand it's deliberate, but I was more question whether we >> actually should have the runtime PM core to behaves like this. >> >> I don't think its behaviour is consistent, as in such case all calls >> to __pm_runtime_resume() should trigger the runtime PM core to update >> the last busy mark. > > Not a bad idea... Yes, it is. :-) Although, I am still concerned about he inconsistent behaviour. > >> Don't get me wrong, I am *not* suggesting we should do that change, as >> it would mean the last busy mark would be updated way too often. > > The updates aren't very expensive. Just one atomic write. It probably > takes less time than acquiring the runtime-PM spinlock. > >> Instead, perhaps it's better to leave the responsibility of updating >> the last busy mark to the runtime PM users solely. > > Maybe, but I think doing it once in the core, like this, can remove the > need for a lot of function calls in drivers. Unfortunate not. Most updates of the last busy mark happens when a device is no longer required to be runtime resumed. As when a driver has completed to serve a request and is about to call pm_runtime_put() (or similar API). So, I still believe doing it in the runtime PM core is just a waste. I think it's better to leave the update to the users entirely, it would become consistent but also more flexible, as one could easily think of situations where you may in some cases want to update the last busy mark and in some other not. > >> > > If we assume that the usb device shouldn't be used with a timeout less >> > > than 2s, then I think we have two options: >> > > >> > > *) As the mmc polling timeout is 1s, there is really no point in >> > > trying to runtime suspend the usb device, it may just be left runtime >> > > resumed all the time. Wasting power, of course! >> > >> > Or we can decrease the USB autosuspend delay to 100 ms. >> >> Yes, something like that makes sense to me. >> >> Unless we decide to turn off autosuspend completely for the usb host >> as you suggested above. Then it would really become clear that the >> sdmmc/memstick drivers gets the responsible for the autosuspend, which >> certainly makes most sense. > > Yes. Okay. I will submit a series with the relevant changes we have come up with during the discussions. I will keep you posted. > >> > > **) Add an interface to allow dynamically changes of the mmc polling >> > > timeout to better suit the current user. >> > >> > Note that the block layer does its own polling for removable media, and >> > it already has a sysfs interface to control the polling interval (or >> > disable it entirely). But I don't know how the MMC stack interacts >> > with the block layer. >> > >> > One awkward point is that the sdmmc and memstick drivers each do their >> > own polling. This is a waste. You can see it in the usbmon trace; >> > every second there are two query-response interactions. Even if >> > there's no good way to reduce the number, we should at least try to >> > synchronize the polls so that the device doesn't need to be resumed >> > twice every second. >> >> Yes, you are right. I just haven't been able to prioritize doing that >> change for MMC. Another thing added on my mmc TODO list. :-) > > To tell the truth, I'm not sure how you would synchronize the polling > activities in the sdmmc and memstick drivers. Move most of it into the > upper MFD driver? I don't know, you may be right! I will have to think about it. > > One point worth mentioning is that if you already know an SDMMC card is > present then there's no reason to poll for a MemoryStick card, and vice > versa. Y
Re: [PATCH] mmc: rtsx_usb_sdmmc: Handle runtime PM while changing led
On Wed, 21 Sep 2016, Ulf Hansson wrote: > > > My concern is also 2s autosuspend timeout which is set for the usb > > > device. Somehow I feel we need to be able "share" more information > > > between a parent-child relationship, in this case between the sdmmc > > > device and the usb device. > > > > I agree, but it's not clear how this should be done. One easy solution > > would be to turn off USB autosuspend and do all the runtime-PM > > management in the sdmmc and memstick drivers. > > Hmm, this could be a very good option. In the end the sdmmc/memstick > drivers knows best about which autosuspend timeout to use. I tend to agree, and so does Oliver. > > > An observation I made, is when the sdmmc device gets runtime resumed > > > (pm_runtime_get_sync()), the parent device (the usb device) may also > > > become runtime resumed (unless it's already). In this sequence, but > > > *only* when actually runtime resuming the usb device, the runtime PM > > > core decides to update the last busy mark for the usb device. Should > > > it really do that? > > > > Yes, that's deliberate. The whole idea of autosuspend is to prevent > > the device from being runtime-suspended too soon after it was used, and > > the PM core considers runtime-resume to be a form of usage. > > I understand it's deliberate, but I was more question whether we > actually should have the runtime PM core to behaves like this. > > I don't think its behaviour is consistent, as in such case all calls > to __pm_runtime_resume() should trigger the runtime PM core to update > the last busy mark. Not a bad idea... > Don't get me wrong, I am *not* suggesting we should do that change, as > it would mean the last busy mark would be updated way too often. The updates aren't very expensive. Just one atomic write. It probably takes less time than acquiring the runtime-PM spinlock. > Instead, perhaps it's better to leave the responsibility of updating > the last busy mark to the runtime PM users solely. Maybe, but I think doing it once in the core, like this, can remove the need for a lot of function calls in drivers. > > > If we assume that the usb device shouldn't be used with a timeout less > > > than 2s, then I think we have two options: > > > > > > *) As the mmc polling timeout is 1s, there is really no point in > > > trying to runtime suspend the usb device, it may just be left runtime > > > resumed all the time. Wasting power, of course! > > > > Or we can decrease the USB autosuspend delay to 100 ms. > > Yes, something like that makes sense to me. > > Unless we decide to turn off autosuspend completely for the usb host > as you suggested above. Then it would really become clear that the > sdmmc/memstick drivers gets the responsible for the autosuspend, which > certainly makes most sense. Yes. > > > **) Add an interface to allow dynamically changes of the mmc polling > > > timeout to better suit the current user. > > > > Note that the block layer does its own polling for removable media, and > > it already has a sysfs interface to control the polling interval (or > > disable it entirely). But I don't know how the MMC stack interacts > > with the block layer. > > > > One awkward point is that the sdmmc and memstick drivers each do their > > own polling. This is a waste. You can see it in the usbmon trace; > > every second there are two query-response interactions. Even if > > there's no good way to reduce the number, we should at least try to > > synchronize the polls so that the device doesn't need to be resumed > > twice every second. > > Yes, you are right. I just haven't been able to prioritize doing that > change for MMC. Another thing added on my mmc TODO list. :-) To tell the truth, I'm not sure how you would synchronize the polling activities in the sdmmc and memstick drivers. Move most of it into the upper MFD driver? One point worth mentioning is that if you already know an SDMMC card is present then there's no reason to poll for a MemoryStick card, and vice versa. 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: [PATCH] mmc: rtsx_usb_sdmmc: Handle runtime PM while changing led
On Wed, 21 Sep 2016, Oliver Neukum wrote: > On Tue, 2016-09-20 at 10:12 -0400, Alan Stern wrote: > > On Tue, 20 Sep 2016, Oliver Neukum wrote: > > > That shouldn't be an issue in this case, at least, not with the current > > code. The sdmmc and memstick drivers block autosuspend if media is > > present. > > Good. > > > > > > Which means that autosuspend matters only when a card isn't present, > > > > and the host is polled every second or so to see whether a card has > > > > been inserted. > > > > > > > > Under those circumstances you probably don't want to use > > > > autosuspend. > > > > That is, resuming before each poll and suspending afterward may use > > > > less energy than staying at full power all the time. > > > > > > Is that based on concrete figures about power consumption? > > > > No. > > Well, I have no idea how to improve this much without hideous > overengineering. > > > > And it seems to me that we need a way to indicate that the heuristics > > > should not be used, but a device immediately suspended. The timer > > > is sensible only if the next wakeup is unknown. > > > > The driver can always turn off autosuspend if it wants to. > > Yes, but this is not the point. A heuristic with a timeout makes > sense only if the uses are unpredictable. If you know with a high > degree of probability when the next activity comes, you ought to either > suspend now or not all until the next activity. > > Likewise the heuristic is appropriate for leaf nodes. You get nothing > from a delay on inner nodes. Almost true, but not quite. When an inner node has more than one leaf beneath it, enabling an autosuspend delay for the inner node can make sense -- particularly if the leaf activities are uncorrelated. > Any storage (generic sense) device > is an inner node. It should suspend immediately after the block > device which is the leaf node. Yes. In this case, however, the USB device has two platform devices beneath it: one for SDMMC and one for MemoryStick cards. 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: [PATCH] mmc: rtsx_usb_sdmmc: Handle runtime PM while changing led
On 20 September 2016 at 16:09, Alan Stern wrote: > > On Tue, 20 Sep 2016, Ulf Hansson wrote: > > > >> I am wondering what you think would be a good autosuspend timeout in > > >> this case? It seems to me that the only thing the rtsx driver really > > >> care about is to tell the parent device that it needs to be runtime > > >> resumed during a certain timeframe, more or less it would like to > > >> inherit the parents settings. > > >> > > >> Other mmc hosts, not being usb-mmc devices, are using an autosuspend > > >> timeout of ~50-100ms but that doesn't seem like good value here, > > >> right? > > > > > > Well, if you decide to let the device go into runtime suspend between > > > polls then there's no reason to use autosuspend at all. Once a poll > > > has ended, you know there won't be any more activity until the next > > > poll. > > > > We could change that, as currently the approach in the mmc core isn't > > that sophisticated. I even think this has been discussed earlier for > > the very similar reasons regards polling card detect mode. > > > > I guess the main reason to why we yet have changed this, is because > > mmc host drivers are using an autosuspend timeout of ~50-100 ms, so in > > the end it haven't been such a big deal. > > No, it isn't. Although at a polling interval of 1 second, it means > reducing the low-power time by as much as 10%. Yes, I agree we shouldn't neglect its impact - especially in this usb use case. I will put it on my TODO list for mmc PM things. > > > > On the other hand, if you decide to keep the device at full power all > > > the time during polling, then any autosuspend timeout larger than 1000 > > > ms would do what you want. > > > > > > Mostly I'm concerned about how this will interact with the USB runtime > > > PM. The thing is, suspending the sdmmc device doesn't save any energy, > > > whereas suspending the USB device does. > > > > Yes, I agree. > > > > My concern is also 2s autosuspend timeout which is set for the usb > > device. Somehow I feel we need to be able "share" more information > > between a parent-child relationship, in this case between the sdmmc > > device and the usb device. > > I agree, but it's not clear how this should be done. One easy solution > would be to turn off USB autosuspend and do all the runtime-PM > management in the sdmmc and memstick drivers. Hmm, this could be a very good option. In the end the sdmmc/memstick drivers knows best about which autosuspend timeout to use. > > > An observation I made, is when the sdmmc device gets runtime resumed > > (pm_runtime_get_sync()), the parent device (the usb device) may also > > become runtime resumed (unless it's already). In this sequence, but > > *only* when actually runtime resuming the usb device, the runtime PM > > core decides to update the last busy mark for the usb device. Should > > it really do that? > > Yes, that's deliberate. The whole idea of autosuspend is to prevent > the device from being runtime-suspended too soon after it was used, and > the PM core considers runtime-resume to be a form of usage. I understand it's deliberate, but I was more question whether we actually should have the runtime PM core to behaves like this. I don't think its behaviour is consistent, as in such case all calls to __pm_runtime_resume() should trigger the runtime PM core to update the last busy mark. Don't get me wrong, I am *not* suggesting we should do that change, as it would mean the last busy mark would be updated way too often. Instead, perhaps it's better to leave the responsibility of updating the last busy mark to the runtime PM users solely. > > > Moreover, I am curious about the 2s usb timeout. Why isn't that chosen > > to something like ~100ms instead? Is there is a long latency to > > runtime resume the usb device or because we fear to wear out the HW, > > which may be powered on/off too frequently? > > When I first implemented runtime PM for the USB stack, I had to choose > a autosuspend timeout. Not having any basis for such a choice, and > figuring that a suspend-resume cycle takes around 100 ms, I decided > that 2 seconds would be a reasonable value. But it's just a default; > drivers and userspace can change it whenever they want. Okay, I see. Thanks for sharing that information. > > > If we assume that the usb device shouldn't be used with a timeout less > > than 2s, then I think we have two options: > > > > *) As the mmc polling timeout is 1s, there is really no point in > > trying to runtime suspend the usb device, it may just be left runtime > > resumed all the time. Wasting power, of course! > > Or we can decrease the USB autosuspend delay to 100 ms. Yes, something like that makes sense to me. Unless we decide to turn off autosuspend completely for the usb host as you suggested above. Then it would really become clear that the sdmmc/memstick drivers gets the responsible for the autosuspend, which certainly makes most sense. > > > **) Add an interface to all
Re: [PATCH] mmc: rtsx_usb_sdmmc: Handle runtime PM while changing led
On Tue, 2016-09-20 at 10:12 -0400, Alan Stern wrote: > On Tue, 20 Sep 2016, Oliver Neukum wrote: > That shouldn't be an issue in this case, at least, not with the current > code. The sdmmc and memstick drivers block autosuspend if media is > present. Good. > > > > Which means that autosuspend matters only when a card isn't present, > > > and the host is polled every second or so to see whether a card has > > > been inserted. > > > > > > Under those circumstances you probably don't want to use > > > autosuspend. > > > That is, resuming before each poll and suspending afterward may use > > > less energy than staying at full power all the time. > > > > Is that based on concrete figures about power consumption? > > No. Well, I have no idea how to improve this much without hideous overengineering. > > And it seems to me that we need a way to indicate that the heuristics > > should not be used, but a device immediately suspended. The timer > > is sensible only if the next wakeup is unknown. > > The driver can always turn off autosuspend if it wants to. Yes, but this is not the point. A heuristic with a timeout makes sense only if the uses are unpredictable. If you know with a high degree of probability when the next activity comes, you ought to either suspend now or not all until the next activity. Likewise the heuristic is appropriate for leaf nodes. You get nothing from a delay on inner nodes. Any storage (generic sense) device is an inner node. It should suspend immediately after the block device which is the leaf node. Regards Oliver -- 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: [PATCH] mmc: rtsx_usb_sdmmc: Handle runtime PM while changing led
On Tue, 20 Sep 2016, Oliver Neukum wrote: > On Mon, 2016-09-19 at 14:02 -0400, Alan Stern wrote: > > > We can for sure enable autosuspend for the sdmmc device, although as > > > soon as an SD card will be detected the rtsx driver will increase > > the > > > runtime PM usage count. The count is decreased when the card is > > > removed (or failed to be initialized), thus runtime suspend is > > > prevented as long as there is a functional card inserted. > > Testing autosuspend with card readers on usb_storage I saw a uniform > response of reporting a medium change event upon resume. > I am afraid other kinds of readers are not better in that regard. That shouldn't be an issue in this case, at least, not with the current code. The sdmmc and memstick drivers block autosuspend if media is present. > > Which means that autosuspend matters only when a card isn't present, > > and the host is polled every second or so to see whether a card has > > been inserted. > > > > Under those circumstances you probably don't want to use > > autosuspend. > > That is, resuming before each poll and suspending afterward may use > > less energy than staying at full power all the time. > > Is that based on concrete figures about power consumption? No. > And it seems to me that we need a way to indicate that the heuristics > should not be used, but a device immediately suspended. The timer > is sensible only if the next wakeup is unknown. The driver can always turn off autosuspend if it wants to. 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: [PATCH] mmc: rtsx_usb_sdmmc: Handle runtime PM while changing led
On Tue, 20 Sep 2016, Ulf Hansson wrote: > >> I am wondering what you think would be a good autosuspend timeout in > >> this case? It seems to me that the only thing the rtsx driver really > >> care about is to tell the parent device that it needs to be runtime > >> resumed during a certain timeframe, more or less it would like to > >> inherit the parents settings. > >> > >> Other mmc hosts, not being usb-mmc devices, are using an autosuspend > >> timeout of ~50-100ms but that doesn't seem like good value here, > >> right? > > > > Well, if you decide to let the device go into runtime suspend between > > polls then there's no reason to use autosuspend at all. Once a poll > > has ended, you know there won't be any more activity until the next > > poll. > > We could change that, as currently the approach in the mmc core isn't > that sophisticated. I even think this has been discussed earlier for > the very similar reasons regards polling card detect mode. > > I guess the main reason to why we yet have changed this, is because > mmc host drivers are using an autosuspend timeout of ~50-100 ms, so in > the end it haven't been such a big deal. No, it isn't. Although at a polling interval of 1 second, it means reducing the low-power time by as much as 10%. > > On the other hand, if you decide to keep the device at full power all > > the time during polling, then any autosuspend timeout larger than 1000 > > ms would do what you want. > > > > Mostly I'm concerned about how this will interact with the USB runtime > > PM. The thing is, suspending the sdmmc device doesn't save any energy, > > whereas suspending the USB device does. > > Yes, I agree. > > My concern is also 2s autosuspend timeout which is set for the usb > device. Somehow I feel we need to be able "share" more information > between a parent-child relationship, in this case between the sdmmc > device and the usb device. I agree, but it's not clear how this should be done. One easy solution would be to turn off USB autosuspend and do all the runtime-PM management in the sdmmc and memstick drivers. > An observation I made, is when the sdmmc device gets runtime resumed > (pm_runtime_get_sync()), the parent device (the usb device) may also > become runtime resumed (unless it's already). In this sequence, but > *only* when actually runtime resuming the usb device, the runtime PM > core decides to update the last busy mark for the usb device. Should > it really do that? Yes, that's deliberate. The whole idea of autosuspend is to prevent the device from being runtime-suspended too soon after it was used, and the PM core considers runtime-resume to be a form of usage. > Moreover, I am curious about the 2s usb timeout. Why isn't that chosen > to something like ~100ms instead? Is there is a long latency to > runtime resume the usb device or because we fear to wear out the HW, > which may be powered on/off too frequently? When I first implemented runtime PM for the USB stack, I had to choose a autosuspend timeout. Not having any basis for such a choice, and figuring that a suspend-resume cycle takes around 100 ms, I decided that 2 seconds would be a reasonable value. But it's just a default; drivers and userspace can change it whenever they want. > If we assume that the usb device shouldn't be used with a timeout less > than 2s, then I think we have two options: > > *) As the mmc polling timeout is 1s, there is really no point in > trying to runtime suspend the usb device, it may just be left runtime > resumed all the time. Wasting power, of course! Or we can decrease the USB autosuspend delay to 100 ms. > **) Add an interface to allow dynamically changes of the mmc polling > timeout to better suit the current user. Note that the block layer does its own polling for removable media, and it already has a sysfs interface to control the polling interval (or disable it entirely). But I don't know how the MMC stack interacts with the block layer. One awkward point is that the sdmmc and memstick drivers each do their own polling. This is a waste. You can see it in the usbmon trace; every second there are two query-response interactions. Even if there's no good way to reduce the number, we should at least try to synchronize the polls so that the device doesn't need to be resumed twice every second. 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: [PATCH] mmc: rtsx_usb_sdmmc: Handle runtime PM while changing led
On Mon, 2016-09-19 at 14:02 -0400, Alan Stern wrote: > > We can for sure enable autosuspend for the sdmmc device, although as > > soon as an SD card will be detected the rtsx driver will increase > the > > runtime PM usage count. The count is decreased when the card is > > removed (or failed to be initialized), thus runtime suspend is > > prevented as long as there is a functional card inserted. Testing autosuspend with card readers on usb_storage I saw a uniform response of reporting a medium change event upon resume. I am afraid other kinds of readers are not better in that regard. > > Which means that autosuspend matters only when a card isn't present, > and the host is polled every second or so to see whether a card has > been inserted. > > Under those circumstances you probably don't want to use > autosuspend. > That is, resuming before each poll and suspending afterward may use > less energy than staying at full power all the time. Is that based on concrete figures about power consumption? And it seems to me that we need a way to indicate that the heuristics should not be used, but a device immediately suspended. The timer is sensible only if the next wakeup is unknown. Regards Oliver -- 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: [PATCH] mmc: rtsx_usb_sdmmc: Handle runtime PM while changing led
On 19 September 2016 at 20:02, Alan Stern wrote: > On Mon, 19 Sep 2016, Ulf Hansson wrote: > >> On 18 September 2016 at 04:30, Alan Stern wrote: >> > On Sat, 17 Sep 2016, Ulf Hansson wrote: >> > >> >> Each access of the parent device (usb device) needs to be done in runtime >> >> resumed state. Currently this isn't case while changing the leds, so let's >> >> add pm_runtime_get_sync() and pm_runtime_put() around these calls. >> >> >> >> Signed-off-by: Ulf Hansson >> >> --- >> >> >> >> While discussing an issue[1] related to runtime PM, I found out that this >> >> minor change at least improves the behavior that has been observed. >> >> >> >> [1] >> >> http://www.spinics.net/lists/linux-usb/msg144634.html >> >> >> >> --- >> >> drivers/mmc/host/rtsx_usb_sdmmc.c | 2 ++ >> >> 1 file changed, 2 insertions(+) >> >> >> >> diff --git a/drivers/mmc/host/rtsx_usb_sdmmc.c >> >> b/drivers/mmc/host/rtsx_usb_sdmmc.c >> >> index 6c71fc9..a59c7fa 100644 >> >> --- a/drivers/mmc/host/rtsx_usb_sdmmc.c >> >> +++ b/drivers/mmc/host/rtsx_usb_sdmmc.c >> >> @@ -1314,6 +1314,7 @@ static void rtsx_usb_update_led(struct work_struct >> >> *work) >> >> container_of(work, struct rtsx_usb_sdmmc, led_work); >> >> struct rtsx_ucr *ucr = host->ucr; >> >> >> >> + pm_runtime_get_sync(sdmmc_dev(host)); >> >> mutex_lock(&ucr->dev_mutex); >> >> >> >> if (host->led.brightness == LED_OFF) >> >> @@ -1322,6 +1323,7 @@ static void rtsx_usb_update_led(struct work_struct >> >> *work) >> >> rtsx_usb_turn_on_led(ucr); >> >> >> >> mutex_unlock(&ucr->dev_mutex); >> >> + pm_runtime_put(sdmmc_dev(host)); >> >> } >> >> #endif >> > >> > The missing aspect here is that this won't stop the parent USB device >> > from going into autosuspend every 2 seconds and then resuming shortly >> > afterward. There are two ways of preventing this: >> > >> > Call usb_mark_last_busy() at appropriate places. >> > >> > Enable autosuspend for the sdmmc device. >> > >> > The second approach would also prevent the sdmmc device from going into >> > autosuspend as soon as the LED update is finished. Maybe that's okay, >> > but if going into suspend is a lightweight procedure then you may want >> > to prevent it. >> > >> >> We can for sure enable autosuspend for the sdmmc device, although as >> soon as an SD card will be detected the rtsx driver will increase the >> runtime PM usage count. The count is decreased when the card is >> removed (or failed to be initialized), thus runtime suspend is >> prevented as long as there is a functional card inserted. > > Which means that autosuspend matters only when a card isn't present, > and the host is polled every second or so to see whether a card has > been inserted. > > Under those circumstances you probably don't want to use autosuspend. > That is, resuming before each poll and suspending afterward may use > less energy than staying at full power all the time. > >> I am wondering what you think would be a good autosuspend timeout in >> this case? It seems to me that the only thing the rtsx driver really >> care about is to tell the parent device that it needs to be runtime >> resumed during a certain timeframe, more or less it would like to >> inherit the parents settings. >> >> Other mmc hosts, not being usb-mmc devices, are using an autosuspend >> timeout of ~50-100ms but that doesn't seem like good value here, >> right? > > Well, if you decide to let the device go into runtime suspend between > polls then there's no reason to use autosuspend at all. Once a poll > has ended, you know there won't be any more activity until the next > poll. We could change that, as currently the approach in the mmc core isn't that sophisticated. I even think this has been discussed earlier for the very similar reasons regards polling card detect mode. I guess the main reason to why we yet have changed this, is because mmc host drivers are using an autosuspend timeout of ~50-100 ms, so in the end it haven't been such a big deal. > > On the other hand, if you decide to keep the device at full power all > the time during polling, then any autosuspend timeout larger than 1000 > ms would do what you want. > > Mostly I'm concerned about how this will interact with the USB runtime > PM. The thing is, suspending the sdmmc device doesn't save any energy, > whereas suspending the USB device does. Yes, I agree. My concern is also 2s autosuspend timeout which is set for the usb device. Somehow I feel we need to be able "share" more information between a parent-child relationship, in this case between the sdmmc device and the usb device. An observation I made, is when the sdmmc device gets runtime resumed (pm_runtime_get_sync()), the parent device (the usb device) may also become runtime resumed (unless it's already). In this sequence, but *only* when actually runtime resuming the usb device, the runtime PM core decides to update the last busy mark for the usb device. Shoul
Re: [PATCH] mmc: rtsx_usb_sdmmc: Handle runtime PM while changing led
On Mon, 19 Sep 2016, Ulf Hansson wrote: > On 18 September 2016 at 04:30, Alan Stern wrote: > > On Sat, 17 Sep 2016, Ulf Hansson wrote: > > > >> Each access of the parent device (usb device) needs to be done in runtime > >> resumed state. Currently this isn't case while changing the leds, so let's > >> add pm_runtime_get_sync() and pm_runtime_put() around these calls. > >> > >> Signed-off-by: Ulf Hansson > >> --- > >> > >> While discussing an issue[1] related to runtime PM, I found out that this > >> minor change at least improves the behavior that has been observed. > >> > >> [1] > >> http://www.spinics.net/lists/linux-usb/msg144634.html > >> > >> --- > >> drivers/mmc/host/rtsx_usb_sdmmc.c | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/drivers/mmc/host/rtsx_usb_sdmmc.c > >> b/drivers/mmc/host/rtsx_usb_sdmmc.c > >> index 6c71fc9..a59c7fa 100644 > >> --- a/drivers/mmc/host/rtsx_usb_sdmmc.c > >> +++ b/drivers/mmc/host/rtsx_usb_sdmmc.c > >> @@ -1314,6 +1314,7 @@ static void rtsx_usb_update_led(struct work_struct > >> *work) > >> container_of(work, struct rtsx_usb_sdmmc, led_work); > >> struct rtsx_ucr *ucr = host->ucr; > >> > >> + pm_runtime_get_sync(sdmmc_dev(host)); > >> mutex_lock(&ucr->dev_mutex); > >> > >> if (host->led.brightness == LED_OFF) > >> @@ -1322,6 +1323,7 @@ static void rtsx_usb_update_led(struct work_struct > >> *work) > >> rtsx_usb_turn_on_led(ucr); > >> > >> mutex_unlock(&ucr->dev_mutex); > >> + pm_runtime_put(sdmmc_dev(host)); > >> } > >> #endif > > > > The missing aspect here is that this won't stop the parent USB device > > from going into autosuspend every 2 seconds and then resuming shortly > > afterward. There are two ways of preventing this: > > > > Call usb_mark_last_busy() at appropriate places. > > > > Enable autosuspend for the sdmmc device. > > > > The second approach would also prevent the sdmmc device from going into > > autosuspend as soon as the LED update is finished. Maybe that's okay, > > but if going into suspend is a lightweight procedure then you may want > > to prevent it. > > > > We can for sure enable autosuspend for the sdmmc device, although as > soon as an SD card will be detected the rtsx driver will increase the > runtime PM usage count. The count is decreased when the card is > removed (or failed to be initialized), thus runtime suspend is > prevented as long as there is a functional card inserted. Which means that autosuspend matters only when a card isn't present, and the host is polled every second or so to see whether a card has been inserted. Under those circumstances you probably don't want to use autosuspend. That is, resuming before each poll and suspending afterward may use less energy than staying at full power all the time. > I am wondering what you think would be a good autosuspend timeout in > this case? It seems to me that the only thing the rtsx driver really > care about is to tell the parent device that it needs to be runtime > resumed during a certain timeframe, more or less it would like to > inherit the parents settings. > > Other mmc hosts, not being usb-mmc devices, are using an autosuspend > timeout of ~50-100ms but that doesn't seem like good value here, > right? Well, if you decide to let the device go into runtime suspend between polls then there's no reason to use autosuspend at all. Once a poll has ended, you know there won't be any more activity until the next poll. On the other hand, if you decide to keep the device at full power all the time during polling, then any autosuspend timeout larger than 1000 ms would do what you want. Mostly I'm concerned about how this will interact with the USB runtime PM. The thing is, suspending the sdmmc device doesn't save any energy, whereas suspending the USB device does. Let's see how well everything works with the patch to the rtsx memstick driver. 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: [PATCH] mmc: rtsx_usb_sdmmc: Handle runtime PM while changing led
On 18 September 2016 at 04:30, Alan Stern wrote: > On Sat, 17 Sep 2016, Ulf Hansson wrote: > >> Each access of the parent device (usb device) needs to be done in runtime >> resumed state. Currently this isn't case while changing the leds, so let's >> add pm_runtime_get_sync() and pm_runtime_put() around these calls. >> >> Signed-off-by: Ulf Hansson >> --- >> >> While discussing an issue[1] related to runtime PM, I found out that this >> minor change at least improves the behavior that has been observed. >> >> [1] >> http://www.spinics.net/lists/linux-usb/msg144634.html >> >> --- >> drivers/mmc/host/rtsx_usb_sdmmc.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/mmc/host/rtsx_usb_sdmmc.c >> b/drivers/mmc/host/rtsx_usb_sdmmc.c >> index 6c71fc9..a59c7fa 100644 >> --- a/drivers/mmc/host/rtsx_usb_sdmmc.c >> +++ b/drivers/mmc/host/rtsx_usb_sdmmc.c >> @@ -1314,6 +1314,7 @@ static void rtsx_usb_update_led(struct work_struct >> *work) >> container_of(work, struct rtsx_usb_sdmmc, led_work); >> struct rtsx_ucr *ucr = host->ucr; >> >> + pm_runtime_get_sync(sdmmc_dev(host)); >> mutex_lock(&ucr->dev_mutex); >> >> if (host->led.brightness == LED_OFF) >> @@ -1322,6 +1323,7 @@ static void rtsx_usb_update_led(struct work_struct >> *work) >> rtsx_usb_turn_on_led(ucr); >> >> mutex_unlock(&ucr->dev_mutex); >> + pm_runtime_put(sdmmc_dev(host)); >> } >> #endif > > The missing aspect here is that this won't stop the parent USB device > from going into autosuspend every 2 seconds and then resuming shortly > afterward. There are two ways of preventing this: > > Call usb_mark_last_busy() at appropriate places. > > Enable autosuspend for the sdmmc device. > > The second approach would also prevent the sdmmc device from going into > autosuspend as soon as the LED update is finished. Maybe that's okay, > but if going into suspend is a lightweight procedure then you may want > to prevent it. > We can for sure enable autosuspend for the sdmmc device, although as soon as an SD card will be detected the rtsx driver will increase the runtime PM usage count. The count is decreased when the card is removed (or failed to be initialized), thus runtime suspend is prevented as long as there is a functional card inserted. I am wondering what you think would be a good autosuspend timeout in this case? It seems to me that the only thing the rtsx driver really care about is to tell the parent device that it needs to be runtime resumed during a certain timeframe, more or less it would like to inherit the parents settings. Other mmc hosts, not being usb-mmc devices, are using an autosuspend timeout of ~50-100ms but that doesn't seem like good value here, right? Kind regards Uffe -- 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: [PATCH] mmc: rtsx_usb_sdmmc: Handle runtime PM while changing led
On Sat, 17 Sep 2016, Ulf Hansson wrote: > Each access of the parent device (usb device) needs to be done in runtime > resumed state. Currently this isn't case while changing the leds, so let's > add pm_runtime_get_sync() and pm_runtime_put() around these calls. > > Signed-off-by: Ulf Hansson > --- > > While discussing an issue[1] related to runtime PM, I found out that this > minor change at least improves the behavior that has been observed. > > [1] > http://www.spinics.net/lists/linux-usb/msg144634.html > > --- > drivers/mmc/host/rtsx_usb_sdmmc.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/mmc/host/rtsx_usb_sdmmc.c > b/drivers/mmc/host/rtsx_usb_sdmmc.c > index 6c71fc9..a59c7fa 100644 > --- a/drivers/mmc/host/rtsx_usb_sdmmc.c > +++ b/drivers/mmc/host/rtsx_usb_sdmmc.c > @@ -1314,6 +1314,7 @@ static void rtsx_usb_update_led(struct work_struct > *work) > container_of(work, struct rtsx_usb_sdmmc, led_work); > struct rtsx_ucr *ucr = host->ucr; > > + pm_runtime_get_sync(sdmmc_dev(host)); > mutex_lock(&ucr->dev_mutex); > > if (host->led.brightness == LED_OFF) > @@ -1322,6 +1323,7 @@ static void rtsx_usb_update_led(struct work_struct > *work) > rtsx_usb_turn_on_led(ucr); > > mutex_unlock(&ucr->dev_mutex); > + pm_runtime_put(sdmmc_dev(host)); > } > #endif The missing aspect here is that this won't stop the parent USB device from going into autosuspend every 2 seconds and then resuming shortly afterward. There are two ways of preventing this: Call usb_mark_last_busy() at appropriate places. Enable autosuspend for the sdmmc device. The second approach would also prevent the sdmmc device from going into autosuspend as soon as the LED update is finished. Maybe that's okay, but if going into suspend is a lightweight procedure then you may want to prevent it. 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
[PATCH] mmc: rtsx_usb_sdmmc: Handle runtime PM while changing led
Each access of the parent device (usb device) needs to be done in runtime resumed state. Currently this isn't case while changing the leds, so let's add pm_runtime_get_sync() and pm_runtime_put() around these calls. Signed-off-by: Ulf Hansson --- While discussing an issue[1] related to runtime PM, I found out that this minor change at least improves the behavior that has been observed. [1] http://www.spinics.net/lists/linux-usb/msg144634.html --- drivers/mmc/host/rtsx_usb_sdmmc.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/mmc/host/rtsx_usb_sdmmc.c b/drivers/mmc/host/rtsx_usb_sdmmc.c index 6c71fc9..a59c7fa 100644 --- a/drivers/mmc/host/rtsx_usb_sdmmc.c +++ b/drivers/mmc/host/rtsx_usb_sdmmc.c @@ -1314,6 +1314,7 @@ static void rtsx_usb_update_led(struct work_struct *work) container_of(work, struct rtsx_usb_sdmmc, led_work); struct rtsx_ucr *ucr = host->ucr; + pm_runtime_get_sync(sdmmc_dev(host)); mutex_lock(&ucr->dev_mutex); if (host->led.brightness == LED_OFF) @@ -1322,6 +1323,7 @@ static void rtsx_usb_update_led(struct work_struct *work) rtsx_usb_turn_on_led(ucr); mutex_unlock(&ucr->dev_mutex); + pm_runtime_put(sdmmc_dev(host)); } #endif -- 1.9.1 -- 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