[PATCH v2 0/6] i2c: xiic: Fix broken locking

[PATCH v2 0/6] i2c: xiic: Fix broken locking

[Marek Vasut]

Booting ZynqMP with XIIC I2C driver shows multitude of race conditions
in the XIIC driver. This is because locking is completely missing from
the driver, and there are odd corner cases where the hardware behaves
strangely.

Most of these races could be triggered easily when booting on SMP
machines, like the ZynqMP which has up to 4 cores. It is sufficient
for the interrupt handler to run on another core than xiic_start_xfer
and the driver fails completely.

This does not add support for long transfers, this only fixes the
driver to be usable at all instead of being completely broken.

The V2 fixes a few remaining details which cropped up in deployment
over the last year or so, so I believe the result should be reasonably
well tested.

Marek Vasut (6):
  i2c: xiic: Fix broken locking on tx_msg
  i2c: xiic: Drop broken interrupt handler
  i2c: xiic: Defer xiic_wakeup() and __xiic_start_xfer() in
    xiic_process()
  i2c: xiic: Switch from waitqueue to completion
  i2c: xiic: Only ever transfer single message
  i2c: xiic: Fix RX IRQ busy check

 drivers/i2c/busses/i2c-xiic.c | 161 +++++++++++++++-------------------
 1 file changed, 69 insertions(+), 92 deletions(-)

Cc: Michal Simek <michal.simek@xilinx.com>
Cc: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
Cc: Wolfram Sang <wsa@kernel.org>

-- 
2.32.0

[PATCH v2 1/6] i2c: xiic: Fix broken locking on tx_msg

[Marek Vasut]

The tx_msg is set from multiple places, sometimes without locking,
which fall apart on any SMP system. Only ever access tx_msg inside
the driver mutex.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Michal Simek <michal.simek@xilinx.com>
Cc: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
Cc: Wolfram Sang <wsa@kernel.org>
---
V2: Initialize RX message pointer in xiic_start_xfer() too
---
 drivers/i2c/busses/i2c-xiic.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index bb93db98404ef..50320dd32eea9 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -170,7 +170,7 @@ struct xiic_i2c {
 #define xiic_tx_space(i2c) ((i2c)->tx_msg->len - (i2c)->tx_pos)
 #define xiic_rx_space(i2c) ((i2c)->rx_msg->len - (i2c)->rx_pos)
 
-static int xiic_start_xfer(struct xiic_i2c *i2c);
+static int xiic_start_xfer(struct xiic_i2c *i2c, struct i2c_msg *msgs, int num);
 static void __xiic_start_xfer(struct xiic_i2c *i2c);
 
 /*
@@ -684,15 +684,25 @@ static void __xiic_start_xfer(struct xiic_i2c *i2c)
 
 }
 
-static int xiic_start_xfer(struct xiic_i2c *i2c)
+static int xiic_start_xfer(struct xiic_i2c *i2c, struct i2c_msg *msgs, int num)
 {
 	int ret;
+
 	mutex_lock(&i2c->lock);
 
+	ret = xiic_busy(i2c);
+	if (ret)
+		goto out;
+
+	i2c->tx_msg = msgs;
+	i2c->rx_msg = NULL;
+	i2c->nmsgs = num;
+
 	ret = xiic_reinit(i2c);
 	if (!ret)
 		__xiic_start_xfer(i2c);
 
+out:
 	mutex_unlock(&i2c->lock);
 
 	return ret;
@@ -710,14 +720,7 @@ static int xiic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 	if (err < 0)
 		return err;
 
-	err = xiic_busy(i2c);
-	if (err)
-		goto out;
-
-	i2c->tx_msg = msgs;
-	i2c->nmsgs = num;
-
-	err = xiic_start_xfer(i2c);
+	err = xiic_start_xfer(i2c, msgs, num);
 	if (err < 0) {
 		dev_err(adap->dev.parent, "Error xiic_start_xfer\n");
 		goto out;
@@ -725,9 +728,11 @@ static int xiic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 
 	if (wait_event_timeout(i2c->wait, (i2c->state == STATE_ERROR) ||
 		(i2c->state == STATE_DONE), HZ)) {
+		mutex_lock(&i2c->lock);
 		err = (i2c->state == STATE_DONE) ? num : -EIO;
 		goto out;
 	} else {
+		mutex_lock(&i2c->lock);
 		i2c->tx_msg = NULL;
 		i2c->rx_msg = NULL;
 		i2c->nmsgs = 0;
@@ -735,6 +740,7 @@ static int xiic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 		goto out;
 	}
 out:
+	mutex_unlock(&i2c->lock);
 	pm_runtime_mark_last_busy(i2c->dev);
 	pm_runtime_put_autosuspend(i2c->dev);
 	return err;
-- 
2.32.0

[PATCH v2 2/6] i2c: xiic: Drop broken interrupt handler

[Marek Vasut]

The interrupt handler is missing locking when reading out registers
and is racing with other threads which might access the driver. Drop
it altogether, so that the threaded interrupt is always executed, as
that one is already serialized by the driver mutex. This also allows
dropping local_irq_save()/local_irq_restore() in xiic_start_recv().

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Michal Simek <michal.simek@xilinx.com>
Cc: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
Cc: Wolfram Sang <wsa@kernel.org>
---
V2: No change
---
 drivers/i2c/busses/i2c-xiic.c | 25 +------------------------
 1 file changed, 1 insertion(+), 24 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 50320dd32eea9..b13166e94d894 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -554,7 +554,6 @@ static void xiic_start_recv(struct xiic_i2c *i2c)
 {
 	u8 rx_watermark;
 	struct i2c_msg *msg = i2c->rx_msg = i2c->tx_msg;
-	unsigned long flags;
 
 	/* Clear and enable Rx full interrupt. */
 	xiic_irq_clr_en(i2c, XIIC_INTR_RX_FULL_MASK | XIIC_INTR_TX_ERROR_MASK);
@@ -570,7 +569,6 @@ static void xiic_start_recv(struct xiic_i2c *i2c)
 		rx_watermark = IIC_RX_FIFO_DEPTH;
 	xiic_setreg8(i2c, XIIC_RFD_REG_OFFSET, rx_watermark - 1);
 
-	local_irq_save(flags);
 	if (!(msg->flags & I2C_M_NOSTART))
 		/* write the address */
 		xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET,
@@ -580,7 +578,6 @@ static void xiic_start_recv(struct xiic_i2c *i2c)
 
 	xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET,
 		msg->len | ((i2c->nmsgs == 1) ? XIIC_TX_DYN_STOP_MASK : 0));
-	local_irq_restore(flags);
 
 	if (i2c->nmsgs == 1)
 		/* very last, enable bus not busy as well */
@@ -620,26 +617,6 @@ static void xiic_start_send(struct xiic_i2c *i2c)
 		XIIC_INTR_BNB_MASK);
 }
 
-static irqreturn_t xiic_isr(int irq, void *dev_id)
-{
-	struct xiic_i2c *i2c = dev_id;
-	u32 pend, isr, ier;
-	irqreturn_t ret = IRQ_NONE;
-	/* Do not processes a devices interrupts if the device has no
-	 * interrupts pending
-	 */
-
-	dev_dbg(i2c->adap.dev.parent, "%s entry\n", __func__);
-
-	isr = xiic_getreg32(i2c, XIIC_IISR_OFFSET);
-	ier = xiic_getreg32(i2c, XIIC_IIER_OFFSET);
-	pend = isr & ier;
-	if (pend)
-		ret = IRQ_WAKE_THREAD;
-
-	return ret;
-}
-
 static void __xiic_start_xfer(struct xiic_i2c *i2c)
 {
 	int first = 1;
@@ -818,7 +795,7 @@ static int xiic_i2c_probe(struct platform_device *pdev)
 	pm_runtime_use_autosuspend(i2c->dev);
 	pm_runtime_set_active(i2c->dev);
 	pm_runtime_enable(i2c->dev);
-	ret = devm_request_threaded_irq(&pdev->dev, irq, xiic_isr,
+	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
 					xiic_process, IRQF_ONESHOT,
 					pdev->name, i2c);
 
-- 
2.32.0

[PATCH v2 3/6] i2c: xiic: Defer xiic_wakeup() and __xiic_start_xfer() in xiic_process()

[Marek Vasut]

The __xiic_start_xfer() manipulates the interrupt flags, xiic_wakeup()
may result in return from xiic_xfer() early. Defer both to the end of
the xiic_process() interrupt thread, so that they are executed after
all the other interrupt bits handling completed and once it completely
safe to perform changes to the interrupt bits in the hardware.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Michal Simek <michal.simek@xilinx.com>
Cc: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
Cc: Wolfram Sang <wsa@kernel.org>
---
V2: No change
---
 drivers/i2c/busses/i2c-xiic.c | 37 ++++++++++++++++++++++++-----------
 1 file changed, 26 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index b13166e94d894..aecdeec579977 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -375,6 +375,9 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
 	struct xiic_i2c *i2c = dev_id;
 	u32 pend, isr, ier;
 	u32 clr = 0;
+	int xfer_more = 0;
+	int wakeup_req = 0;
+	int wakeup_code = 0;
 
 	/* Get the interrupt Status from the IPIF. There is no clearing of
 	 * interrupts in the IPIF. Interrupts must be cleared at the source.
@@ -411,10 +414,14 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
 		 */
 		xiic_reinit(i2c);
 
-		if (i2c->rx_msg)
-			xiic_wakeup(i2c, STATE_ERROR);
-		if (i2c->tx_msg)
-			xiic_wakeup(i2c, STATE_ERROR);
+		if (i2c->rx_msg) {
+			wakeup_req = 1;
+			wakeup_code = STATE_ERROR;
+		}
+		if (i2c->tx_msg) {
+			wakeup_req = 1;
+			wakeup_code = STATE_ERROR;
+		}
 	}
 	if (pend & XIIC_INTR_RX_FULL_MASK) {
 		/* Receive register/FIFO is full */
@@ -448,8 +455,7 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
 				i2c->tx_msg++;
 				dev_dbg(i2c->adap.dev.parent,
 					"%s will start next...\n", __func__);
-
-				__xiic_start_xfer(i2c);
+				xfer_more = 1;
 			}
 		}
 	}
@@ -463,11 +469,13 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
 		if (!i2c->tx_msg)
 			goto out;
 
-		if ((i2c->nmsgs == 1) && !i2c->rx_msg &&
-			xiic_tx_space(i2c) == 0)
-			xiic_wakeup(i2c, STATE_DONE);
+		wakeup_req = 1;
+
+		if (i2c->nmsgs == 1 && !i2c->rx_msg &&
+		    xiic_tx_space(i2c) == 0)
+			wakeup_code = STATE_DONE;
 		else
-			xiic_wakeup(i2c, STATE_ERROR);
+			wakeup_code = STATE_ERROR;
 	}
 	if (pend & (XIIC_INTR_TX_EMPTY_MASK | XIIC_INTR_TX_HALF_MASK)) {
 		/* Transmit register/FIFO is empty or ½ empty */
@@ -491,7 +499,7 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
 			if (i2c->nmsgs > 1) {
 				i2c->nmsgs--;
 				i2c->tx_msg++;
-				__xiic_start_xfer(i2c);
+				xfer_more = 1;
 			} else {
 				xiic_irq_dis(i2c, XIIC_INTR_TX_HALF_MASK);
 
@@ -509,6 +517,13 @@ static irqreturn_t xiic_process(int irq, void *dev_id)
 	dev_dbg(i2c->adap.dev.parent, "%s clr: 0x%x\n", __func__, clr);
 
 	xiic_setreg32(i2c, XIIC_IISR_OFFSET, clr);
+	if (xfer_more)
+		__xiic_start_xfer(i2c);
+	if (wakeup_req)
+		xiic_wakeup(i2c, wakeup_code);
+
+	WARN_ON(xfer_more && wakeup_req);
+
 	mutex_unlock(&i2c->lock);
 	return IRQ_HANDLED;
 }
-- 
2.32.0

[PATCH v2 4/6] i2c: xiic: Switch from waitqueue to completion

[Marek Vasut]

There will never be threads queueing up in the xiic_xmit(), use
completion synchronization primitive to wait for the interrupt
handler thread to complete instead as it is much better fit and
there is no need to overload it for this purpose.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Michal Simek <michal.simek@xilinx.com>
Cc: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
Cc: Wolfram Sang <wsa@kernel.org>
---
V2: Add separate timeout for the transfer completion, since on SMP zynqmp
    it can happen that the interrupt handler thread is preempted for longer
    than a second (measurements indicate below 2 seconds most of the time).
    So add a separate timeout and make it long to give this a chance.
---
 drivers/i2c/busses/i2c-xiic.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index aecdeec579977..f7277629923ff 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -23,7 +23,7 @@
 #include <linux/platform_device.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
-#include <linux/wait.h>
+#include <linux/completion.h>
 #include <linux/platform_data/i2c-xiic.h>
 #include <linux/io.h>
 #include <linux/slab.h>
@@ -48,7 +48,7 @@ enum xiic_endian {
  * struct xiic_i2c - Internal representation of the XIIC I2C bus
  * @dev: Pointer to device structure
  * @base: Memory base of the HW registers
- * @wait: Wait queue for callers
+ * @completion:	Completion for callers
  * @adap: Kernel adapter representation
  * @tx_msg: Messages from above to be sent
  * @lock: Mutual exclusion
@@ -64,7 +64,7 @@ enum xiic_endian {
 struct xiic_i2c {
 	struct device *dev;
 	void __iomem *base;
-	wait_queue_head_t wait;
+	struct completion completion;
 	struct i2c_adapter adap;
 	struct i2c_msg *tx_msg;
 	struct mutex lock;
@@ -160,6 +160,9 @@ struct xiic_i2c {
 #define XIIC_PM_TIMEOUT		1000	/* ms */
 /* timeout waiting for the controller to respond */
 #define XIIC_I2C_TIMEOUT	(msecs_to_jiffies(1000))
+/* timeout waiting for the controller finish transfers */
+#define XIIC_XFER_TIMEOUT	(msecs_to_jiffies(10000))
+
 /*
  * The following constant is used for the device global interrupt enable
  * register, to enable all interrupts for the device, this is the only bit
@@ -367,7 +370,7 @@ static void xiic_wakeup(struct xiic_i2c *i2c, int code)
 	i2c->rx_msg = NULL;
 	i2c->nmsgs = 0;
 	i2c->state = code;
-	wake_up(&i2c->wait);
+	complete(&i2c->completion);
 }
 
 static irqreturn_t xiic_process(int irq, void *dev_id)
@@ -689,6 +692,7 @@ static int xiic_start_xfer(struct xiic_i2c *i2c, struct i2c_msg *msgs, int num)
 	i2c->tx_msg = msgs;
 	i2c->rx_msg = NULL;
 	i2c->nmsgs = num;
+	init_completion(&i2c->completion);
 
 	ret = xiic_reinit(i2c);
 	if (!ret)
@@ -715,23 +719,23 @@ static int xiic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 	err = xiic_start_xfer(i2c, msgs, num);
 	if (err < 0) {
 		dev_err(adap->dev.parent, "Error xiic_start_xfer\n");
-		goto out;
+		return err;
 	}
 
-	if (wait_event_timeout(i2c->wait, (i2c->state == STATE_ERROR) ||
-		(i2c->state == STATE_DONE), HZ)) {
-		mutex_lock(&i2c->lock);
-		err = (i2c->state == STATE_DONE) ? num : -EIO;
-		goto out;
-	} else {
-		mutex_lock(&i2c->lock);
+	err = wait_for_completion_timeout(&i2c->completion, XIIC_XFER_TIMEOUT);
+	mutex_lock(&i2c->lock);
+	if (err == 0) {	/* Timeout */
 		i2c->tx_msg = NULL;
 		i2c->rx_msg = NULL;
 		i2c->nmsgs = 0;
 		err = -ETIMEDOUT;
-		goto out;
+	} else if (err < 0) {	/* Completion error */
+		i2c->tx_msg = NULL;
+		i2c->rx_msg = NULL;
+		i2c->nmsgs = 0;
+	} else {
+		err = (i2c->state == STATE_DONE) ? num : -EIO;
 	}
-out:
 	mutex_unlock(&i2c->lock);
 	pm_runtime_mark_last_busy(i2c->dev);
 	pm_runtime_put_autosuspend(i2c->dev);
@@ -793,7 +797,6 @@ static int xiic_i2c_probe(struct platform_device *pdev)
 	i2c->adap.dev.of_node = pdev->dev.of_node;
 
 	mutex_init(&i2c->lock);
-	init_waitqueue_head(&i2c->wait);
 
 	i2c->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(i2c->clk))
-- 
2.32.0

[PATCH v2 5/6] i2c: xiic: Only ever transfer single message

[Marek Vasut]

Transferring multiple messages via XIIC suffers from strange interaction
between the interrupt status/enable register flags. These flags are being
reused in the hardware to indicate different things for read and write
transfer, and doing multiple transactions becomes horribly complex. Just
send a single transaction and reload the controller with another message
once the transaction is done in the interrupt handler thread.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Michal Simek <michal.simek@xilinx.com>
Cc: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
Cc: Wolfram Sang <wsa@kernel.org>
---
V2: No change
---
 drivers/i2c/busses/i2c-xiic.c | 44 ++++++++---------------------------
 1 file changed, 10 insertions(+), 34 deletions(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index f7277629923ff..6cd7830fe4898 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -609,8 +609,6 @@ static void xiic_start_send(struct xiic_i2c *i2c)
 {
 	struct i2c_msg *msg = i2c->tx_msg;
 
-	xiic_irq_clr(i2c, XIIC_INTR_TX_ERROR_MASK);
-
 	dev_dbg(i2c->adap.dev.parent, "%s entry, msg: %p, len: %d",
 		__func__, msg, msg->len);
 	dev_dbg(i2c->adap.dev.parent, "%s entry, ISR: 0x%x, CR: 0x%x\n",
@@ -628,16 +626,17 @@ static void xiic_start_send(struct xiic_i2c *i2c)
 		xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, data);
 	}
 
-	xiic_fill_tx_fifo(i2c);
-
 	/* Clear any pending Tx empty, Tx Error and then enable them. */
 	xiic_irq_clr_en(i2c, XIIC_INTR_TX_EMPTY_MASK | XIIC_INTR_TX_ERROR_MASK |
-		XIIC_INTR_BNB_MASK);
+		XIIC_INTR_BNB_MASK |
+		((i2c->nmsgs > 1 || xiic_tx_space(i2c)) ?
+			XIIC_INTR_TX_HALF_MASK : 0));
+
+	xiic_fill_tx_fifo(i2c);
 }
 
 static void __xiic_start_xfer(struct xiic_i2c *i2c)
 {
-	int first = 1;
 	int fifo_space = xiic_tx_fifo_space(i2c);
 	dev_dbg(i2c->adap.dev.parent, "%s entry, msg: %p, fifos space: %d\n",
 		__func__, i2c->tx_msg, fifo_space);
@@ -648,35 +647,12 @@ static void __xiic_start_xfer(struct xiic_i2c *i2c)
 	i2c->rx_pos = 0;
 	i2c->tx_pos = 0;
 	i2c->state = STATE_START;
-	while ((fifo_space >= 2) && (first || (i2c->nmsgs > 1))) {
-		if (!first) {
-			i2c->nmsgs--;
-			i2c->tx_msg++;
-			i2c->tx_pos = 0;
-		} else
-			first = 0;
-
-		if (i2c->tx_msg->flags & I2C_M_RD) {
-			/* we dont date putting several reads in the FIFO */
-			xiic_start_recv(i2c);
-			return;
-		} else {
-			xiic_start_send(i2c);
-			if (xiic_tx_space(i2c) != 0) {
-				/* the message could not be completely sent */
-				break;
-			}
-		}
-
-		fifo_space = xiic_tx_fifo_space(i2c);
+	if (i2c->tx_msg->flags & I2C_M_RD) {
+		/* we dont date putting several reads in the FIFO */
+		xiic_start_recv(i2c);
+	} else {
+		xiic_start_send(i2c);
 	}
-
-	/* there are more messages or the current one could not be completely
-	 * put into the FIFO, also enable the half empty interrupt
-	 */
-	if (i2c->nmsgs > 1 || xiic_tx_space(i2c))
-		xiic_irq_clr_en(i2c, XIIC_INTR_TX_HALF_MASK);
-
 }
 
 static int xiic_start_xfer(struct xiic_i2c *i2c, struct i2c_msg *msgs, int num)
-- 
2.32.0

[PATCH v2 6/6] i2c: xiic: Fix RX IRQ busy check

[Marek Vasut]

In case the XIIC does TX/RX transfer, make sure no other kernel thread
can start another TX transfer at the same time. This could happen since
the driver only checks tx_msg for being non-NULL and returns -EBUSY in
that case, however it is necessary to check also rx_msg for the same.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Michal Simek <michal.simek@xilinx.com>
Cc: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
Cc: Wolfram Sang <wsa@kernel.org>
---
V2: New patch
---
 drivers/i2c/busses/i2c-xiic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
index 6cd7830fe4898..eb789cfb99739 100644
--- a/drivers/i2c/busses/i2c-xiic.c
+++ b/drivers/i2c/busses/i2c-xiic.c
@@ -543,7 +543,7 @@ static int xiic_busy(struct xiic_i2c *i2c)
 	int tries = 3;
 	int err;
 
-	if (i2c->tx_msg)
+	if (i2c->tx_msg || i2c->rx_msg)
 		return -EBUSY;
 
 	/* In single master mode bus can only be busy, when in use by this
-- 
2.32.0

Re: [PATCH v2 0/6] i2c: xiic: Fix broken locking

[Michal Simek]

+ravi

On 8/23/21 11:41 PM, Marek Vasut wrote:
> Booting ZynqMP with XIIC I2C driver shows multitude of race conditions
> in the XIIC driver. This is because locking is completely missing from
> the driver, and there are odd corner cases where the hardware behaves
> strangely.
> 
> Most of these races could be triggered easily when booting on SMP
> machines, like the ZynqMP which has up to 4 cores. It is sufficient
> for the interrupt handler to run on another core than xiic_start_xfer
> and the driver fails completely.
> 
> This does not add support for long transfers, this only fixes the
> driver to be usable at all instead of being completely broken.
> 
> The V2 fixes a few remaining details which cropped up in deployment
> over the last year or so, so I believe the result should be reasonably
> well tested.
> 
> Marek Vasut (6):
>   i2c: xiic: Fix broken locking on tx_msg
>   i2c: xiic: Drop broken interrupt handler
>   i2c: xiic: Defer xiic_wakeup() and __xiic_start_xfer() in
>     xiic_process()
>   i2c: xiic: Switch from waitqueue to completion
>   i2c: xiic: Only ever transfer single message
>   i2c: xiic: Fix RX IRQ busy check
> 
>  drivers/i2c/busses/i2c-xiic.c | 161 +++++++++++++++-------------------
>  1 file changed, 69 insertions(+), 92 deletions(-)
> 
> Cc: Michal Simek <michal.simek@xilinx.com>
> Cc: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> Cc: Wolfram Sang <wsa@kernel.org>
> 

RE: [PATCH v2 0/6] i2c: xiic: Fix broken locking

[Raviteja Narayanam]

> -----Original Message-----
> From: Michal Simek <michal.simek@xilinx.com>
> Sent: Tuesday, August 24, 2021 12:29 PM
> To: Marek Vasut <marex@denx.de>; linux-i2c@vger.kernel.org; Raviteja
> Narayanam <rna@xilinx.com>
> Cc: Michal Simek <michals@xilinx.com>; Shubhrajyoti Datta
> <shubhraj@xilinx.com>; Wolfram Sang <wsa@kernel.org>
> Subject: Re: [PATCH v2 0/6] i2c: xiic: Fix broken locking
> 
> +ravi
> 
> On 8/23/21 11:41 PM, Marek Vasut wrote:
> > Booting ZynqMP with XIIC I2C driver shows multitude of race conditions
> > in the XIIC driver. This is because locking is completely missing from
> > the driver, and there are odd corner cases where the hardware behaves
> > strangely.
> >
> > Most of these races could be triggered easily when booting on SMP
> > machines, like the ZynqMP which has up to 4 cores. It is sufficient
> > for the interrupt handler to run on another core than xiic_start_xfer
> > and the driver fails completely.
> >
> > This does not add support for long transfers, this only fixes the
> > driver to be usable at all instead of being completely broken.
> >
> > The V2 fixes a few remaining details which cropped up in deployment
> > over the last year or so, so I believe the result should be reasonably
> > well tested.

Thanks a lot for the patches, Marek. 
I have tested these on our boards and they are working fine. 

I will rebase my patch series on top of this and send after rc1. 

> >
> > Marek Vasut (6):
> >   i2c: xiic: Fix broken locking on tx_msg
> >   i2c: xiic: Drop broken interrupt handler
> >   i2c: xiic: Defer xiic_wakeup() and __xiic_start_xfer() in
> >     xiic_process()
> >   i2c: xiic: Switch from waitqueue to completion
> >   i2c: xiic: Only ever transfer single message
> >   i2c: xiic: Fix RX IRQ busy check
> >
> >  drivers/i2c/busses/i2c-xiic.c | 161
> > +++++++++++++++-------------------
> >  1 file changed, 69 insertions(+), 92 deletions(-)
> >
> > Cc: Michal Simek <michal.simek@xilinx.com>
> > Cc: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> > Cc: Wolfram Sang <wsa@kernel.org>
> >

Re: [PATCH v2 0/6] i2c: xiic: Fix broken locking

[Michal Simek]

On 8/27/21 10:31 AM, Raviteja Narayanam wrote:
> 
> 
>> -----Original Message-----
>> From: Michal Simek <michal.simek@xilinx.com>
>> Sent: Tuesday, August 24, 2021 12:29 PM
>> To: Marek Vasut <marex@denx.de>; linux-i2c@vger.kernel.org; Raviteja
>> Narayanam <rna@xilinx.com>
>> Cc: Michal Simek <michals@xilinx.com>; Shubhrajyoti Datta
>> <shubhraj@xilinx.com>; Wolfram Sang <wsa@kernel.org>
>> Subject: Re: [PATCH v2 0/6] i2c: xiic: Fix broken locking
>>
>> +ravi
>>
>> On 8/23/21 11:41 PM, Marek Vasut wrote:
>>> Booting ZynqMP with XIIC I2C driver shows multitude of race conditions
>>> in the XIIC driver. This is because locking is completely missing from
>>> the driver, and there are odd corner cases where the hardware behaves
>>> strangely.
>>>
>>> Most of these races could be triggered easily when booting on SMP
>>> machines, like the ZynqMP which has up to 4 cores. It is sufficient
>>> for the interrupt handler to run on another core than xiic_start_xfer
>>> and the driver fails completely.
>>>
>>> This does not add support for long transfers, this only fixes the
>>> driver to be usable at all instead of being completely broken.
>>>
>>> The V2 fixes a few remaining details which cropped up in deployment
>>> over the last year or so, so I believe the result should be reasonably
>>> well tested.
> 
> Thanks a lot for the patches, Marek. 
> I have tested these on our boards and they are working fine. 
> 
> I will rebase my patch series on top of this and send after rc1. 

Wolfram: Can you please merge this series? Ravi's series will come on
the top of this one.

Acked-by: Michal Simek <michal.simek@xilinx.com>

Thanks,
Michal

Re: [PATCH v2 0/6] i2c: xiic: Fix broken locking

[Wolfram Sang]

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

On Mon, Aug 23, 2021 at 11:41:39PM +0200, Marek Vasut wrote:
> Booting ZynqMP with XIIC I2C driver shows multitude of race conditions
> in the XIIC driver. This is because locking is completely missing from
> the driver, and there are odd corner cases where the hardware behaves
> strangely.
> 
> Most of these races could be triggered easily when booting on SMP
> machines, like the ZynqMP which has up to 4 cores. It is sufficient
> for the interrupt handler to run on another core than xiic_start_xfer
> and the driver fails completely.
> 
> This does not add support for long transfers, this only fixes the
> driver to be usable at all instead of being completely broken.
> 
> The V2 fixes a few remaining details which cropped up in deployment
> over the last year or so, so I believe the result should be reasonably
> well tested.
> 
> Marek Vasut (6):
>   i2c: xiic: Fix broken locking on tx_msg
>   i2c: xiic: Drop broken interrupt handler
>   i2c: xiic: Defer xiic_wakeup() and __xiic_start_xfer() in
>     xiic_process()
>   i2c: xiic: Switch from waitqueue to completion
>   i2c: xiic: Only ever transfer single message
>   i2c: xiic: Fix RX IRQ busy check
> 

Applied to for-next, thanks everyone!


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

Re: [PATCH v2 0/6] i2c: xiic: Fix broken locking

[Michal Simek]

On 9/14/21 12:29 PM, Wolfram Sang wrote:
> On Mon, Aug 23, 2021 at 11:41:39PM +0200, Marek Vasut wrote:
>> Booting ZynqMP with XIIC I2C driver shows multitude of race conditions
>> in the XIIC driver. This is because locking is completely missing from
>> the driver, and there are odd corner cases where the hardware behaves
>> strangely.
>>
>> Most of these races could be triggered easily when booting on SMP
>> machines, like the ZynqMP which has up to 4 cores. It is sufficient
>> for the interrupt handler to run on another core than xiic_start_xfer
>> and the driver fails completely.
>>
>> This does not add support for long transfers, this only fixes the
>> driver to be usable at all instead of being completely broken.
>>
>> The V2 fixes a few remaining details which cropped up in deployment
>> over the last year or so, so I believe the result should be reasonably
>> well tested.
>>
>> Marek Vasut (6):
>>   i2c: xiic: Fix broken locking on tx_msg
>>   i2c: xiic: Drop broken interrupt handler
>>   i2c: xiic: Defer xiic_wakeup() and __xiic_start_xfer() in
>>     xiic_process()
>>   i2c: xiic: Switch from waitqueue to completion
>>   i2c: xiic: Only ever transfer single message
>>   i2c: xiic: Fix RX IRQ busy check
>>
> 
> Applied to for-next, thanks everyone!
> 

Thx.
FYI: Ravi will be sending his series on the top of this one.

Thanks,
Michal