| Message ID | 20200906185039.22700-7-digetx@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | None | expand |
07.09.2020 11:10, Andy Shevchenko пишет: > On Sun, Sep 6, 2020 at 9:51 PM Dmitry Osipenko <digetx@gmail.com> wrote: >> >> The runtime PM is guaranteed to be always available on Tegra after commit >> 40b2bb1b132a ("ARM: tegra: enforce PM requirement"). Hence let's remove >> all the RPM-availability checking and handling from the code. > >> + ret = pm_runtime_get_sync(i2c_dev->dev); >> + if (ret < 0) { >> + dev_err(dev, "runtime resume failed\n"); >> + goto disable_rpm; > > As in the original code here is a refcount leak. > Should call pm_runtime_put_noidle(). (Possible to use goto put_rpm; > but in that case the code a bit confusing to the reader) Good point! I already forgot about this RPM API problem! I'll add a patch to address this. Would be great if anyone could put effort into changing the default get_sync() behaviour and add get_sync_nofail(). Otherwise this will be a never ending problem.
On Mon, Sep 7, 2020 at 6:25 PM Dmitry Osipenko <digetx@gmail.com> wrote: > 07.09.2020 18:05, Andy Shevchenko пишет: > > On Mon, Sep 7, 2020 at 5:32 PM Dmitry Osipenko <digetx@gmail.com> wrote: > >> 07.09.2020 11:10, Andy Shevchenko пишет: ... > >> Would be great if anyone could put effort into changing the default > >> get_sync() behaviour and add get_sync_nofail(). Otherwise this will be a > >> never ending problem. > > > > I didn't get this. For time being the API (yes, with its all cons) has > > the clear usage: > > a) don't check for errors -- you are fine > > b) if you start checking errors, keep in mind refcounting. > > > > So, I don't see how nofail() can fix b) case. > > > > It's a very unintuitive behaviour which none of other APIs have. I would > never expect the refcount to be bumped in a case of error, this is a > clear drawback of the API, IMO. I agree. > Perhaps this is not seen as a problem by > people who have excellent memory and can easily remember about existence > of such non-standard quirks, or by people who're touching the RPM code > frequently. ...or by running coccinelle script.
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c index 4a9375fc71d8..c296095ace87 100644 --- a/drivers/i2c/busses/i2c-tegra.c +++ b/drivers/i2c/busses/i2c-tegra.c @@ -1773,18 +1773,10 @@ static int tegra_i2c_probe(struct platform_device *pdev) if (!i2c_dev->is_vi) pm_runtime_irq_safe(&pdev->dev); pm_runtime_enable(&pdev->dev); - if (!pm_runtime_enabled(&pdev->dev)) { - ret = tegra_i2c_runtime_resume(&pdev->dev); - if (ret < 0) { - dev_err(&pdev->dev, "runtime resume failed\n"); - goto unprepare_div_clk; - } - } else { - ret = pm_runtime_get_sync(i2c_dev->dev); - if (ret < 0) { - dev_err(&pdev->dev, "runtime resume failed\n"); - goto disable_rpm; - } + ret = pm_runtime_get_sync(i2c_dev->dev); + if (ret < 0) { + dev_err(dev, "runtime resume failed\n"); + goto disable_rpm; } if (i2c_dev->is_multimaster_mode) { @@ -1843,16 +1835,10 @@ static int tegra_i2c_probe(struct platform_device *pdev) clk_disable(i2c_dev->div_clk); put_rpm: - if (pm_runtime_enabled(&pdev->dev)) - pm_runtime_put_sync(&pdev->dev); - else - tegra_i2c_runtime_suspend(&pdev->dev); + pm_runtime_put_sync(&pdev->dev); disable_rpm: - if (pm_runtime_enabled(&pdev->dev)) - pm_runtime_disable(&pdev->dev); - -unprepare_div_clk: + pm_runtime_disable(&pdev->dev); clk_unprepare(i2c_dev->div_clk); unprepare_slow_clk: @@ -1874,8 +1860,6 @@ static int tegra_i2c_remove(struct platform_device *pdev) clk_disable(i2c_dev->div_clk); pm_runtime_disable(&pdev->dev); - if (!pm_runtime_status_suspended(&pdev->dev)) - tegra_i2c_runtime_suspend(&pdev->dev); clk_unprepare(i2c_dev->div_clk); clk_unprepare(i2c_dev->slow_clk);
The runtime PM is guaranteed to be always available on Tegra after commit 40b2bb1b132a ("ARM: tegra: enforce PM requirement"). Hence let's remove all the RPM-availability checking and handling from the code. Reviewed-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> --- drivers/i2c/busses/i2c-tegra.c | 28 ++++++---------------------- 1 file changed, 6 insertions(+), 22 deletions(-)