[PATCH] i2c: ocores: synchronize startup with interrupt handling

[PATCH] i2c: ocores: synchronize startup with interrupt handling

[Matthias Goergens]

A delayed interrupt can retain the previous transfer's pending STATUS
and message pointer while the next transfer is initialized. It can then
advance the new transfer before its address transfer completes.

Hold process_lock across control setup, transfer initialization and the
START command. Read STATUS and snapshot the message under the same lock
in the interrupt handler, and run transfer processing with the lock
held.

Found by Sashiko AI review.

Link: https://sashiko.dev/#/patchset/20260813153155.3953577-1-ruoyuw560@gmail.com
---
 drivers/i2c/busses/i2c-ocores.c | 45 ++++++++++++++++++---------------
 1 file changed, 25 insertions(+), 20 deletions(-)

diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
index ff024cf5f92d..e661693827c5 100644
--- a/drivers/i2c/busses/i2c-ocores.c
+++ b/drivers/i2c/busses/i2c-ocores.c
@@ -28,8 +28,8 @@
 #include <linux/jiffies.h>
 
 /*
- * 'process_lock' exists because ocores_process() and ocores_process_timeout()
- * can't run in parallel.
+ * 'process_lock' serializes transfer startup with interrupt and timeout
+ * processing.
  */
 struct ocores_i2c {
 	void __iomem *base;
@@ -148,27 +148,23 @@ static inline u8 oc_getreg(struct ocores_i2c *i2c, int reg)
 
 static void ocores_process(struct ocores_i2c *i2c, u8 stat)
 {
-	struct i2c_msg *msg = i2c->msg;
-	unsigned long flags;
+	struct i2c_msg *msg;
 
-	/*
-	 * If we spin here is because we are in timeout, so we are going
-	 * to be in STATE_ERROR. See ocores_process_timeout()
-	 */
-	spin_lock_irqsave(&i2c->process_lock, flags);
+	lockdep_assert_held(&i2c->process_lock);
+	msg = i2c->msg;
 
 	if ((i2c->state == STATE_DONE) || (i2c->state == STATE_ERROR)) {
 		/* stop has been sent */
 		oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_IACK);
 		wake_up(&i2c->wait);
-		goto out;
+		return;
 	}
 
 	/* error? */
 	if (stat & OCI2C_STAT_ARBLOST) {
 		i2c->state = STATE_ERROR;
 		oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_STOP);
-		goto out;
+		return;
 	}
 
 	if ((i2c->state == STATE_START) || (i2c->state == STATE_WRITE)) {
@@ -178,7 +174,7 @@ static void ocores_process(struct ocores_i2c *i2c, u8 stat)
 		if (stat & OCI2C_STAT_NACK) {
 			i2c->state = STATE_ERROR;
 			oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_STOP);
-			goto out;
+			return;
 		}
 	} else {
 		msg->buf[i2c->pos++] = oc_getreg(i2c, OCI2C_DATA);
@@ -200,14 +196,14 @@ static void ocores_process(struct ocores_i2c *i2c, u8 stat)
 
 				oc_setreg(i2c, OCI2C_DATA, addr);
 				oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_START);
-				goto out;
+				return;
 			}
 			i2c->state = (msg->flags & I2C_M_RD)
 				? STATE_READ : STATE_WRITE;
 		} else {
 			i2c->state = STATE_DONE;
 			oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_STOP);
-			goto out;
+			return;
 		}
 	}
 
@@ -218,25 +214,31 @@ static void ocores_process(struct ocores_i2c *i2c, u8 stat)
 		oc_setreg(i2c, OCI2C_DATA, msg->buf[i2c->pos++]);
 		oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_WRITE);
 	}
-
-out:
-	spin_unlock_irqrestore(&i2c->process_lock, flags);
 }
 
 static irqreturn_t ocores_isr(int irq, void *dev_id)
 {
 	struct ocores_i2c *i2c = dev_id;
-	u8 stat = oc_getreg(i2c, OCI2C_STATUS);
+	unsigned long flags;
+	u8 stat;
+
+	spin_lock_irqsave(&i2c->process_lock, flags);
+	stat = oc_getreg(i2c, OCI2C_STATUS);
 
 	if (i2c->flags & OCORES_FLAG_BROKEN_IRQ) {
 		if ((stat & OCI2C_STAT_IF) && !(stat & OCI2C_STAT_BUSY))
-			return IRQ_NONE;
+			goto out_none;
 	} else if (!(stat & OCI2C_STAT_IF)) {
-		return IRQ_NONE;
+		goto out_none;
 	}
 	ocores_process(i2c, stat);
+	spin_unlock_irqrestore(&i2c->process_lock, flags);
 
 	return IRQ_HANDLED;
+
+out_none:
+	spin_unlock_irqrestore(&i2c->process_lock, flags);
+	return IRQ_NONE;
 }
 
 /**
@@ -356,8 +358,10 @@ static int ocores_xfer_core(struct ocores_i2c *i2c,
 			    bool polling)
 {
 	int ret = 0;
+	unsigned long flags;
 	u8 ctrl;
 
+	spin_lock_irqsave(&i2c->process_lock, flags);
 	ctrl = oc_getreg(i2c, OCI2C_CONTROL);
 	if (polling)
 		oc_setreg(i2c, OCI2C_CONTROL, ctrl & ~OCI2C_CTRL_IEN);
@@ -371,6 +375,7 @@ static int ocores_xfer_core(struct ocores_i2c *i2c,
 
 	oc_setreg(i2c, OCI2C_DATA, i2c_8bit_addr_from_msg(i2c->msg));
 	oc_setreg(i2c, OCI2C_CMD, OCI2C_CMD_START);
+	spin_unlock_irqrestore(&i2c->process_lock, flags);
 
 	if (polling) {
 		ret = ocores_process_polling(i2c);

base-commit: d89e1d186ec9271bf78e62771779e9fc03bf0fc3
prerequisite-patch-id: 44b1c2680cb029871e123eb11eab7764a807ef24
prerequisite-patch-id: f4dfbbaaec2d208ac255451cf032c22f0e7fed2d
prerequisite-patch-id: ff6a574f35399a0059df3a759cbd2a558d24300c
-- 
2.56.0

Re: [PATCH] i2c: ocores: synchronize startup with interrupt handling

[Markus Elfring]

…
> Found by Sashiko AI review.
> 
> Link: https://sashiko.dev/#/patchset/20260813153155.3953577-1-ruoyuw560@gmail.com
> ---
…

How do you think about to add any tags accordingly?
* Fixes
* Cc
* Signed-off-by


…
> +++ b/drivers/i2c/busses/i2c-ocores.c
…
>  static irqreturn_t ocores_isr(int irq, void *dev_id)
>  {
>  	struct ocores_i2c *i2c = dev_id;
> -	u8 stat = oc_getreg(i2c, OCI2C_STATUS);
> +	unsigned long flags;
> +	u8 stat;
> +
> +	spin_lock_irqsave(&i2c->process_lock, flags);
> +	stat = oc_getreg(i2c, OCI2C_STATUS);
>  
>  	if (i2c->flags & OCORES_FLAG_BROKEN_IRQ) {
…
>  	}
>  	ocores_process(i2c, stat);
> +	spin_unlock_irqrestore(&i2c->process_lock, flags);
>  
>  	return IRQ_HANDLED;
…

Under which circumstances would lock guards become applicable for
affected software components?
https://elixir.bootlin.com/linux/v7.3/source/include/linux/spinlock.h#L642-L645

Regards,
Markus