i2c: designware: fix race between subsequent xfers

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.

Quoted 6 lines · 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.

I already sent a similar patch ;-)

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

However, it is not yet picked by Wolfram.

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.

Hi Christian,

Quoted 8 lines · 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.

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?

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 Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/[email protected]>

---
 drivers/i2c/busses/i2c-designware-core.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

drivers/i2c/busses/i2c-designware-core.c +7 -7

index c41ca63..a1d3d95 100644
@@ -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);
 

Signature · 1 line

1.8.2.rc0.22.gb3600c3

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?

Quoted 15 lines · 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.

Quoted 3 lines

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?

Signed-off-by Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/[email protected]>

---
 drivers/i2c/busses/i2c-designware-core.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

drivers/i2c/busses/i2c-designware-core.c +7 -7

index c41ca63..a1d3d95 100644
@@ -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);
 

Signature · 1 line

1.8.2.rc0.22.gb3600c3

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 <[email protected]>

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.

Quoted 4 lines · Christian Ruppert

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.

Quoted 7 lines

- 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 <[email protected]>

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.

Quoted 3 lines

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.

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 <[email protected]>

---
 drivers/i2c/busses/i2c-designware-core.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

drivers/i2c/busses/i2c-designware-core.c +0 -0

index b75d292..55a9991 100644
@@ -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");

On Fri, Jun 07, 2013 at 10:51:23AM +0200, Christian Ruppert wrote:
[...]

drivers/i2c/busses/i2c-designware-core.c i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) · line 600

Quoted 4 lines

drivers/i2c/busses/i2c-designware-core.c +4 -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.

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?

Quoted 2 lines · Wolfram Sang

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?

Quoted 6 lines · Christian Ruppert

Quoted 2 lines · Wolfram Sang

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.

Quoted 7 lines · 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).

Quoted 5 lines

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 "---".

Quoted 14 lines

Signed-off-by Christian Ruppert <[email protected]>

---
 drivers/i2c/busses/i2c-designware-core.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

drivers/i2c/busses/i2c-designware-core.c +0 -0

index b75d292..55a9991 100644
@@ -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");

drivers/i2c/busses/i2c-designware-core.c i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) · line 600

Quoted 10 lines

drivers/i2c/busses/i2c-designware-core.c +5 -0

+		/* 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!