[PATCH] i2c: img-scb: fix PM device usage count

[PATCH] i2c: img-scb: 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
error branch.

Fixes: 93222bd9b966 ("i2c: img-scb: Add runtime PM")
Signed-off-by: Tobias Jordan <Tobias.Jordan@elektrobit.com>
---
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-img-scb.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
index f038858b6c54..569a1a8a2753 100644
--- a/drivers/i2c/busses/i2c-img-scb.c
+++ b/drivers/i2c/busses/i2c-img-scb.c
@@ -1061,8 +1061,10 @@ static int img_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 	}
 
 	ret = pm_runtime_get_sync(adap->dev.parent);
-	if (ret < 0)
+	if (ret < 0) {
+		pm_runtime_put_noidle(adap->dev.parent);
 		return ret;
+	}
 
 	for (i = 0; i < num; i++) {
 		struct i2c_msg *msg = &msgs[i];
@@ -1162,8 +1164,10 @@ static int img_i2c_init(struct img_i2c *i2c)
 	int ret;
 
 	ret = pm_runtime_get_sync(i2c->adap.dev.parent);
-	if (ret < 0)
+	if (ret < 0) {
+		pm_runtime_put_noidle(i2c->adap.dev.parent);
 		return ret;
+	}
 
 	rev = img_i2c_readl(i2c, SCB_CORE_REV_REG);
 	if ((rev & 0x00ffffff) < 0x00020200) {
-- 
2.11.0

Re: [SIL2review] [PATCH] i2c: img-scb: fix PM device usage count

[Nicholas Mc Guire]

On Sat, Feb 24, 2018 at 11:43:03PM +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
> error branch.

the increment is unconditional:
  pm_runtime_get_sync
  -> __pm_runtime_resume(dev, RPM_GET_PUT);
     -> if (rpmflags & RPM_GET_PUT) atomic_inc(&dev->power.usage_count);

the decrement though is conditional:
  pm_runtime_put_noidle
  -> atomic_add_unless(&dev->power.usage_count, -1, 0);

would it not be possible to use an atomic_dec() here rahter than
atomic_add_unless() ? probably only a few cylces

Also just wondering - could one not decrement in pm_runtime_get_sync on the
error path rather than defering this to the caller and fixing it there ?
something like:

int __pm_runtime_resume(struct device *dev, int rpmflags)
{
	...
	if (rpmflags & RPM_GET_PUT) 
		atomic_inc(&dev->power.usage_count);

        spin_lock_irqsave(&dev->power.lock, flags);
        retval = rpm_resume(dev, rpmflags);
        spin_unlock_irqrestore(&dev->power.lock, flags);

	if (retival < 0)
		atomic_dec(&dev->power.usage_count);

	return retval;
}

that would require other changes then I guess (did not look into it).
but if resume fails does it ever make sense to increment the use count ?

> 
> Fixes: 93222bd9b966 ("i2c: img-scb: Add runtime PM")
> Signed-off-by: Tobias Jordan <Tobias.Jordan@elektrobit.com>

Reviewed-by: Nicholas Mc Guire <der.herr@hofr.at>

> ---
> 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-img-scb.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
> index f038858b6c54..569a1a8a2753 100644
> --- a/drivers/i2c/busses/i2c-img-scb.c
> +++ b/drivers/i2c/busses/i2c-img-scb.c
> @@ -1061,8 +1061,10 @@ static int img_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
>  	}
>  
>  	ret = pm_runtime_get_sync(adap->dev.parent);
> -	if (ret < 0)
> +	if (ret < 0) {
> +		pm_runtime_put_noidle(adap->dev.parent);
>  		return ret;
> +	}
>  
>  	for (i = 0; i < num; i++) {
>  		struct i2c_msg *msg = &msgs[i];
> @@ -1162,8 +1164,10 @@ static int img_i2c_init(struct img_i2c *i2c)
>  	int ret;
>  
>  	ret = pm_runtime_get_sync(i2c->adap.dev.parent);
> -	if (ret < 0)
> +	if (ret < 0) {
> +		pm_runtime_put_noidle(i2c->adap.dev.parent);
>  		return ret;
> +	}
>  
>  	rev = img_i2c_readl(i2c, SCB_CORE_REV_REG);
>  	if ((rev & 0x00ffffff) < 0x00020200) {
> -- 
> 2.11.0
> 
> _______________________________________________
> SIL2review mailing list
> SIL2review@lists.osadl.org
> https://lists.osadl.org/mailman/listinfo/sil2review

Re: [SIL2review] [PATCH] i2c: img-scb: fix PM device usage count

[Tobias Jordan]

Hi!

On Sun, 25 Feb 2018 13:20:14 +0000 Nicholas Mc Guire wrote:

> the decrement though is conditional:
>   pm_runtime_put_noidle
>   -> atomic_add_unless(&dev->power.usage_count, -1, 0);  

pm_runtime_put_noidle is playing it safe by not decrementing past 0, I
think that's a good thing.

> Also just wondering - could one not decrement in pm_runtime_get_sync
> on the error path rather than defering this to the caller and fixing
> it there ?

That's what I've asked linux-pm in the linked discussion:

> > https://marc.info/?l=linux-pm&m=151904483924999&w=2

As far as I've understood the idea is that most "error" return values
actually are a result of disabled runtime PM, and that should be
transparent to the caller. Looking at the code, that's what the vast
majority of callers do - they just ignore the return value of
pm_runtime_get_sync, and somewhere later have an
unconditional pm_runtime_put_... call.

So the only issue are callers that don't ignore the pm_runtime_get_sync
return value, probably because they're having some kind of special
requirements for error handling. For those, they need to ensure that a
proper _put_ is done somewhere in the error path.

> Reviewed-by: Nicholas Mc Guire <der.herr@hofr.at>

Thanks for the review!,

Tobias

Re: [SIL2review] [PATCH] i2c: img-scb: fix PM device usage count

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 990 bytes --]


> As far as I've understood the idea is that most "error" return values
> actually are a result of disabled runtime PM, and that should be
> transparent to the caller. Looking at the code, that's what the vast
> majority of callers do - they just ignore the return value of
> pm_runtime_get_sync, and somewhere later have an
> unconditional pm_runtime_put_... call.
> 
> So the only issue are callers that don't ignore the pm_runtime_get_sync
> return value, probably because they're having some kind of special
> requirements for error handling. For those, they need to ensure that a
> proper _put_ is done somewhere in the error path.

Is it easily recognizable if the drivers check the error code because
there is a reason or if they do it "out of habit"? For the latter, I
agree that the better fix would be to remove the error check altogether.
Otherwise the code becomes more complex for no reason and even less
people are then brave enough to simplify it later.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [SIL2review] [PATCH] i2c: img-scb: fix PM device usage count

[Tobias.Jordan]

Hi,

> Is it easily recognizable if the drivers check the error code because
> there is a reason or if they do it "out of habit"?

Probably by looking closely at the implementation of the PM callouts
for the driver, but I couldn't find a pattern that would be easy to
recognize. Maybe I didn't look close enough ;-)

I concur that removing the check would be a better approach if it's
clear that it's just done "out of habit".

Actually, the real problem is to find the drivers that need to have
the check in, add/fix it for them, and remove it for all others.

Unfortunately, all I'm currently able to do is finding the parts that
are inconsistent.

Tobias