[PATCH 1/2] i2c: designware: fix race between subsequent xfers

[PATCH 1/2] i2c: designware: fix race between subsequent xfers

[Christian Ruppert]

The designware block is not always properly disabled in the case of
transfer errors. Interrupts from aborted transfers might be handled
after the data structures for the following transfer are initialised but
before the hardware is set up. This might corrupt the data structures to
the point that the system is stuck in an infinite interrupt loop (where
FIFOs are never emptied).
This patch cleanly disables the designware-i2c hardware at the end of
every transfer, successful or not.

Signed-off-by: Christian Ruppert <christian.ruppert-ux6zf3SgZrrQT0dZR+AlfA@public.gmane.org>
---
 drivers/i2c/busses/i2c-designware-core.c |   14 +++++++++++---
 1 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 6c0e776..65c0c7a 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -588,10 +588,20 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	ret = wait_for_completion_interruptible_timeout(&dev->cmd_complete, HZ);
 	if (ret == 0) {
 		dev_err(dev->dev, "controller timed out\n");
+		/* i2c_dw_init implicitly disables the adapter */
 		i2c_dw_init(dev);
 		ret = -ETIMEDOUT;
 		goto done;
-	} else if (ret < 0)
+	}
+
+	/*
+	 * We must disable the adapter before unlocking the &dev->lock mutex
+	 * below. Otherwise the hardware might continue generating interrupts
+	 * which in turn causes a race condition with the following transfer.
+	 */
+	__i2c_dw_enable(dev, false);
+
+	if (ret < 0)
 		goto done;
 
 	if (dev->msg_err) {
@@ -601,8 +611,6 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 
 	/* no error */
 	if (likely(!dev->cmd_err)) {
-		/* Disable the adapter */
-		__i2c_dw_enable(dev, false);
 		ret = num;
 		goto done;
 	}
-- 
1.7.1

[PATCH 2/2] i2c: designware: make i2c xfers non-interruptible

[Christian Ruppert]

When the process at the source of an i2c transfer is killed in the
middle of the transfer, the transfer is interrupted. Interrupted
transfers might cause buggy slaves on the bus (or higher level drivers)
to go haywire.
This patch forces ongoing i2c transfers to finish properly, even if the
initiating process is killed.

Signed-off-by: Christian Ruppert <christian.ruppert-ux6zf3SgZrrQT0dZR+AlfA@public.gmane.org>
---
 drivers/i2c/busses/i2c-designware-core.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 65c0c7a..d903368 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -585,7 +585,7 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	i2c_dw_xfer_init(dev);
 
 	/* wait for tx to complete */
-	ret = wait_for_completion_interruptible_timeout(&dev->cmd_complete, HZ);
+	ret = wait_for_completion_timeout(&dev->cmd_complete, HZ);
 	if (ret == 0) {
 		dev_err(dev->dev, "controller timed out\n");
 		/* i2c_dw_init implicitly disables the adapter */
-- 
1.7.1

Re: [PATCH 2/2] i2c: designware: make i2c xfers non-interruptible

[Mika Westerberg]

On Thu, Jun 06, 2013 at 03:43:36PM +0200, Christian Ruppert wrote:
> When the process at the source of an i2c transfer is killed in the
> middle of the transfer, the transfer is interrupted. Interrupted
> transfers might cause buggy slaves on the bus (or higher level drivers)
> to go haywire.
> This patch forces ongoing i2c transfers to finish properly, even if the
> initiating process is killed.

I already sent a similar patch ;-)

https://patchwork.kernel.org/patch/2601241/

However, it is not yet picked by Wolfram.

Re: [PATCH 2/2] i2c: designware: make i2c xfers non-interruptible

[Christian Ruppert]

On Fri, Jun 07, 2013 at 08:25:55AM +0300, Mika Westerberg wrote:
> On Thu, Jun 06, 2013 at 03:43:36PM +0200, Christian Ruppert wrote:
> > When the process at the source of an i2c transfer is killed in the
> > middle of the transfer, the transfer is interrupted. Interrupted
> > transfers might cause buggy slaves on the bus (or higher level drivers)
> > to go haywire.
> > This patch forces ongoing i2c transfers to finish properly, even if the
> > initiating process is killed.
> 
> I already sent a similar patch ;-)
> 
> https://patchwork.kernel.org/patch/2601241/
> 
> However, it is not yet picked by Wolfram.

Oops, this looks like a human race condition :) I checked main line and
Wolfram's git but didn't think of checking patchwork. I'll rebase the
second patch on yours.

-- 
  Christian Ruppert              ,          <christian.ruppert@abilis.com>
                                /|
  Tel: +41/(0)22 816 19-42     //|                 3, Chemin du Pré-Fleuri
                             _// | bilis Systems   CH-1228 Plan-les-Ouates

Re: [PATCH 1/2] i2c: designware: fix race between subsequent xfers

[Mika Westerberg]

Hi Christian,

On Thu, Jun 06, 2013 at 03:43:35PM +0200, Christian Ruppert wrote:
> The designware block is not always properly disabled in the case of
> transfer errors. Interrupts from aborted transfers might be handled
> after the data structures for the following transfer are initialised but
> before the hardware is set up. This might corrupt the data structures to
> the point that the system is stuck in an infinite interrupt loop (where
> FIFOs are never emptied).
> This patch cleanly disables the designware-i2c hardware at the end of
> every transfer, successful or not.

Have you tried with the latest mainline driver? There is a commit that
solves similar problem:

2a2d95e9d6d29e7	i2c: designware: always clear interrupts before enabling them

Maybe it helps?

Re: [PATCH 1/2] i2c: designware: fix race between subsequent xfers

[Christian Ruppert]

On Fri, Jun 07, 2013 at 08:23:53AM +0300, Mika Westerberg wrote:
> Hi Christian,
> 
> On Thu, Jun 06, 2013 at 03:43:35PM +0200, Christian Ruppert wrote:
> > The designware block is not always properly disabled in the case of
> > transfer errors. Interrupts from aborted transfers might be handled
> > after the data structures for the following transfer are initialised but
> > before the hardware is set up. This might corrupt the data structures to
> > the point that the system is stuck in an infinite interrupt loop (where
> > FIFOs are never emptied).
> > This patch cleanly disables the designware-i2c hardware at the end of
> > every transfer, successful or not.
> 
> Have you tried with the latest mainline driver? There is a commit that
> solves similar problem:
> 
> 2a2d95e9d6d29e7	i2c: designware: always clear interrupts before enabling them
> 
> Maybe it helps?

Hi Mika,

Thanks for the hint but I have checked both main line and Wolfram's
branch and I saw this patch. I actually hoped it would fix our problem
but it didn't.

Here some more details: We experienced system lockups (complete lock up,
no reaction whatsoever) in long-term tests under heavy system load with
lots of scheduling and forking/killing. These lockups could be traced to
the I2C driver which after some time ended up in an incoherent state:
i2c_dw_isr was being called with DW_IC_INTR_RX_FULL but
dev->msg_read_idx == dev->msgs_num. This resulted in the FIFO never
being emptied by i2c_dw_read. Since the DW_IC_INTR_RX_FULL interrupt is
cleared by emptying the FIFO, this situation results in an IRQ loop
locking up the system.

We found that the situation systematically occurs just after the
originating process is interrupted (premature return of
wait_for_completion_interruptible_timeout) and further analysis showed
the race condition: Interrupts from the previous transfer are sometimes
triggered after the initialisation of dev in the beginning of
i2c_dw_xfer, thus corrupting the state. If these interrupts occur before
dev is initialised everything works fine.

An alternative solution would probably be to make sure the hardware is
disabled before initialising the dev structure in i2c_dw_xfer.

Greetings,
  Christian

-- 
  Christian Ruppert              ,          <christian.ruppert@abilis.com>
                                /|
  Tel: +41/(0)22 816 19-42     //|                 3, Chemin du Pré-Fleuri
                             _// | bilis Systems   CH-1228 Plan-les-Ouates

[PATCH V2] i2c: designware: fix race between subsequent xfers

[Christian Ruppert]

The designware block is not always properly disabled in the case of
transfer errors. Interrupts from aborted transfers might be handled
after the data structures for the following transfer are initialised but
before the hardware is set up. This can corrupt the data structures to
the point that the system is stuck in an infinite interrupt loop (where
FIFOs are never emptied because dev->msg_read_idx == dev->msgs_num).

This patch cleanly disables the designware-i2c hardware at the end of
every transfer, be it successful or not.

This patch requires https://patchwork.kernel.org/patch/2601241/ to be
applied first.

Signed-off-by: Christian Ruppert <christian.ruppert@abilis.com>
---
 drivers/i2c/busses/i2c-designware-core.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index b75d292..55a9991 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -588,11 +588,19 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	ret = wait_for_completion_timeout(&dev->cmd_complete, HZ);
 	if (ret == 0) {
 		dev_err(dev->dev, "controller timed out\n");
+		/* i2c_dw_init implicitly disables the adapter */
 		i2c_dw_init(dev);
 		ret = -ETIMEDOUT;
 		goto done;
 	}
 
+	/*
+	 * We must disable the adapter before unlocking the &dev->lock mutex
+	 * below. Otherwise the hardware might continue generating interrupts
+	 * which in turn causes a race condition with the following transfer.
+	 */
+	__i2c_dw_enable(dev, false);
+
 	if (dev->msg_err) {
 		ret = dev->msg_err;
 		goto done;
@@ -600,8 +608,6 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 
 	/* no error */
 	if (likely(!dev->cmd_err)) {
-		/* Disable the adapter */
-		__i2c_dw_enable(dev, false);
 		ret = num;
 		goto done;
 	}
-- 
1.7.1

Re: [PATCH V2] i2c: designware: fix race between subsequent xfers

[Andy Shevchenko]

On Fri, Jun 7, 2013 at 11:51 AM, Christian Ruppert
<christian.ruppert-ux6zf3SgZrrQT0dZR+AlfA@public.gmane.org> wrote:
> The designware block is not always properly disabled in the case of
> transfer errors. Interrupts from aborted transfers might be handled
> after the data structures for the following transfer are initialised but
> before the hardware is set up. This can corrupt the data structures to
> the point that the system is stuck in an infinite interrupt loop (where
> FIFOs are never emptied because dev->msg_read_idx == dev->msgs_num).
>
> This patch cleanly disables the designware-i2c hardware at the end of
> every transfer, be it successful or not.
>
> This patch requires https://patchwork.kernel.org/patch/2601241/ to be
> applied first.

What if you just move disabling code from i2c_dw_xfer_init() to the
top of i2c_dw_xfer() ?
Will it help?

--
With Best Regards,
Andy Shevchenko

[RFC PATCH] i2c-designware-core: disable adapter before fill dev structure

[Andy Shevchenko]

Signed-off-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/i2c/busses/i2c-designware-core.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index c41ca63..a1d3d95 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -366,9 +366,6 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
 	struct i2c_msg *msgs = dev->msgs;
 	u32 ic_con;
 
-	/* Disable the adapter */
-	__i2c_dw_enable(dev, false);
-
 	/* set the slave (target) address */
 	dw_writel(dev, msgs[dev->msg_write_idx].addr, DW_IC_TAR);
 
@@ -561,6 +558,13 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	mutex_lock(&dev->lock);
 	pm_runtime_get_sync(dev->dev);
 
+	ret = i2c_dw_wait_bus_not_busy(dev);
+	if (ret < 0)
+		goto done;
+
+	/* Disable the adapter */
+	__i2c_dw_enable(dev, false);
+
 	INIT_COMPLETION(dev->cmd_complete);
 	dev->msgs = msgs;
 	dev->msgs_num = num;
@@ -572,10 +576,6 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	dev->abort_source = 0;
 	dev->rx_outstanding = 0;
 
-	ret = i2c_dw_wait_bus_not_busy(dev);
-	if (ret < 0)
-		goto done;
-
 	/* start the transfers */
 	i2c_dw_xfer_init(dev);
 
-- 
1.8.2.rc0.22.gb3600c3

Re: [RFC PATCH] i2c-designware-core: disable adapter before fill dev structure

[Christian Ruppert]

Hi Andy,

I like your patch and I just did some testing on it. Unluckily, it
doesn't solve the lock-up problem.

I haven't investigated any further but I suspect that on top of the
cases I observed when debugging this (interrupts after initialisation of
dev, easy to prove) there are more obscure cases in which interrupts are
generated in an unexpected manner after errors. The interrupt-driven
transfer state machine of the driver implicitly relies on the fact that
all updates of dev which are related to the same transfer are performed
between the mutex_lock and mutex_unlock calls in i2c_dw_xfer. Thus, I
decided it was safer to disable the block before releasing the mutex
when I wrote my patch.

That said, I think the sequencing at transfer initialisation is more
logical with your patch and I wonder if it is still worth applying. Any
other opinions on this?

Greetings,
  Christian

On Fri, Jun 07, 2013 at 12:30:01PM +0300, Andy Shevchenko wrote:
> Signed-off-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-designware-core.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
> index c41ca63..a1d3d95 100644
> --- a/drivers/i2c/busses/i2c-designware-core.c
> +++ b/drivers/i2c/busses/i2c-designware-core.c
> @@ -366,9 +366,6 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
>  	struct i2c_msg *msgs = dev->msgs;
>  	u32 ic_con;
>  
> -	/* Disable the adapter */
> -	__i2c_dw_enable(dev, false);
> -
>  	/* set the slave (target) address */
>  	dw_writel(dev, msgs[dev->msg_write_idx].addr, DW_IC_TAR);
>  
> @@ -561,6 +558,13 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>  	mutex_lock(&dev->lock);
>  	pm_runtime_get_sync(dev->dev);
>  
> +	ret = i2c_dw_wait_bus_not_busy(dev);
> +	if (ret < 0)
> +		goto done;
> +
> +	/* Disable the adapter */
> +	__i2c_dw_enable(dev, false);
> +
>  	INIT_COMPLETION(dev->cmd_complete);
>  	dev->msgs = msgs;
>  	dev->msgs_num = num;
> @@ -572,10 +576,6 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>  	dev->abort_source = 0;
>  	dev->rx_outstanding = 0;
>  
> -	ret = i2c_dw_wait_bus_not_busy(dev);
> -	if (ret < 0)
> -		goto done;
> -
>  	/* start the transfers */
>  	i2c_dw_xfer_init(dev);
>  
> -- 
> 1.8.2.rc0.22.gb3600c3
> 

-- 
  Christian Ruppert              ,          <christian.ruppert@abilis.com>
                                /|
  Tel: +41/(0)22 816 19-42     //|                 3, Chemin du Pré-Fleuri
                             _// | bilis Systems   CH-1228 Plan-les-Ouates

Re: [RFC PATCH] i2c-designware-core: disable adapter before fill dev structure

[Wolfram Sang]

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

On Fri, Jun 07, 2013 at 03:01:34PM +0200, Christian Ruppert wrote:
> Hi Andy,
> 
> I like your patch and I just did some testing on it. Unluckily, it
> doesn't solve the lock-up problem.
> 
> I haven't investigated any further but I suspect that on top of the
> cases I observed when debugging this (interrupts after initialisation of
> dev, easy to prove) there are more obscure cases in which interrupts are
> generated in an unexpected manner after errors. The interrupt-driven
> transfer state machine of the driver implicitly relies on the fact that
> all updates of dev which are related to the same transfer are performed
> between the mutex_lock and mutex_unlock calls in i2c_dw_xfer. Thus, I
> decided it was safer to disable the block before releasing the mutex
> when I wrote my patch.
> 
> That said, I think the sequencing at transfer initialisation is more
> logical with your patch and I wonder if it is still worth applying. Any
> other opinions on this?

Ping. There are:

[V2] i2c: designware: fix race between subsequent xfers
[RFC] i2c-designware-core: disable adapter before fill dev structure

What is the consensus of those two patches?


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

Re: [RFC PATCH] i2c-designware-core: disable adapter before fill dev structure

[Christian Ruppert]

On Tue, Jun 11, 2013 at 08:40:37PM +0200, Wolfram Sang wrote:
> On Fri, Jun 07, 2013 at 03:01:34PM +0200, Christian Ruppert wrote:
> > Hi Andy,
> > 
> > I like your patch and I just did some testing on it. Unluckily, it
> > doesn't solve the lock-up problem.
> > 
> > I haven't investigated any further but I suspect that on top of the
> > cases I observed when debugging this (interrupts after initialisation of
> > dev, easy to prove) there are more obscure cases in which interrupts are
> > generated in an unexpected manner after errors. The interrupt-driven
> > transfer state machine of the driver implicitly relies on the fact that
> > all updates of dev which are related to the same transfer are performed
> > between the mutex_lock and mutex_unlock calls in i2c_dw_xfer. Thus, I
> > decided it was safer to disable the block before releasing the mutex
> > when I wrote my patch.
> > 
> > That said, I think the sequencing at transfer initialisation is more
> > logical with your patch and I wonder if it is still worth applying. Any
> > other opinions on this?
> 
> Ping. There are:
> 
> [V2] i2c: designware: fix race between subsequent xfers
> [RFC] i2c-designware-core: disable adapter before fill dev structure
> 
> What is the consensus of those two patches?

Although I almost like Andy's code better than my own it doesn't seem to
fix the issue we're aiming at (system lock ups due to undesired
interrupts) in all cases. The "[V2] i2c: designware: fix race between
subsequent xfers" is currently the only patch preventing those lock ups
in our tests.

That said, IMHO Andy's patch seems to be a valuable code clean up
nevertheless and could be applied in addition to the other. I suggest I
give the combination of both patches some additional testing on the
occasion and tag Andy's RFC with tested-by me in case it's stable.

Greetings,
  Christian

-- 
  Christian Ruppert              ,          <christian.ruppert@abilis.com>
                                /|
  Tel: +41/(0)22 816 19-42     //|                 3, Chemin du Pré-Fleuri
                             _// | bilis Systems   CH-1228 Plan-les-Ouates

Re: [RFC PATCH] i2c-designware-core: disable adapter before fill dev structure

[Christian Ruppert]

Hello,

As promised, I gave this one some over-night stress testing and I can
confirm what I said previously:

- The patch does _not_ solve the interrupt loop lockups on its own.
- The patch works well in conjunction with
  http://patchwork.ozlabs.org/patch/249622/ (which in turn depends on
  Mika's patch). Under this condition you can assume
  Tested-By: Christian Ruppert <christian.ruppert-ux6zf3SgZrrQT0dZR+AlfA@public.gmane.org>

I still think the code is more logical with this patch than without it
and I am in favour of applying both (if Andy agrees that is). We must
keep in mind, however, that http://patchwork.ozlabs.org/patch/249622
does fix a real problem we can observe on our chip and for our TB10x
product we do require some form of it for stability reasons.

Greetings,
  Christian

On Fri, Jun 07, 2013 at 12:30:01PM +0300, Andy Shevchenko wrote:
> Signed-off-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-designware-core.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
> index c41ca63..a1d3d95 100644
> --- a/drivers/i2c/busses/i2c-designware-core.c
> +++ b/drivers/i2c/busses/i2c-designware-core.c
> @@ -366,9 +366,6 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
>  	struct i2c_msg *msgs = dev->msgs;
>  	u32 ic_con;
>  
> -	/* Disable the adapter */
> -	__i2c_dw_enable(dev, false);
> -
>  	/* set the slave (target) address */
>  	dw_writel(dev, msgs[dev->msg_write_idx].addr, DW_IC_TAR);
>  
> @@ -561,6 +558,13 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>  	mutex_lock(&dev->lock);
>  	pm_runtime_get_sync(dev->dev);
>  
> +	ret = i2c_dw_wait_bus_not_busy(dev);
> +	if (ret < 0)
> +		goto done;
> +
> +	/* Disable the adapter */
> +	__i2c_dw_enable(dev, false);
> +
>  	INIT_COMPLETION(dev->cmd_complete);
>  	dev->msgs = msgs;
>  	dev->msgs_num = num;
> @@ -572,10 +576,6 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>  	dev->abort_source = 0;
>  	dev->rx_outstanding = 0;
>  
> -	ret = i2c_dw_wait_bus_not_busy(dev);
> -	if (ret < 0)
> -		goto done;
> -
>  	/* start the transfers */
>  	i2c_dw_xfer_init(dev);
>  
> -- 
> 1.8.2.rc0.22.gb3600c3
> 

-- 
  Christian Ruppert              ,          <christian.ruppert@abilis.com>
                                /|
  Tel: +41/(0)22 816 19-42     //|                 3, Chemin du Pré-Fleuri
                             _// | bilis Systems   CH-1228 Plan-les-Ouates

Re: [RFC PATCH] i2c-designware-core: disable adapter before fill dev structure

[Andy Shevchenko]

On Thu, 2013-06-13 at 10:16 +0200, Christian Ruppert wrote: 
> As promised, I gave this one some over-night stress testing and I can
> confirm what I said previously:
> 
> - The patch does _not_ solve the interrupt loop lockups on its own.

So, it just means my assumptions about what is happening there were
wrong.

> - The patch works well in conjunction with
>   http://patchwork.ozlabs.org/patch/249622/ (which in turn depends on
>   Mika's patch). Under this condition you can assume
>   Tested-By: Christian Ruppert <christian.ruppert-ux6zf3SgZrrQT0dZR+AlfA@public.gmane.org>
> 
> I still think the code is more logical with this patch than without it
> and I am in favour of applying both (if Andy agrees that is).

Since my patch doesn't fix anything, I think we may drop it away.

>  We must keep in mind, however, that http://patchwork.ozlabs.org/patch/249622
> does fix a real problem we can observe on our chip and for our TB10x
> product we do require some form of it for stability reasons.

I feel like a real fix is to add a memory barier to make changes in the
structure consistent. However, I have no clue where.


-- 
Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Intel Finland Oy

Re: [RFC PATCH] i2c-designware-core: disable adapter before fill dev structure

[Christian Ruppert]

On Thu, Jun 13, 2013 at 11:33:56AM +0300, Andy Shevchenko wrote:
> On Thu, 2013-06-13 at 10:16 +0200, Christian Ruppert wrote: 
> > As promised, I gave this one some over-night stress testing and I can
> > confirm what I said previously:
> > 
> > - The patch does _not_ solve the interrupt loop lockups on its own.
> 
> So, it just means my assumptions about what is happening there were
> wrong.

So were my initial ones... Or at least insufficient.

> > - The patch works well in conjunction with
> >   http://patchwork.ozlabs.org/patch/249622/ (which in turn depends on
> >   Mika's patch). Under this condition you can assume
> >   Tested-By: Christian Ruppert <christian.ruppert-ux6zf3SgZrrQT0dZR+AlfA@public.gmane.org>
> > 
> > I still think the code is more logical with this patch than without it
> > and I am in favour of applying both (if Andy agrees that is).
> 
> Since my patch doesn't fix anything, I think we may drop it away.
> 
> >  We must keep in mind, however, that http://patchwork.ozlabs.org/patch/249622
> > does fix a real problem we can observe on our chip and for our TB10x
> > product we do require some form of it for stability reasons.
> 
> I feel like a real fix is to add a memory barier to make changes in the
> structure consistent. However, I have no clue where.

I'm still not sure about the interrupt behaviour of the dw-i2c block in
the case of error (and since our problem is fixed it's difficult to
justify spending time to investigate further). I suspect that the thing
in some situations sends spurious interrupts which confuse the state
machine - in which case memory barriers won't help us either.

Greetings,
  Christian

-- 
  Christian Ruppert              ,          <christian.ruppert@abilis.com>
                                /|
  Tel: +41/(0)22 816 19-42     //|                 3, Chemin du Pré-Fleuri
                             _// | bilis Systems   CH-1228 Plan-les-Ouates

Re: [PATCH V2] i2c: designware: fix race between subsequent xfers

[Wolfram Sang]

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

On Fri, Jun 07, 2013 at 10:51:23AM +0200, Christian Ruppert wrote:
> The designware block is not always properly disabled in the case of
> transfer errors. Interrupts from aborted transfers might be handled
> after the data structures for the following transfer are initialised but
> before the hardware is set up. This can corrupt the data structures to
> the point that the system is stuck in an infinite interrupt loop (where
> FIFOs are never emptied because dev->msg_read_idx == dev->msgs_num).
> 
> This patch cleanly disables the designware-i2c hardware at the end of
> every transfer, be it successful or not.
> 
> This patch requires https://patchwork.kernel.org/patch/2601241/ to be
> applied first.

These last two lines should be below "---".

> 
> Signed-off-by: Christian Ruppert <christian.ruppert-ux6zf3SgZrrQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-designware-core.c |   10 ++++++++--
>  1 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
> index b75d292..55a9991 100644
> --- a/drivers/i2c/busses/i2c-designware-core.c
> +++ b/drivers/i2c/busses/i2c-designware-core.c
> @@ -588,11 +588,19 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>  	ret = wait_for_completion_timeout(&dev->cmd_complete, HZ);
>  	if (ret == 0) {
>  		dev_err(dev->dev, "controller timed out\n");
> +		/* i2c_dw_init implicitly disables the adapter */
>  		i2c_dw_init(dev);
>  		ret = -ETIMEDOUT;
>  		goto done;
>  	}
>  
> +	/*
> +	 * We must disable the adapter before unlocking the &dev->lock mutex
> +	 * below. Otherwise the hardware might continue generating interrupts
> +	 * which in turn causes a race condition with the following transfer.

I added "Needs some more investigation if the additional interrupts are
a hardware bug or this driver doesn't handle them correctly yet." to the
comment and

Applied to for-next, thanks!

BTW since I am currently here: i2c-designware-core should be in the
'algos' directory, no?

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

Re: [PATCH V2] i2c: designware: fix race between subsequent xfers

[Christian Ruppert]

On Fri, Jun 14, 2013 at 04:37:41PM +0200, Wolfram Sang wrote:
> On Fri, Jun 07, 2013 at 10:51:23AM +0200, Christian Ruppert wrote:
> [...]
> > +	/*
> > +	 * We must disable the adapter before unlocking the &dev->lock mutex
> > +	 * below. Otherwise the hardware might continue generating interrupts
> > +	 * which in turn causes a race condition with the following transfer.
> 
> I added "Needs some more investigation if the additional interrupts are
> a hardware bug or this driver doesn't handle them correctly yet." to the
> comment and

Good idea, thanks.

> Applied to for-next, thanks!
> 
> BTW since I am currently here: i2c-designware-core should be in the
> 'algos' directory, no?

At the risk of passing for a complete moron: What exactly is the
difference between I2C algos and I2C bus drivers?

Greetings,
  Christian

-- 
  Christian Ruppert              ,          <christian.ruppert@abilis.com>
                                /|
  Tel: +41/(0)22 816 19-42     //|                 3, Chemin du Pré-Fleuri
                             _// | bilis Systems   CH-1228 Plan-les-Ouates

Re: [PATCH V2] i2c: designware: fix race between subsequent xfers

[Jean Delvare]

On Mon, 17 Jun 2013 10:19:33 +0200, Christian Ruppert wrote:
> On Fri, Jun 14, 2013 at 04:37:41PM +0200, Wolfram Sang wrote:
> > BTW since I am currently here: i2c-designware-core should be in the
> > 'algos' directory, no?
> 
> At the risk of passing for a complete moron: What exactly is the
> difference between I2C algos and I2C bus drivers?

The i2c/algos directory contains abstracted code which is common to
multiple hardware implementations. The most popular of these is
i2c-algo-bit which implements software-only I2C over virtually any pair
of controllable pins (parallel port, GPIOs, etc.)

As a general rule, i2c/algos should only contain reusable, architecture
and platform independent code. All the actual hardware access should be
delegated to the bus drivers, through callbacks. If this can't be done
easily then i2c/algos is not the right place.

-- 
Jean Delvare

Re: [PATCH V2] i2c: designware: fix race between subsequent xfers

[Christian Ruppert]

On Mon, Jun 17, 2013 at 10:33:36AM +0200, Jean Delvare wrote:
> On Mon, 17 Jun 2013 10:19:33 +0200, Christian Ruppert wrote:
> > On Fri, Jun 14, 2013 at 04:37:41PM +0200, Wolfram Sang wrote:
> > > BTW since I am currently here: i2c-designware-core should be in the
> > > 'algos' directory, no?
> > 
> > At the risk of passing for a complete moron: What exactly is the
> > difference between I2C algos and I2C bus drivers?
> 
> The i2c/algos directory contains abstracted code which is common to
> multiple hardware implementations. The most popular of these is
> i2c-algo-bit which implements software-only I2C over virtually any pair
> of controllable pins (parallel port, GPIOs, etc.)
> 
> As a general rule, i2c/algos should only contain reusable, architecture
> and platform independent code. All the actual hardware access should be
> delegated to the bus drivers, through callbacks. If this can't be done
> easily then i2c/algos is not the right place.

In this case, busses is the right place for the i2c-designware-core. This
file contains the actual driver implementation (i.e. register access,
interrupt handling etc.) for a dedicated I2C bus driver hardware block
used in SOCs.

Greetings,
  Christian

-- 
  Christian Ruppert              ,          <christian.ruppert@abilis.com>
                                /|
  Tel: +41/(0)22 816 19-42     //|                 3, Chemin du Pré-Fleuri
                             _// | bilis Systems   CH-1228 Plan-les-Ouates

Re: [PATCH V2] i2c: designware: fix race between subsequent xfers

[Mika Westerberg]

On Fri, Jun 14, 2013 at 04:37:41PM +0200, Wolfram Sang wrote:
> On Fri, Jun 07, 2013 at 10:51:23AM +0200, Christian Ruppert wrote:
> > The designware block is not always properly disabled in the case of
> > transfer errors. Interrupts from aborted transfers might be handled
> > after the data structures for the following transfer are initialised but
> > before the hardware is set up. This can corrupt the data structures to
> > the point that the system is stuck in an infinite interrupt loop (where
> > FIFOs are never emptied because dev->msg_read_idx == dev->msgs_num).
> > 
> > This patch cleanly disables the designware-i2c hardware at the end of
> > every transfer, be it successful or not.
> > 
> > This patch requires https://patchwork.kernel.org/patch/2601241/ to be
> > applied first.
> 
> These last two lines should be below "---".
> 
> > 
> > Signed-off-by: Christian Ruppert <christian.ruppert@abilis.com>
> > ---
> >  drivers/i2c/busses/i2c-designware-core.c |   10 ++++++++--
> >  1 files changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
> > index b75d292..55a9991 100644
> > --- a/drivers/i2c/busses/i2c-designware-core.c
> > +++ b/drivers/i2c/busses/i2c-designware-core.c
> > @@ -588,11 +588,19 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> >  	ret = wait_for_completion_timeout(&dev->cmd_complete, HZ);
> >  	if (ret == 0) {
> >  		dev_err(dev->dev, "controller timed out\n");
> > +		/* i2c_dw_init implicitly disables the adapter */
> >  		i2c_dw_init(dev);
> >  		ret = -ETIMEDOUT;
> >  		goto done;
> >  	}
> >  
> > +	/*
> > +	 * We must disable the adapter before unlocking the &dev->lock mutex
> > +	 * below. Otherwise the hardware might continue generating interrupts
> > +	 * which in turn causes a race condition with the following transfer.
> 
> I added "Needs some more investigation if the additional interrupts are
> a hardware bug or this driver doesn't handle them correctly yet." to the
> comment and
> 
> Applied to for-next, thanks!

Sorry for the late response (was on vacation). This patch looks good to me
as well. Thanks for applying it!