| Message ID | 1352284106-24988-1-git-send-email-ch.naveen@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 07 Nov 2012 15:58:26 +0530, Naveen Krishna Chatradhi wrote: > 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.) > > Signed-off-by: Olof Johansson <olofj@chromium.org> > Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com> > --- > drivers/i2c/busses/i2c-s3c2410.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c > index 3e0335f..dbaf920 100644 > --- a/drivers/i2c/busses/i2c-s3c2410.c > +++ b/drivers/i2c/busses/i2c-s3c2410.c > @@ -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; > } Acked-by: Jean Delvare <khali@linux-fr.org> (Not perfect but still better than before.)
On 7 November 2012 16:14, Jean Delvare <khali@linux-fr.org> wrote: > On Wed, 07 Nov 2012 15:58:26 +0530, Naveen Krishna Chatradhi wrote: >> 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.) > >> >> Signed-off-by: Olof Johansson <olofj@chromium.org> >> Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com> >> --- >> drivers/i2c/busses/i2c-s3c2410.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c >> index 3e0335f..dbaf920 100644 >> --- a/drivers/i2c/busses/i2c-s3c2410.c >> +++ b/drivers/i2c/busses/i2c-s3c2410.c >> @@ -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; >> } > > Acked-by: Jean Delvare <khali@linux-fr.org> 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 ?? > > (Not perfect but still better than before.) > > -- > Jean Delvare
On Mon, 7 Jan 2013 17:35:25 +0530, Naveen Krishna Ch wrote: > On 7 November 2012 16:14, Jean Delvare <khali@linux-fr.org> wrote: > > On Wed, 07 Nov 2012 15:58:26 +0530, Naveen Krishna Chatradhi wrote: > >> 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 <olofj@chromium.org> > >> Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com> > >> --- > >> drivers/i2c/busses/i2c-s3c2410.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c > >> index 3e0335f..dbaf920 100644 > >> --- a/drivers/i2c/busses/i2c-s3c2410.c > >> +++ b/drivers/i2c/busses/i2c-s3c2410.c > >> @@ -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; > >> } > > > > Acked-by: Jean Delvare <khali@linux-fr.org> > 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.
On Wed, Nov 07, 2012 at 11:44:37AM +0100, Jean Delvare wrote: > On Wed, 07 Nov 2012 15:58:26 +0530, Naveen Krishna Chatradhi wrote: > > 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
2013/1/24 Wolfram Sang <w.sang@pengutronix.de> > On Wed, Nov 07, 2012 at 11:44:37AM +0100, Jean Delvare wrote: > > On Wed, 07 Nov 2012 15:58:26 +0530, Naveen Krishna Chatradhi wrote: > > > 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? > 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
[Silly gmail defaulting to html the first time around, sorry for the re-send to those not on lists] 2013/1/24 Wolfram Sang <w.sang@pengutronix.de>: > On Wed, Nov 07, 2012 at 11:44:37AM +0100, Jean Delvare wrote: >> On Wed, 07 Nov 2012 15:58:26 +0530, Naveen Krishna Chatradhi wrote: >> > 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? 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
> below setting suspended = 1. Upstream implementation of the same > functionality will be implemented differently, most likely. Thanks, so I'll discard this patch.
diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c index 3e0335f..dbaf920 100644 --- a/drivers/i2c/busses/i2c-s3c2410.c +++ b/drivers/i2c/busses/i2c-s3c2410.c @@ -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; }