[PATCH 0/2] omap i2c interrupt handler fixes

[PATCH 0/2] omap i2c interrupt handler fixes

[Alexander Shishkin]

Hi,

This is the second version of the patch that I've sent to linux-omap to
address this issue. This time I've moved the whole errata workaround bit
to a separate function to get rid of too long lines and a couple of extra
levels of indentation.

The actual fix is the same as the first time, it adds a timeout to a busy
loop which happens to take place in an interrupt handler and is capable
of hanging the kernel.

Regards,
--
Alex

[PATCH 1/2] omap i2c: make errata 1.153 workaround a separate function

[Alexander Shishkin]

This is to avoid insanely long lines and levels of indentation.

Signed-off-by: Alexander Shishkin <virtuoso@slind.org>
CC: linux-i2c@vger.kernel.org
CC: linux-omap@vger.kernel.org
---
 drivers/i2c/busses/i2c-omap.c |   43 ++++++++++++++++++++++------------------
 1 files changed, 24 insertions(+), 19 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 75bf3ad..ad8242a 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -671,6 +671,27 @@ omap_i2c_rev1_isr(int this_irq, void *dev_id)
 #define omap_i2c_rev1_isr		NULL
 #endif
 
+/*
+ * OMAP3430 Errata 1.153: When an XRDY/XDR is hit, wait for XUDF before writing
+ * data to DATA_REG. Otherwise some data bytes can be lost while transferring
+ * them from the memory to the I2C interface.
+ */
+static int omap3430_workaround(struct omap_i2c_dev *dev, u16 *stat, int *err)
+{
+	while (!(*stat & OMAP_I2C_STAT_XUDF)) {
+		if (*stat & (OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL)) {
+			omap_i2c_ack_stat(dev, *stat & (OMAP_I2C_STAT_XRDY |
+							OMAP_I2C_STAT_XDR));
+			*err |= OMAP_I2C_STAT_XUDF;
+			return -1;
+		}
+		cpu_relax();
+		*stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
+	}
+
+	return 0;
+}
+
 static irqreturn_t
 omap_i2c_isr(int this_irq, void *dev_id)
 {
@@ -794,25 +815,9 @@ complete:
 					break;
 				}
 
-				/*
-				 * OMAP3430 Errata 1.153: When an XRDY/XDR
-				 * is hit, wait for XUDF before writing data
-				 * to DATA_REG. Otherwise some data bytes can
-				 * be lost while transferring them from the
-				 * memory to the I2C interface.
-				 */
-
-				if (dev->rev <= OMAP_I2C_REV_ON_3430) {
-						while (!(stat & OMAP_I2C_STAT_XUDF)) {
-							if (stat & (OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL)) {
-								omap_i2c_ack_stat(dev, stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR));
-								err |= OMAP_I2C_STAT_XUDF;
-								goto complete;
-							}
-							cpu_relax();
-							stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
-						}
-				}
+				if (dev->rev <= OMAP_I2C_REV_ON_3430 &&
+				    omap3430_workaround(dev, &stat, &err))
+					goto complete;
 
 				omap_i2c_write_reg(dev, OMAP_I2C_DATA_REG, w);
 			}
-- 
1.6.3.3

[PATCH 1/2] omap i2c: make errata 1.153 workaround a separate function

[Alexander Shishkin]

From: Alexander Shishkin <ext-alexander.shishkin@nokia.com>

This is to avoid insanely long lines and levels of indentation.

Signed-off-by: Alexander Shishkin <ext-alexander.shishkin@nokia.com>
---
 drivers/i2c/busses/i2c-omap.c |   43 ++++++++++++++++++++++------------------
 1 files changed, 24 insertions(+), 19 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 75bf3ad..ad8242a 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -671,6 +671,27 @@ omap_i2c_rev1_isr(int this_irq, void *dev_id)
 #define omap_i2c_rev1_isr		NULL
 #endif
 
+/*
+ * OMAP3430 Errata 1.153: When an XRDY/XDR is hit, wait for XUDF before writing
+ * data to DATA_REG. Otherwise some data bytes can be lost while transferring
+ * them from the memory to the I2C interface.
+ */
+static int omap3430_workaround(struct omap_i2c_dev *dev, u16 *stat, int *err)
+{
+	while (!(*stat & OMAP_I2C_STAT_XUDF)) {
+		if (*stat & (OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL)) {
+			omap_i2c_ack_stat(dev, *stat & (OMAP_I2C_STAT_XRDY |
+							OMAP_I2C_STAT_XDR));
+			*err |= OMAP_I2C_STAT_XUDF;
+			return -1;
+		}
+		cpu_relax();
+		*stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
+	}
+
+	return 0;
+}
+
 static irqreturn_t
 omap_i2c_isr(int this_irq, void *dev_id)
 {
@@ -794,25 +815,9 @@ complete:
 					break;
 				}
 
-				/*
-				 * OMAP3430 Errata 1.153: When an XRDY/XDR
-				 * is hit, wait for XUDF before writing data
-				 * to DATA_REG. Otherwise some data bytes can
-				 * be lost while transferring them from the
-				 * memory to the I2C interface.
-				 */
-
-				if (dev->rev <= OMAP_I2C_REV_ON_3430) {
-						while (!(stat & OMAP_I2C_STAT_XUDF)) {
-							if (stat & (OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL)) {
-								omap_i2c_ack_stat(dev, stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR));
-								err |= OMAP_I2C_STAT_XUDF;
-								goto complete;
-							}
-							cpu_relax();
-							stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
-						}
-				}
+				if (dev->rev <= OMAP_I2C_REV_ON_3430 &&
+				    omap3430_workaround(dev, &stat, &err))
+					goto complete;
 
 				omap_i2c_write_reg(dev, OMAP_I2C_DATA_REG, w);
 			}
-- 
1.6.3.3

[PATCH 2/2] omap i2c: add a timeout to the busy waiting

[Alexander Shishkin]

The errata 1.153 workaround is busy waiting on XUDF bit in interrupt
context, which may lead to kernel hangs. The problem can be reproduced
by running the bus with wrong (too high) speed.

Signed-off-by: Alexander Shishkin <virtuoso-0lOfPCoBze7YtjvyW6yDsg@public.gmane.org>
CC: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
CC: linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
---
 drivers/i2c/busses/i2c-omap.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index ad8242a..b474c20 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -678,7 +678,9 @@ omap_i2c_rev1_isr(int this_irq, void *dev_id)
  */
 static int omap3430_workaround(struct omap_i2c_dev *dev, u16 *stat, int *err)
 {
-	while (!(*stat & OMAP_I2C_STAT_XUDF)) {
+	unsigned long timeout = 10000;
+
+	while (!(*stat & OMAP_I2C_STAT_XUDF && --timeout)) {
 		if (*stat & (OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL)) {
 			omap_i2c_ack_stat(dev, *stat & (OMAP_I2C_STAT_XRDY |
 							OMAP_I2C_STAT_XDR));
@@ -689,6 +691,9 @@ static int omap3430_workaround(struct omap_i2c_dev *dev, u16 *stat, int *err)
 		*stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
 	}
 
+	if (!timeout)
+		dev_err(dev->dev, "timeout waiting on XUDF bit\n");
+
 	return 0;
 }
 
-- 
1.6.3.3

Re: [PATCH 1/2] omap i2c: make errata 1.153 workaround a separate function

[Alexander Shishkin]

On Wed, Dec 16, 2009 at 03:43:04 +0200, Alexander Shishkin wrote:
> From: Alexander Shishkin <ext-alexander.shishkin@nokia.com>

Please disregard this, I've got the emails wrong here. I'll resend
shortly.

Regards,
--
Alex

[PATCH 0/2][RESEND] omap i2c interrupt handler fixes

[Alexander Shishkin]

Hi,

This is the second version of the patch that I've sent to linux-omap to
address this issue. This time I've moved the whole errata workaround bit
to a separate function to get rid of too long lines and a couple of extra
levels of indentation.

The actual fix is the same as the first time, it adds a timeout to a busy
loop which happens to take place in an interrupt handler and is capable
of hanging the kernel.

Regards,
--
Alex

[PATCH 1/2] omap i2c: make errata 1.153 workaround a separate function

[Alexander Shishkin]

This is to avoid insanely long lines and levels of indentation.

Signed-off-by: Alexander Shishkin <virtuoso-0lOfPCoBze7YtjvyW6yDsg@public.gmane.org>
CC: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
CC: linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
---
 drivers/i2c/busses/i2c-omap.c |   43 ++++++++++++++++++++++------------------
 1 files changed, 24 insertions(+), 19 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 75bf3ad..ad8242a 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -671,6 +671,27 @@ omap_i2c_rev1_isr(int this_irq, void *dev_id)
 #define omap_i2c_rev1_isr		NULL
 #endif
 
+/*
+ * OMAP3430 Errata 1.153: When an XRDY/XDR is hit, wait for XUDF before writing
+ * data to DATA_REG. Otherwise some data bytes can be lost while transferring
+ * them from the memory to the I2C interface.
+ */
+static int omap3430_workaround(struct omap_i2c_dev *dev, u16 *stat, int *err)
+{
+	while (!(*stat & OMAP_I2C_STAT_XUDF)) {
+		if (*stat & (OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL)) {
+			omap_i2c_ack_stat(dev, *stat & (OMAP_I2C_STAT_XRDY |
+							OMAP_I2C_STAT_XDR));
+			*err |= OMAP_I2C_STAT_XUDF;
+			return -1;
+		}
+		cpu_relax();
+		*stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
+	}
+
+	return 0;
+}
+
 static irqreturn_t
 omap_i2c_isr(int this_irq, void *dev_id)
 {
@@ -794,25 +815,9 @@ complete:
 					break;
 				}
 
-				/*
-				 * OMAP3430 Errata 1.153: When an XRDY/XDR
-				 * is hit, wait for XUDF before writing data
-				 * to DATA_REG. Otherwise some data bytes can
-				 * be lost while transferring them from the
-				 * memory to the I2C interface.
-				 */
-
-				if (dev->rev <= OMAP_I2C_REV_ON_3430) {
-						while (!(stat & OMAP_I2C_STAT_XUDF)) {
-							if (stat & (OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL)) {
-								omap_i2c_ack_stat(dev, stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR));
-								err |= OMAP_I2C_STAT_XUDF;
-								goto complete;
-							}
-							cpu_relax();
-							stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
-						}
-				}
+				if (dev->rev <= OMAP_I2C_REV_ON_3430 &&
+				    omap3430_workaround(dev, &stat, &err))
+					goto complete;
 
 				omap_i2c_write_reg(dev, OMAP_I2C_DATA_REG, w);
 			}
-- 
1.6.3.3

[PATCH 2/2] omap i2c: add a timeout to the busy waiting

[Alexander Shishkin]

The errata 1.153 workaround is busy waiting on XUDF bit in interrupt
context, which may lead to kernel hangs. The problem can be reproduced
by running the bus with wrong (too high) speed.

Signed-off-by: Alexander Shishkin <virtuoso@slind.org>
CC: linux-i2c@vger.kernel.org
CC: linux-omap@vger.kernel.org
---
 drivers/i2c/busses/i2c-omap.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index ad8242a..b474c20 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -678,7 +678,9 @@ omap_i2c_rev1_isr(int this_irq, void *dev_id)
  */
 static int omap3430_workaround(struct omap_i2c_dev *dev, u16 *stat, int *err)
 {
-	while (!(*stat & OMAP_I2C_STAT_XUDF)) {
+	unsigned long timeout = 10000;
+
+	while (!(*stat & OMAP_I2C_STAT_XUDF && --timeout)) {
 		if (*stat & (OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL)) {
 			omap_i2c_ack_stat(dev, *stat & (OMAP_I2C_STAT_XRDY |
 							OMAP_I2C_STAT_XDR));
@@ -689,6 +691,9 @@ static int omap3430_workaround(struct omap_i2c_dev *dev, u16 *stat, int *err)
 		*stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
 	}
 
+	if (!timeout)
+		dev_err(dev->dev, "timeout waiting on XUDF bit\n");
+
 	return 0;
 }
 
-- 
1.6.3.3

Re: [PATCH 2/2] omap i2c: add a timeout to the busy waiting

[Menon, Nishanth]

Alexander Shishkin said the following on 12/16/2009 07:32 PM:
> The errata 1.153 workaround is busy waiting on XUDF bit in interrupt
> context, which may lead to kernel hangs. The problem can be reproduced
> by running the bus with wrong (too high) speed.
>
> Signed-off-by: Alexander Shishkin <virtuoso@slind.org>
> CC: linux-i2c@vger.kernel.org
> CC: linux-omap@vger.kernel.org
> ---
>  drivers/i2c/busses/i2c-omap.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index ad8242a..b474c20 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -678,7 +678,9 @@ omap_i2c_rev1_isr(int this_irq, void *dev_id)
>   */
>  static int omap3430_workaround(struct omap_i2c_dev *dev, u16 *stat, int *err)
>  {
> -	while (!(*stat & OMAP_I2C_STAT_XUDF)) {
> +	unsigned long timeout = 10000;
> +
> +	while (!(*stat & OMAP_I2C_STAT_XUDF && --timeout)) {
>   
a) timeout without using an actual delay is not a good idea - consider 
each OPP - we can go upto 1ghz on 3630,
the actual time for 10000 iterations will depend on the MPU speed.
b) how did you arrive at the 10k iteration limit?

>  		if (*stat & (OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL)) {
>  			omap_i2c_ack_stat(dev, *stat & (OMAP_I2C_STAT_XRDY |
>  							OMAP_I2C_STAT_XDR));
> @@ -689,6 +691,9 @@ static int omap3430_workaround(struct omap_i2c_dev *dev, u16 *stat, int *err)
>  		*stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
>  	}
>  
> +	if (!timeout)
> +		dev_err(dev->dev, "timeout waiting on XUDF bit\n");
> +
>  	return 0;
>  }
>  
>   
Regards,
Nishanth Menon

Re: [PATCH 2/2] omap i2c: add a timeout to the busy waiting

[Alexander Shishkin]

On Thu, Dec 17, 2009 at 08:38:39 +0530, Menon, Nishanth wrote:
> Alexander Shishkin said the following on 12/16/2009 07:32 PM:
> >The errata 1.153 workaround is busy waiting on XUDF bit in interrupt
> >context, which may lead to kernel hangs. The problem can be reproduced
> >by running the bus with wrong (too high) speed.
> >
> >Signed-off-by: Alexander Shishkin <virtuoso-0lOfPCoBze7YtjvyW6yDsg@public.gmane.org>
> >CC: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> >CC: linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> >---
> > drivers/i2c/busses/i2c-omap.c |    7 ++++++-
> > 1 files changed, 6 insertions(+), 1 deletions(-)
> >
> >diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> >index ad8242a..b474c20 100644
> >--- a/drivers/i2c/busses/i2c-omap.c
> >+++ b/drivers/i2c/busses/i2c-omap.c
> >@@ -678,7 +678,9 @@ omap_i2c_rev1_isr(int this_irq, void *dev_id)
> >  */
> > static int omap3430_workaround(struct omap_i2c_dev *dev, u16 *stat, int *err)
> > {
> >-	while (!(*stat & OMAP_I2C_STAT_XUDF)) {
> >+	unsigned long timeout = 10000;
> >+
> >+	while (!(*stat & OMAP_I2C_STAT_XUDF && --timeout)) {
> a) timeout without using an actual delay is not a good idea -
> consider each OPP - we can go upto 1ghz on 3630,
> the actual time for 10000 iterations will depend on the MPU speed.

Well, I could calculate the timeout value based on current operating speed,
I guess. Or a delay. Perhaps OMAP_I2C_TIMEOUT can be used here?

> b) how did you arrive at the 10k iteration limit?

It was random, but then it seemed ok considering the l4 latency.

> > 		if (*stat & (OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL)) {
> > 			omap_i2c_ack_stat(dev, *stat & (OMAP_I2C_STAT_XRDY |
> > 							OMAP_I2C_STAT_XDR));
> >@@ -689,6 +691,9 @@ static int omap3430_workaround(struct omap_i2c_dev *dev, u16 *stat, int *err)
> > 		*stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
> > 	}
> >+	if (!timeout)
> >+		dev_err(dev->dev, "timeout waiting on XUDF bit\n");
> >+
> > 	return 0;
> > }
> Regards,
> Nishanth Menon
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] omap i2c: add a timeout to the busy waiting

[Menon, Nishanth]

Alexander Shishkin said the following on 12/17/2009 07:01 PM:
> On Thu, Dec 17, 2009 at 08:38:39 +0530, Menon, Nishanth wrote:
>   
>> Alexander Shishkin said the following on 12/16/2009 07:32 PM:
>>     
>>> The errata 1.153 workaround is busy waiting on XUDF bit in interrupt
>>> context, which may lead to kernel hangs. The problem can be reproduced
>>> by running the bus with wrong (too high) speed.
>>>
>>> Signed-off-by: Alexander Shishkin <virtuoso-0lOfPCoBze7YtjvyW6yDsg@public.gmane.org>
>>> CC: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>> CC: linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>> ---
>>> drivers/i2c/busses/i2c-omap.c |    7 ++++++-
>>> 1 files changed, 6 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
>>> index ad8242a..b474c20 100644
>>> --- a/drivers/i2c/busses/i2c-omap.c
>>> +++ b/drivers/i2c/busses/i2c-omap.c
>>> @@ -678,7 +678,9 @@ omap_i2c_rev1_isr(int this_irq, void *dev_id)
>>>  */
>>> static int omap3430_workaround(struct omap_i2c_dev *dev, u16 *stat, int *err)
>>> {
>>> -	while (!(*stat & OMAP_I2C_STAT_XUDF)) {
>>> +	unsigned long timeout = 10000;
>>> +
>>> +	while (!(*stat & OMAP_I2C_STAT_XUDF && --timeout)) {
>>>       
>> a) timeout without using an actual delay is not a good idea -
>> consider each OPP - we can go upto 1ghz on 3630,
>> the actual time for 10000 iterations will depend on the MPU speed.
>>     
>
> Well, I could calculate the timeout value based on current operating speed,
> I guess. Or a delay. Perhaps OMAP_I2C_TIMEOUT can be used here?
>   
it might be an overkill trying to generate counter based on opp speeds. 
wondering if this delay is required in the first place..

Moiz Sonasath / Vikram P, (commit cd086d3a) could probably comment?
Ref: http://www.mail-archive.com/linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg15317.html

>   
>> b) how did you arrive at the 10k iteration limit?
>>     
>
> It was random, but then it seemed ok considering the l4 latency.
>   
I had guessed this is an emperical value. It will be good to know the 
exact rationale on why a timeout will happen, else we will have all kind 
of questions pending trying to figure out if a timeout happened.

>   
>>> 		if (*stat & (OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL)) {
>>> 			omap_i2c_ack_stat(dev, *stat & (OMAP_I2C_STAT_XRDY |
>>> 							OMAP_I2C_STAT_XDR));
>>> @@ -689,6 +691,9 @@ static int omap3430_workaround(struct omap_i2c_dev *dev, u16 *stat, int *err)
>>> 		*stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
>>> 	}
>>> +	if (!timeout)
>>> +		dev_err(dev->dev, "timeout waiting on XUDF bit\n");
>>> +
>>> 	return 0;
>>> }
Regards,
Nishanth Menon

RE: [PATCH 2/2] omap i2c: add a timeout to the busy waiting

[Sonasath, Moiz]

Alex,

> -----Original Message-----
> From: Menon, Nishanth
> Sent: Thursday, December 17, 2009 7:59 AM
> To: Menon, Nishanth; ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org; linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org;
> linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Sonasath, Moiz; Pandita, Vikram
> Subject: Re: [PATCH 2/2] omap i2c: add a timeout to the busy waiting
> 
> Alexander Shishkin said the following on 12/17/2009 07:01 PM:
> > On Thu, Dec 17, 2009 at 08:38:39 +0530, Menon, Nishanth wrote:
> >
> >> Alexander Shishkin said the following on 12/16/2009 07:32 PM:
> >>
> >>> The errata 1.153 workaround is busy waiting on XUDF bit in interrupt
> >>> context, which may lead to kernel hangs. The problem can be reproduced
> >>> by running the bus with wrong (too high) speed.
> >>>
> >>> Signed-off-by: Alexander Shishkin <virtuoso-0lOfPCoBze7YtjvyW6yDsg@public.gmane.org>
> >>> CC: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> >>> CC: linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> >>> ---
> >>> drivers/i2c/busses/i2c-omap.c |    7 ++++++-
> >>> 1 files changed, 6 insertions(+), 1 deletions(-)
> >>>
> >>> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-
> omap.c
> >>> index ad8242a..b474c20 100644
> >>> --- a/drivers/i2c/busses/i2c-omap.c
> >>> +++ b/drivers/i2c/busses/i2c-omap.c
> >>> @@ -678,7 +678,9 @@ omap_i2c_rev1_isr(int this_irq, void *dev_id)
> >>>  */
> >>> static int omap3430_workaround(struct omap_i2c_dev *dev, u16 *stat,
> int *err)
> >>> {
> >>> -	while (!(*stat & OMAP_I2C_STAT_XUDF)) {
> >>> +	unsigned long timeout = 10000;
> >>> +
> >>> +	while (!(*stat & OMAP_I2C_STAT_XUDF && --timeout)) {
> >>>
> >> a) timeout without using an actual delay is not a good idea -
> >> consider each OPP - we can go upto 1ghz on 3630,
> >> the actual time for 10000 iterations will depend on the MPU speed.
> >>
> >
> > Well, I could calculate the timeout value based on current operating
> speed,
> > I guess. Or a delay. Perhaps OMAP_I2C_TIMEOUT can be used here?
> >
> it might be an overkill trying to generate counter based on opp speeds.
> wondering if this delay is required in the first place..

The reason why this timeout was not put in place was that if the I2C controller is functional, the XUDF has to set after XRDY/XDR are set, the only case where it might not be set is when there is a sudden NACK | AL condition produced on the bus and the transfer stops (thereby the FIFO is not getting empty). The code takes care of this error situation so IMHO we can do without a timeout here.

--- Moiz
> 
> Moiz Sonasath / Vikram P, (commit cd086d3a) could probably comment?
> Ref: http://www.mail-archive.com/linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg15317.html
> 
> >
> >> b) how did you arrive at the 10k iteration limit?
> >>
> >
> > It was random, but then it seemed ok considering the l4 latency.
> >
> I had guessed this is an emperical value. It will be good to know the
> exact rationale on why a timeout will happen, else we will have all kind
> of questions pending trying to figure out if a timeout happened.
> 
> >
> >>> 		if (*stat & (OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL)) {
> >>> 			omap_i2c_ack_stat(dev, *stat & (OMAP_I2C_STAT_XRDY |
> >>> 							OMAP_I2C_STAT_XDR));
> >>> @@ -689,6 +691,9 @@ static int omap3430_workaround(struct omap_i2c_dev
> *dev, u16 *stat, int *err)
> >>> 		*stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
> >>> 	}
> >>> +	if (!timeout)
> >>> +		dev_err(dev->dev, "timeout waiting on XUDF bit\n");
> >>> +
> >>> 	return 0;
> >>> }
> Regards,
> Nishanth Menon

Re: [PATCH 2/2] omap i2c: add a timeout to the busy waiting

[Aaro Koskinen]

Hi,

Sonasath, Moiz wrote:
>> From: Menon, Nishanth
>> Alexander Shishkin said the following on 12/17/2009 07:01 PM:
>>> Well, I could calculate the timeout value based on current operating
>> speed,
>>> I guess. Or a delay. Perhaps OMAP_I2C_TIMEOUT can be used here?
>>>
>> it might be an overkill trying to generate counter based on opp speeds.
>> wondering if this delay is required in the first place..
> 
> The reason why this timeout was not put in place was that if the I2C
> controller is functional, the XUDF has to set after XRDY/XDR are set,
> the only case where it might not be set is when there is a sudden
> NACK | AL condition produced on the bus and the transfer stops
> (thereby the FIFO is not getting empty). The code takes care of this
> error situation so IMHO we can do without a timeout here.

We are able to reproduce an error situation, where none of XUDF|NACK|AL
gets set. In that case the kernel hangs. If we add the timeout, we are
able to recover. If you want to have a robust driver, you cannot have a
loop which is not guaranteed to terminate.

A.