| Message ID | 20200601061640.27632-1-dinghao.liu@zju.edu.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] i2c: imx-lpi2c: Fix runtime PM imbalance on error | expand |
From: Dinghao Liu <dinghao.liu@zju.edu.cn> Sent: Monday, June 1, 2020 2:17 PM > pm_runtime_get_sync() increments the runtime PM usage counter even the > call returns an error code. Thus a corresponding decrement is needed on the > error handling path to keep the counter balanced. > > Fix this by adding the missed function call. > > Fixes: 13d6eb20fc79a ("i2c: imx-lpi2c: add runtime pm support") > Co-developed-by: Markus Elfring <Markus.Elfring@web.de> > Signed-off-by: Markus Elfring <Markus.Elfring@web.de> > Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn> Reviewed-by: Fugang Duan <fugang.duan@nxp.com> > --- > > Changelog: > > v2: - Use pm_runtime_put_noidle() instead of > pm_runtime_put_autosuspend(). > > v3: - Refine commit message. > --- > drivers/i2c/busses/i2c-imx-lpi2c.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c > b/drivers/i2c/busses/i2c-imx-lpi2c.c > index 94743ba581fe..bdee02dff284 100644 > --- a/drivers/i2c/busses/i2c-imx-lpi2c.c > +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c > @@ -260,8 +260,10 @@ static int lpi2c_imx_master_enable(struct > lpi2c_imx_struct *lpi2c_imx) > int ret; > > ret = pm_runtime_get_sync(lpi2c_imx->adapter.dev.parent); > - if (ret < 0) > + if (ret < 0) { > + pm_runtime_put_noidle(lpi2c_imx->adapter.dev.parent); > return ret; > + } > > temp = MCR_RST; > writel(temp, lpi2c_imx->base + LPI2C_MCR); > -- > 2.17.1
> pm_runtime_get_sync() increments the runtime PM usage counter even > the call returns an error code. Thus a corresponding decrement is > needed on the error handling path to keep the counter balanced. > > Fix this by adding the missed function call. How do you think about a wording variant like the following? Change description: The PM runtime usage counter is incremented even if a call of the function “pm_runtime_get_sync” failed. Thus decrement it also in an error case so that the reference counting is kept consistent. Regards, Markus
On Mon, Jun 01, 2020 at 02:16:40PM +0800, Dinghao Liu wrote: > pm_runtime_get_sync() increments the runtime PM usage counter even > the call returns an error code. Thus a corresponding decrement is > needed on the error handling path to keep the counter balanced. Can you point me to a discussion where it was decided that this is a proper fix? I'd think we rather should fix pm_runtime_get_sync() but maybe there are technical reasons against it.
> > Can you point me to a discussion where it was decided that this is a > proper fix? I'd think we rather should fix pm_runtime_get_sync() but > maybe there are technical reasons against it. > There is a discussion here: https://lkml.org/lkml/2020/5/20/1100 There are many use cases that suppose pm_runtime_get_sync() will always increment the usage counter and do not check its return value. So I don't think we should adjust this function directly. As for this API, Dan suggested a replacement (wrapper) for later developers. I think this is the best solution. https://lore.kernel.org/patchwork/patch/1245375/ Regards, Dinghao
> From: Wolfram Sang <wsa@kernel.org> > Sent: Sunday, June 14, 2020 5:12 PM > > On Mon, Jun 01, 2020 at 02:16:40PM +0800, Dinghao Liu wrote: > > pm_runtime_get_sync() increments the runtime PM usage counter even the > > call returns an error code. Thus a corresponding decrement is needed > > on the error handling path to keep the counter balanced. > > Can you point me to a discussion where it was decided that this is a proper fix? > I'd think we rather should fix pm_runtime_get_sync() but maybe there are > technical reasons against it. I had the same feeling. Copy pm guys to comments. Regards Aisheng
On Mon, Jun 15, 2020 at 06:33:40AM +0000, Aisheng Dong wrote: > > From: Wolfram Sang <wsa@kernel.org> > > Sent: Sunday, June 14, 2020 5:12 PM > > > > On Mon, Jun 01, 2020 at 02:16:40PM +0800, Dinghao Liu wrote: > > > pm_runtime_get_sync() increments the runtime PM usage counter even the > > > call returns an error code. Thus a corresponding decrement is needed > > > on the error handling path to keep the counter balanced. > > > > Can you point me to a discussion where it was decided that this is a proper fix? > > I'd think we rather should fix pm_runtime_get_sync() but maybe there are > > technical reasons against it. > > I had the same feeling. > Copy pm guys to comments. I started a seperate thread: https://lkml.org/lkml/2020/6/14/76 Still, on-going discussion if the proper fix is to remove the error check.
> I started a seperate thread: > > https://lkml.org/lkml/2020/6/14/76 > > Still, on-going discussion if the proper fix is to remove the error check. I find that a bit of additional information can make such a link safer. RFC: a failing pm_runtime_get increases the refcnt? https://lore.kernel.org/lkml/20200614090751.GA2878@kunai/ How will the clarification of corresponding software aspects evolve further? Regards, Markus
diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c index 94743ba581fe..bdee02dff284 100644 --- a/drivers/i2c/busses/i2c-imx-lpi2c.c +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c @@ -260,8 +260,10 @@ static int lpi2c_imx_master_enable(struct lpi2c_imx_struct *lpi2c_imx) int ret; ret = pm_runtime_get_sync(lpi2c_imx->adapter.dev.parent); - if (ret < 0) + if (ret < 0) { + pm_runtime_put_noidle(lpi2c_imx->adapter.dev.parent); return ret; + } temp = MCR_RST; writel(temp, lpi2c_imx->base + LPI2C_MCR);
pm_runtime_get_sync() increments the runtime PM usage counter even the call returns an error code. Thus a corresponding decrement is needed on the error handling path to keep the counter balanced. Fix this by adding the missed function call. Fixes: 13d6eb20fc79a ("i2c: imx-lpi2c: add runtime pm support") Co-developed-by: Markus Elfring <Markus.Elfring@web.de> Signed-off-by: Markus Elfring <Markus.Elfring@web.de> Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn> --- Changelog: v2: - Use pm_runtime_put_noidle() instead of pm_runtime_put_autosuspend(). v3: - Refine commit message. --- drivers/i2c/busses/i2c-imx-lpi2c.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)