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

Guennadi Liakhovetski
2011-04-20T14:50:43Z

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