[v2] i2c: ocores: generate stop condition after timeout in polling mode

[PATCH v2] i2c: ocores: generate stop condition after timeout in polling mode

Posted by Matthias Schiffer 2 years, 4 months ago

From: Gregor Herburger <gregor.herburger@tq-group.com>

In polling mode, no stop condition is generated after a timeout. This
causes SCL to remain low and thereby block the bus. If this happens
during a transfer it can cause slaves to misinterpret the subsequent
transfer and return wrong values.

To solve this, pass the ETIMEDOUT error up from ocores_process_polling()
instead of setting STATE_ERROR directly. The caller is adjusted to call
ocores_process_timeout() on error both in polling and in IRQ mode, which
will set STATE_ERROR and generate a stop condition.

Fixes: 69c8c0c0efa8 ("i2c: ocores: add polling interface")
Signed-off-by: Gregor Herburger <gregor.herburger@tq-group.com>
Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---

v2: style improvements based on feedback from Federico and Andrew. I went
    with a slightly different solution than Andrew suggested to avoid using
    the ret variable for two different kinds of returns.

 drivers/i2c/busses/i2c-ocores.c | 35 ++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
index a0af027db04c1..2e575856c5cd5 100644
--- a/drivers/i2c/busses/i2c-ocores.c
+++ b/drivers/i2c/busses/i2c-ocores.c
@@ -342,18 +342,18 @@ static int ocores_poll_wait(struct ocores_i2c *i2c)
  * ocores_isr(), we just add our polling code around it.
  *
  * It can run in atomic context
+ *
+ * Return: 0 on success, -ETIMEDOUT on timeout
  */
-static void ocores_process_polling(struct ocores_i2c *i2c)
+static int ocores_process_polling(struct ocores_i2c *i2c)
 {
-	while (1) {
-		irqreturn_t ret;
-		int err;
+	irqreturn_t ret;
+	int err = 0;
 
+	while (1) {
 		err = ocores_poll_wait(i2c);
-		if (err) {
-			i2c->state = STATE_ERROR;
+		if (err)
 			break; /* timeout */
-		}
 
 		ret = ocores_isr(-1, i2c);
 		if (ret == IRQ_NONE)
@@ -364,13 +364,15 @@ static void ocores_process_polling(struct ocores_i2c *i2c)
 					break;
 		}
 	}
+
+	return err;
 }
 
 static int ocores_xfer_core(struct ocores_i2c *i2c,
 			    struct i2c_msg *msgs, int num,
 			    bool polling)
 {
-	int ret;
+	int ret = 0;
 	u8 ctrl;
 
 	ctrl = oc_getreg(i2c, OCI2C_CONTROL);
@@ -388,15 +390,16 @@ static int ocores_xfer_core(struct ocores_i2c *i2c,
 	oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_START);
 
 	if (polling) {
-		ocores_process_polling(i2c);
+		ret = ocores_process_polling(i2c);
 	} else {
-		ret = wait_event_timeout(i2c->wait,
-					 (i2c->state == STATE_ERROR) ||
-					 (i2c->state == STATE_DONE), HZ);
-		if (ret == 0) {
-			ocores_process_timeout(i2c);
-			return -ETIMEDOUT;
-		}
+		if (wait_event_timeout(i2c->wait,
+				       (i2c->state == STATE_ERROR) ||
+				       (i2c->state == STATE_DONE), HZ) == 0)
+			ret = -ETIMEDOUT;
+	}
+	if (ret) {
+		ocores_process_timeout(i2c);
+		return ret;
 	}
 
 	return (i2c->state == STATE_DONE) ? num : -EIO;
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH v2] i2c: ocores: generate stop condition after timeout in polling mode

Posted by Wolfram Sang 2 years, 4 months ago

On Thu, Apr 13, 2023 at 11:37:37AM +0200, Matthias Schiffer wrote:
> From: Gregor Herburger <gregor.herburger@tq-group.com>
> 
> In polling mode, no stop condition is generated after a timeout. This
> causes SCL to remain low and thereby block the bus. If this happens
> during a transfer it can cause slaves to misinterpret the subsequent
> transfer and return wrong values.
> 
> To solve this, pass the ETIMEDOUT error up from ocores_process_polling()
> instead of setting STATE_ERROR directly. The caller is adjusted to call
> ocores_process_timeout() on error both in polling and in IRQ mode, which
> will set STATE_ERROR and generate a stop condition.
> 
> Fixes: 69c8c0c0efa8 ("i2c: ocores: add polling interface")
> Signed-off-by: Gregor Herburger <gregor.herburger@tq-group.com>
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>

Applied to for-current, thanks everyone!

Re: [PATCH v2] i2c: ocores: generate stop condition after timeout in polling mode

Posted by Federico Vaga 2 years, 4 months ago

On Thu, Apr 13, 2023 at 11:37:37AM +0200, Matthias Schiffer wrote:
>From: Gregor Herburger <gregor.herburger@tq-group.com>
>
>In polling mode, no stop condition is generated after a timeout. This
>causes SCL to remain low and thereby block the bus. If this happens
>during a transfer it can cause slaves to misinterpret the subsequent
>transfer and return wrong values.
>
>To solve this, pass the ETIMEDOUT error up from ocores_process_polling()
>instead of setting STATE_ERROR directly. The caller is adjusted to call
>ocores_process_timeout() on error both in polling and in IRQ mode, which
>will set STATE_ERROR and generate a stop condition.
>
>Fixes: 69c8c0c0efa8 ("i2c: ocores: add polling interface")
>Signed-off-by: Gregor Herburger <gregor.herburger@tq-group.com>
>Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
>---
>
>v2: style improvements based on feedback from Federico and Andrew. I went
>    with a slightly different solution than Andrew suggested to avoid using
>    the ret variable for two different kinds of returns.
>
> drivers/i2c/busses/i2c-ocores.c | 35 ++++++++++++++++++---------------
> 1 file changed, 19 insertions(+), 16 deletions(-)

Reviewed-by: Federico Vaga <federico.vaga@cern.ch>

-- 
Federico Vaga - CERN BE-CEM-EDL

Re: [PATCH v2] i2c: ocores: generate stop condition after timeout in polling mode

Posted by Andrew Lunn 2 years, 4 months ago

On Thu, Apr 13, 2023 at 11:37:37AM +0200, Matthias Schiffer wrote:
> From: Gregor Herburger <gregor.herburger@tq-group.com>
> 
> In polling mode, no stop condition is generated after a timeout. This
> causes SCL to remain low and thereby block the bus. If this happens
> during a transfer it can cause slaves to misinterpret the subsequent
> transfer and return wrong values.
> 
> To solve this, pass the ETIMEDOUT error up from ocores_process_polling()
> instead of setting STATE_ERROR directly. The caller is adjusted to call
> ocores_process_timeout() on error both in polling and in IRQ mode, which
> will set STATE_ERROR and generate a stop condition.
> 
> Fixes: 69c8c0c0efa8 ("i2c: ocores: add polling interface")
> Signed-off-by: Gregor Herburger <gregor.herburger@tq-group.com>
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v2] i2c: ocores: generate stop condition after timeout in polling mode

Posted by Peter Korsgaard 2 years, 4 months ago

>>>>> "Matthias" == Matthias Schiffer <matthias.schiffer@ew.tq-group.com> writes:

 > From: Gregor Herburger <gregor.herburger@tq-group.com>
 > In polling mode, no stop condition is generated after a timeout. This
 > causes SCL to remain low and thereby block the bus. If this happens
 > during a transfer it can cause slaves to misinterpret the subsequent
 > transfer and return wrong values.

 > To solve this, pass the ETIMEDOUT error up from ocores_process_polling()
 > instead of setting STATE_ERROR directly. The caller is adjusted to call
 > ocores_process_timeout() on error both in polling and in IRQ mode, which
 > will set STATE_ERROR and generate a stop condition.

 > Fixes: 69c8c0c0efa8 ("i2c: ocores: add polling interface")
 > Signed-off-by: Gregor Herburger <gregor.herburger@tq-group.com>
 > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
 > ---

 > v2: style improvements based on feedback from Federico and Andrew. I went
 >     with a slightly different solution than Andrew suggested to avoid using
 >     the ret variable for two different kinds of returns.

Acked-by: Peter Korsgaard <peter@korsgaard.com>

-- 
Bye, Peter Korsgaard