[1/3] 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 <alexander.sverdlin@nokia.com>
---
 drivers/i2c/busses/i2c-davinci.c |  219 ++++++++++++++++---------------------
 1 files changed, 95 insertions(+), 124 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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@nokia.com>
> ---
>   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;
>   
>

Hello!

On 12/03/15 14:16, ext Grygorii.Strashko@linaro.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@nokia.com>
>> > ---
>> >   drivers/i2c/busses/i2c-davinci.c |  219 ++++++++++++++++---------------------
>> >   1 files changed, 95 insertions(+), 124 deletions(-)

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