[PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend()

[PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend()

[Guennadi Liakhovetski]

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)

Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend()

[Ohad Ben-Cohen]

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.

Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend()

[Guennadi Liakhovetski]

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/

Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend()

[Ohad Ben-Cohen]

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.

Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend()

[Alan Stern]

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

Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend()

[Guennadi Liakhovetski]

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/

Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend()

[Alan Stern]

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

Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend()

[Guennadi Liakhovetski]

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/

Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend()

[Alan Stern]

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

Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend()

[Rafael J. Wysocki]

[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

Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend()

[Alan Stern]

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

Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend()

[Rafael J. Wysocki]

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

Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend()

[Alan Stern]

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

Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend()

[Rafael J. Wysocki]

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

Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend()

[Alan Stern]

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

Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend()

[Rafael J. Wysocki]

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

Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend()

[Alan Stern]

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

Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend()

[Rafael J. Wysocki]

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

Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend()

[Alan Stern]

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);
 	}
 }
 

Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend()

[Rafael J. Wysocki]

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

Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend()

[Rafael J. Wysocki]

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

Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend()

[Alan Stern]

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

Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend()

[Rafael J. Wysocki]

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);
 	}
 }
 

Re: [linux-pm] [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend()

[Rafael J. Wysocki]

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);
 	}
 }
 

Re: [linux-pm] [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend()

[Guennadi Liakhovetski]

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/

Re: [linux-pm] [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend()

[Rafael J. Wysocki]

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

Re: [linux-pm] [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend()

[Rafael J. Wysocki]

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);
>  	}
>  }
>