* [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend() @ 2011-04-19 10:46 Guennadi Liakhovetski 2011-04-19 12:44 ` Ohad Ben-Cohen 0 siblings, 1 reply; 27+ messages in thread From: Guennadi Liakhovetski @ 2011-04-19 10:46 UTC (permalink / raw) To: linux-mmc; +Cc: Alan Stern, Magnus Damm, Simon Horman, Rafael J. Wysocki MMC bus PM operations implement a .runtime_idle() method, which calls pm_runtime_suspend(), but this call is not balanced by a resume counterpart, which causes problems with repeated card-plug and driver-load cycles. Removing this method fixes the disbalance. Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> --- With this patch and with v2 of the MMCIF PM patch, that I'll be posting shortly, I can load / unload the driver, insert and remove the card and suspend and wake up the system multiple times and each time the full PM cycle is performed, going down to the platform callbacks. However, it might well be, that there's a more correct way to achieve the same. diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c index 63667a8..44866a6 100644 --- a/drivers/mmc/core/bus.c +++ b/drivers/mmc/core/bus.c @@ -158,15 +158,9 @@ static int mmc_runtime_resume(struct device *dev) return mmc_power_restore_host(card->host); } -static int mmc_runtime_idle(struct device *dev) -{ - return pm_runtime_suspend(dev); -} - static const struct dev_pm_ops mmc_bus_pm_ops = { .runtime_suspend = mmc_runtime_suspend, .runtime_resume = mmc_runtime_resume, - .runtime_idle = mmc_runtime_idle, }; #define MMC_PM_OPS_PTR (&mmc_bus_pm_ops) ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend() 2011-04-19 10:46 [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend() Guennadi Liakhovetski @ 2011-04-19 12:44 ` Ohad Ben-Cohen 2011-04-19 13:23 ` Guennadi Liakhovetski 0 siblings, 1 reply; 27+ messages in thread From: Ohad Ben-Cohen @ 2011-04-19 12:44 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: linux-mmc, Alan Stern, Magnus Damm, Simon Horman, Rafael J. Wysocki On Tue, Apr 19, 2011 at 1:46 PM, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote: > MMC bus PM operations implement a .runtime_idle() method, which calls > pm_runtime_suspend(), but this call is not balanced by a resume > counterpart, What's the exact flow you refer to ? > which causes problems with repeated card-plug and driver-load > cycles. Can you please be more specific ? What are you trying to achieve, what are the problems you encounter ? > Removing this method fixes the disbalance. I'm not sure which disbalance you're referring to, but if you'll remove this method you will break MMC/SDIO runtime PM. More specifically, without having this ->runtime_idle() handler, the last user giving up its power usage_count (e.g. by calling pm_runtime_put{_sync}) will not end up powering down the MMC card. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend() 2011-04-19 12:44 ` Ohad Ben-Cohen @ 2011-04-19 13:23 ` Guennadi Liakhovetski 2011-04-19 14:16 ` Ohad Ben-Cohen 2011-04-19 14:26 ` Alan Stern 0 siblings, 2 replies; 27+ messages in thread From: Guennadi Liakhovetski @ 2011-04-19 13:23 UTC (permalink / raw) To: Ohad Ben-Cohen Cc: linux-mmc, Alan Stern, Magnus Damm, Simon Horman, Rafael J. Wysocki On Tue, 19 Apr 2011, Ohad Ben-Cohen wrote: > On Tue, Apr 19, 2011 at 1:46 PM, Guennadi Liakhovetski > <g.liakhovetski@gmx.de> wrote: > > MMC bus PM operations implement a .runtime_idle() method, which calls > > pm_runtime_suspend(), but this call is not balanced by a resume > > counterpart, > > What's the exact flow you refer to ? Seeing a "struct dev_pm_ops" instance with .runtime_suspend(), .runtime_resume(), and .runtime_idle() methods I understand, that "suspend" and "resume" are two counterparts, that balance each other. Now with "idle" I am not sure which method should balance it. With platform devices in the generic case idle ends up calling pm_generic_runtime_idle(), which then calls pm_runtime_suspend(). So, there should be a balancing pm_runtime_resume() somewhere? > > which causes problems with repeated card-plug and driver-load > > cycles. > > Can you please be more specific ? What are you trying to achieve, what > are the problems you encounter ? See this patch: http://article.gmane.org/gmane.linux.ports.sh.devel/10724 The purpose of that patch is to (1) implement runtime PM in a way, that whenever the driver is unloaded or the card is ejected the interface is powered down, and (2) implement system-wide PM. For (1) doing the usual probe() { ... pm_runtime_enable(); pm_runtime_resume(); ... } remove() { ... pm_runtime_suspend(); pm_runtime_dieabls(); ... } set_ios() { ... if (power_down) pm_runtime_put(); if (power_up) pm_runtime_get_sync(); ... } Doesn't work: some internal MMC counters become disbalanced and after one card replug or driver reloading the interface gets stuck with power either permanently on or off. > > Removing this method fixes the disbalance. > > I'm not sure which disbalance you're referring to, but if you'll > remove this method you will break MMC/SDIO runtime PM. > > More specifically, without having this ->runtime_idle() handler, the > last user giving up its power usage_count (e.g. by calling > pm_runtime_put{_sync}) will not end up powering down the MMC card. How do they work then? Who does the pm_runtime_resume() to undo the effects of the pm_runtime_suspend(), or is it the platform runtime-pm, that is implementing the "idle" method wrongly? Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend() 2011-04-19 13:23 ` Guennadi Liakhovetski @ 2011-04-19 14:16 ` Ohad Ben-Cohen 2011-04-19 14:26 ` Alan Stern 1 sibling, 0 replies; 27+ messages in thread From: Ohad Ben-Cohen @ 2011-04-19 14:16 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: linux-mmc, Alan Stern, Magnus Damm, Simon Horman, Rafael J. Wysocki On Tue, Apr 19, 2011 at 4:23 PM, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote: > Seeing a "struct dev_pm_ops" instance with .runtime_suspend(), > .runtime_resume(), and .runtime_idle() methods I understand, that > "suspend" and "resume" are two counterparts, that balance each other. Now > with "idle" I am not sure which method should balance it. With platform > devices in the generic case idle ends up calling > pm_generic_runtime_idle(), which then calls pm_runtime_suspend(). So, > there should be a balancing pm_runtime_resume() somewhere? There are many ways with which the dev_pm_ops handlers get called, but none of them include imbalance. E.g. take a look how pm_runtime_{get,put}_sync balance each other, while involving all three runtime pm handlers that you've specified (suspend/resume/idle). Can you point out an existing device/flow that demonstrates a runtime pm imbalance ? >> More specifically, without having this ->runtime_idle() handler, the >> last user giving up its power usage_count (e.g. by calling >> pm_runtime_put{_sync}) will not end up powering down the MMC card. > > How do they work then? Who does the pm_runtime_resume() to undo the > effects of the pm_runtime_suspend() Let's take SDIO for example; note how sdio_bus_probe and sdio_bus_remove balance each other with respect to runtime PM api invocations. > or is it the platform runtime-pm that is implementing the "idle" method wrongly? I'm not following what's wrong exactly. If you point out specific code and specify exactly the issues you witness, it might be easier to help. Thanks, Ohad. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend() 2011-04-19 13:23 ` Guennadi Liakhovetski 2011-04-19 14:16 ` Ohad Ben-Cohen @ 2011-04-19 14:26 ` Alan Stern 2011-04-19 22:59 ` Guennadi Liakhovetski 1 sibling, 1 reply; 27+ messages in thread From: Alan Stern @ 2011-04-19 14:26 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: Ohad Ben-Cohen, linux-mmc, Magnus Damm, Simon Horman, Rafael J. Wysocki On Tue, 19 Apr 2011, Guennadi Liakhovetski wrote: > On Tue, 19 Apr 2011, Ohad Ben-Cohen wrote: > > > On Tue, Apr 19, 2011 at 1:46 PM, Guennadi Liakhovetski > > <g.liakhovetski@gmx.de> wrote: > > > MMC bus PM operations implement a .runtime_idle() method, which calls > > > pm_runtime_suspend(), but this call is not balanced by a resume > > > counterpart, > > > > What's the exact flow you refer to ? > > Seeing a "struct dev_pm_ops" instance with .runtime_suspend(), > .runtime_resume(), and .runtime_idle() methods I understand, that > "suspend" and "resume" are two counterparts, that balance each other. No, they do not balance each other. There is no "suspend counter" or "resume counter". Whenever a suspend or resume call is made, it overrides all previous calls. This is different from dev_pm_info's usage_count value, which _is_ a counter. However it does not count suspend or resume calls; instead it counts the number of routines that need to use the device. When usage_count drops to 0, the PM core assumes the device is no longer being used and may therefore be suspended safely (at which point the core invokes the pm_idle callback). If usage_count > 0 then attempts to call pm_runtime_suspend will fail. > Now > with "idle" I am not sure which method should balance it. With platform > devices in the generic case idle ends up calling > pm_generic_runtime_idle(), which then calls pm_runtime_suspend(). So, > there should be a balancing pm_runtime_resume() somewhere? No. If the device is idle, suspending it is appropriate. The device will be resumed later when a driver needs to use it. > > > which causes problems with repeated card-plug and driver-load > > > cycles. > > > > Can you please be more specific ? What are you trying to achieve, what > > are the problems you encounter ? > > See this patch: > > http://article.gmane.org/gmane.linux.ports.sh.devel/10724 > > The purpose of that patch is to (1) implement runtime PM in a way, that > whenever the driver is unloaded or the card is ejected the interface is > powered down, and (2) implement system-wide PM. For (1) doing the usual > > probe() > { > ... > pm_runtime_enable(); > pm_runtime_resume(); > ... > } > > remove() > { > ... > pm_runtime_suspend(); > pm_runtime_dieabls(); > ... > } > > set_ios() > { > ... > if (power_down) > pm_runtime_put(); > if (power_up) > pm_runtime_get_sync(); > ... > } > > Doesn't work: some internal MMC counters become disbalanced and after one > card replug or driver reloading the interface gets stuck with power either > permanently on or off. I'm not familiar enough with the inner workings of the MMC subsystem to help much with this. You'd have to explain things in sufficient detail first. > > > Removing this method fixes the disbalance. > > > > I'm not sure which disbalance you're referring to, but if you'll > > remove this method you will break MMC/SDIO runtime PM. > > > > More specifically, without having this ->runtime_idle() handler, the > > last user giving up its power usage_count (e.g. by calling > > pm_runtime_put{_sync}) will not end up powering down the MMC card. > > How do they work then? Who does the pm_runtime_resume() to undo the > effects of the pm_runtime_suspend(), or is it the platform runtime-pm, > that is implementing the "idle" method wrongly? Put it this way: When do you want the interface to be powered up and powered down? That is, what events should cause the power level to change? The code that handles those events is responsible for calling the appropriate PM runtime routines. Alan Stern ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend() 2011-04-19 14:26 ` Alan Stern @ 2011-04-19 22:59 ` Guennadi Liakhovetski 2011-04-20 14:22 ` Alan Stern 0 siblings, 1 reply; 27+ messages in thread From: Guennadi Liakhovetski @ 2011-04-19 22:59 UTC (permalink / raw) To: Alan Stern Cc: Ohad Ben-Cohen, linux-mmc, Magnus Damm, Simon Horman, Rafael J. Wysocki Thanks to all for clarifications. Since everyone is convinced, that that idle function in mmc bus.c is appropriate, I restored it and managed to achieve my goals also without it by adjusting the platform runtime pm and power domain prototype support. But I still need those "cheating" calls to pm_runtime_put_noidle(&pdev->dev); and pm_runtime_get_noresume(&pdev->dev); in the sh_mmcif.c driver. If I use the patch as posted at http://article.gmane.org/gmane.linux.ports.sh.devel/10724 but without those two calls, and load and unload the driver, while a card is plugged in, unloading the driver doesn't power down the interface, because the usage_count == 1 also after the kernel has soft-ejected the card mmc0: card 0001 removed With my put_noidle() / get_noresume() hack all cases of modprobe / rmmod, card insert / eject work correctly. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend() 2011-04-19 22:59 ` Guennadi Liakhovetski @ 2011-04-20 14:22 ` Alan Stern 2011-04-20 14:50 ` Guennadi Liakhovetski 0 siblings, 1 reply; 27+ messages in thread From: Alan Stern @ 2011-04-20 14:22 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: Ohad Ben-Cohen, linux-mmc, Magnus Damm, Simon Horman, Rafael J. Wysocki On Wed, 20 Apr 2011, Guennadi Liakhovetski wrote: > Thanks to all for clarifications. Since everyone is convinced, that that > idle function in mmc bus.c is appropriate, I restored it and managed to > achieve my goals also without it by adjusting the platform runtime pm and > power domain prototype support. > > But I still need those "cheating" calls to > > pm_runtime_put_noidle(&pdev->dev); > and > pm_runtime_get_noresume(&pdev->dev); > > in the sh_mmcif.c driver. If I use the patch as posted at > > http://article.gmane.org/gmane.linux.ports.sh.devel/10724 > > but without those two calls, and load and unload the driver, while a card > is plugged in, unloading the driver doesn't power down the interface, > because the usage_count == 1 also after the kernel has soft-ejected the > card You haven't explained why you want the interface to be powered down when the driver is unloaded. Normally, unloading a driver is pretty much the exact reverse of loading it -- if the interface wasn't powered down before the driver was loaded originally then it shouldn't be powered down after the driver is unloaded. Conversely, if the interface _was_ powered down before the driver was loaded originally, then making unload the exact reverse of load will naturally leave the interface powered down, with no need for any extra "cheating" calls. > mmc0: card 0001 removed > > With my put_noidle() / get_noresume() hack all cases of modprobe / rmmod, > card insert / eject work correctly. Depending on the subsystem, those calls may not be "cheating" at all. When a subsystem has to deal with a variety of device drivers, some of which may not support runtime PM, a standard trick is for the subsystem to increment the usage_count value before probing. Drivers that aren't runtime-PM-aware will leave the usage_count alone and therefore the device won't ever be runtime-suspended. Drivers that _are_ runtime-PM-aware will know to decrement the usage_count in their probe routine and increment it in their remove routine. However this involves calling pm_runtime_put_noidle() during probe and pm_runtime_get_noresume() during remove -- not during module initialization and module unloading. Alan Stern ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend() 2011-04-20 14:22 ` Alan Stern @ 2011-04-20 14:50 ` Guennadi Liakhovetski 2011-04-20 15:12 ` Alan Stern 0 siblings, 1 reply; 27+ messages in thread From: Guennadi Liakhovetski @ 2011-04-20 14:50 UTC (permalink / raw) To: Alan Stern Cc: Ohad Ben-Cohen, linux-mmc, Magnus Damm, Simon Horman, Rafael J. Wysocki On Wed, 20 Apr 2011, Alan Stern wrote: > On Wed, 20 Apr 2011, Guennadi Liakhovetski wrote: > > > Thanks to all for clarifications. Since everyone is convinced, that that > > idle function in mmc bus.c is appropriate, I restored it and managed to > > achieve my goals also without it by adjusting the platform runtime pm and > > power domain prototype support. > > > > But I still need those "cheating" calls to > > > > pm_runtime_put_noidle(&pdev->dev); > > and > > pm_runtime_get_noresume(&pdev->dev); > > > > in the sh_mmcif.c driver. If I use the patch as posted at > > > > http://article.gmane.org/gmane.linux.ports.sh.devel/10724 > > > > but without those two calls, and load and unload the driver, while a card > > is plugged in, unloading the driver doesn't power down the interface, > > because the usage_count == 1 also after the kernel has soft-ejected the > > card > > You haven't explained why you want the interface to be powered down > when the driver is unloaded. Normally, unloading a driver is pretty > much the exact reverse of loading it -- if the interface wasn't powered > down before the driver was loaded originally then it shouldn't be > powered down after the driver is unloaded. The interface was powered down before loading. And yes, sorry, I actually meant probing and removing, which in case of platform devices is almost equivalent to driver loading and unloading, yes, you can also use bind / unbind to do the same. > Conversely, if the interface _was_ powered down before the driver was > loaded originally, then making unload the exact reverse of load will > naturally leave the interface powered down, with no need for any extra > "cheating" calls. Not quite, see below. > > mmc0: card 0001 removed > > > > With my put_noidle() / get_noresume() hack all cases of modprobe / rmmod, > > card insert / eject work correctly. > > Depending on the subsystem, those calls may not be "cheating" at all. > When a subsystem has to deal with a variety of device drivers, some of > which may not support runtime PM, a standard trick is for the subsystem > to increment the usage_count value before probing. Drivers that aren't > runtime-PM-aware will leave the usage_count alone and therefore the > device won't ever be runtime-suspended. Drivers that _are_ > runtime-PM-aware will know to decrement the usage_count in their probe > routine and increment it in their remove routine. > > However this involves calling pm_runtime_put_noidle() during probe and > pm_runtime_get_noresume() during remove -- not during module > initialization and module unloading. As I said above - I did mean probe() and remove(). Now, I have now traced down the cause of my problem. In dd.c::driver_probe_device(): pm_runtime_get_noresume(dev); pm_runtime_barrier(dev); ret = really_probe(dev, drv); pm_runtime_put_sync(dev); Which means, if originally usage_count = 0 disable_depth = 1 the first get_noresume() increments usage_count = 1 In probe() we do enable(), which decrements disable_depth = 0 and resume(), which poweres everything up. If we now return from probe(), put_sync() will power down and decrement usage_count = 0 which is ok for sh_mmcif.c. If there is no card in the slot, we keep interface powered down, card polling works also in powered down mode if there's a GPIO, or the MMC core will wake us up every time to check for a new card. Next, let's say, a card has been inserted, and we rmmod the driver. dd.c::__device_release_driver() does pm_runtime_get_noresume(dev); pm_runtime_barrier(dev); .remove(); pm_runtime_put_sync(dev); the first get_noresume() increments usage_count = 1 Then if we go the "exact inverse" route, we do suspend(), which does nothing, because usage_count > 0, then we do disable(), which increments disable_depth = 1 After returning from .remove() pm_runtime_put_sync() is executed, but also it does nothing, because disable_depth > 0. The interface stays powered on. So, as you see, the problem is on the .remove() path. I work around it by doing pm_runtime_put_sync(&pdev->dev); pm_runtime_disable(&pdev->dev); pm_runtime_get_noresume(&pdev->dev); in the driver. This way the first call decrements usage_count = 0 and powers down, the second one increments disable_depth = 1 the third one usage_count = 1 Then, back in dd.c again usage_count = 0 and all is happy:-) But the above triplet looks kinda ugly to me. Is this really how it is supposed to work?... Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend() 2011-04-20 14:50 ` Guennadi Liakhovetski @ 2011-04-20 15:12 ` Alan Stern 2011-04-20 20:06 ` Rafael J. Wysocki 0 siblings, 1 reply; 27+ messages in thread From: Alan Stern @ 2011-04-20 15:12 UTC (permalink / raw) To: Guennadi Liakhovetski Cc: Ohad Ben-Cohen, linux-mmc, Magnus Damm, Simon Horman, Rafael J. Wysocki On Wed, 20 Apr 2011, Guennadi Liakhovetski wrote: > As I said above - I did mean probe() and remove(). Now, I have now traced > down the cause of my problem. In dd.c::driver_probe_device(): > > pm_runtime_get_noresume(dev); > pm_runtime_barrier(dev); > ret = really_probe(dev, drv); > pm_runtime_put_sync(dev); > > Which means, if originally > > usage_count = 0 > disable_depth = 1 > > the first get_noresume() increments > > usage_count = 1 > > In probe() we do enable(), which decrements > > disable_depth = 0 > > and resume(), which poweres everything up. If we now return from probe(), > put_sync() will power down and decrement > > usage_count = 0 > > which is ok for sh_mmcif.c. If there is no card in the slot, we keep > interface powered down, card polling works also in powered down mode if > there's a GPIO, or the MMC core will wake us up every time to check for a > new card. Next, let's say, a card has been inserted, and we rmmod the > driver. dd.c::__device_release_driver() does > > pm_runtime_get_noresume(dev); > pm_runtime_barrier(dev); > .remove(); > pm_runtime_put_sync(dev); > > the first get_noresume() increments > > usage_count = 1 > > Then if we go the "exact inverse" route, we do suspend(), which does > nothing, because usage_count > 0, then we do disable(), which increments > > disable_depth = 1 > > After returning from .remove() pm_runtime_put_sync() is executed, but also > it does nothing, because disable_depth > 0. The interface stays powered > on. So, as you see, the problem is on the .remove() path. I work around it > by doing > > pm_runtime_put_sync(&pdev->dev); > pm_runtime_disable(&pdev->dev); > pm_runtime_get_noresume(&pdev->dev); > > in the driver. This way the first call decrements > > usage_count = 0 > > and powers down, the second one increments > > disable_depth = 1 > > the third one > > usage_count = 1 > > Then, back in dd.c again > > usage_count = 0 > > and all is happy:-) But the above triplet looks kinda ugly to me. Is this > really how it is supposed to work?... Ah, now I see the problem. It looks like we did not give sufficient thought to the case where a device starts off (and therefore should finish up) in a powered-down state. Calling pm_runtime_put_sync() after unbinding the device driver seems a little futile -- with no driver, the subsystem may not be able to power-down the device! Rafael, how do you think we should handle this? Get rid of the pm_runtime_get_no_resume() and pm_runtime_put_sync() calls in dd.c:__device_release_driver()? Alan Stern ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend() 2011-04-20 15:12 ` Alan Stern @ 2011-04-20 20:06 ` Rafael J. Wysocki 2011-04-20 21:16 ` Alan Stern 0 siblings, 1 reply; 27+ messages in thread From: Rafael J. Wysocki @ 2011-04-20 20:06 UTC (permalink / raw) To: Alan Stern Cc: Guennadi Liakhovetski, Ohad Ben-Cohen, linux-mmc, Magnus Damm, Simon Horman, Linux PM mailing list [Added linux-pm to the CC list.] On Wednesday, April 20, 2011, Alan Stern wrote: > On Wed, 20 Apr 2011, Guennadi Liakhovetski wrote: ... > Ah, now I see the problem. It looks like we did not give sufficient > thought to the case where a device starts off (and therefore should > finish up) in a powered-down state. Calling pm_runtime_put_sync() > after unbinding the device driver seems a little futile -- with no > driver, the subsystem may not be able to power-down the device! > > Rafael, how do you think we should handle this? Get rid of the > pm_runtime_get_no_resume() and pm_runtime_put_sync() calls in > dd.c:__device_release_driver()? I think we need pm_runtime_barrier() in there. Otherwise we risk removing the driver while there's a runtime PM request pending. But we can move the pm_runtime_put_sync() before driver_sysfs_remove(). Would that help? Rafael ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend() 2011-04-20 20:06 ` Rafael J. Wysocki @ 2011-04-20 21:16 ` Alan Stern 2011-04-20 21:44 ` Rafael J. Wysocki 0 siblings, 1 reply; 27+ messages in thread From: Alan Stern @ 2011-04-20 21:16 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Guennadi Liakhovetski, Ohad Ben-Cohen, linux-mmc, Magnus Damm, Simon Horman, Linux PM mailing list On Wed, 20 Apr 2011, Rafael J. Wysocki wrote: > On Wednesday, April 20, 2011, Alan Stern wrote: > > On Wed, 20 Apr 2011, Guennadi Liakhovetski wrote: > ... > > Ah, now I see the problem. It looks like we did not give sufficient > > thought to the case where a device starts off (and therefore should > > finish up) in a powered-down state. Calling pm_runtime_put_sync() > > after unbinding the device driver seems a little futile -- with no > > driver, the subsystem may not be able to power-down the device! > > > > Rafael, how do you think we should handle this? Get rid of the > > pm_runtime_get_no_resume() and pm_runtime_put_sync() calls in > > dd.c:__device_release_driver()? > > I think we need pm_runtime_barrier() in there. Otherwise we risk > removing the driver while there's a runtime PM request pending. > > But we can move the pm_runtime_put_sync() before driver_sysfs_remove(). What happens if another runtime PM request is queued between the put_sync() and the remove callback? We may need a safe way to prevent async runtime PM requests while still allowing synchronous requests. Alan Stern ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend() 2011-04-20 21:16 ` Alan Stern @ 2011-04-20 21:44 ` Rafael J. Wysocki 2011-04-21 13:58 ` Alan Stern 0 siblings, 1 reply; 27+ messages in thread From: Rafael J. Wysocki @ 2011-04-20 21:44 UTC (permalink / raw) To: Alan Stern Cc: Guennadi Liakhovetski, Ohad Ben-Cohen, linux-mmc, Magnus Damm, Simon Horman, Linux PM mailing list On Wednesday, April 20, 2011, Alan Stern wrote: > On Wed, 20 Apr 2011, Rafael J. Wysocki wrote: > > > On Wednesday, April 20, 2011, Alan Stern wrote: > > > On Wed, 20 Apr 2011, Guennadi Liakhovetski wrote: > > ... > > > Ah, now I see the problem. It looks like we did not give sufficient > > > thought to the case where a device starts off (and therefore should > > > finish up) in a powered-down state. Calling pm_runtime_put_sync() > > > after unbinding the device driver seems a little futile -- with no > > > driver, the subsystem may not be able to power-down the device! > > > > > > Rafael, how do you think we should handle this? Get rid of the > > > pm_runtime_get_no_resume() and pm_runtime_put_sync() calls in > > > dd.c:__device_release_driver()? > > > > I think we need pm_runtime_barrier() in there. Otherwise we risk > > removing the driver while there's a runtime PM request pending. > > > > But we can move the pm_runtime_put_sync() before driver_sysfs_remove(). > > What happens if another runtime PM request is queued between the > put_sync() and the remove callback? We may need a safe way to prevent > async runtime PM requests while still allowing synchronous requests. What about making a rule that it is invalid to schedule a future suspend or queue a resume request of a device whose driver is being removed? Arguably, we can't prevent people from shooting themselves in the foot this way or another and I'm not sure if this particular case is worth additional handling. Thanks, Rafael ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend() 2011-04-20 21:44 ` Rafael J. Wysocki @ 2011-04-21 13:58 ` Alan Stern 2011-04-21 18:00 ` Rafael J. Wysocki 0 siblings, 1 reply; 27+ messages in thread From: Alan Stern @ 2011-04-21 13:58 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Guennadi Liakhovetski, Ohad Ben-Cohen, linux-mmc, Magnus Damm, Simon Horman, Linux PM mailing list On Wed, 20 Apr 2011, Rafael J. Wysocki wrote: > On Wednesday, April 20, 2011, Alan Stern wrote: > > On Wed, 20 Apr 2011, Rafael J. Wysocki wrote: > > > > > On Wednesday, April 20, 2011, Alan Stern wrote: > > > > On Wed, 20 Apr 2011, Guennadi Liakhovetski wrote: > > > ... > > > > Ah, now I see the problem. It looks like we did not give sufficient > > > > thought to the case where a device starts off (and therefore should > > > > finish up) in a powered-down state. Calling pm_runtime_put_sync() > > > > after unbinding the device driver seems a little futile -- with no > > > > driver, the subsystem may not be able to power-down the device! > > > > > > > > Rafael, how do you think we should handle this? Get rid of the > > > > pm_runtime_get_no_resume() and pm_runtime_put_sync() calls in > > > > dd.c:__device_release_driver()? > > > > > > I think we need pm_runtime_barrier() in there. Otherwise we risk > > > removing the driver while there's a runtime PM request pending. > > > > > > But we can move the pm_runtime_put_sync() before driver_sysfs_remove(). > > > > What happens if another runtime PM request is queued between the > > put_sync() and the remove callback? We may need a safe way to prevent > > async runtime PM requests while still allowing synchronous requests. > > What about making a rule that it is invalid to schedule a future suspend > or queue a resume request of a device whose driver is being removed? > > Arguably, we can't prevent people from shooting themselves in the foot this > way or another and I'm not sure if this particular case is worth additional > handling. After thinking about this, I tend to agree. The synchronization issues, combined with the unknown needs of the driver, make this very difficult to handle in the PM core. Here's another possible approach: If a driver wants to leave its device in a powered-down state after unbinding then it can invoke its own runtime_suspend callback directly, in the following way: ... unregister all child devices below dev ... pm_runtime_disable(dev); if (dev->power.runtime_status != RPM_SUSPENDED) { pm_set_suspended(dev); my_runtime_suspend_callback(dev); } There may be issues regarding coordination with the subsystem or the power domain; at the moment it's not clear what should be done. Maybe the runtime-PM core should include an API for directly invoking the appropriate callbacks. Alan Stern ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend() 2011-04-21 13:58 ` Alan Stern @ 2011-04-21 18:00 ` Rafael J. Wysocki 2011-04-21 18:36 ` Alan Stern 0 siblings, 1 reply; 27+ messages in thread From: Rafael J. Wysocki @ 2011-04-21 18:00 UTC (permalink / raw) To: Alan Stern Cc: Guennadi Liakhovetski, Ohad Ben-Cohen, linux-mmc, Magnus Damm, Simon Horman, Linux PM mailing list On Thursday, April 21, 2011, Alan Stern wrote: > On Wed, 20 Apr 2011, Rafael J. Wysocki wrote: > > > On Wednesday, April 20, 2011, Alan Stern wrote: > > > On Wed, 20 Apr 2011, Rafael J. Wysocki wrote: > > > > > > > On Wednesday, April 20, 2011, Alan Stern wrote: > > > > > On Wed, 20 Apr 2011, Guennadi Liakhovetski wrote: > > > > ... > > > > > Ah, now I see the problem. It looks like we did not give sufficient > > > > > thought to the case where a device starts off (and therefore should > > > > > finish up) in a powered-down state. Calling pm_runtime_put_sync() > > > > > after unbinding the device driver seems a little futile -- with no > > > > > driver, the subsystem may not be able to power-down the device! > > > > > > > > > > Rafael, how do you think we should handle this? Get rid of the > > > > > pm_runtime_get_no_resume() and pm_runtime_put_sync() calls in > > > > > dd.c:__device_release_driver()? > > > > > > > > I think we need pm_runtime_barrier() in there. Otherwise we risk > > > > removing the driver while there's a runtime PM request pending. > > > > > > > > But we can move the pm_runtime_put_sync() before driver_sysfs_remove(). > > > > > > What happens if another runtime PM request is queued between the > > > put_sync() and the remove callback? We may need a safe way to prevent > > > async runtime PM requests while still allowing synchronous requests. > > > > What about making a rule that it is invalid to schedule a future suspend > > or queue a resume request of a device whose driver is being removed? > > > > Arguably, we can't prevent people from shooting themselves in the foot this > > way or another and I'm not sure if this particular case is worth additional > > handling. > > After thinking about this, I tend to agree. The synchronization > issues, combined with the unknown needs of the driver, make this very > difficult to handle in the PM core. > > Here's another possible approach: If a driver wants to leave its device > in a powered-down state after unbinding then it can invoke its own > runtime_suspend callback directly, in the following way: > > ... unregister all child devices below dev ... > pm_runtime_disable(dev); > if (dev->power.runtime_status != RPM_SUSPENDED) { > pm_set_suspended(dev); > my_runtime_suspend_callback(dev); > } I think this would work too, but then possibly many drivers would have to do the same thing in their "remove" routines. > There may be issues regarding coordination with the subsystem or the > power domain; at the moment it's not clear what should be done. Maybe > the runtime-PM core should include an API for directly invoking the > appropriate callbacks. If we choose this approach, then yes, we should provide a suitable API, but I'm still thinking it would be simpler to move the pm_runtime_put_sync() before driver_sysfs_remove() and make the rule as I said previously. :-) Rafael ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend() 2011-04-21 18:00 ` Rafael J. Wysocki @ 2011-04-21 18:36 ` Alan Stern 2011-04-21 20:05 ` Rafael J. Wysocki 0 siblings, 1 reply; 27+ messages in thread From: Alan Stern @ 2011-04-21 18:36 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Guennadi Liakhovetski, Ohad Ben-Cohen, linux-mmc, Magnus Damm, Simon Horman, Linux PM mailing list On Thu, 21 Apr 2011, Rafael J. Wysocki wrote: > > > What about making a rule that it is invalid to schedule a future suspend > > > or queue a resume request of a device whose driver is being removed? > > > > > > Arguably, we can't prevent people from shooting themselves in the foot this > > > way or another and I'm not sure if this particular case is worth additional > > > handling. > > > > After thinking about this, I tend to agree. The synchronization > > issues, combined with the unknown needs of the driver, make this very > > difficult to handle in the PM core. > > > > Here's another possible approach: If a driver wants to leave its device > > in a powered-down state after unbinding then it can invoke its own > > runtime_suspend callback directly, in the following way: > > > > ... unregister all child devices below dev ... > > pm_runtime_disable(dev); > > if (dev->power.runtime_status != RPM_SUSPENDED) { > > pm_set_suspended(dev); > > my_runtime_suspend_callback(dev); > > } > > I think this would work too, but then possibly many drivers would have to > do the same thing in their "remove" routines. > > > There may be issues regarding coordination with the subsystem or the > > power domain; at the moment it's not clear what should be done. Maybe > > the runtime-PM core should include an API for directly invoking the > > appropriate callbacks. > > If we choose this approach, then yes, we should provide a suitable API, but > I'm still thinking it would be simpler to move the pm_runtime_put_sync() before driver_sysfs_remove() and make the rule as I said previously. :-) The problem is synchronization. At what point is the driver supposed to stop queuing runtime PM requests? It would have to be sometime before the pm_runtime_barrier() call. How is the driver supposed to know when that point is reached? The remove routine isn't called until later. Alan Stern ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend() 2011-04-21 18:36 ` Alan Stern @ 2011-04-21 20:05 ` Rafael J. Wysocki 2011-04-21 21:48 ` Alan Stern 0 siblings, 1 reply; 27+ messages in thread From: Rafael J. Wysocki @ 2011-04-21 20:05 UTC (permalink / raw) To: Alan Stern Cc: Guennadi Liakhovetski, Ohad Ben-Cohen, linux-mmc, Magnus Damm, Simon Horman, Linux PM mailing list On Thursday, April 21, 2011, Alan Stern wrote: > On Thu, 21 Apr 2011, Rafael J. Wysocki wrote: > > > > > What about making a rule that it is invalid to schedule a future suspend > > > > or queue a resume request of a device whose driver is being removed? > > > > > > > > Arguably, we can't prevent people from shooting themselves in the foot this > > > > way or another and I'm not sure if this particular case is worth additional > > > > handling. > > > > > > After thinking about this, I tend to agree. The synchronization > > > issues, combined with the unknown needs of the driver, make this very > > > difficult to handle in the PM core. > > > > > > Here's another possible approach: If a driver wants to leave its device > > > in a powered-down state after unbinding then it can invoke its own > > > runtime_suspend callback directly, in the following way: > > > > > > ... unregister all child devices below dev ... > > > pm_runtime_disable(dev); > > > if (dev->power.runtime_status != RPM_SUSPENDED) { > > > pm_set_suspended(dev); > > > my_runtime_suspend_callback(dev); > > > } > > > > I think this would work too, but then possibly many drivers would have to > > do the same thing in their "remove" routines. > > > > > There may be issues regarding coordination with the subsystem or the > > > power domain; at the moment it's not clear what should be done. Maybe > > > the runtime-PM core should include an API for directly invoking the > > > appropriate callbacks. > > > > If we choose this approach, then yes, we should provide a suitable API, but > > I'm still thinking it would be simpler to move the pm_runtime_put_sync() before driver_sysfs_remove() and make the rule as I said previously. :-) > > The problem is synchronization. At what point is the driver supposed > to stop queuing runtime PM requests? It would have to be sometime > before the pm_runtime_barrier() call. How is the driver supposed to > know when that point is reached? The remove routine isn't called until > later. Executing the driver's callback is not an ideal solution either, because it simply may be insufficient (it may be necessary to execute the power domain and/or subsystem callbacks, pretty much what rpm_suspend() does, but without taking the usage counter into consideration). Moreover, if we want the driver's ->remove() to do the cleanup anyway, there's not much point in doing any cleanup before in the core. Also, there's a little problem that the bus ->remove() is called before the driver's ->remove(), so it may not be entirely possible to power down the device when the driver's ->remove() is called already. I think the current code is better than any of the alternatives considered so far. Rafael ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend() 2011-04-21 20:05 ` Rafael J. Wysocki @ 2011-04-21 21:48 ` Alan Stern 2011-04-21 22:06 ` Rafael J. Wysocki 0 siblings, 1 reply; 27+ messages in thread From: Alan Stern @ 2011-04-21 21:48 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Guennadi Liakhovetski, Ohad Ben-Cohen, linux-mmc, Magnus Damm, Simon Horman, Linux PM mailing list On Thu, 21 Apr 2011, Rafael J. Wysocki wrote: > > > If we choose this approach, then yes, we should provide a suitable API, but > > > I'm still thinking it would be simpler to move the pm_runtime_put_sync() before driver_sysfs_remove() and make the rule as I said previously. :-) > > > > The problem is synchronization. At what point is the driver supposed > > to stop queuing runtime PM requests? It would have to be sometime > > before the pm_runtime_barrier() call. How is the driver supposed to > > know when that point is reached? The remove routine isn't called until > > later. > > Executing the driver's callback is not an ideal solution either, because > it simply may be insufficient (it may be necessary to execute the power > domain and/or subsystem callbacks, pretty much what rpm_suspend() does, > but without taking the usage counter into consideration). That's why I suggested a new API. It could do the right callbacks. > Moreover, if we want the driver's ->remove() to do the cleanup anyway, > there's not much point in doing any cleanup before in the core. Also, > there's a little problem that the bus ->remove() is called before the > driver's ->remove(), so it may not be entirely possible to power down > the device when the driver's ->remove() is called already. Actually, the bus->remove() callback (if there is one) is responsible for invoking the driver's callback. The subsystem should be smart enough to handle runtime PM requests while the driver's remove callback is running. > I think the current code is better than any of the alternatives considered > so far. Then you think Guennadi should leave his patch as it is, including the "reversed" put/get? Alan Stern ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend() 2011-04-21 21:48 ` Alan Stern @ 2011-04-21 22:06 ` Rafael J. Wysocki 2011-04-22 15:20 ` Alan Stern 0 siblings, 1 reply; 27+ messages in thread From: Rafael J. Wysocki @ 2011-04-21 22:06 UTC (permalink / raw) To: Alan Stern Cc: Guennadi Liakhovetski, Ohad Ben-Cohen, linux-mmc, Magnus Damm, Simon Horman, Linux PM mailing list On Thursday, April 21, 2011, Alan Stern wrote: > On Thu, 21 Apr 2011, Rafael J. Wysocki wrote: > > > > > If we choose this approach, then yes, we should provide a suitable API, but > > > > I'm still thinking it would be simpler to move the pm_runtime_put_sync() before driver_sysfs_remove() and make the rule as I said previously. :-) > > > > > > The problem is synchronization. At what point is the driver supposed > > > to stop queuing runtime PM requests? It would have to be sometime > > > before the pm_runtime_barrier() call. How is the driver supposed to > > > know when that point is reached? The remove routine isn't called until > > > later. > > > > Executing the driver's callback is not an ideal solution either, because > > it simply may be insufficient (it may be necessary to execute the power > > domain and/or subsystem callbacks, pretty much what rpm_suspend() does, > > but without taking the usage counter into consideration). > > That's why I suggested a new API. It could do the right callbacks. > > > Moreover, if we want the driver's ->remove() to do the cleanup anyway, > > there's not much point in doing any cleanup before in the core. Also, > > there's a little problem that the bus ->remove() is called before the > > driver's ->remove(), so it may not be entirely possible to power down > > the device when the driver's ->remove() is called already. > > Actually, the bus->remove() callback (if there is one) is responsible > for invoking the driver's callback. Ah, sorry, I misread the code in __device_release_driver() (too little coffee perhaps). > The subsystem should be smart enough to handle runtime PM requests while > the driver's remove callback is running. If we make such a rule, we may as well remove all of the runtime PM calls from __device_release_driver(). > > I think the current code is better than any of the alternatives considered > > so far. > > Then you think Guennadi should leave his patch as it is, including the > "reversed" put/get? This, or we need to remove the runtime PM calls from __device_release_driver(). I'm a bit worried about the driver_sysfs_remove() and the bus notifier that in theory may affect the runtime PM callbacks potentially executed before ->remove() is called. Rafael ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend() 2011-04-21 22:06 ` Rafael J. Wysocki @ 2011-04-22 15:20 ` Alan Stern 2011-04-22 20:22 ` Rafael J. Wysocki 0 siblings, 1 reply; 27+ messages in thread From: Alan Stern @ 2011-04-22 15:20 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Guennadi Liakhovetski, Ohad Ben-Cohen, linux-mmc, Magnus Damm, Simon Horman, Linux PM mailing list On Fri, 22 Apr 2011, Rafael J. Wysocki wrote: > > The subsystem should be smart enough to handle runtime PM requests while > > the driver's remove callback is running. > > If we make such a rule, we may as well remove all of the runtime PM > calls from __device_release_driver(). > > > > I think the current code is better than any of the alternatives considered > > > so far. > > > > Then you think Guennadi should leave his patch as it is, including the > > "reversed" put/get? > > This, or we need to remove the runtime PM calls from __device_release_driver(). Let's go back to first principles. The underlying problem we want to solve is that runtime PM callbacks race with driver unbinding. In a worst-case scenario, a driver module might be unbound and unloaded from memory and then a runtime PM callback could occur, causing an invalid memory access. Related to this is the fact that some drivers want to use runtime PM from within their remove routines. This implies that the PM core shouldn't simply disallow all runtime PM callbacks during unbinding. As it happens, the PM core doesn't call drivers' runtime PM routines directly. Instead it calls bus, type, class, and power-domain routines -- which may in turn invoke the driver routines. Put together, this all suggests that the PM core can't solve the underlying problem and shouldn't try. Instead, it should be up to the subsystems to insure they don't make invalid callbacks. For example, the USB subsystem does this by explicitly doing pm_runtime_get_sync() before unbinding a driver. Other subsystems would want to use a different approach. If we add this requirement then yes, it would make sense to remove the get_noresume and put_sync calls from __device_release_driver(). We probably want to keep the barrier, though. > I'm a bit worried about the driver_sysfs_remove() and the bus notifier that > in theory may affect the runtime PM callbacks potentially executed before > ->remove() is called. The driver_sysfs_remove() call merely gets rid of a couple of symlinks in sysfs. I don't think that will impact runtime PM. The bus notifier might, however. Perhaps the barrier should be moved down, after the notifier call. How does this patch look? Alan Stern drivers/base/dd.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) Index: usb-2.6/drivers/base/dd.c =================================================================== --- usb-2.6.orig/drivers/base/dd.c +++ usb-2.6/drivers/base/dd.c @@ -316,15 +316,13 @@ static void __device_release_driver(stru drv = dev->driver; if (drv) { - pm_runtime_get_noresume(dev); - pm_runtime_barrier(dev); - driver_sysfs_remove(dev); if (dev->bus) blocking_notifier_call_chain(&dev->bus->p->bus_notifier, BUS_NOTIFY_UNBIND_DRIVER, dev); + pm_runtime_barrier(dev); if (dev->bus && dev->bus->remove) dev->bus->remove(dev); @@ -337,8 +335,6 @@ static void __device_release_driver(stru blocking_notifier_call_chain(&dev->bus->p->bus_notifier, BUS_NOTIFY_UNBOUND_DRIVER, dev); - - pm_runtime_put_sync(dev); } } ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend() 2011-04-22 15:20 ` Alan Stern @ 2011-04-22 20:22 ` Rafael J. Wysocki 2011-04-22 20:25 ` Rafael J. Wysocki 0 siblings, 1 reply; 27+ messages in thread From: Rafael J. Wysocki @ 2011-04-22 20:22 UTC (permalink / raw) To: Alan Stern Cc: Guennadi Liakhovetski, Ohad Ben-Cohen, linux-mmc, Magnus Damm, Simon Horman, Linux PM mailing list On Friday, April 22, 2011, Alan Stern wrote: > On Fri, 22 Apr 2011, Rafael J. Wysocki wrote: > > > > The subsystem should be smart enough to handle runtime PM requests while > > > the driver's remove callback is running. > > > > If we make such a rule, we may as well remove all of the runtime PM > > calls from __device_release_driver(). > > > > > > I think the current code is better than any of the alternatives considered > > > > so far. > > > > > > Then you think Guennadi should leave his patch as it is, including the > > > "reversed" put/get? > > > > This, or we need to remove the runtime PM calls from __device_release_driver(). > > Let's go back to first principles. The underlying problem we want to > solve is that runtime PM callbacks race with driver unbinding. In a > worst-case scenario, a driver module might be unbound and unloaded from > memory and then a runtime PM callback could occur, causing an invalid > memory access. > > Related to this is the fact that some drivers want to use runtime PM > from within their remove routines. This implies that the PM core > shouldn't simply disallow all runtime PM callbacks during unbinding. > > As it happens, the PM core doesn't call drivers' runtime PM routines > directly. Instead it calls bus, type, class, and power-domain > routines -- which may in turn invoke the driver routines. > > Put together, this all suggests that the PM core can't solve the > underlying problem and shouldn't try. Instead, it should be up to the > subsystems to insure they don't make invalid callbacks. For example, > the USB subsystem does this by explicitly doing pm_runtime_get_sync() > before unbinding a driver. Other subsystems would want to use a > different approach. > > If we add this requirement then yes, it would make sense to remove the > get_noresume and put_sync calls from __device_release_driver(). We > probably want to keep the barrier, though. > > > I'm a bit worried about the driver_sysfs_remove() and the bus notifier that > > in theory may affect the runtime PM callbacks potentially executed before > > ->remove() is called. > > The driver_sysfs_remove() call merely gets rid of a couple of symlinks > in sysfs. I don't think that will impact runtime PM. > > The bus notifier might, however. Not only it might, but it actually does. There are platforms currently in the ARM tree where the runtime PM hadling of devices is set up and cleaned up by the bus notifier, so after the notifier has run the device will be handled differently. > Perhaps the barrier should be moved down, after the notifier call. > How does this patch look? > > drivers/base/dd.c | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > Index: usb-2.6/drivers/base/dd.c > =================================================================== > --- usb-2.6.orig/drivers/base/dd.c > +++ usb-2.6/drivers/base/dd.c > @@ -316,15 +316,13 @@ static void __device_release_driver(stru > > drv = dev->driver; > if (drv) { > - pm_runtime_get_noresume(dev); > - pm_runtime_barrier(dev); > - > driver_sysfs_remove(dev); > > if (dev->bus) > blocking_notifier_call_chain(&dev->bus->p->bus_notifier, > BUS_NOTIFY_UNBIND_DRIVER, > dev); > + pm_runtime_barrier(dev); The barrier would not prevent the race between the notifier and runtie PM from taking place. Why don't we do something like this instead: --- drivers/base/dd.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) Index: linux-2.6/drivers/base/dd.c =================================================================== --- linux-2.6.orig/drivers/base/dd.c +++ linux-2.6/drivers/base/dd.c @@ -326,6 +326,8 @@ static void __device_release_driver(stru BUS_NOTIFY_UNBIND_DRIVER, dev); + pm_runtime_put_sync(dev); + if (dev->bus && dev->bus->remove) dev->bus->remove(dev); else if (drv->remove) @@ -338,7 +340,6 @@ static void __device_release_driver(stru BUS_NOTIFY_UNBOUND_DRIVER, dev); - pm_runtime_put_sync(dev); } } That should eliminate the race between the notifier and runtime PM and still allow the bus/driver to use runtime PM in the ->remove() callbacks. Thanks, Rafael ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend() 2011-04-22 20:22 ` Rafael J. Wysocki @ 2011-04-22 20:25 ` Rafael J. Wysocki 2011-04-22 21:20 ` Alan Stern 0 siblings, 1 reply; 27+ messages in thread From: Rafael J. Wysocki @ 2011-04-22 20:25 UTC (permalink / raw) To: Alan Stern Cc: Guennadi Liakhovetski, Ohad Ben-Cohen, linux-mmc, Magnus Damm, Simon Horman, Linux PM mailing list On Friday, April 22, 2011, Rafael J. Wysocki wrote: > On Friday, April 22, 2011, Alan Stern wrote: > > On Fri, 22 Apr 2011, Rafael J. Wysocki wrote: > > > > > > The subsystem should be smart enough to handle runtime PM requests while > > > > the driver's remove callback is running. > > > > > > If we make such a rule, we may as well remove all of the runtime PM > > > calls from __device_release_driver(). > > > > > > > > I think the current code is better than any of the alternatives considered > > > > > so far. > > > > > > > > Then you think Guennadi should leave his patch as it is, including the > > > > "reversed" put/get? > > > > > > This, or we need to remove the runtime PM calls from __device_release_driver(). > > > > Let's go back to first principles. The underlying problem we want to > > solve is that runtime PM callbacks race with driver unbinding. In a > > worst-case scenario, a driver module might be unbound and unloaded from > > memory and then a runtime PM callback could occur, causing an invalid > > memory access. > > > > Related to this is the fact that some drivers want to use runtime PM > > from within their remove routines. This implies that the PM core > > shouldn't simply disallow all runtime PM callbacks during unbinding. > > > > As it happens, the PM core doesn't call drivers' runtime PM routines > > directly. Instead it calls bus, type, class, and power-domain > > routines -- which may in turn invoke the driver routines. > > > > Put together, this all suggests that the PM core can't solve the > > underlying problem and shouldn't try. Instead, it should be up to the > > subsystems to insure they don't make invalid callbacks. For example, > > the USB subsystem does this by explicitly doing pm_runtime_get_sync() > > before unbinding a driver. Other subsystems would want to use a > > different approach. > > > > If we add this requirement then yes, it would make sense to remove the > > get_noresume and put_sync calls from __device_release_driver(). We > > probably want to keep the barrier, though. > > > > > I'm a bit worried about the driver_sysfs_remove() and the bus notifier that > > > in theory may affect the runtime PM callbacks potentially executed before > > > ->remove() is called. > > > > The driver_sysfs_remove() call merely gets rid of a couple of symlinks > > in sysfs. I don't think that will impact runtime PM. > > > > The bus notifier might, however. > > Not only it might, but it actually does. There are platforms currently in > the ARM tree where the runtime PM hadling of devices is set up and cleaned up > by the bus notifier, so after the notifier has run the device will be handled > differently. > > > Perhaps the barrier should be moved down, after the notifier call. > > How does this patch look? > > > > drivers/base/dd.c | 6 +----- > > 1 file changed, 1 insertion(+), 5 deletions(-) > > > > Index: usb-2.6/drivers/base/dd.c > > =================================================================== > > --- usb-2.6.orig/drivers/base/dd.c > > +++ usb-2.6/drivers/base/dd.c > > @@ -316,15 +316,13 @@ static void __device_release_driver(stru > > > > drv = dev->driver; > > if (drv) { > > - pm_runtime_get_noresume(dev); > > - pm_runtime_barrier(dev); > > - > > driver_sysfs_remove(dev); > > > > if (dev->bus) > > blocking_notifier_call_chain(&dev->bus->p->bus_notifier, > > BUS_NOTIFY_UNBIND_DRIVER, > > dev); > > + pm_runtime_barrier(dev); > > The barrier would not prevent the race between the notifier and runtie PM > from taking place. Why don't we do something like this instead: > > --- > drivers/base/dd.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > Index: linux-2.6/drivers/base/dd.c > =================================================================== > --- linux-2.6.orig/drivers/base/dd.c > +++ linux-2.6/drivers/base/dd.c > @@ -326,6 +326,8 @@ static void __device_release_driver(stru > BUS_NOTIFY_UNBIND_DRIVER, > dev); > > + pm_runtime_put_sync(dev); > + In fact, I think this one may be _noidle. If we allow the bus/driver to do what they wont, we might as well let them handle the "device idle" case from ->remove(). > if (dev->bus && dev->bus->remove) > dev->bus->remove(dev); > else if (drv->remove) > @@ -338,7 +340,6 @@ static void __device_release_driver(stru > BUS_NOTIFY_UNBOUND_DRIVER, > dev); > > - pm_runtime_put_sync(dev); > } > } Thanks, Rafael ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend() 2011-04-22 20:25 ` Rafael J. Wysocki @ 2011-04-22 21:20 ` Alan Stern 2011-04-22 22:11 ` Rafael J. Wysocki 0 siblings, 1 reply; 27+ messages in thread From: Alan Stern @ 2011-04-22 21:20 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Guennadi Liakhovetski, Ohad Ben-Cohen, linux-mmc, Magnus Damm, Simon Horman, Linux PM mailing list On Fri, 22 Apr 2011, Rafael J. Wysocki wrote: > > The barrier would not prevent the race between the notifier and runtie PM > > from taking place. Why don't we do something like this instead: > > > > --- > > drivers/base/dd.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > Index: linux-2.6/drivers/base/dd.c > > =================================================================== > > --- linux-2.6.orig/drivers/base/dd.c > > +++ linux-2.6/drivers/base/dd.c > > @@ -326,6 +326,8 @@ static void __device_release_driver(stru > > BUS_NOTIFY_UNBIND_DRIVER, > > dev); > > > > + pm_runtime_put_sync(dev); > > + > > In fact, I think this one may be _noidle. If we allow the bus/driver > to do what they wont, we might as well let them handle the "device idle" > case from ->remove(). Maybe... But keeping it put_sync doesn't do any harm. In Guennadi's case, it might allow him to get rid of the pm_runtime_suspend() call in the remove routine. > > if (dev->bus && dev->bus->remove) > > dev->bus->remove(dev); > > else if (drv->remove) > > @@ -338,7 +340,6 @@ static void __device_release_driver(stru > > BUS_NOTIFY_UNBOUND_DRIVER, > > dev); > > > > - pm_runtime_put_sync(dev); > > } > > } Basically this is okay with me, and it should allow Guennadi to avoid the extra put/get pair. Do you know if any other subsystems rely on the usage_count being > 0 during unbinding? Alan Stern ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend() 2011-04-22 21:20 ` Alan Stern @ 2011-04-22 22:11 ` Rafael J. Wysocki 2011-04-25 10:29 ` [linux-pm] " Rafael J. Wysocki 0 siblings, 1 reply; 27+ messages in thread From: Rafael J. Wysocki @ 2011-04-22 22:11 UTC (permalink / raw) To: Alan Stern Cc: Guennadi Liakhovetski, Ohad Ben-Cohen, linux-mmc, Magnus Damm, Simon Horman, Linux PM mailing list On Friday, April 22, 2011, Alan Stern wrote: > On Fri, 22 Apr 2011, Rafael J. Wysocki wrote: > > > > The barrier would not prevent the race between the notifier and runtie PM > > > from taking place. Why don't we do something like this instead: > > > > > > --- > > > drivers/base/dd.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > Index: linux-2.6/drivers/base/dd.c > > > =================================================================== > > > --- linux-2.6.orig/drivers/base/dd.c > > > +++ linux-2.6/drivers/base/dd.c > > > @@ -326,6 +326,8 @@ static void __device_release_driver(stru > > > BUS_NOTIFY_UNBIND_DRIVER, > > > dev); > > > > > > + pm_runtime_put_sync(dev); > > > + > > > > In fact, I think this one may be _noidle. If we allow the bus/driver > > to do what they wont, we might as well let them handle the "device idle" > > case from ->remove(). > > Maybe... But keeping it put_sync doesn't do any harm. In Guennadi's > case, it might allow him to get rid of the pm_runtime_suspend() call in > the remove routine. > > > > if (dev->bus && dev->bus->remove) > > > dev->bus->remove(dev); > > > else if (drv->remove) > > > @@ -338,7 +340,6 @@ static void __device_release_driver(stru > > > BUS_NOTIFY_UNBOUND_DRIVER, > > > dev); > > > > > > - pm_runtime_put_sync(dev); > > > } > > > } > > Basically this is okay with me, and it should allow Guennadi to avoid > the extra put/get pair. OK, so I'm going to put the appended patch into my linux-next branch (hopefully, the problem is explained sufficiently in the changelog). > Do you know if any other subsystems rely on the usage_count > being > 0 during unbinding? Do you mean if I know of subsystems that the patch below may cause to fail? No, I don't. :-) Rafael --- From: Rafael J. Wysocki <rjw@sisk.pl> Subject: PM / Runtime: Allow runtime suspend to be done from remove callbacks The driver core prevents race conditions between device runtime PM and driver removal from happening by incrementing the runtime PM usage counter of the device and executing pm_runtime_barrier() before running the bus notifier and the ->remove() callbacks provided by the device's subsystem or driver. This guarantees that, if a future runtime suspend of the device has been scheduled or a runtime resume or idle request has been queued up right before the driver removal, it will be canceled or waited for to complete and no other asynchronous runtime suspend or idle requests for the device will be put into the PM workqueue until the ->remove() callback returns. However, this also means that the device's subsystem or driver will not be able put it into the suspended state by calling pm_runtime_suspend() from its ->remove() routine. This turns out to be a major inconvenience for some subsystems and drivers that want to leave the devices they handle in the suspended state. To allow subsystems and drivers to put devices into the suspended state by calling pm_runtime_suspend() from their ->remove() routines execute pm_runtime_put_sync() after running the bus notifier in __device_release_driver(). [The bus notifier still needs to be protected from racing with runtime PM routines, because it is used by some subsystems to carry out operations affecting the runtime PM functionality.] This will require subsystems and drivers to make their ->remove() callbacks avoid races with runtime PM directly, but it will allow of more flexibility in the handling of devices during the removal of their drivers. Reported-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl> --- drivers/base/dd.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) Index: linux-2.6/drivers/base/dd.c =================================================================== --- linux-2.6.orig/drivers/base/dd.c +++ linux-2.6/drivers/base/dd.c @@ -326,6 +326,8 @@ static void __device_release_driver(stru BUS_NOTIFY_UNBIND_DRIVER, dev); + pm_runtime_put_sync(dev); + if (dev->bus && dev->bus->remove) dev->bus->remove(dev); else if (drv->remove) @@ -338,7 +340,6 @@ static void __device_release_driver(stru BUS_NOTIFY_UNBOUND_DRIVER, dev); - pm_runtime_put_sync(dev); } } ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [linux-pm] [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend() 2011-04-22 22:11 ` Rafael J. Wysocki @ 2011-04-25 10:29 ` Rafael J. Wysocki 2011-04-26 10:44 ` Guennadi Liakhovetski 2011-04-28 22:12 ` Rafael J. Wysocki 0 siblings, 2 replies; 27+ messages in thread From: Rafael J. Wysocki @ 2011-04-25 10:29 UTC (permalink / raw) To: linux-pm; +Cc: Alan Stern, linux-mmc, Magnus Damm, Guennadi Liakhovetski, LKML On Saturday, April 23, 2011, Rafael J. Wysocki wrote: > On Friday, April 22, 2011, Alan Stern wrote: > > On Fri, 22 Apr 2011, Rafael J. Wysocki wrote: > > > > > > The barrier would not prevent the race between the notifier and runtie PM > > > > from taking place. Why don't we do something like this instead: > > > > > > > > --- > > > > drivers/base/dd.c | 3 ++- > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > Index: linux-2.6/drivers/base/dd.c > > > > =================================================================== > > > > --- linux-2.6.orig/drivers/base/dd.c > > > > +++ linux-2.6/drivers/base/dd.c > > > > @@ -326,6 +326,8 @@ static void __device_release_driver(stru > > > > BUS_NOTIFY_UNBIND_DRIVER, > > > > dev); > > > > > > > > + pm_runtime_put_sync(dev); > > > > + > > > > > > In fact, I think this one may be _noidle. If we allow the bus/driver > > > to do what they wont, we might as well let them handle the "device idle" > > > case from ->remove(). > > > > Maybe... But keeping it put_sync doesn't do any harm. In Guennadi's > > case, it might allow him to get rid of the pm_runtime_suspend() call in > > the remove routine. > > > > > > if (dev->bus && dev->bus->remove) > > > > dev->bus->remove(dev); > > > > else if (drv->remove) > > > > @@ -338,7 +340,6 @@ static void __device_release_driver(stru > > > > BUS_NOTIFY_UNBOUND_DRIVER, > > > > dev); > > > > > > > > - pm_runtime_put_sync(dev); > > > > } > > > > } > > > > Basically this is okay with me, and it should allow Guennadi to avoid > > the extra put/get pair. > > OK, so I'm going to put the appended patch into my linux-next branch > (hopefully, the problem is explained sufficiently in the changelog). I thought about that a bit more and came to the conclusion that we should do things a bit differently in __device_release_driver(). Namely, the fact that the device can be resumed (either synchronously or asynchronously) after the pm_runtime_barrier() has returned may be problematic too, because it may race with the bus notifier in some cases. For this reason, I think it would be better to do pm_runtime_get_sync() instead of the pm_runtime_get_noresume() and pm_runtime_barrier(). So, I think the appended patch would be better than the previous one. Thanks, Rafael --- From: Rafael J. Wysocki <rjw@sisk.pl> Subject: PM / Runtime: Rework runtime PM handling during driver removal The driver core tries to prevent race conditions between runtime PM and driver removal from happening by incrementing the runtime PM usage counter of the device and executing pm_runtime_barrier() before running the bus notifier and the ->remove() callbacks provided by the device's subsystem or driver. This guarantees that, if a future runtime suspend of the device has been scheduled or a runtime resume or idle request has been queued up right before the driver removal, it will be canceled or waited for to complete and no other asynchronous runtime suspend or idle requests for the device will be put into the PM workqueue until the ->remove() callback returns. However, it doesn't prevent resume requests from being queued up after pm_runtime_barrier() has been called and it doesn't prevent pm_runtime_resume() from executing the device subsystem's runtime resume callback. Morever, it prevents the device's subsystem or driver from putting the device into the suspended state by calling pm_runtime_suspend() from its ->remove() routine. This turns out to be a major inconvenience for some subsystems and drivers that want to leave the devices they handle in the suspended state. To really prevent runtime PM callbacks from racing with the bus notifier callback in __device_release_driver(), which is necessary, because the notifier is used by some subsystems to carry out operations affecting the runtime PM functionality, use pm_runtime_get_sync() instead of the combination of pm_runtime_get_noresume() and pm_runtime_barrier(). This will resume the device if it's in the suspended state and will prevent it from being suspended again until pm_runtime_put_*() is called. To allow subsystems and drivers to put devices into the suspended state by calling pm_runtime_suspend() from their ->remove() routines, execute pm_runtime_put_sync() after running the bus notifier in __device_release_driver(). This will require subsystems and drivers to make their ->remove() callbacks avoid races with runtime PM directly, but it will allow of more flexibility in the handling of devices during the removal of their drivers. Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl> --- drivers/base/dd.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) Index: linux-2.6/drivers/base/dd.c =================================================================== --- linux-2.6.orig/drivers/base/dd.c +++ linux-2.6/drivers/base/dd.c @@ -316,8 +316,7 @@ static void __device_release_driver(stru drv = dev->driver; if (drv) { - pm_runtime_get_noresume(dev); - pm_runtime_barrier(dev); + pm_runtime_get_sync(dev); driver_sysfs_remove(dev); @@ -326,6 +325,8 @@ static void __device_release_driver(stru BUS_NOTIFY_UNBIND_DRIVER, dev); + pm_runtime_put_sync(dev); + if (dev->bus && dev->bus->remove) dev->bus->remove(dev); else if (drv->remove) @@ -338,7 +339,6 @@ static void __device_release_driver(stru BUS_NOTIFY_UNBOUND_DRIVER, dev); - pm_runtime_put_sync(dev); } } ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [linux-pm] [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend() 2011-04-25 10:29 ` [linux-pm] " Rafael J. Wysocki @ 2011-04-26 10:44 ` Guennadi Liakhovetski 2011-04-26 11:51 ` Rafael J. Wysocki 2011-04-28 22:12 ` Rafael J. Wysocki 1 sibling, 1 reply; 27+ messages in thread From: Guennadi Liakhovetski @ 2011-04-26 10:44 UTC (permalink / raw) To: Rafael J. Wysocki; +Cc: linux-pm, Alan Stern, linux-mmc, Magnus Damm, LKML Hi On Mon, 25 Apr 2011, Rafael J. Wysocki wrote: > On Saturday, April 23, 2011, Rafael J. Wysocki wrote: > > On Friday, April 22, 2011, Alan Stern wrote: > > > On Fri, 22 Apr 2011, Rafael J. Wysocki wrote: > > > > > > > > The barrier would not prevent the race between the notifier and runtie PM > > > > > from taking place. Why don't we do something like this instead: > > > > > > > > > > --- > > > > > drivers/base/dd.c | 3 ++- > > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > > > Index: linux-2.6/drivers/base/dd.c > > > > > =================================================================== > > > > > --- linux-2.6.orig/drivers/base/dd.c > > > > > +++ linux-2.6/drivers/base/dd.c > > > > > @@ -326,6 +326,8 @@ static void __device_release_driver(stru > > > > > BUS_NOTIFY_UNBIND_DRIVER, > > > > > dev); > > > > > > > > > > + pm_runtime_put_sync(dev); > > > > > + > > > > > > > > In fact, I think this one may be _noidle. If we allow the bus/driver > > > > to do what they wont, we might as well let them handle the "device idle" > > > > case from ->remove(). > > > > > > Maybe... But keeping it put_sync doesn't do any harm. In Guennadi's > > > case, it might allow him to get rid of the pm_runtime_suspend() call in > > > the remove routine. > > > > > > > > if (dev->bus && dev->bus->remove) > > > > > dev->bus->remove(dev); > > > > > else if (drv->remove) > > > > > @@ -338,7 +340,6 @@ static void __device_release_driver(stru > > > > > BUS_NOTIFY_UNBOUND_DRIVER, > > > > > dev); > > > > > > > > > > - pm_runtime_put_sync(dev); > > > > > } > > > > > } > > > > > > Basically this is okay with me, and it should allow Guennadi to avoid > > > the extra put/get pair. > > > > OK, so I'm going to put the appended patch into my linux-next branch > > (hopefully, the problem is explained sufficiently in the changelog). > > I thought about that a bit more and came to the conclusion that we should > do things a bit differently in __device_release_driver(). Namely, the fact > that the device can be resumed (either synchronously or asynchronously) after > the pm_runtime_barrier() has returned may be problematic too, because it > may race with the bus notifier in some cases. For this reason, I think it > would be better to do pm_runtime_get_sync() instead of the > pm_runtime_get_noresume() and pm_runtime_barrier(). > > So, I think the appended patch would be better than the previous one. I refrained in taking part in the general rtpm API behaviour, I'd rather rely on others here. If you push this your patch, I'll have to change my TMIO/SDHI and MMCIF patches as follows: diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c index 1889d64..3a22e55 100644 --- a/drivers/mmc/host/sh_mmcif.c +++ b/drivers/mmc/host/sh_mmcif.c @@ -1117,6 +1117,8 @@ static int __devexit sh_mmcif_remove(struct platform_device *pdev) struct sh_mmcif_host *host = platform_get_drvdata(pdev); int irq[2]; + pm_runtime_get_sync(&pdev->dev); + mmc_remove_host(host->mmc); sh_mmcif_release_dma(host); @@ -1137,7 +1139,6 @@ static int __devexit sh_mmcif_remove(struct platform_device *pdev) mmc_free_host(host->mmc); pm_runtime_put_sync(&pdev->dev); pm_runtime_disable(&pdev->dev); - pm_runtime_get_noresume(&pdev->dev); return 0; } diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c index 26598f1..86eaa68 100644 --- a/drivers/mmc/host/tmio_mmc_pio.c +++ b/drivers/mmc/host/tmio_mmc_pio.c @@ -956,17 +956,9 @@ void tmio_mmc_host_remove(struct tmio_mmc_host *host) iounmap(host->ctl); mmc_free_host(host->mmc); - /* - * Now rtpm usage_count = 2, because we incremented it once in probe() - * above, and dd.c incremented it again, before calling .release(). So. - * to power the device down we have to decrement the counter to 0 and - * suspend it, because after our disable() suspending from dd.c will - * only decrement the counter, but not call any callbacks - */ - pm_runtime_put_noidle(&pdev->dev); + /* Compensate for pm_runtime_get_sync() in probe() above */ pm_runtime_put_sync(&pdev->dev); pm_runtime_disable(&pdev->dev); - pm_runtime_get_noresume(&pdev->dev); } EXPORT_SYMBOL(tmio_mmc_host_remove); Is this your patch final and shall I submit updated versions of my patches or shall I wait for your patch to take its final form and hit "next?" Thanks Guennadi > > Thanks, > Rafael > > --- > From: Rafael J. Wysocki <rjw@sisk.pl> > Subject: PM / Runtime: Rework runtime PM handling during driver removal > > The driver core tries to prevent race conditions between runtime PM > and driver removal from happening by incrementing the runtime PM > usage counter of the device and executing pm_runtime_barrier() before > running the bus notifier and the ->remove() callbacks provided by the > device's subsystem or driver. This guarantees that, if a future > runtime suspend of the device has been scheduled or a runtime resume > or idle request has been queued up right before the driver removal, > it will be canceled or waited for to complete and no other > asynchronous runtime suspend or idle requests for the device will be > put into the PM workqueue until the ->remove() callback returns. > However, it doesn't prevent resume requests from being queued up > after pm_runtime_barrier() has been called and it doesn't prevent > pm_runtime_resume() from executing the device subsystem's runtime > resume callback. Morever, it prevents the device's subsystem or > driver from putting the device into the suspended state by calling > pm_runtime_suspend() from its ->remove() routine. This turns out to > be a major inconvenience for some subsystems and drivers that want to > leave the devices they handle in the suspended state. > > To really prevent runtime PM callbacks from racing with the bus > notifier callback in __device_release_driver(), which is necessary, > because the notifier is used by some subsystems to carry out > operations affecting the runtime PM functionality, use > pm_runtime_get_sync() instead of the combination of > pm_runtime_get_noresume() and pm_runtime_barrier(). This will resume > the device if it's in the suspended state and will prevent it from > being suspended again until pm_runtime_put_*() is called. > > To allow subsystems and drivers to put devices into the suspended > state by calling pm_runtime_suspend() from their ->remove() routines, > execute pm_runtime_put_sync() after running the bus notifier in > __device_release_driver(). This will require subsystems and drivers > to make their ->remove() callbacks avoid races with runtime PM > directly, but it will allow of more flexibility in the handling of > devices during the removal of their drivers. > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl> > --- > drivers/base/dd.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > Index: linux-2.6/drivers/base/dd.c > =================================================================== > --- linux-2.6.orig/drivers/base/dd.c > +++ linux-2.6/drivers/base/dd.c > @@ -316,8 +316,7 @@ static void __device_release_driver(stru > > drv = dev->driver; > if (drv) { > - pm_runtime_get_noresume(dev); > - pm_runtime_barrier(dev); > + pm_runtime_get_sync(dev); > > driver_sysfs_remove(dev); > > @@ -326,6 +325,8 @@ static void __device_release_driver(stru > BUS_NOTIFY_UNBIND_DRIVER, > dev); > > + pm_runtime_put_sync(dev); > + > if (dev->bus && dev->bus->remove) > dev->bus->remove(dev); > else if (drv->remove) > @@ -338,7 +339,6 @@ static void __device_release_driver(stru > BUS_NOTIFY_UNBOUND_DRIVER, > dev); > > - pm_runtime_put_sync(dev); > } > } > > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [linux-pm] [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend() 2011-04-26 10:44 ` Guennadi Liakhovetski @ 2011-04-26 11:51 ` Rafael J. Wysocki 0 siblings, 0 replies; 27+ messages in thread From: Rafael J. Wysocki @ 2011-04-26 11:51 UTC (permalink / raw) To: Guennadi Liakhovetski; +Cc: linux-pm, Alan Stern, linux-mmc, Magnus Damm, LKML On Tuesday, April 26, 2011, Guennadi Liakhovetski wrote: > Hi Hi, > On Mon, 25 Apr 2011, Rafael J. Wysocki wrote: > > > On Saturday, April 23, 2011, Rafael J. Wysocki wrote: > > > On Friday, April 22, 2011, Alan Stern wrote: > > > > On Fri, 22 Apr 2011, Rafael J. Wysocki wrote: > > > > > > > > > > The barrier would not prevent the race between the notifier and runtie PM > > > > > > from taking place. Why don't we do something like this instead: > > > > > > > > > > > > --- > > > > > > drivers/base/dd.c | 3 ++- > > > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > > > > > Index: linux-2.6/drivers/base/dd.c > > > > > > =================================================================== > > > > > > --- linux-2.6.orig/drivers/base/dd.c > > > > > > +++ linux-2.6/drivers/base/dd.c > > > > > > @@ -326,6 +326,8 @@ static void __device_release_driver(stru > > > > > > BUS_NOTIFY_UNBIND_DRIVER, > > > > > > dev); > > > > > > > > > > > > + pm_runtime_put_sync(dev); > > > > > > + > > > > > > > > > > In fact, I think this one may be _noidle. If we allow the bus/driver > > > > > to do what they wont, we might as well let them handle the "device idle" > > > > > case from ->remove(). > > > > > > > > Maybe... But keeping it put_sync doesn't do any harm. In Guennadi's > > > > case, it might allow him to get rid of the pm_runtime_suspend() call in > > > > the remove routine. > > > > > > > > > > if (dev->bus && dev->bus->remove) > > > > > > dev->bus->remove(dev); > > > > > > else if (drv->remove) > > > > > > @@ -338,7 +340,6 @@ static void __device_release_driver(stru > > > > > > BUS_NOTIFY_UNBOUND_DRIVER, > > > > > > dev); > > > > > > > > > > > > - pm_runtime_put_sync(dev); > > > > > > } > > > > > > } > > > > > > > > Basically this is okay with me, and it should allow Guennadi to avoid > > > > the extra put/get pair. > > > > > > OK, so I'm going to put the appended patch into my linux-next branch > > > (hopefully, the problem is explained sufficiently in the changelog). > > > > I thought about that a bit more and came to the conclusion that we should > > do things a bit differently in __device_release_driver(). Namely, the fact > > that the device can be resumed (either synchronously or asynchronously) after > > the pm_runtime_barrier() has returned may be problematic too, because it > > may race with the bus notifier in some cases. For this reason, I think it > > would be better to do pm_runtime_get_sync() instead of the > > pm_runtime_get_noresume() and pm_runtime_barrier(). > > > > So, I think the appended patch would be better than the previous one. > > I refrained in taking part in the general rtpm API behaviour, I'd rather > rely on others here. If you push this your patch, I'll have to change my > TMIO/SDHI and MMCIF patches as follows: > > diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c > index 1889d64..3a22e55 100644 > --- a/drivers/mmc/host/sh_mmcif.c > +++ b/drivers/mmc/host/sh_mmcif.c > @@ -1117,6 +1117,8 @@ static int __devexit sh_mmcif_remove(struct platform_device *pdev) > struct sh_mmcif_host *host = platform_get_drvdata(pdev); > int irq[2]; > > + pm_runtime_get_sync(&pdev->dev); > + > mmc_remove_host(host->mmc); > sh_mmcif_release_dma(host); > > @@ -1137,7 +1139,6 @@ static int __devexit sh_mmcif_remove(struct platform_device *pdev) > mmc_free_host(host->mmc); > pm_runtime_put_sync(&pdev->dev); > pm_runtime_disable(&pdev->dev); > - pm_runtime_get_noresume(&pdev->dev); > > return 0; > } > diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c > index 26598f1..86eaa68 100644 > --- a/drivers/mmc/host/tmio_mmc_pio.c > +++ b/drivers/mmc/host/tmio_mmc_pio.c > @@ -956,17 +956,9 @@ void tmio_mmc_host_remove(struct tmio_mmc_host *host) > iounmap(host->ctl); > mmc_free_host(host->mmc); > > - /* > - * Now rtpm usage_count = 2, because we incremented it once in probe() > - * above, and dd.c incremented it again, before calling .release(). So. > - * to power the device down we have to decrement the counter to 0 and > - * suspend it, because after our disable() suspending from dd.c will > - * only decrement the counter, but not call any callbacks > - */ > - pm_runtime_put_noidle(&pdev->dev); > + /* Compensate for pm_runtime_get_sync() in probe() above */ > pm_runtime_put_sync(&pdev->dev); > pm_runtime_disable(&pdev->dev); > - pm_runtime_get_noresume(&pdev->dev); > } > EXPORT_SYMBOL(tmio_mmc_host_remove); So you won't get the reversed _put/_get calls any more here, good. :-) > Is this your patch final and shall I submit updated versions of my patches This patch will be final if there are no objections from other people. > or shall I wait for your patch to take its final form and hit "next?" It generally is better to wait for a patch to appear in linux-next before basing you work on top of it. Thanks, Rafael ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [linux-pm] [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend() 2011-04-25 10:29 ` [linux-pm] " Rafael J. Wysocki 2011-04-26 10:44 ` Guennadi Liakhovetski @ 2011-04-28 22:12 ` Rafael J. Wysocki 1 sibling, 0 replies; 27+ messages in thread From: Rafael J. Wysocki @ 2011-04-28 22:12 UTC (permalink / raw) To: linux-pm; +Cc: Alan Stern, linux-mmc, Magnus Damm, Guennadi Liakhovetski, LKML On Monday, April 25, 2011, Rafael J. Wysocki wrote: > On Saturday, April 23, 2011, Rafael J. Wysocki wrote: > > On Friday, April 22, 2011, Alan Stern wrote: > > > On Fri, 22 Apr 2011, Rafael J. Wysocki wrote: > > > > > > > > The barrier would not prevent the race between the notifier and runtie PM > > > > > from taking place. Why don't we do something like this instead: > > > > > > > > > > --- > > > > > drivers/base/dd.c | 3 ++- > > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > > > Index: linux-2.6/drivers/base/dd.c > > > > > =================================================================== > > > > > --- linux-2.6.orig/drivers/base/dd.c > > > > > +++ linux-2.6/drivers/base/dd.c > > > > > @@ -326,6 +326,8 @@ static void __device_release_driver(stru > > > > > BUS_NOTIFY_UNBIND_DRIVER, > > > > > dev); > > > > > > > > > > + pm_runtime_put_sync(dev); > > > > > + > > > > > > > > In fact, I think this one may be _noidle. If we allow the bus/driver > > > > to do what they wont, we might as well let them handle the "device idle" > > > > case from ->remove(). > > > > > > Maybe... But keeping it put_sync doesn't do any harm. In Guennadi's > > > case, it might allow him to get rid of the pm_runtime_suspend() call in > > > the remove routine. > > > > > > > > if (dev->bus && dev->bus->remove) > > > > > dev->bus->remove(dev); > > > > > else if (drv->remove) > > > > > @@ -338,7 +340,6 @@ static void __device_release_driver(stru > > > > > BUS_NOTIFY_UNBOUND_DRIVER, > > > > > dev); > > > > > > > > > > - pm_runtime_put_sync(dev); > > > > > } > > > > > } > > > > > > Basically this is okay with me, and it should allow Guennadi to avoid > > > the extra put/get pair. > > > > OK, so I'm going to put the appended patch into my linux-next branch > > (hopefully, the problem is explained sufficiently in the changelog). > > I thought about that a bit more and came to the conclusion that we should > do things a bit differently in __device_release_driver(). Namely, the fact > that the device can be resumed (either synchronously or asynchronously) after > the pm_runtime_barrier() has returned may be problematic too, because it > may race with the bus notifier in some cases. For this reason, I think it > would be better to do pm_runtime_get_sync() instead of the > pm_runtime_get_noresume() and pm_runtime_barrier(). > > So, I think the appended patch would be better than the previous one. Well, there haven't been any objections, so I'm adding the patch below to my linux-next branch. Thanks, Rafael > --- > From: Rafael J. Wysocki <rjw@sisk.pl> > Subject: PM / Runtime: Rework runtime PM handling during driver removal > > The driver core tries to prevent race conditions between runtime PM > and driver removal from happening by incrementing the runtime PM > usage counter of the device and executing pm_runtime_barrier() before > running the bus notifier and the ->remove() callbacks provided by the > device's subsystem or driver. This guarantees that, if a future > runtime suspend of the device has been scheduled or a runtime resume > or idle request has been queued up right before the driver removal, > it will be canceled or waited for to complete and no other > asynchronous runtime suspend or idle requests for the device will be > put into the PM workqueue until the ->remove() callback returns. > However, it doesn't prevent resume requests from being queued up > after pm_runtime_barrier() has been called and it doesn't prevent > pm_runtime_resume() from executing the device subsystem's runtime > resume callback. Morever, it prevents the device's subsystem or > driver from putting the device into the suspended state by calling > pm_runtime_suspend() from its ->remove() routine. This turns out to > be a major inconvenience for some subsystems and drivers that want to > leave the devices they handle in the suspended state. > > To really prevent runtime PM callbacks from racing with the bus > notifier callback in __device_release_driver(), which is necessary, > because the notifier is used by some subsystems to carry out > operations affecting the runtime PM functionality, use > pm_runtime_get_sync() instead of the combination of > pm_runtime_get_noresume() and pm_runtime_barrier(). This will resume > the device if it's in the suspended state and will prevent it from > being suspended again until pm_runtime_put_*() is called. > > To allow subsystems and drivers to put devices into the suspended > state by calling pm_runtime_suspend() from their ->remove() routines, > execute pm_runtime_put_sync() after running the bus notifier in > __device_release_driver(). This will require subsystems and drivers > to make their ->remove() callbacks avoid races with runtime PM > directly, but it will allow of more flexibility in the handling of > devices during the removal of their drivers. > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl> > --- > drivers/base/dd.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > Index: linux-2.6/drivers/base/dd.c > =================================================================== > --- linux-2.6.orig/drivers/base/dd.c > +++ linux-2.6/drivers/base/dd.c > @@ -316,8 +316,7 @@ static void __device_release_driver(stru > > drv = dev->driver; > if (drv) { > - pm_runtime_get_noresume(dev); > - pm_runtime_barrier(dev); > + pm_runtime_get_sync(dev); > > driver_sysfs_remove(dev); > > @@ -326,6 +325,8 @@ static void __device_release_driver(stru > BUS_NOTIFY_UNBIND_DRIVER, > dev); > > + pm_runtime_put_sync(dev); > + > if (dev->bus && dev->bus->remove) > dev->bus->remove(dev); > else if (drv->remove) > @@ -338,7 +339,6 @@ static void __device_release_driver(stru > BUS_NOTIFY_UNBOUND_DRIVER, > dev); > > - pm_runtime_put_sync(dev); > } > } > ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2011-04-28 22:12 UTC | newest] Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-04-19 10:46 [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend() Guennadi Liakhovetski 2011-04-19 12:44 ` Ohad Ben-Cohen 2011-04-19 13:23 ` Guennadi Liakhovetski 2011-04-19 14:16 ` Ohad Ben-Cohen 2011-04-19 14:26 ` Alan Stern 2011-04-19 22:59 ` Guennadi Liakhovetski 2011-04-20 14:22 ` Alan Stern 2011-04-20 14:50 ` Guennadi Liakhovetski 2011-04-20 15:12 ` Alan Stern 2011-04-20 20:06 ` Rafael J. Wysocki 2011-04-20 21:16 ` Alan Stern 2011-04-20 21:44 ` Rafael J. Wysocki 2011-04-21 13:58 ` Alan Stern 2011-04-21 18:00 ` Rafael J. Wysocki 2011-04-21 18:36 ` Alan Stern 2011-04-21 20:05 ` Rafael J. Wysocki 2011-04-21 21:48 ` Alan Stern 2011-04-21 22:06 ` Rafael J. Wysocki 2011-04-22 15:20 ` Alan Stern 2011-04-22 20:22 ` Rafael J. Wysocki 2011-04-22 20:25 ` Rafael J. Wysocki 2011-04-22 21:20 ` Alan Stern 2011-04-22 22:11 ` Rafael J. Wysocki 2011-04-25 10:29 ` [linux-pm] " Rafael J. Wysocki 2011-04-26 10:44 ` Guennadi Liakhovetski 2011-04-26 11:51 ` Rafael J. Wysocki 2011-04-28 22:12 ` Rafael J. Wysocki
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).