| Message ID | 20200513171206.6600-1-wsa@kernel.org (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [RFC] mmc: tmio: properly balance RPM on remove | expand |
On Wed, 13 May 2020 at 19:12, Wolfram Sang <wsa@kernel.org> wrote: > > From: Wolfram Sang <wsa+renesas@sang-engineering.com> > > Because we enabled the device with _noresume, we should disable it with > _noidle to match the ref counting of the clocks during remove(). > > Reported-by: Geert Uytterhoeven <geert+renesas@glider.be> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > > I think this is the proper fix to the problem Geert reported [1]. I am > not sure about a proper Fixes-tag, though. The corresponding _noidle > call in the probe()-error-path was added with: > > aa86f1a38875 ("mmc: tmio: Fixup runtime PM management during probe") > > However, from my understanding this is more fitting? > > 1b32999e205b ("mmc: tmio: Avoid boilerplate code in ->runtime_suspend()") > > But maybe my understanding of the situation is still not perfect and > even the commit message is bogus? Ulf, since both mentioned commits are > from you, could you have a look? Thanks in advance! I did start to look at it, but got sidetracked with other things. Anyway, I will check in the next couple of days. Kind regards Uffe > > Wolfram > > [1] Message-ID: <alpine.DEB.2.21.2004291630090.4052@ramsan.of.borg> > > drivers/mmc/host/tmio_mmc_core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c > index 9520bd94cf43..9a4ae954553b 100644 > --- a/drivers/mmc/host/tmio_mmc_core.c > +++ b/drivers/mmc/host/tmio_mmc_core.c > @@ -1235,7 +1235,7 @@ void tmio_mmc_host_remove(struct tmio_mmc_host *host) > pm_runtime_dont_use_autosuspend(&pdev->dev); > if (host->native_hotplug) > pm_runtime_put_noidle(&pdev->dev); > - pm_runtime_put_sync(&pdev->dev); > + pm_runtime_put_noidle(&pdev->dev); > pm_runtime_disable(&pdev->dev); > } > EXPORT_SYMBOL_GPL(tmio_mmc_host_remove); > -- > 2.20.1 >
> I did start to look at it, but got sidetracked with other things. > Anyway, I will check in the next couple of days. Much appreciated, thanks!
Hi Wolfram, On Wed, May 13, 2020 at 7:12 PM Wolfram Sang <wsa@kernel.org> wrote: > From: Wolfram Sang <wsa+renesas@sang-engineering.com> > > Because we enabled the device with _noresume, we should disable it with > _noidle to match the ref counting of the clocks during remove(). > > Reported-by: Geert Uytterhoeven <geert+renesas@glider.be> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > I think this is the proper fix to the problem Geert reported [1]. I am Thanks, I can confirm this fixes the issue on unbind, so Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> > not sure about a proper Fixes-tag, though. The corresponding _noidle > call in the probe()-error-path was added with: > > aa86f1a38875 ("mmc: tmio: Fixup runtime PM management during probe") That one also added a pm_runtime_put() call to the normal probe path, but a pm_runtime_put_noidle() call to the error path, causing another possible imbalance, which is still there. So I guess the former has to be replaced by a _noidle call, too? Also, the conditional if (_host->native_hotplug) pm_runtime_get_noresume(&pdev->dev); in probe() is not undone in the error path. > However, from my understanding this is more fitting? > > 1b32999e205b ("mmc: tmio: Avoid boilerplate code in ->runtime_suspend()") > > But maybe my understanding of the situation is still not perfect and > even the commit message is bogus? Ulf, since both mentioned commits are > from you, could you have a look? Thanks in advance! > > Wolfram > > [1] Message-ID: <alpine.DEB.2.21.2004291630090.4052@ramsan.of.borg> Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c index 9520bd94cf43..9a4ae954553b 100644 --- a/drivers/mmc/host/tmio_mmc_core.c +++ b/drivers/mmc/host/tmio_mmc_core.c @@ -1235,7 +1235,7 @@ void tmio_mmc_host_remove(struct tmio_mmc_host *host) pm_runtime_dont_use_autosuspend(&pdev->dev); if (host->native_hotplug) pm_runtime_put_noidle(&pdev->dev); - pm_runtime_put_sync(&pdev->dev); + pm_runtime_put_noidle(&pdev->dev); pm_runtime_disable(&pdev->dev); } EXPORT_SYMBOL_GPL(tmio_mmc_host_remove);