| Message ID | Pine.LNX.4.64.1104152011090.18593@axis700.grange (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Fri, 15 Apr 2011, Guennadi Liakhovetski wrote: > Restore the initial RPM_SUSPENDED runtime pm status, when disabling, > otherwise the following enable will not function. This happens, e.g., > when unloading and reloading drivers. > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> > Cc: Simon Horman <horms@verge.net.au> > Cc: Magnus Damm <damm@opensource.se> > --- > > No, I do not claim to understand the thousand of states, flags, and > counters, this just happens to fix the problem for me. Feel free to make a > correct solution out of this. > > drivers/base/power/runtime.c | 4 +++- > 1 files changed, 3 insertions(+), 1 deletions(-) > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c > index 3172c60..83d6898 100644 > --- a/drivers/base/power/runtime.c > +++ b/drivers/base/power/runtime.c > @@ -1012,8 +1012,10 @@ void __pm_runtime_disable(struct device *dev, bool check_resume) > pm_runtime_put_noidle(dev); > } > > - if (!dev->power.disable_depth++) > + if (!dev->power.disable_depth++) { > __pm_runtime_barrier(dev); > + dev->power.runtime_status = RPM_SUSPENDED; > + } > > out: > spin_unlock_irq(&dev->power.lock); This certainly doesn't look right. Can you explain in more detail the problem you are trying to solve? Alan Stern
On Fri, 15 Apr 2011, Alan Stern wrote: > On Fri, 15 Apr 2011, Guennadi Liakhovetski wrote: > > > Restore the initial RPM_SUSPENDED runtime pm status, when disabling, > > otherwise the following enable will not function. This happens, e.g., > > when unloading and reloading drivers. > > > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> > > Cc: Simon Horman <horms@verge.net.au> > > Cc: Magnus Damm <damm@opensource.se> > > --- > > > > No, I do not claim to understand the thousand of states, flags, and > > counters, this just happens to fix the problem for me. Feel free to make a > > correct solution out of this. > > > > drivers/base/power/runtime.c | 4 +++- > > 1 files changed, 3 insertions(+), 1 deletions(-) > > > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c > > index 3172c60..83d6898 100644 > > --- a/drivers/base/power/runtime.c > > +++ b/drivers/base/power/runtime.c > > @@ -1012,8 +1012,10 @@ void __pm_runtime_disable(struct device *dev, bool check_resume) > > pm_runtime_put_noidle(dev); > > } > > > > - if (!dev->power.disable_depth++) > > + if (!dev->power.disable_depth++) { > > __pm_runtime_barrier(dev); > > + dev->power.runtime_status = RPM_SUSPENDED; > > + } > > > > out: > > spin_unlock_irq(&dev->power.lock); > > This certainly doesn't look right. Can you explain in more detail the > problem you are trying to solve? Yes, I can. On the first loading of an MMC driver, which does the standard pm_runtime_enable(&pdev->dev); ret = pm_runtime_resume(&pdev->dev); in .probe() and pm_runtime_disable(&pdev->dev); in .release() (see [1]), with an inserted card, on the first modprobe I see a full pm-runtime run down to platform_pm_runtime_resume() and to platform_pm_runtime_suspend() on rmmod. On a repeated modprobe platform_pm_runtime_resume() does not get called, because dev->power.runtime_status != RPM_SUSPENDED, instead, it is still RPM_ACTIVE, so, rpm_resume() exits prematurily. [1] http://thread.gmane.org/gmane.linux.kernel.mmc/7364 Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/
On Fri, 15 Apr 2011, Guennadi Liakhovetski wrote: > On Fri, 15 Apr 2011, Alan Stern wrote: > > > On Fri, 15 Apr 2011, Guennadi Liakhovetski wrote: > > > > > Restore the initial RPM_SUSPENDED runtime pm status, when disabling, > > > otherwise the following enable will not function. This happens, e.g., > > > when unloading and reloading drivers. ... > > This certainly doesn't look right. Can you explain in more detail the > > problem you are trying to solve? > > Yes, I can. On the first loading of an MMC driver, which does the standard > > pm_runtime_enable(&pdev->dev); > ret = pm_runtime_resume(&pdev->dev); > > in .probe() and > > pm_runtime_disable(&pdev->dev); > > in .release() (see [1]), with an inserted card, on the first modprobe I > see a full pm-runtime run down to platform_pm_runtime_resume() and to > platform_pm_runtime_suspend() on rmmod. On a repeated modprobe > platform_pm_runtime_resume() does not get called, because > dev->power.runtime_status != RPM_SUSPENDED, instead, it is still > RPM_ACTIVE, so, rpm_resume() exits prematurily. I see. And is it true that you really do want to set the status to RPM_SUSPENDED without touching the hardware, i.e., without disabling any clocks or gating any power supplies? If so, then your release routine should simply do pm_set_suspended(&pdev->dev); after calling pm_runtime_disable(). There's no need to modify the runtime PM core. Alan Stern
On Fri, 15 Apr 2011, Alan Stern wrote: > On Fri, 15 Apr 2011, Guennadi Liakhovetski wrote: > > > On Fri, 15 Apr 2011, Alan Stern wrote: > > > > > On Fri, 15 Apr 2011, Guennadi Liakhovetski wrote: > > > > > > > Restore the initial RPM_SUSPENDED runtime pm status, when disabling, > > > > otherwise the following enable will not function. This happens, e.g., > > > > when unloading and reloading drivers. > > ... > > > > This certainly doesn't look right. Can you explain in more detail the > > > problem you are trying to solve? > > > > Yes, I can. On the first loading of an MMC driver, which does the standard > > > > pm_runtime_enable(&pdev->dev); > > ret = pm_runtime_resume(&pdev->dev); > > > > in .probe() and > > > > pm_runtime_disable(&pdev->dev); > > > > in .release() (see [1]), with an inserted card, on the first modprobe I > > see a full pm-runtime run down to platform_pm_runtime_resume() and to > > platform_pm_runtime_suspend() on rmmod. On a repeated modprobe > > platform_pm_runtime_resume() does not get called, because > > dev->power.runtime_status != RPM_SUSPENDED, instead, it is still > > RPM_ACTIVE, so, rpm_resume() exits prematurily. > > I see. And is it true that you really do want to set the status to > RPM_SUSPENDED without touching the hardware, i.e., without disabling > any clocks or gating any power supplies? I'd expected, that pm_runtime_disable() would do all the necessary suspending too in such a case. Isn't this "enable-resume-disable" sequence standard and shouldn't this suffice for the complete cycle? Thanks Guennadi > If so, then your release routine should simply do > > pm_set_suspended(&pdev->dev); > > after calling pm_runtime_disable(). There's no need to modify the > runtime PM core. > > Alan Stern > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/
On Fri, 15 Apr 2011, Guennadi Liakhovetski wrote: > > > Yes, I can. On the first loading of an MMC driver, which does the standard > > > > > > pm_runtime_enable(&pdev->dev); > > > ret = pm_runtime_resume(&pdev->dev); > > > > > > in .probe() and > > > > > > pm_runtime_disable(&pdev->dev); > > > > > > in .release() (see [1]), with an inserted card, on the first modprobe I > > > see a full pm-runtime run down to platform_pm_runtime_resume() and to > > > platform_pm_runtime_suspend() on rmmod. On a repeated modprobe > > > platform_pm_runtime_resume() does not get called, because > > > dev->power.runtime_status != RPM_SUSPENDED, instead, it is still > > > RPM_ACTIVE, so, rpm_resume() exits prematurily. > > > > I see. And is it true that you really do want to set the status to > > RPM_SUSPENDED without touching the hardware, i.e., without disabling > > any clocks or gating any power supplies? > > I'd expected, that pm_runtime_disable() would do all the necessary > suspending too in such a case. No, pm_runtime_disable() merely prevents the runtime PM callbacks from being used; it doesn't change the device's power status. If you really want to suspend the device, call pm_runtime_suspend() before the runtime_disable. And if you want to leave the device alone but change the power status value, call pm_set_suspended() after the runtime_disable. > Isn't this "enable-resume-disable" sequence > standard and shouldn't this suffice for the complete cycle? It isn't standard as far as I know. More common is: enable supend resume suspend resume suspend resume ... (many repeats of suspend resume) ... disable Obviously this would be slightly different if the device's default state (i.e., its state before the kernel detected it) was unpowered. Alan Stern
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c index 3172c60..83d6898 100644 --- a/drivers/base/power/runtime.c +++ b/drivers/base/power/runtime.c @@ -1012,8 +1012,10 @@ void __pm_runtime_disable(struct device *dev, bool check_resume) pm_runtime_put_noidle(dev); } - if (!dev->power.disable_depth++) + if (!dev->power.disable_depth++) { __pm_runtime_barrier(dev); + dev->power.runtime_status = RPM_SUSPENDED; + } out: spin_unlock_irq(&dev->power.lock);
Restore the initial RPM_SUSPENDED runtime pm status, when disabling, otherwise the following enable will not function. This happens, e.g., when unloading and reloading drivers. Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> Cc: Simon Horman <horms@verge.net.au> Cc: Magnus Damm <damm@opensource.se> --- No, I do not claim to understand the thousand of states, flags, and counters, this just happens to fix the problem for me. Feel free to make a correct solution out of this. drivers/base/power/runtime.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)