[PATCH 1/3] i2c: davinci: Rework racy ISR

[PATCH 1/3] i2c: davinci: Rework racy ISR

[Alexander Sverdlin]

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 <alexander.sverdlin-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
---
 drivers/i2c/busses/i2c-davinci.c |  219 ++++++++++++++++---------------------
 1 files changed, 95 insertions(+), 124 deletions(-)

diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 6dc7ff5..98759ae 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -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;
 

Re: [PATCH 1/3] i2c: davinci: Rework racy ISR

[Grygorii.Strashko-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org]

On 03/11/2015 03:08 PM, Alexander Sverdlin wrote:
> 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.

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?

> 
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
> ---
>   drivers/i2c/busses/i2c-davinci.c |  219 ++++++++++++++++---------------------
>   1 files changed, 95 insertions(+), 124 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
> index 6dc7ff5..98759ae 100644
> --- a/drivers/i2c/busses/i2c-davinci.c
> +++ b/drivers/i2c/busses/i2c-davinci.c
> @@ -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;
>   
> 


-- 
regards,
-grygorii

Re: [PATCH 1/3] i2c: davinci: Rework racy ISR

[Alexander Sverdlin]

Hello!

On 12/03/15 14:16, ext Grygorii.Strashko-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org wrote:
>> 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.
> 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". 

> 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
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.
 
>> > 
>> > Signed-off-by: Alexander Sverdlin <alexander.sverdlin-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
>> > ---
>> >   drivers/i2c/busses/i2c-davinci.c |  219 ++++++++++++++++---------------------
>> >   1 files changed, 95 insertions(+), 124 deletions(-)

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH 1/3] i2c: davinci: Rework racy ISR

[Wolfram Sang]

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

On Fri, Mar 13, 2015 at 09:03:33AM +0100, Alexander Sverdlin wrote:
> Hello!
> 
> On 12/03/15 14:16, ext Grygorii.Strashko-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org wrote:
> >> 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.
> > 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". 
> 
> > 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
> 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?


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

Re: [PATCH 1/3] i2c: davinci: Rework racy ISR

[Grygorii.Strashko-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org]

Hi Wolfram, Alexander,
On 04/03/2015 11:15 PM, Wolfram Sang wrote:
> On Fri, Mar 13, 2015 at 09:03:33AM +0100, Alexander Sverdlin wrote:
>> Hello!
>>
>> On 12/03/15 14:16, ext Grygorii.Strashko-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org wrote:
>>>> 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.
>>> 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".
>>
>>> 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

In my opinion, this one can be split as it fixes few issues at once (see below).

>> 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?
> 

As i mentioned before, this patch should get Acked/Tested-by from Davinci community at least
(I understand the issue and that it should be fixed, but personally I don't like the way it's done)
- The of ISR code have not been  changed significantly from the very beginning of Davinci I2C driver's life :(
- As I understand from commits history your patch most probably will break I2C_FUNC_SMBUS_QUICK, but I can't
  verify it (c6c7c72 i2c: davinci: Fix smbus Oops with AIC33 usage).

So I have following propositions:
1) Get rid of obsolete code left after commit 5a0d5f5 i2c-davinci: Fix signal handling bug
because commit 900ef80 'i2c: davinci: don't use interruptible completion" coverts
wait_for_completion_interruptible_timeout() -> wait_for_completion_timeout() [part of this patch]

2) Add i2c_davinci_wait_bus_not_busy() at the end of i2c_davinci_xfer(), so driver will
   wait BF before continue (should fix STP issue)

3) Give a try below diff which could fix NACK -> ARDY issue
--
diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 4788a32..a053c55 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -485,9 +485,8 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
        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);
+               /* assume STP will be sent from ISR
+                * TODO: msg->flags & I2C_M_IGNORE_NAK ?*/
                return -EREMOTEIO;
        }
        return -EIO;
@@ -581,7 +580,6 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void *dev_id)
                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:
@@ -594,8 +592,9 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void *dev_id)
                                w |= DAVINCI_I2C_MDR_STP;
                                davinci_i2c_write_reg(dev,
                                                      DAVINCI_I2C_MDR_REG, w);
+                       } else {
+                               complete(&dev->cmd_complete);
                        }
-                       complete(&dev->cmd_complete);
                        break;
 
                case DAVINCI_I2C_IVR_RDR:

4) Clean up ICSTR in i2c_davinci_xfer_msg() [part of this patch], follows
   SPRUH77A - TRM 'OMAP-L138 DSP+ARM Processor'
   23.2.11.1 Configuring the I2C in Master Receiver Mode and Servicing Receive Data via CPU

5) Correct IRQ configuration in i2c_davinci_xfer_msg(), now DAVINCI_I2C_IMR_RRDY and
 DAVINCI_I2C_IMR_XRDY will never be cleared once set.

6) [optional] Enable/disable IRQ in i2c_davinci_xfer_msg() or davinci_xfer_msg() only when 
   driver is ready to handle it.

I'll be glad to help if needed, but the main problem from my side is that I have no HW to test it
(buggy scenario with NACK) and see no way to simulate it.

regards,
-grygorii