[PATCH] i2c: imx: Fix PM device usage count

[PATCH] i2c: imx: Fix PM device usage count

[Tobias Jordan]

pm_runtime_get_sync() increases the device's usage count even when
reporting an error, so add a call to pm_runtime_put_noidle() in the
related error branches.

Fixes: 588eb93ea49f ("i2c: imx: add runtime pm support to improve the
  performance")
Signed-off-by: Tobias Jordan <Tobias.Jordan@elektrobit.com>
---
In i2c_imx_xfer(), one could also move the "out" label up (in front of
the call to pm_runtime_put_autosuspend()), but I'm not sure what the
underlying error scenario is; calling _put_noidle() seems to be the
safer bet.

This is one of a number of patches for problems found using coccinelle 
scripting in the SIL2LinuxMP project. The patch has been compile-tested;
it's based on linux-next-20180223.

For a discussion of the corresponding issue, see
https://marc.info/?l=linux-pm&m=151904483924999&w=2

 drivers/i2c/busses/i2c-imx.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 999557729ad2..dae3c923d42c 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -890,8 +890,10 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter,
 	dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
 
 	result = pm_runtime_get_sync(i2c_imx->adapter.dev.parent);
-	if (result < 0)
+	if (result < 0) {
+		pm_runtime_put_noidle(i2c_imx->adapter.dev.parent);
 		goto out;
+	}
 
 	/* Start I2C transfer */
 	result = i2c_imx_start(i2c_imx);
@@ -1179,8 +1181,10 @@ static int i2c_imx_remove(struct platform_device *pdev)
 	int ret;
 
 	ret = pm_runtime_get_sync(&pdev->dev);
-	if (ret < 0)
+	if (ret < 0) {
+		pm_runtime_put_noidle(&pdev->dev);
 		return ret;
+	}
 
 	/* remove adapter */
 	dev_dbg(&i2c_imx->adapter.dev, "adapter removed\n");
-- 
2.11.0

Re: [PATCH] i2c: imx: Fix PM device usage count

[Uwe Kleine-König]

On Sat, Feb 24, 2018 at 11:43:28PM +0100, Tobias Jordan wrote:
> pm_runtime_get_sync() increases the device's usage count even when
> reporting an error, so add a call to pm_runtime_put_noidle() in the
> related error branches.
> 
> Fixes: 588eb93ea49f ("i2c: imx: add runtime pm support to improve the
>   performance")
> Signed-off-by: Tobias Jordan <Tobias.Jordan@elektrobit.com>
> ---
> In i2c_imx_xfer(), one could also move the "out" label up (in front of
> the call to pm_runtime_put_autosuspend()), but I'm not sure what the
> underlying error scenario is; calling _put_noidle() seems to be the
> safer bet.
> 
> This is one of a number of patches for problems found using coccinelle 
> scripting in the SIL2LinuxMP project. The patch has been compile-tested;
> it's based on linux-next-20180223.
> 
> For a discussion of the corresponding issue, see
> https://marc.info/?l=linux-pm&m=151904483924999&w=2

I don't get the original mail, so reply here. In reply to the question I
would have asked here, too:

> Why isn't ...get_sync() directly calling ...put_noidle() but relies on
> the driver implementation to do it? It seems unintuitive for a _get_
> function to increase the usage count although returning an error.

Rafael replied:

> Because ...get_sync() returns an error when runtime PM is disabled and
> we wanted that case to be transparent for the users of it.
> 
> In the majority of cases (if not always) errors returned by
> ...get_sync()
> mean disabled runtime PM or flaky hardware and the latter is much less
> common (and generally there's not much to do about them in the kernel
> anyway).

If pm_runtime_get_sync() should be transparent for the users if PM is
disable, why not simply return success then? Or introduce a return value
convention like:

	<0 (i.e. -ESOMETHIN) is error
	0 success
	1 PM is disabled

(Taking a quick glimpse there seem to be already some cases where 1 or
-ESOMETHING is returned, but I didn't find documentation explaining the
return values.)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] i2c: imx: Fix PM device usage count

[Rafael J. Wysocki]

On Sunday, February 25, 2018 8:57:48 PM CET Uwe Kleine-König wrote:
> On Sat, Feb 24, 2018 at 11:43:28PM +0100, Tobias Jordan wrote:
> > pm_runtime_get_sync() increases the device's usage count even when
> > reporting an error, so add a call to pm_runtime_put_noidle() in the
> > related error branches.
> > 
> > Fixes: 588eb93ea49f ("i2c: imx: add runtime pm support to improve the
> >   performance")
> > Signed-off-by: Tobias Jordan <Tobias.Jordan@elektrobit.com>
> > ---
> > In i2c_imx_xfer(), one could also move the "out" label up (in front of
> > the call to pm_runtime_put_autosuspend()), but I'm not sure what the
> > underlying error scenario is; calling _put_noidle() seems to be the
> > safer bet.
> > 
> > This is one of a number of patches for problems found using coccinelle 
> > scripting in the SIL2LinuxMP project. The patch has been compile-tested;
> > it's based on linux-next-20180223.
> > 
> > For a discussion of the corresponding issue, see
> > https://marc.info/?l=linux-pm&m=151904483924999&w=2
> 
> I don't get the original mail, so reply here. In reply to the question I
> would have asked here, too:
> 
> > Why isn't ...get_sync() directly calling ...put_noidle() but relies on
> > the driver implementation to do it? It seems unintuitive for a _get_
> > function to increase the usage count although returning an error.
> 
> Rafael replied:
> 
> > Because ...get_sync() returns an error when runtime PM is disabled and
> > we wanted that case to be transparent for the users of it.
> > 
> > In the majority of cases (if not always) errors returned by
> > ...get_sync()
> > mean disabled runtime PM or flaky hardware and the latter is much less
> > common (and generally there's not much to do about them in the kernel
> > anyway).
> 
> If pm_runtime_get_sync() should be transparent for the users if PM is
> disable, why not simply return success then?

Because *some* users of it want to get the error.

They generally need to handle the rumtime PM disabled situation differently.

Thanks,
Rafael