i2c: samsung: resume race fix

PATCH New Naveen Krishna Chatradhi 5 participants 8 messages Nov 7, 2012 – Jan 24, 2013

Subsystems (2)

Arm Samsung Exynos I2C subsystem host drivers

lore

Share & tools

lore

Naveen Krishna Chatradhi

author

2012-11-07 10:28 UTC #0 lore raw

PATCH 1/1 i2c: samsung: resume race fix

Don't unmark the device as suspended until after it's been re-setup.

The main race would be w.r.t. an i2c driver that gets resumed at the same time (asyncronously), that is allowed to do a transfer since suspended is set to 0 before reinit, but really should have seen the -EIO return instead.

Signed-off-by Olof Johansson <[email protected]>

Signed-off-by Naveen Krishna Chatradhi <[email protected]>

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

drivers/i2c/busses/i2c-s3c2410.c +1 -1

index 3e0335f..dbaf920 100644
@@ -1134,10 +1134,10 @@ static int s3c24xx_i2c_resume(struct device *dev)
 	struct platform_device *pdev = to_platform_device(dev);
 	struct s3c24xx_i2c *i2c = platform_get_drvdata(pdev);
 
-	i2c->suspended = 0;
 	clk_prepare_enable(i2c->clk);
 	s3c24xx_i2c_init(i2c);
 	clk_disable_unprepare(i2c->clk);
+	i2c->suspended = 0;
 
 	return 0;
 }

Signature · 1 line

1.7.9.5

Jean Delvare

2012-11-07 10:44 UTC #1 lore raw

PATCH 1/1 i2c: samsung: resume race fix

Quoted 6 lines · Naveen Krishna Chatradhi

Don't unmark the device as suspended until after it's been re-setup.
The main race would be w.r.t. an i2c driver that gets resumed at the same time (asyncronously), that is allowed to do a transfer since suspended is set to 0 before reinit, but really should have seen the -EIO return instead.

I thought that the suspend order was children first and the resume order was parent first?

If this can really happen then I am afraid this is an issue for more than just i2c-s3c2410. The proposed solution is also not really satisfactory, as the i2c client will certainly still fail to resume properly (the only improvement is that now the failure is no longer silent.)

Quoted 13 lines

Signed-off-by Olof Johansson <[email protected]>
Signed-off-by Naveen Krishna Chatradhi <[email protected]>
---
 drivers/i2c/busses/i2c-s3c2410.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/i2c/busses/i2c-s3c2410.c +0 -0
index 3e0335f..dbaf920 100644
@@ -1134,10 +1134,10 @@ static int s3c24xx_i2c_resume(struct device *dev)
 	struct platform_device *pdev = to_platform_device(dev);

drivers/i2c/busses/i2c-s3c2410.c static int s3c24xx_i2c_resume(struct device *dev) · line 1143

Quoted 10 lines

drivers/i2c/busses/i2c-s3c2410.c +1 -1

 	struct s3c24xx_i2c *i2c = platform_get_drvdata(pdev);
 
-	i2c->suspended = 0;
 	clk_prepare_enable(i2c->clk);
 	s3c24xx_i2c_init(i2c);
 	clk_disable_unprepare(i2c->clk);
+	i2c->suspended = 0;
 
 	return 0;
 }

Acked-by Jean Delvare <[email protected]>

(Not perfect but still better than before.)

Signature · 1 line

Jean Delvare

Naveen Krishna Ch

2013-01-07 12:05 UTC #2 lore raw

PATCH 1/1 i2c: samsung: resume race fix

Quoted 38 lines · Jean Delvare

Quoted 6 lines · Naveen Krishna Chatradhi
Don't unmark the device as suspended until after it's been re-setup.
The main race would be w.r.t. an i2c driver that gets resumed at the same time (asyncronously), that is allowed to do a transfer since suspended is set to 0 before reinit, but really should have seen the -EIO return instead.
I thought that the suspend order was children first and the resume order was parent first?
If this can really happen then I am afraid this is an issue for more than just i2c-s3c2410. The proposed solution is also not really satisfactory, as the i2c client will certainly still fail to resume properly (the only improvement is that now the failure is no longer silent.)
Quoted 21 lines
Signed-off-by Olof Johansson <[email protected]>
Signed-off-by Naveen Krishna Chatradhi <[email protected]>
---
 drivers/i2c/busses/i2c-s3c2410.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/i2c/busses/i2c-s3c2410.c +1 -1
index 3e0335f..dbaf920 100644
@@ -1134,10 +1134,10 @@ static int s3c24xx_i2c_resume(struct device *dev)
      struct platform_device *pdev = to_platform_device(dev);
      struct s3c24xx_i2c *i2c = platform_get_drvdata(pdev);
 
-     i2c->suspended = 0;
      clk_prepare_enable(i2c->clk);
      s3c24xx_i2c_init(i2c);
      clk_disable_unprepare(i2c->clk);
+     i2c->suspended = 0;
 

Quoted 4 lines

drivers/i2c/busses/i2c-s3c2410.c static int s3c24xx_i2c_resume(struct device *dev) · line 1143
Quoted 2 lines
drivers/i2c/busses/i2c-s3c2410.c +0 -0
      return 0;
 }
Acked-by Jean Delvare <[email protected]>

I don't see this patch landed any where in linux-i2c tree, Though it was acked. Was it missed or should i be doing something for this to be merged ??

Quoted 5 lines

(Not perfect but still better than before.)
--
Jean Delvare

Signature · 2 lines

Shine bright,
(: Nav :)

Jean Delvare

2013-01-07 12:25 UTC #3 lore raw

PATCH 1/1 i2c: samsung: resume race fix

Quoted 29 lines · Naveen Krishna Ch

Quoted 28 lines · Jean Delvare
Quoted 27 lines · Naveen Krishna Chatradhi
Don't unmark the device as suspended until after it's been re-setup.
The main race would be w.r.t. an i2c driver that gets resumed at the same time (asyncronously), that is allowed to do a transfer since suspended is set to 0 before reinit, but really should have seen the -EIO return instead.
Signed-off-by Olof Johansson <[email protected]>
Signed-off-by Naveen Krishna Chatradhi <[email protected]>
---
 drivers/i2c/busses/i2c-s3c2410.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/i2c/busses/i2c-s3c2410.c +1 -1
index 3e0335f..dbaf920 100644
@@ -1134,10 +1134,10 @@ static int s3c24xx_i2c_resume(struct device *dev)
      struct platform_device *pdev = to_platform_device(dev);
      struct s3c24xx_i2c *i2c = platform_get_drvdata(pdev);
 
-     i2c->suspended = 0;
      clk_prepare_enable(i2c->clk);
      s3c24xx_i2c_init(i2c);
      clk_disable_unprepare(i2c->clk);
+     i2c->suspended = 0;
 

Quoted 6 lines

Quoted 4 lines
drivers/i2c/busses/i2c-s3c2410.c static int s3c24xx_i2c_resume(struct device *dev) · line 1143
Quoted 2 lines
drivers/i2c/busses/i2c-s3c2410.c +0 -0
      return 0;
 }
Acked-by Jean Delvare <[email protected]>
I don't see this patch landed any where in linux-i2c tree, Though it was acked. Was it missed or should i be doing something for this to be merged ??

Nothing needed from your side AFAIK, Wolfram should pick patches when I ack them, maybe this one was simply overlooked.

Signature · 1 line

Jean Delvare

Wolfram Sang

2013-01-24 13:27 UTC #4 lore raw

PATCH 1/1 i2c: samsung: resume race fix

In reply to Jean Delvare

Quoted 10 lines · Jean Delvare

Quoted 6 lines · Naveen Krishna Chatradhi
Don't unmark the device as suspended until after it's been re-setup.
The main race would be w.r.t. an i2c driver that gets resumed at the same time (asyncronously), that is allowed to do a transfer since suspended is set to 0 before reinit, but really should have seen the -EIO return instead.
I thought that the suspend order was children first and the resume order was parent first?

Same here, why does it not work this way?

Regards,

Wolfram

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/20130124/818565c5/attachment-0001.sig>

Olof Johansson

2013-01-24 16:41 UTC #5 lore raw

PATCH 1/1 i2c: samsung: resume race fix

2013/1/24 Wolfram Sang <[email protected]>

Quoted 6 lines · Jean Delvare

Quoted 4 lines · Naveen Krishna Chatradhi
Quoted 3 lines
Don't unmark the device as suspended until after it's been re-setup.
The main race would be w.r.t. an i2c driver that gets resumed at the
same

Quoted 9 lines

Quoted 6 lines
Quoted 3 lines
time (asyncronously), that is allowed to do a transfer since suspended is set to 0 before reinit, but really should have seen the -EIO return instead.
I thought that the suspend order was children first and the resume order was parent first?
Same here, why does it not work this way?

Sorry for being quiet on this so far, I didn't notice the controversy until now.

This is actually about half of what the original fix was (which I wrote, thus my signed-off-by). The original one was related to us having the GPIO handshake for a shared bus (see separate discussions on how that should be upstreamed, and the work on that). For reference, that patch is at:
https://gerrit.chromium.org/gerrit/#/c/28126/1/drivers/i2c/busses/i2c-s3c2410.c
.

So, I'm not sure that this patch is really needed. The significant part of the original one was the move of our internal s3c24xx_i2c_dt_gpio_free() below setting suspended = 1. Upstream implementation of the same functionality will be implemented differently, most likely.

-Olof
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130124/9785789b/attachment-0001.html>

Wolfram Sang

2013-01-24 16:44 UTC #6 lore raw

PATCH 1/1 i2c: samsung: resume race fix

Quoted 3 lines

below setting suspended = 1. Upstream implementation of the same functionality will be implemented differently, most likely.

Thanks, so I'll discard this patch.

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/20130124/8dca191b/attachment.sig>

Olof Johansson

2013-01-24 16:42 UTC #7 lore raw

PATCH 1/1 i2c: samsung: resume race fix

In reply to Wolfram Sang

[Silly gmail defaulting to html the first time around, sorry for the re-send to those not on lists]

2013/1/24 Wolfram Sang <[email protected]>:

Quoted 9 lines · Jean Delvare

Quoted 8 lines · Naveen Krishna Chatradhi
Quoted 6 lines
Don't unmark the device as suspended until after it's been re-setup.
The main race would be w.r.t. an i2c driver that gets resumed at the same time (asyncronously), that is allowed to do a transfer since suspended is set to 0 before reinit, but really should have seen the -EIO return instead.

Quoted 4 lines

Quoted 2 lines
I thought that the suspend order was children first and the resume order was parent first?
Same here, why does it not work this way?

Sorry for being quiet on this so far, I didn't notice the controversy until now.

-Olof