[U-Boot] [PATCH] i2c: mxs_i2c: Squash endless loop

[U-Boot] [PATCH] i2c: mxs_i2c: Squash endless loop

[Marek Vasut]

The endless waiting for a bit to be set can cause a hang, add a timeout
so we prevent such situation. A testcase for such a hang is below. The
testcase assumes a device to be present at address 0x50 and a device to
NOT be present at address 0x42 . Also note that the "sleep 1" induced
delays are imperative for this bug to manifest .

i2c read 0x42 0x0.2 0x10 0x42000000 ; sleep 1 ; \
i2c read 0x50 0x0.2 0x10 0x42000000 ; sleep 1 ; \
i2c read 0x42 0x0.2 0x10 0x42000000

The expected result of the above command is:

Error reading the chip.
Error reading the chip.

While without this patch, we observe a hang in the last read from 0x42
precisely when waiting for this bit to be set.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Fabio Estevam <fabio.estevam@freescale.com>
Cc: Heiko Schocher <hs@denx.de>
Cc: Stefano Babic <sbabic@denx.de>
---
 drivers/i2c/mxs_i2c.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/mxs_i2c.c b/drivers/i2c/mxs_i2c.c
index 46106b7..49300d4 100644
--- a/drivers/i2c/mxs_i2c.c
+++ b/drivers/i2c/mxs_i2c.c
@@ -150,6 +150,7 @@ int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len)
 {
 	struct mxs_i2c_regs *i2c_regs = (struct mxs_i2c_regs *)MXS_I2C0_BASE;
 	uint32_t tmp = 0;
+	int timeout = MXS_I2C_MAX_TIMEOUT;
 	int ret;
 	int i;
 
@@ -169,9 +170,17 @@ int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len)
 
 	for (i = 0; i < len; i++) {
 		if (!(i & 3)) {
-			while (readl(&i2c_regs->hw_i2c_queuestat) &
-				I2C_QUEUESTAT_RD_QUEUE_EMPTY)
-				;
+			while (--timeout) {
+				tmp = readl(&i2c_regs->hw_i2c_queuestat);
+				if (!(tmp & I2C_QUEUESTAT_RD_QUEUE_EMPTY))
+					break;
+			}
+
+			if (!timeout) {
+				debug("MXS I2C: Failed receiving data!\n");
+				return -EINVAL;
+			}
+
 			tmp = readl(&i2c_regs->hw_i2c_queuedata);
 		}
 		buffer[i] = tmp & 0xff;
-- 
1.8.4.rc3

[U-Boot] [PATCH] i2c: mxs_i2c: Squash endless loop

[Stefano Babic]

Hi Marek,

On 02/11/2013 18:23, Marek Vasut wrote:

> +			if (!timeout) {
> +				debug("MXS I2C: Failed receiving data!\n");
> +				return -EINVAL;
> +			}
> +

This is a real error and not a debug information. IMHO it should be
better to print the error unconditionally with puts/printf, reporting
that the timer elapsed.

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH] i2c: mxs_i2c: Squash endless loop

[Marek Vasut]

Hi Stefano,

> Hi Marek,
> 
> On 02/11/2013 18:23, Marek Vasut wrote:
> > +			if (!timeout) {
> > +				debug("MXS I2C: Failed receiving data!\n");
> > +				return -EINVAL;
> > +			}
> > +
> 
> This is a real error and not a debug information. IMHO it should be
> better to print the error unconditionally with puts/printf, reporting
> that the timer elapsed.

Returning -EINVAL will make the i2c stack trigger an output, so having it 
duplicated here is pointless I believe.

Best regards,
Marek Vasut

[U-Boot] [PATCH] i2c: mxs_i2c: Squash endless loop

[Stefano Babic]

Hi Marek,

On 04/11/2013 12:50, Marek Vasut wrote:
> Hi Stefano,
> 
>> Hi Marek,
>>
>> On 02/11/2013 18:23, Marek Vasut wrote:
>>> +			if (!timeout) {
>>> +				debug("MXS I2C: Failed receiving data!\n");
>>> +				return -EINVAL;
>>> +			}
>>> +
>>
>> This is a real error and not a debug information. IMHO it should be
>> better to print the error unconditionally with puts/printf, reporting
>> that the timer elapsed.
> 
> Returning -EINVAL will make the i2c stack trigger an output, so having it 
> duplicated here is pointless I believe.

Agree on that. But then, should we not return -ETIMEDOUT (-110) ? We
should print the error code in the i2c stack (do_i2c_read) instead of
checking only if the return value is not null, as we do now.

Best regards,
Stefano Babic


-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH] i2c: mxs_i2c: Squash endless loop

[Heiko Schocher]

Hello Stefano, Marek,

Am 04.11.2013 13:03, schrieb Stefano Babic:
> Hi Marek,
>
> On 04/11/2013 12:50, Marek Vasut wrote:
>> Hi Stefano,
>>
>>> Hi Marek,
>>>
>>> On 02/11/2013 18:23, Marek Vasut wrote:
>>>> +			if (!timeout) {
>>>> +				debug("MXS I2C: Failed receiving data!\n");
>>>> +				return -EINVAL;
>>>> +			}
>>>> +
>>>
>>> This is a real error and not a debug information. IMHO it should be
>>> better to print the error unconditionally with puts/printf, reporting
>>> that the timer elapsed.
>>
>> Returning -EINVAL will make the i2c stack trigger an output, so having it
>> duplicated here is pointless I believe.
>
> Agree on that. But then, should we not return -ETIMEDOUT (-110) ? We

Yes, that should be -ETIMEDOUT

> should print the error code in the i2c stack (do_i2c_read) instead of
> checking only if the return value is not null, as we do now.

Yep, printing in do_i2c_read()  the error code would be nice. Patches
are welcome :-)

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH] i2c: mxs_i2c: Squash endless loop

[Marek Vasut]

Hi Heiko,

> Hello Stefano, Marek,
> 
> Am 04.11.2013 13:03, schrieb Stefano Babic:
> > Hi Marek,
> > 
> > On 04/11/2013 12:50, Marek Vasut wrote:
> >> Hi Stefano,
> >> 
> >>> Hi Marek,
> >>> 
> >>> On 02/11/2013 18:23, Marek Vasut wrote:
> >>>> +			if (!timeout) {
> >>>> +				debug("MXS I2C: Failed receiving data!
\n");
> >>>> +				return -EINVAL;
> >>>> +			}
> >>>> +
> >>> 
> >>> This is a real error and not a debug information. IMHO it should be
> >>> better to print the error unconditionally with puts/printf, reporting
> >>> that the timer elapsed.
> >> 
> >> Returning -EINVAL will make the i2c stack trigger an output, so having
> >> it duplicated here is pointless I believe.
> > 
> > Agree on that. But then, should we not return -ETIMEDOUT (-110) ? We
> 
> Yes, that should be -ETIMEDOUT

Full ACK.

> > should print the error code in the i2c stack (do_i2c_read) instead of
> > checking only if the return value is not null, as we do now.
> 
> Yep, printing in do_i2c_read()  the error code would be nice. Patches
> are welcome :-)

OK, shall I also print out an error message then?

Best regards,
Marek Vasut

[U-Boot] [PATCH] i2c: mxs_i2c: Squash endless loop

[Heiko Schocher]

Hello Marek,

Am 04.11.2013 14:13, schrieb Marek Vasut:
> Hi Heiko,
>
>> Hello Stefano, Marek,
>>
>> Am 04.11.2013 13:03, schrieb Stefano Babic:
>>> Hi Marek,
>>>
>>> On 04/11/2013 12:50, Marek Vasut wrote:
>>>> Hi Stefano,
>>>>
>>>>> Hi Marek,
>>>>>
>>>>> On 02/11/2013 18:23, Marek Vasut wrote:
>>>>>> +			if (!timeout) {
>>>>>> +				debug("MXS I2C: Failed receiving data!
> \n");
>>>>>> +				return -EINVAL;
>>>>>> +			}
>>>>>> +
>>>>>
>>>>> This is a real error and not a debug information. IMHO it should be
>>>>> better to print the error unconditionally with puts/printf, reporting
>>>>> that the timer elapsed.
>>>>
>>>> Returning -EINVAL will make the i2c stack trigger an output, so having
>>>> it duplicated here is pointless I believe.
>>>
>>> Agree on that. But then, should we not return -ETIMEDOUT (-110) ? We
>>
>> Yes, that should be -ETIMEDOUT
>
> Full ACK.
>
>>> should print the error code in the i2c stack (do_i2c_read) instead of
>>> checking only if the return value is not null, as we do now.
>>
>> Yep, printing in do_i2c_read()  the error code would be nice. Patches
>> are welcome :-)
>
> OK, shall I also print out an error message then?

You mean in the mxs i2c driver? I think, this is not needed, as
Stefano suggested.

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH] i2c: mxs_i2c: Squash endless loop

[Marek Vasut]

Dear Heiko Schocher,

> Hello Marek,
> 
> Am 04.11.2013 14:13, schrieb Marek Vasut:
> > Hi Heiko,
> > 
> >> Hello Stefano, Marek,
> >> 
> >> Am 04.11.2013 13:03, schrieb Stefano Babic:
> >>> Hi Marek,
> >>> 
> >>> On 04/11/2013 12:50, Marek Vasut wrote:
> >>>> Hi Stefano,
> >>>> 
> >>>>> Hi Marek,
> >>>>> 
> >>>>> On 02/11/2013 18:23, Marek Vasut wrote:
> >>>>>> +			if (!timeout) {
> >>>>>> +				debug("MXS I2C: Failed receiving data!
> > 
> > \n");
> > 
> >>>>>> +				return -EINVAL;
> >>>>>> +			}
> >>>>>> +
> >>>>> 
> >>>>> This is a real error and not a debug information. IMHO it should be
> >>>>> better to print the error unconditionally with puts/printf, reporting
> >>>>> that the timer elapsed.
> >>>> 
> >>>> Returning -EINVAL will make the i2c stack trigger an output, so having
> >>>> it duplicated here is pointless I believe.
> >>> 
> >>> Agree on that. But then, should we not return -ETIMEDOUT (-110) ? We
> >> 
> >> Yes, that should be -ETIMEDOUT
> > 
> > Full ACK.
> > 
> >>> should print the error code in the i2c stack (do_i2c_read) instead of
> >>> checking only if the return value is not null, as we do now.
> >> 
> >> Yep, printing in do_i2c_read()  the error code would be nice. Patches
> >> are welcome :-)
> > 
> > OK, shall I also print out an error message then?
> 
> You mean in the mxs i2c driver? I think, this is not needed, as
> Stefano suggested.

OK, thanks! V2 out.

Best regards,
Marek Vasut