| Message ID | 20200906185039.22700-7-digetx@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Improvements for Tegra I2C driver | expand |
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) > } > put_rpm: > + pm_runtime_put_sync(&pdev->dev); > > disable_rpm:
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 5:32 PM Dmitry Osipenko <digetx@gmail.com> wrote: > 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. 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.
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 пишет: >>> 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. > > 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. 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.
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.
07.09.2020 18:34, Andy Shevchenko пишет: > 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. > Technically it shouldn't be a big problem to change the code, but I could imagine the amount of effort it will take to get the changes merged. IIRC, there was also a push back to a such change from the RPM maintainer, so there could be difficulties beyond the code changes.
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(-)