i2c: davinci: Rework racy ISR

There is one big problem in the current design: ISR accesses the controller registers in parallel with i2c_davinci_xfer_msg() in process context. The whole logic is not obvious, many operations are performed in process context while ISR is always enabled and does something asynchronous even while it's not expected. We have faced these races on 4-cores Keystone chip. Some examples:

- when non-existing device is accessed first comes NAK IRQ, then ARDY IRQ. After NAK we already jump out of wait_for_completion_timeout() and depending on how lucky we are ARDY IRQ will access MDR register in the middle of some other operation in process context;

- STOP condition is triggered in many places in the driver, in ISR, in i2c_davinci_xfer_msg(), but there is no code which guarantees that STOP will be really completed. We have seen many STOP conditions simply missing in back-to-back transfers, when next i2c_davinci_xfer_msg() call simply overwrites MDR register while STOP is still not generated.

So, make the design more robust and obvious:
- leave hot path (buffers management) in ISR, remove other registers access from ISR;
- introduce second synchronization point, to make sure that STOP condition is really generated and it's safe to begin next transfer;
- simplify the state machine;
- enable IRQs only when they are expected, disable them in ISR when transfer is completed/failed;
- STOP is normally set simultaneously with START condition (in case of last message); only special case when STOP is additionally generated is received NAK -- this case is handled separately.

Signed-off-by Alexander Sverdlin <[email protected]>

---
  drivers/i2c/busses/i2c-davinci.c |  219 ++++++++++++++++---------------------
  1 files changed, 95 insertions(+), 124 deletions(-)

drivers/i2c/busses/i2c-davinci.c +95 -124

index 6dc7ff5..98759ae 100644
@@ -72,10 +72,19 @@
  #define DAVINCI_I2C_STR_BB	BIT(12)
  #define DAVINCI_I2C_STR_RSFULL	BIT(11)
  #define DAVINCI_I2C_STR_SCD	BIT(5)
+#define DAVINCI_I2C_STR_ICXRDY	BIT(4)
+#define DAVINCI_I2C_STR_ICRDRDY	BIT(3)
  #define DAVINCI_I2C_STR_ARDY	BIT(2)
  #define DAVINCI_I2C_STR_NACK	BIT(1)
  #define DAVINCI_I2C_STR_AL	BIT(0)
 
+#define DAVINCI_I2C_STR_ALL	(DAVINCI_I2C_STR_SCD | \
+				 DAVINCI_I2C_STR_ICXRDY | \
+				 DAVINCI_I2C_STR_ICRDRDY | \
+				 DAVINCI_I2C_STR_ARDY | \
+				 DAVINCI_I2C_STR_NACK | \
+				 DAVINCI_I2C_STR_AL)
+
  #define DAVINCI_I2C_MDR_NACK	BIT(15)
  #define DAVINCI_I2C_MDR_STT	BIT(13)
  #define DAVINCI_I2C_MDR_STP	BIT(11)

@@ -98,12 +107,10 @@ struct davinci_i2c_dev {
  	void __iomem		*base;
  	struct completion	cmd_complete;
  	struct clk              *clk;
-	int			cmd_err;
+	u32			cmd_stat;
  	u8			*buf;
  	size_t			buf_len;
  	int			irq;
-	int			stop;
-	u8			terminate;
  	struct i2c_adapter	adapter;
  #ifdef CONFIG_CPU_FREQ
  	struct completion	xfr_complete;

@@ -256,9 +263,6 @@ static int i2c_davinci_init(struct davinci_i2c_dev *dev)
  	/* Take the I2C module out of reset: */
  	davinci_i2c_reset_ctrl(dev, 1);
 
-	/* Enable interrupts */
-	davinci_i2c_write_reg(dev, DAVINCI_I2C_IMR_REG, I2C_DAVINCI_INTR_ALL);
-
  	return 0;
  }
 

@@ -293,6 +297,36 @@ static int i2c_davinci_wait_bus_not_busy(struct davinci_i2c_dev *dev,
  	return 0;
  }
 
+static int i2c_davinci_wait_for_completion(struct davinci_i2c_dev *dev)
+{
+	int r;
+
+	r = wait_for_completion_timeout(&dev->cmd_complete, dev->adapter.timeout);
+	if (r == 0) {
+		/* Disable IRQ, sources were NOT masked by the handler */
+		disable_irq(dev->irq);
+
+		dev_err(dev->dev, "controller timed out\n");
+		davinci_i2c_recover_bus(dev);
+		i2c_davinci_init(dev);
+
+		/* It's safe to enable IRQ after reset */
+		enable_irq(dev->irq);
+
+		return -ETIMEDOUT;
+	}
+
+	/* If it wasn't a timeout, then the IRQs are masked */
+
+	if (r < 0) {
+		dev_err(dev->dev, "abnormal termination buf_len=%i\n",
+		        dev->buf_len);
+		return r;
+	}
+
+	return 0;
+}
+
  /*
   * Low level master read/write transaction. This function is called
   * from i2c_davinci_xfer.

@@ -315,12 +349,10 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
 
  	dev->buf = msg->buf;
  	dev->buf_len = msg->len;
-	dev->stop = stop;
 
  	davinci_i2c_write_reg(dev, DAVINCI_I2C_CNT_REG, dev->buf_len);
 
  	reinit_completion(&dev->cmd_complete);
-	dev->cmd_err = 0;
 
  	/* Take I2C out of reset and configure it as master */
  	flag = DAVINCI_I2C_MDR_IRS | DAVINCI_I2C_MDR_MST;

@@ -333,16 +365,6 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
  	if (msg->len == 0)
  		flag |= DAVINCI_I2C_MDR_RM;
 
-	/* Enable receive or transmit interrupts */
-	w = davinci_i2c_read_reg(dev, DAVINCI_I2C_IMR_REG);
-	if (msg->flags & I2C_M_RD)
-		w |= DAVINCI_I2C_IMR_RRDY;
-	else
-		w |= DAVINCI_I2C_IMR_XRDY;
-	davinci_i2c_write_reg(dev, DAVINCI_I2C_IMR_REG, w);
-
-	dev->terminate = 0;
-
  	/*
  	 * Write mode register first as needed for correct behaviour
  	 * on OMAP-L138, but don't set STT yet to avoid a race with XRDY

@@ -350,6 +372,9 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
  	 */
  	davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
 
+	/* Clear IRQ flags */
+	davinci_i2c_write_reg(dev, DAVINCI_I2C_STR_REG, DAVINCI_I2C_STR_ALL);
+
  	/*
  	 * First byte should be set here, not after interrupt,
  	 * because transmit-data-ready interrupt can come before

@@ -368,49 +393,48 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
  		flag |= DAVINCI_I2C_MDR_STP;
  	davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
 
-	r = wait_for_completion_timeout(&dev->cmd_complete, dev->adapter.timeout);
-	if (r == 0) {
-		dev_err(dev->dev, "controller timed out\n");
-		davinci_i2c_recover_bus(dev);
-		i2c_davinci_init(dev);
-		dev->buf_len = 0;
-		return -ETIMEDOUT;
-	}
-	if (dev->buf_len) {
-		/* This should be 0 if all bytes were transferred
-		 * or dev->cmd_err denotes an error.
-		 */
-		if (r >= 0) {
-			dev_err(dev->dev, "abnormal termination buf_len=%i\n",
-				dev->buf_len);
-			r = -EREMOTEIO;
-		}
-		dev->terminate = 1;
-		wmb();
-		dev->buf_len = 0;
-	}
-	if (r < 0)
+	/* Enable status interrupts */
+	w = I2C_DAVINCI_INTR_ALL;
+	/* Enable receive or transmit interrupts */
+	if (msg->flags & I2C_M_RD)
+		w |= DAVINCI_I2C_IMR_RRDY;
+	else
+		w |= DAVINCI_I2C_IMR_XRDY;
+	davinci_i2c_write_reg(dev, DAVINCI_I2C_IMR_REG, w);
+
+	r = i2c_davinci_wait_for_completion(dev);
+	if (r)
  		return r;
 
-	/* no error */
-	if (likely(!dev->cmd_err))
+	switch (dev->cmd_stat) {
+	case DAVINCI_I2C_IVR_SCD:
+	case DAVINCI_I2C_IVR_ARDY:
  		return msg->len;
 
-	/* We have an error */
-	if (dev->cmd_err & DAVINCI_I2C_STR_AL) {
+	case DAVINCI_I2C_IVR_AL:
  		i2c_davinci_init(dev);
  		return -EIO;
  	}
 
-	if (dev->cmd_err & DAVINCI_I2C_STR_NACK) {
-		if (msg->flags & I2C_M_IGNORE_NAK)
-			return msg->len;
-		w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
-		w |= DAVINCI_I2C_MDR_STP;
-		davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
-		return -EREMOTEIO;
-	}
-	return -EIO;
+	/* We are here because of NAK */
+
+	if (msg->flags & I2C_M_IGNORE_NAK)
+		return msg->len;
+
+	reinit_completion(&dev->cmd_complete);
+	/* Clear IRQ flags */
+	davinci_i2c_write_reg(dev, DAVINCI_I2C_STR_REG, DAVINCI_I2C_STR_ALL);
+	/* We going to wait for SCD IRQ */
+	davinci_i2c_write_reg(dev, DAVINCI_I2C_IMR_REG, I2C_DAVINCI_INTR_ALL);
+
+	w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
+	w |= DAVINCI_I2C_MDR_STP;
+	davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
+
+	r = i2c_davinci_wait_for_completion(dev);
+	if (r)
+		return r;
+	return -EREMOTEIO;
  }
 
  /*

@@ -451,27 +475,6 @@ static u32 i2c_davinci_func(struct i2c_adapter *adap)
  	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
  }
 
-static void terminate_read(struct davinci_i2c_dev *dev)
-{
-	u16 w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
-	w |= DAVINCI_I2C_MDR_NACK;
-	davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
-
-	/* Throw away data */
-	davinci_i2c_read_reg(dev, DAVINCI_I2C_DRR_REG);
-	if (!dev->terminate)
-		dev_err(dev->dev, "RDR IRQ while no data requested\n");
-}
-static void terminate_write(struct davinci_i2c_dev *dev)
-{
-	u16 w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
-	w |= DAVINCI_I2C_MDR_RM | DAVINCI_I2C_MDR_STP;
-	davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
-
-	if (!dev->terminate)
-		dev_dbg(dev->dev, "TDR IRQ while no data to send\n");
-}
-
  /*
   * Interrupt service routine. This gets called whenever an I2C interrupt
   * occurs.

@@ -491,49 +494,19 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void *dev_id)
  		}
 
  		switch (stat) {
-		case DAVINCI_I2C_IVR_AL:
-			/* Arbitration lost, must retry */
-			dev->cmd_err |= DAVINCI_I2C_STR_AL;
-			dev->buf_len = 0;
-			complete(&dev->cmd_complete);
-			break;
-
-		case DAVINCI_I2C_IVR_NACK:
-			dev->cmd_err |= DAVINCI_I2C_STR_NACK;
-			dev->buf_len = 0;
-			complete(&dev->cmd_complete);
-			break;
-
-		case DAVINCI_I2C_IVR_ARDY:
-			davinci_i2c_write_reg(dev,
-				DAVINCI_I2C_STR_REG, DAVINCI_I2C_STR_ARDY);
-			if (((dev->buf_len == 0) && (dev->stop != 0)) ||
-			    (dev->cmd_err & DAVINCI_I2C_STR_NACK)) {
-				w = davinci_i2c_read_reg(dev,
-							 DAVINCI_I2C_MDR_REG);
-				w |= DAVINCI_I2C_MDR_STP;
-				davinci_i2c_write_reg(dev,
-						      DAVINCI_I2C_MDR_REG, w);
-			}
-			complete(&dev->cmd_complete);
-			break;
-
  		case DAVINCI_I2C_IVR_RDR:
  			if (dev->buf_len) {
  				*dev->buf++ =
  				    davinci_i2c_read_reg(dev,
  							 DAVINCI_I2C_DRR_REG);
  				dev->buf_len--;
-				if (dev->buf_len)
-					continue;
-
-				davinci_i2c_write_reg(dev,
-					DAVINCI_I2C_STR_REG,
-					DAVINCI_I2C_IMR_RRDY);
-			} else {
-				/* signal can terminate transfer */
-				terminate_read(dev);
+				continue;
  			}
+
+			/* Read transfer completed, mask the IRQ */
+			w = davinci_i2c_read_reg(dev, DAVINCI_I2C_IMR_REG);
+			w &= ~DAVINCI_I2C_IMR_RRDY;
+			davinci_i2c_write_reg(dev, DAVINCI_I2C_IMR_REG, w);
  			break;
 
  		case DAVINCI_I2C_IVR_XRDY:

@@ -541,24 +514,22 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void *dev_id)
  				davinci_i2c_write_reg(dev, DAVINCI_I2C_DXR_REG,
  						      *dev->buf++);
  				dev->buf_len--;
-				if (dev->buf_len)
-					continue;
-
-				w = davinci_i2c_read_reg(dev,
-							 DAVINCI_I2C_IMR_REG);
-				w &= ~DAVINCI_I2C_IMR_XRDY;
-				davinci_i2c_write_reg(dev,
-						      DAVINCI_I2C_IMR_REG,
-						      w);
-			} else {
-				/* signal can terminate transfer */
-				terminate_write(dev);
+				continue;
  			}
+
+			/* Write transfer completed, mask the IRQ */
+			w = davinci_i2c_read_reg(dev, DAVINCI_I2C_IMR_REG);
+			w &= ~DAVINCI_I2C_IMR_XRDY;
+			davinci_i2c_write_reg(dev, DAVINCI_I2C_IMR_REG, w);
  			break;
 
+		case DAVINCI_I2C_IVR_AL:
+		case DAVINCI_I2C_IVR_NACK:
  		case DAVINCI_I2C_IVR_SCD:
-			davinci_i2c_write_reg(dev,
-				DAVINCI_I2C_STR_REG, DAVINCI_I2C_STR_SCD);
+		case DAVINCI_I2C_IVR_ARDY:
+			dev->cmd_stat = stat;
+			/* Mask all IRQs */
+			davinci_i2c_write_reg(dev, DAVINCI_I2C_IMR_REG, 0);
  			complete(&dev->cmd_complete);
  			break;
 

Quoted 28 lines

There is one big problem in the current design: ISR accesses the controller

Quoted 27 lines

registers in parallel with i2c_davinci_xfer_msg() in process context. The whole logic is not obvious, many operations are performed in process context while ISR is always enabled and does something asynchronous even while it's not expected. We have faced these races on 4-cores Keystone chip. Some examples:

- when non-existing device is accessed first comes NAK IRQ, then ARDY IRQ. After NAK we already jump out of wait_for_completion_timeout() and depending on how lucky we are ARDY IRQ will access MDR register in the middle of some other operation in process context;

- STOP condition is triggered in many places in the driver, in ISR, in i2c_davinci_xfer_msg(), but there is no code which guarantees that STOP will be really completed. We have seen many STOP conditions simply missing in back-to-back transfers, when next i2c_davinci_xfer_msg() call simply overwrites MDR register while STOP is still not generated.

So, make the design more robust and obvious:
- leave hot path (buffers management) in ISR, remove other registers access from ISR;
- introduce second synchronization point, to make sure that STOP condition is really generated and it's safe to begin next transfer;
- simplify the state machine;
- enable IRQs only when they are expected, disable them in ISR when transfer is completed/failed;
- STOP is normally set simultaneously with START condition (in case of last message); only special case when STOP is additionally generated is received NAK -- this case is handled separately.

I'm not sure about this change (- It's too significant and definitely will need more review & Tested-by.

We need to be careful with it, especially taking into account DAVINCI_I2C_MDR_RM mode and future changes like https://lkml.org/lkml/2014/5/1/348.

May be you can split it?

Quoted 5 lines

Quoted 5 lines

Signed-off-by Alexander Sverdlin <[email protected]>

---
  drivers/i2c/busses/i2c-davinci.c |  219 ++++++++++++++++---------------------
  1 files changed, 95 insertions(+), 124 deletions(-)

Hello!

Quoted 28 lines · On 12/03/15 14:16, ext [email protected]…

Quoted 28 lines

There is one big problem in the current design: ISR accesses the controller

Quoted 27 lines

registers in parallel with i2c_davinci_xfer_msg() in process context. The whole logic is not obvious, many operations are performed in process context while ISR is always enabled and does something asynchronous even while it's not expected. We have faced these races on 4-cores Keystone chip. Some examples:

- when non-existing device is accessed first comes NAK IRQ, then ARDY IRQ. After NAK we already jump out of wait_for_completion_timeout() and depending on how lucky we are ARDY IRQ will access MDR register in the middle of some other operation in process context;

- STOP condition is triggered in many places in the driver, in ISR, in i2c_davinci_xfer_msg(), but there is no code which guarantees that STOP will be really completed. We have seen many STOP conditions simply missing in back-to-back transfers, when next i2c_davinci_xfer_msg() call simply overwrites MDR register while STOP is still not generated.

So, make the design more robust and obvious:
- leave hot path (buffers management) in ISR, remove other registers access from ISR;
- introduce second synchronization point, to make sure that STOP condition is really generated and it's safe to begin next transfer;
- simplify the state machine;
- enable IRQs only when they are expected, disable them in ISR when transfer is completed/failed;
- STOP is normally set simultaneously with START condition (in case of last message); only special case when STOP is additionally generated is received NAK -- this case is handled separately.

Quoted 1 line

I'm not sure about this change (- It's too significant and definitely will need more review & Tested-by.

Maybe you can offer this patch the customers who suddenly cannot access the devices on the bus until reboot... Because it's not a "bus lockup".

Quoted 3 lines

We need to be careful with it, especially taking into account DAVINCI_I2C_MDR_RM mode and future changes like https://lkml.org/lkml/2014/5/1/348.

Quoted 1 line

May be you can split it?

I can may be split it into two patches, but I'm not sure about the result. Each of them will only solve 50% of the problem and then nobody will see a clear benefit applying only one. But what I can offer you is to spend the same effort on rebasing the "DAVINCI_I2C_MDR_RM mode" patch you are referring to. I can rebase it and take it into my series if you wish.

Quoted 36 lines · Alexander Sverdlin

Hello!

Quoted 28 lines · On 12/03/15 14:16, ext [email protected]…

Quoted 28 lines

There is one big problem in the current design: ISR accesses the controller

Quoted 27 lines

registers in parallel with i2c_davinci_xfer_msg() in process context. The whole logic is not obvious, many operations are performed in process context while ISR is always enabled and does something asynchronous even while it's not expected. We have faced these races on 4-cores Keystone chip. Some examples:

- when non-existing device is accessed first comes NAK IRQ, then ARDY IRQ. After NAK we already jump out of wait_for_completion_timeout() and depending on how lucky we are ARDY IRQ will access MDR register in the middle of some other operation in process context;

- STOP condition is triggered in many places in the driver, in ISR, in i2c_davinci_xfer_msg(), but there is no code which guarantees that STOP will be really completed. We have seen many STOP conditions simply missing in back-to-back transfers, when next i2c_davinci_xfer_msg() call simply overwrites MDR register while STOP is still not generated.

So, make the design more robust and obvious:
- leave hot path (buffers management) in ISR, remove other registers access from

    ISR;
- introduce second synchronization point, to make sure that STOP condition is
    really generated and it's safe to begin next transfer;
- simplify the state machine;
- enable IRQs only when they are expected, disable them in ISR when transfer is
    completed/failed;

- STOP is normally set simultaneously with START condition (in case of last message); only special case when STOP is additionally generated is received NAK -- this case is handled separately.

Quoted 1 line

I'm not sure about this change (- It's too significant and definitely will need more review & Tested-by.

Maybe you can offer this patch the customers who suddenly cannot access the devices on the bus until reboot... Because it's not a "bus lockup".

Quoted 7 lines

Quoted 4 lines

We need to be careful with it, especially taking into account DAVINCI_I2C_MDR_RM mode and future changes like https://lkml.org/lkml/2014/5/1/348.

May be you can split it?

I can may be split it into two patches, but I'm not sure about the result. Each of them will only solve 50% of the problem and then nobody will see a clear benefit applying only one. But what I can offer you is

Quoted 2 lines

to spend the same effort on rebasing the "DAVINCI_I2C_MDR_RM mode" patch you are referring to. I can rebase it and take it into my series if you wish.

So, shall I take this into i2c/for-next?