[PATCH 0/2] i2c: rcar: increase robustness against long SMIs

[PATCH 0/2] i2c: rcar: increase robustness against long SMIs

[Wolfram Sang]

A customer reported problems with the R-Car I2C driver with System
Management Interrupts lasting longer than 30us. Let's not comment about
SMIs taking this long (and even up to 200us), but improve the driver to
handle such situations.

Wolfram Sang (2):
  i2c: rcar: avoid race condition with SMIs
  i2c: rcar: refactor handling of first message

 drivers/i2c/busses/i2c-rcar.c | 76 ++++++++++++++++++-----------------
 1 file changed, 39 insertions(+), 37 deletions(-)

-- 
2.35.1

[PATCH 1/2] i2c: rcar: avoid race condition with SMIs

[Wolfram Sang]

A customer experienced a race condition with 'repeated starts' when a
System Management Interrupt took over for 30us and more. The problem was
that during the SMI a new MAT interrupt came in because we set up the
'repeated start' condition. But the old one was not acknowledged yet.
So, when it was acknowledged after the SMI, the new MAT interrupt was
lost, confusing the state machine of the driver.

The fix consists of two parts. First, we do not clear the status
register for 'repeated starts' when preparing the next message anymore.
The interrupt handlers for sending and receiving data is now solely
responsible for that and it makes the code easier to follow, in fact.
Secondly, clearing the status register is now split up to handle MAT
interrupts independently. This avoids the race condition because the old
MAT interrupt will be now cleared before we initiate the "repeated
start" condition.

Reported-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-rcar.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index ca61dbe218bf..143cb4186daa 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -97,9 +97,6 @@
 #define RCAR_IRQ_RECV	(MNR | MAL | MST | MAT | MDR)
 #define RCAR_IRQ_STOP	(MST)
 
-#define RCAR_IRQ_ACK_SEND	(~(MAT | MDE) & 0x7F)
-#define RCAR_IRQ_ACK_RECV	(~(MAT | MDR) & 0x7F)
-
 #define ID_LAST_MSG	(1 << 0)
 #define ID_FIRST_MSG	(1 << 1)
 #define ID_DONE		(1 << 2)
@@ -161,6 +158,11 @@ static u32 rcar_i2c_read(struct rcar_i2c_priv *priv, int reg)
 	return readl(priv->io + reg);
 }
 
+static void rcar_i2c_clear_irq(struct rcar_i2c_priv *priv, u32 val)
+{
+	writel(~val & 0x7f, priv->io + ICMSR);
+}
+
 static int rcar_i2c_get_scl(struct i2c_adapter *adap)
 {
 	struct rcar_i2c_priv *priv = i2c_get_adapdata(adap);
@@ -356,7 +358,7 @@ static void rcar_i2c_prepare_msg(struct rcar_i2c_priv *priv)
 			priv->flags &= ~ID_P_REP_AFTER_RD;
 		else
 			rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_START);
-		rcar_i2c_write(priv, ICMSR, 0);
+		/* ICMSR is cleared in interrupt handlers */
 	}
 }
 
@@ -476,11 +478,15 @@ static bool rcar_i2c_dma(struct rcar_i2c_priv *priv)
 static void rcar_i2c_irq_send(struct rcar_i2c_priv *priv, u32 msr)
 {
 	struct i2c_msg *msg = priv->msg;
+	u32 irqs_to_clear = MDE;
 
 	/* FIXME: sometimes, unknown interrupt happened. Do nothing */
 	if (!(msr & MDE))
 		return;
 
+	if (msr & MAT)
+		irqs_to_clear |= MAT;
+
 	/* Check if DMA can be enabled and take over */
 	if (priv->pos == 1 && rcar_i2c_dma(priv))
 		return;
@@ -504,32 +510,32 @@ static void rcar_i2c_irq_send(struct rcar_i2c_priv *priv, u32 msr)
 		 * [ICRXTX] -> [SHIFT] -> [I2C bus]
 		 */
 
-		if (priv->flags & ID_LAST_MSG) {
+		if (priv->flags & ID_LAST_MSG)
 			/*
 			 * If current msg is the _LAST_ msg,
 			 * prepare stop condition here.
 			 * ID_DONE will be set on STOP irq.
 			 */
 			rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_STOP);
-		} else {
+		else
 			rcar_i2c_next_msg(priv);
-			return;
-		}
 	}
 
-	rcar_i2c_write(priv, ICMSR, RCAR_IRQ_ACK_SEND);
+	rcar_i2c_clear_irq(priv, irqs_to_clear);
 }
 
 static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr)
 {
 	struct i2c_msg *msg = priv->msg;
 	bool recv_len_init = priv->pos == 0 && msg->flags & I2C_M_RECV_LEN;
+	u32 irqs_to_clear = MDR;
 
 	/* FIXME: sometimes, unknown interrupt happened. Do nothing */
 	if (!(msr & MDR))
 		return;
 
 	if (msr & MAT) {
+		irqs_to_clear |= MAT;
 		/*
 		 * Address transfer phase finished, but no data at this point.
 		 * Try to use DMA to receive data.
@@ -570,8 +576,8 @@ static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr)
 
 	if (priv->pos == msg->len && !(priv->flags & ID_LAST_MSG))
 		rcar_i2c_next_msg(priv);
-	else
-		rcar_i2c_write(priv, ICMSR, RCAR_IRQ_ACK_RECV);
+
+	rcar_i2c_clear_irq(priv, irqs_to_clear);
 }
 
 static bool rcar_i2c_slave_irq(struct rcar_i2c_priv *priv)
-- 
2.35.1

[PATCH 2/2] i2c: rcar: refactor handling of first message

[Wolfram Sang]

After moving ICMSR handling to interrupt handlers previously to fix a
race condition, we can now also move ICMSR handling for the first
message out of the function to prepare a message. By introducing a
seperate function to initialize the first message, we can not only
remove some code duplication but the remaining code is also easier to
follow. The function to prepare a message is much simpler without ICMSR
handling.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-rcar.c | 50 ++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 27 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 143cb4186daa..f69903397285 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -98,7 +98,6 @@
 #define RCAR_IRQ_STOP	(MST)
 
 #define ID_LAST_MSG	(1 << 0)
-#define ID_FIRST_MSG	(1 << 1)
 #define ID_DONE		(1 << 2)
 #define ID_ARBLOST	(1 << 3)
 #define ID_NACK		(1 << 4)
@@ -333,11 +332,19 @@ static int rcar_i2c_clock_calculate(struct rcar_i2c_priv *priv)
 	return 0;
 }
 
+/*
+ * We don't have a test case but the HW engineers say that the write order of
+ * ICMSR and ICMCR depends on whether we issue START or REP_START. So, ICMSR
+ * handling is outside of this function. First messages clear ICMSR before this
+ * function, interrupt handlers clear the relevant bits after this function.
+ */
 static void rcar_i2c_prepare_msg(struct rcar_i2c_priv *priv)
 {
 	int read = !!rcar_i2c_is_recv(priv);
 
 	priv->pos = 0;
+	priv->flags &= ID_P_MASK;
+
 	if (priv->msgs_left == 1)
 		priv->flags |= ID_LAST_MSG;
 
@@ -345,29 +352,27 @@ static void rcar_i2c_prepare_msg(struct rcar_i2c_priv *priv)
 	if (!priv->atomic_xfer)
 		rcar_i2c_write(priv, ICMIER, read ? RCAR_IRQ_RECV : RCAR_IRQ_SEND);
 
-	/*
-	 * We don't have a test case but the HW engineers say that the write order
-	 * of ICMSR and ICMCR depends on whether we issue START or REP_START. Since
-	 * it didn't cause a drawback for me, let's rather be safe than sorry.
-	 */
-	if (priv->flags & ID_FIRST_MSG) {
-		rcar_i2c_write(priv, ICMSR, 0);
+	if (priv->flags & ID_P_REP_AFTER_RD)
+		priv->flags &= ~ID_P_REP_AFTER_RD;
+	else
 		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_START);
-	} else {
-		if (priv->flags & ID_P_REP_AFTER_RD)
-			priv->flags &= ~ID_P_REP_AFTER_RD;
-		else
-			rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_START);
-		/* ICMSR is cleared in interrupt handlers */
-	}
+}
+
+static void rcar_i2c_first_msg(struct rcar_i2c_priv *priv,
+			       struct i2c_msg *msgs, int num)
+{
+	priv->msg = msgs;
+	priv->msgs_left = num;
+	rcar_i2c_write(priv, ICMSR, 0); /* must be before preparing msg */
+	rcar_i2c_prepare_msg(priv);
 }
 
 static void rcar_i2c_next_msg(struct rcar_i2c_priv *priv)
 {
 	priv->msg++;
 	priv->msgs_left--;
-	priv->flags &= ID_P_MASK;
 	rcar_i2c_prepare_msg(priv);
+	/* ICMSR handling must come afterwards in the irq handler */
 }
 
 static void rcar_i2c_cleanup_dma(struct rcar_i2c_priv *priv, bool terminate)
@@ -852,11 +857,7 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap,
 	for (i = 0; i < num; i++)
 		rcar_i2c_request_dma(priv, msgs + i);
 
-	/* init first message */
-	priv->msg = msgs;
-	priv->msgs_left = num;
-	priv->flags = (priv->flags & ID_P_MASK) | ID_FIRST_MSG;
-	rcar_i2c_prepare_msg(priv);
+	rcar_i2c_first_msg(priv, msgs, num);
 
 	time_left = wait_event_timeout(priv->wait, priv->flags & ID_DONE,
 				     num * adap->timeout);
@@ -906,12 +907,7 @@ static int rcar_i2c_master_xfer_atomic(struct i2c_adapter *adap,
 		goto out;
 
 	rcar_i2c_init(priv);
-
-	/* init first message */
-	priv->msg = msgs;
-	priv->msgs_left = num;
-	priv->flags = (priv->flags & ID_P_MASK) | ID_FIRST_MSG;
-	rcar_i2c_prepare_msg(priv);
+	rcar_i2c_first_msg(priv, msgs, num);
 
 	j = jiffies + num * adap->timeout;
 	do {
-- 
2.35.1

Re: [PATCH 1/2] i2c: rcar: avoid race condition with SMIs

[Wolfram Sang]

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

On Fri, May 20, 2022 at 12:33:24PM +0200, Wolfram Sang wrote:
> A customer experienced a race condition with 'repeated starts' when a
> System Management Interrupt took over for 30us and more. The problem was
> that during the SMI a new MAT interrupt came in because we set up the
> 'repeated start' condition. But the old one was not acknowledged yet.
> So, when it was acknowledged after the SMI, the new MAT interrupt was
> lost, confusing the state machine of the driver.
> 
> The fix consists of two parts. First, we do not clear the status
> register for 'repeated starts' when preparing the next message anymore.
> The interrupt handlers for sending and receiving data is now solely
> responsible for that and it makes the code easier to follow, in fact.
> Secondly, clearing the status register is now split up to handle MAT
> interrupts independently. This avoids the race condition because the old
> MAT interrupt will be now cleared before we initiate the "repeated
> start" condition.
> 
> Reported-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] i2c: rcar: refactor handling of first message

[Wolfram Sang]

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

On Fri, May 20, 2022 at 12:33:25PM +0200, Wolfram Sang wrote:
> After moving ICMSR handling to interrupt handlers previously to fix a
> race condition, we can now also move ICMSR handling for the first
> message out of the function to prepare a message. By introducing a
> seperate function to initialize the first message, we can not only
> remove some code duplication but the remaining code is also easier to
> follow. The function to prepare a message is much simpler without ICMSR
> handling.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]