Skip to contentSkip to conversation

I2C: OMAP: xfer: fix runtime PM get/put balance on error

PATCH New Kevin Hilman 4 participants 6 messages Jun 27, 2012 – Aug 18, 2012

Subsystems (4)
I2C subsystem host drivers OMAP2+ support OMAP1 support OMAP I2C driver
lore
Share & tools
lore

Kevin Hilman

author
#0 lore raw

PATCH 1/1 I2C: OMAP: xfer: fix runtime PM get/put balance on error

In omap_i2c_xfer(), ensure pm_runtime_put() is called, even on failure.

Without this, after a failed xfer, the runtime PM usecount will have been incremented, but not decremented causing the usecount to never reach zero after a failure. This keeps the device always runtime PM enabled which keeps the enclosing power domain active, and prevents full-chip retention/off from happening during idle.

Cc Shubhrajyoti D <[email protected]>
Signed-off-by Kevin Hilman <[email protected]>

---
This patch applies to current i2c-embedded/for-next branch

 drivers/i2c/busses/i2c-omap.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/i2c/busses/i2c-omap.c +1 -1
index 9895fa7..b105733 100644
@@ -582,7 +582,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 
 	r = pm_runtime_get_sync(dev->dev);
 	if (IS_ERR_VALUE(r))
-		return r;
+		goto out;
 
 	r = omap_i2c_wait_for_bb(dev);
 	if (r < 0)
Signature · 1 line
1.7.9.2
#1 lore raw

PATCH 1/1 I2C: OMAP: xfer: fix runtime PM get/put balance on error

Hi Kevin,
Thanks for the patch ,
a doubt below

Quoted 2 lines · Kevin Hilman

In omap_i2c_xfer(), ensure pm_runtime_put() is called, even on failure.

So the failure means that the usecount is incremented. However the device was not
enabled. In that case could we consider

void pm_runtime_put_noidle(struct device *dev);
- decrement the device's usage counter

Which will only decrement the counter and does not try to disable it.

However I am not sure what happens if you try to disable an already disabled device.

Quoted 3 lines

Without this, after a failed xfer, the runtime PM usecount will have been incremented, but not decremented

Agree.

Quoted 33 lines

causing the usecount to never
reach zero after a failure. ?This keeps the device always runtime PM enabled which keeps the enclosing power domain active, and prevents full-chip retention/off from happening during idle.

Cc Shubhrajyoti D <[email protected]>
Signed-off-by Kevin Hilman <[email protected]>

---
This patch applies to current i2c-embedded/for-next branch

?drivers/i2c/busses/i2c-omap.c | ? ?2 +-
?1 file changed, 1 insertion(+), 1 deletion(-)

drivers/i2c/busses/i2c-omap.c +1 -2
index 9895fa7..b105733 100644
@@ -582,7 +582,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 
 ? ? ? ?r = pm_runtime_get_sync(dev->dev);
 ? ? ? ?if (IS_ERR_VALUE(r))
- ? ? ? ? ? ? ? return r;
+ ? ? ? ? ? ? ? goto out;
 
 ? ? ? ?r = omap_i2c_wait_for_bb(dev);
 ? ? ? ?if (r < 0)
--

1.7.9.2

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo at vger.kernel.org
More majordomo info at ?http://vger.kernel.org/majordomo-info.html

author
#2 lore raw

PATCH 1/1 I2C: OMAP: xfer: fix runtime PM get/put balance on error

Hi Shubhrajyoti,

Quoted 3 lines · Shubhrajyoti Datta

Hi Kevin,
Thanks for the patch ,
a doubt below

Thanks for the review.

Quoted 6 lines · Kevin Hilman
Quoted 2 lines

In omap_i2c_xfer(), ensure pm_runtime_put() is called, even on failure.

So the failure means that the usecount is incremented. However the device was not
enabled.

Correct.

Quoted 6 lines

In that case could we consider

void pm_runtime_put_noidle(struct device *dev);
- decrement the device's usage counter

Which will only decrement the counter and does not try to disable it.

No, that is not needed.

Quoted 2 lines

However I am not sure what happens if you try to disable an already disabled device.

The runtime PM core already knows that the device is not enabled so will not need to call the disable hooks. It already needs this functionality to support the _get_noresume() and _put_noidle() calls.

I tested this on 3730/OveroSTORM where I was seeing this i2c xfer failure (and thus a failure in MMC suspend, which uses I2C to control the regulator.)

Hope that helps clarify,

If so, Wolfram, can you queue this up in your i2c-embedded/for-next branch since this problem was introduced there.

Thanks,

Kevin

Quoted 43 lines
Quoted 3 lines

Without this, after a failed xfer, the runtime PM usecount will have been incremented, but not decremented

Agree.

Quoted 29 lines

causing the usecount to never
reach zero after a failure. ?This keeps the device always runtime PM enabled which keeps the enclosing power domain active, and prevents full-chip retention/off from happening during idle.

Cc Shubhrajyoti D <[email protected]>
Signed-off-by Kevin Hilman <[email protected]>

---
This patch applies to current i2c-embedded/for-next branch

?drivers/i2c/busses/i2c-omap.c | ? ?2 +-
?1 file changed, 1 insertion(+), 1 deletion(-)

drivers/i2c/busses/i2c-omap.c +1 -2
index 9895fa7..b105733 100644
@@ -582,7 +582,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 
 ? ? ? ?r = pm_runtime_get_sync(dev->dev);
 ? ? ? ?if (IS_ERR_VALUE(r))
- ? ? ? ? ? ? ? return r;
+ ? ? ? ? ? ? ? goto out;
 
 ? ? ? ?r = omap_i2c_wait_for_bb(dev);
 ? ? ? ?if (r < 0)
--

1.7.9.2

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo at vger.kernel.org
More majordomo info at ?http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

#3 lore raw

PATCH 1/1 I2C: OMAP: xfer: fix runtime PM get/put balance on error

Quoted 31 lines · Kevin Hilman

Hi Shubhrajyoti,

Quoted 3 lines · Shubhrajyoti Datta

Hi Kevin,
Thanks for the patch ,
a doubt below

Thanks for the review.

Quoted 6 lines · Kevin Hilman
Quoted 2 lines

In omap_i2c_xfer(), ensure pm_runtime_put() is called, even on failure.

So the failure means that the usecount is incremented. However the device was not
enabled.

Correct.

Quoted 6 lines

In that case could we consider

void pm_runtime_put_noidle(struct device *dev);
- decrement the device's usage counter

Which will only decrement the counter and does not try to disable it.

No, that is not needed.

Quoted 2 lines

However I am not sure what happens if you try to disable an already disabled device.

The runtime PM core already knows that the device is not enabled so will not need to call the disable hooks. It already needs this functionality to support the _get_noresume() and _put_noidle() calls.

Quoted 5 lines

I tested this on 3730/OveroSTORM where I was seeing this i2c xfer failure (and thus a failure in MMC suspend, which uses I2C to control the regulator.)

Hope that helps clarify,

Thanks a lot for the clarification.

Quoted 15 lines

If so, Wolfram, can you queue this up in your i2c-embedded/for-next branch since this problem was introduced there.

Thanks,

Kevin

Quoted 8 lines
Quoted 2 lines

Without this, after a failed xfer, the runtime PM usecount will have been incremented, but not decremented

Agree.

Quoted 4 lines

causing the usecount to never
reach zero after a failure. This keeps the device always runtime PM enabled which keeps the enclosing power domain active, and prevents full-chip retention/off from happening during idle.

Reviewed-by Shubhrajyoti D <[email protected]>
Quoted 32 lines
Quoted 32 lines
Quoted 24 lines
Cc Shubhrajyoti D <[email protected]>
Signed-off-by Kevin Hilman <[email protected]>

---
This patch applies to current i2c-embedded/for-next branch

 drivers/i2c/busses/i2c-omap.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/i2c/busses/i2c-omap.c +1 -2
index 9895fa7..b105733 100644
@@ -582,7 +582,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 
       r = pm_runtime_get_sync(dev->dev);
       if (IS_ERR_VALUE(r))
-               return r;
+               goto out;
 
       r = omap_i2c_wait_for_bb(dev);
       if (r < 0)
--

1.7.9.2

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#4 lore raw

PATCH 1/1 I2C: OMAP: xfer: fix runtime PM get/put balance on error

In reply to Kevin Hilman

Hi Wolfram,

Quoted 9 lines · Kevin Hilman

In omap_i2c_xfer(), ensure pm_runtime_put() is called, even on failure.

Without this, after a failed xfer, the runtime PM usecount will have been incremented, but not decremented causing the usecount to never reach zero after a failure. This keeps the device always runtime PM enabled which keeps the enclosing power domain active, and prevents full-chip retention/off from happening during idle.

Quoted 4 lines
Cc Shubhrajyoti D <[email protected]>
Signed-off-by Kevin Hilman <[email protected]>

---
This patch applies to current i2c-embedded/for-next branch

This one is needed for v3.6. Can you queue this up as a fix for v3.6-rc?

Thanks,

Kevin

Quoted 16 lines
 drivers/i2c/busses/i2c-omap.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/i2c/busses/i2c-omap.c +1 -1
index 9895fa7..b105733 100644
@@ -582,7 +582,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 
 	r = pm_runtime_get_sync(dev->dev);
 	if (IS_ERR_VALUE(r))
-		return r;
+		goto out;
 
 	r = omap_i2c_wait_for_bb(dev);
 	if (r < 0)
#5 lore raw

PATCH 1/1 I2C: OMAP: xfer: fix runtime PM get/put balance on error

Quoted 13 lines · Kevin Hilman

Hi Wolfram,

Quoted 9 lines · Kevin Hilman

In omap_i2c_xfer(), ensure pm_runtime_put() is called, even on failure.

Without this, after a failed xfer, the runtime PM usecount will have been incremented, but not decremented causing the usecount to never reach zero after a failure. This keeps the device always runtime PM enabled which keeps the enclosing power domain active, and prevents full-chip retention/off from happening during idle.

Quoted 7 lines
Quoted 4 lines
Cc Shubhrajyoti D <[email protected]>
Signed-off-by Kevin Hilman <[email protected]>

---
This patch applies to current i2c-embedded/for-next branch

This one is needed for v3.6. Can you queue this up as a fix for v3.6-rc?

Done, applied to -current, thanks!

Signature · 9 lines
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120818/5491bc9d/attachment.sig>