Message ID | 20180224224328.2pfki7rqcxqetmlz@agrajag.zerfleddert.de |
---|---|
State | Deferred |
Headers | show |
Series | i2c: imx: Fix PM device usage count | expand |
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
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
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");
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(-)