[PATCH] i2c: samsung: resume race fix

[PATCH] i2c: samsung: resume race fix

[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 <olofj-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Signed-off-by: Naveen Krishna Chatradhi <ch.naveen-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 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;
 }
-- 
1.7.9.5

Re: [PATCH] i2c: samsung: resume race fix

[Jean Delvare]

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-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Signed-off-by: Naveen Krishna Chatradhi <ch.naveen-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
>  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-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>

(Not perfect but still better than before.)

-- 
Jean Delvare

Re: [PATCH] i2c: samsung: resume race fix

[Naveen Krishna Ch]

On 7 November 2012 16:14, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.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-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> Signed-off-by: Naveen Krishna Chatradhi <ch.naveen-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>> ---
>>  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-PUYAD+kWke1g9hUCZPvPmw@public.gmane.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



-- 
Shine bright,
(: Nav :)

Re: [PATCH] i2c: samsung: resume race fix

[Jean Delvare]

On Mon, 7 Jan 2013 17:35:25 +0530, Naveen Krishna Ch wrote:
> On 7 November 2012 16:14, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.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-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> >> Signed-off-by: Naveen Krishna Chatradhi <ch.naveen-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> >> ---
> >>  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-PUYAD+kWke1g9hUCZPvPmw@public.gmane.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.

-- 
Jean Delvare

Re: [PATCH] i2c: samsung: resume race fix

[Wolfram Sang]

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

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

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] i2c: samsung: resume race fix

[Olof Johansson]

[-- Attachment #1.1: Type: text/plain, Size: 1405 bytes --]

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

[-- Attachment #1.2: Type: text/html, Size: 1972 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] i2c: samsung: resume race fix

[Wolfram Sang]

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


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

Thanks, so I'll discard this patch.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] i2c: samsung: resume race fix

[Olof Johansson]

[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-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>:
> 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