[PATCH 0/3] i2c: qcom-geni: Add I2C bus recovery support

[PATCH 0/3] i2c: qcom-geni: Add I2C bus recovery support

[Aniket Randive]

An I2C target that gets interrupted during a transaction can hold SDA low,
leaving the bus stuck and preventing further transfers.

This series adds bus recovery support to the Qualcomm GENI I2C driver using
the hardware native I2C_BUS_CLEAR and I2C_STOP_ON_BUS M_CMD opcodes. These
commands generate up to nine SCL pulses, which is the standard I2C bus
recovery sequence, and then issue a STOP condition to release the bus
without requiring GPIO based recovery.

Recovery is triggered automatically from geni_i2c_xfer() when a transfer
fails and the SE_GENI_IOS register indicates that SDA remains low. The
recovery callback is also registered through i2c_bus_recovery_info, allowing
recovery to be initiated through the I2C core i2c_recover_bus() API.

The patches are organized as follows:

  [1/3] dmaengine: qcom: gpi: Add I2C_BUS_CLEAR and I2C_STOP_ON_BUS opcodes
  [2/3] i2c: qcom-geni: Add bus recovery support for FIFO mode
  [3/3] i2c: qcom-geni: Add bus recovery support for GPI DMA mode

Signed-off-by: Aniket Randive <aniket.randive@oss.qualcomm.com>
---
Aniket Randive (3):
      dmaengine: qcom: gpi: Add I2C bus recovery opcode support
      i2c: qcom-geni: Add bus recovery support for FIFO mode
      i2c: qcom-geni: Add bus recovery support for GPI DMA mode

 drivers/dma/qcom/gpi.c             |  30 ++++++
 drivers/i2c/busses/i2c-qcom-geni.c | 185 ++++++++++++++++++++++++++++++++++++-
 include/linux/dma/qcom-gpi-dma.h   |   2 +
 3 files changed, 215 insertions(+), 2 deletions(-)
---
base-commit: a8406e6c0b793ce0788019683837c40855b55995
change-id: 20260706-i2c_bus_recovery-8f0c20283d99

Best regards,
--  
Aniket Randive <aniket.randive@oss.qualcomm.com>

[PATCH v1 1/3] dmaengine: qcom: gpi: Add I2C bus recovery opcode support

[Aniket Randive]

The I2C_BUS_CLEAR and I2C_STOP_ON_BUS GENI sequencer opcodes release a
stuck I2C bus, but the GPI DMA driver has no way to emit these
zero-payload control commands, so an I2C controller using GPI DMA
cannot recover the bus.

Add I2C_BUS_CLEAR and I2C_STOP_ON_BUS to enum i2c_op so I2C drivers can
request them through gpi_i2c_config.op. Handle them in
gpi_create_i2c_tre() by emitting a CONFIG TRE (when set_config is set)
followed by a GO TRE with IEOT set and no DMA TRE, and reserve the
matching number of ring entries in gpi_prep_slave_sg().

Signed-off-by: Aniket Randive <aniket.randive@oss.qualcomm.com>
---
 drivers/dma/qcom/gpi.c           | 30 ++++++++++++++++++++++++++++++
 include/linux/dma/qcom-gpi-dma.h |  2 ++
 2 files changed, 32 insertions(+)

diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
index a5055a6273af..14a74f0a602d 100644
--- a/drivers/dma/qcom/gpi.c
+++ b/drivers/dma/qcom/gpi.c
@@ -1620,6 +1620,17 @@ gpi_peripheral_config(struct dma_chan *chan, struct dma_slave_config *config)
 	return 0;
 }
 
+static void gpi_create_i2c_go_recovery_tre(struct gpi_i2c_config *i2c,
+					   struct gpi_tre *tre)
+{
+	tre->dword[0] = u32_encode_bits(i2c->op, TRE_I2C_GO_CMD);
+	tre->dword[1] = 0;
+	tre->dword[2] = 0;
+
+	tre->dword[3] = u32_encode_bits(TRE_TYPE_GO, TRE_FLAGS_TYPE);
+	tre->dword[3] |= u32_encode_bits(1, TRE_FLAGS_IEOT);
+}
+
 static int gpi_create_i2c_tre(struct gchan *chan, struct gpi_desc *desc,
 			      struct scatterlist *sgl, enum dma_transfer_direction direction,
 			      unsigned long flags)
@@ -1650,6 +1661,12 @@ static int gpi_create_i2c_tre(struct gchan *chan, struct gpi_desc *desc,
 		tre->dword[3] |= u32_encode_bits(1, TRE_FLAGS_CHAIN);
 	}
 
+	if (i2c->op == I2C_BUS_CLEAR || i2c->op == I2C_STOP_ON_BUS) {
+		gpi_create_i2c_go_recovery_tre(i2c, &desc->tre[tre_idx]);
+		tre_idx++;
+		goto log_tre;
+	}
+
 	/* create the GO tre for Tx */
 	if (i2c->op == I2C_WRITE) {
 		tre = &desc->tre[tre_idx];
@@ -1692,6 +1709,7 @@ static int gpi_create_i2c_tre(struct gchan *chan, struct gpi_desc *desc,
 			tre->dword[3] |= u32_encode_bits(1, TRE_FLAGS_BEI);
 	}
 
+log_tre:
 	for (i = 0; i < tre_idx; i++)
 		dev_dbg(dev, "TRE:%d %x:%x:%x:%x\n", i, desc->tre[i].dword[0],
 			desc->tre[i].dword[1], desc->tre[i].dword[2], desc->tre[i].dword[3]);
@@ -1825,6 +1843,18 @@ gpi_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 		nr_tre = 2;
 	if (direction == DMA_DEV_TO_MEM) /* rx */
 		nr_tre = 1;
+	/*
+	 * Recovery opcodes do not require DMA data TREs, only CONFIG
+	 * (for set_config) and GO TREs. Since gpi_prep_slave_sg() is
+	 * shared with SPI, verify the channel is I2C before accessing
+	 * the configuration data.
+	 */
+	if (gchan->protocol == QCOM_GPI_I2C) {
+		struct gpi_i2c_config *i2c = gchan->config;
+
+		if (i2c->op == I2C_BUS_CLEAR || i2c->op == I2C_STOP_ON_BUS)
+			nr_tre = set_config ? 2 : 1;
+	}
 
 	/* calculate # of elements required & available */
 	nr = gpi_ring_num_elements_avail(ch_ring);
diff --git a/include/linux/dma/qcom-gpi-dma.h b/include/linux/dma/qcom-gpi-dma.h
index 332be28427e4..b9b75c302825 100644
--- a/include/linux/dma/qcom-gpi-dma.h
+++ b/include/linux/dma/qcom-gpi-dma.h
@@ -52,6 +52,8 @@ struct gpi_spi_config {
 enum i2c_op {
 	I2C_WRITE = 1,
 	I2C_READ,
+	I2C_BUS_CLEAR = 6,
+	I2C_STOP_ON_BUS,
 };
 
 /**

-- 
2.34.1

[PATCH v1 2/3] i2c: qcom-geni: Add bus recovery support for FIFO mode

[Aniket Randive]

I2C transfers in FIFO mode can fail with -EPROTO, -ETIMEDOUT or
-EAGAIN when a target holds SDA low, leaving the bus stuck and
preventing subsequent transactions.

Add bus recovery support using the I2C_BUS_CLEAR and I2C_STOP_ON_BUS
hardware opcodes to restore the bus to an idle state. Check the SDA
line state through SE_GENI_IOS.RX_DATA_IN and skip recovery when the
bus is already free. Trigger recovery automatically from
geni_i2c_xfer() on -EPROTO, -ETIMEDOUT and -EAGAIN (arbitration-lost)
errors, and register the recovery callback through i2c_bus_recovery_info
to allow recovery via i2c_recover_bus().

This adds bus recovery support for FIFO mode only.

Signed-off-by: Aniket Randive <aniket.randive@oss.qualcomm.com>
---
 drivers/i2c/busses/i2c-qcom-geni.c | 109 +++++++++++++++++++++++++++++++++++++
 1 file changed, 109 insertions(+)

diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index 658636c1ee0e..9fa1a8ac400c 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -138,6 +138,7 @@ struct geni_i2c_dev {
 	u32 num_msgs;
 	struct geni_i2c_gpi_multi_desc_xfer i2c_multi_desc_config;
 	const struct geni_i2c_desc *dev_data;
+	struct i2c_bus_recovery_info rinfo;
 };
 
 struct geni_i2c_err_log {
@@ -956,6 +957,90 @@ static int geni_i2c_fifo_xfer(struct geni_i2c_dev *gi2c,
 	return num;
 }
 
+static int geni_i2c_fifo_bus_cmd(struct geni_i2c_dev *gi2c, u32 cmd)
+{
+	unsigned long time_left;
+	unsigned long flags;
+
+	/*
+	 * Clear cur so the IRQ handler does not attempt FIFO watermark
+	 * filling or draining while the recovery opcode is in flight.
+	 * cur and err are shared with geni_i2c_irq(), which reads cur and
+	 * writes err under gi2c->lock, so take the lock around this reset.
+	 */
+	spin_lock_irqsave(&gi2c->lock, flags);
+	gi2c->cur = NULL;
+	gi2c->err = 0;
+	spin_unlock_irqrestore(&gi2c->lock, flags);
+	geni_se_select_mode(&gi2c->se, GENI_SE_FIFO);
+	reinit_completion(&gi2c->done);
+
+	geni_se_setup_m_cmd(&gi2c->se, cmd, 0);
+	time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT);
+	if (!time_left) {
+		dev_err(gi2c->se.dev, "timeout waiting for bus cmd %u\n", cmd);
+		gi2c->abort_done = false;
+		geni_se_abort_m_cmd(&gi2c->se);
+		time_left = ABORT_TIMEOUT;
+		do {
+			time_left = wait_for_completion_timeout(&gi2c->done, time_left);
+		} while (!gi2c->abort_done && time_left);
+
+		if (!time_left)
+			dev_err(gi2c->se.dev, "abort timed out for bus cmd %u\n", cmd);
+
+		return -ETIMEDOUT;
+	}
+
+	/*
+	 * ARB_LOST and BUS_PROTO interrupts may be reported while the bus
+	 * transitions from stuck to idle during the recovery sequence.
+	 * The opcode completed successfully so treat these as success.
+	 */
+	if (gi2c->err == -EAGAIN || gi2c->err == -EPROTO)
+		return 0;
+
+	return gi2c->err;
+}
+
+static int geni_i2c_recover_bus(struct i2c_adapter *adap)
+{
+	struct geni_i2c_dev *gi2c = i2c_get_adapdata(adap);
+	int ret;
+
+	ret = pm_runtime_get_sync(gi2c->se.dev);
+	if (ret < 0) {
+		dev_err(gi2c->se.dev, "bus recovery failed, error turning SE resources:%d\n", ret);
+		pm_runtime_put_noidle(gi2c->se.dev);
+		return ret;
+	}
+
+	/* SDA is high means bus is free */
+	if (readl_relaxed(gi2c->se.base + SE_GENI_IOS) & RX_DATA_IN) {
+		pm_runtime_put_autosuspend(gi2c->se.dev);
+		return 0;
+	}
+
+	ret = geni_i2c_fifo_bus_cmd(gi2c, I2C_BUS_CLEAR);
+	if (!ret)
+		ret = geni_i2c_fifo_bus_cmd(gi2c, I2C_STOP_ON_BUS);
+
+	/*
+	 * Recovery succeeds only once the slave releases SDA, so the bus
+	 * state is the authority: RX_DATA_IN high means the bus is free,
+	 * while a clean opcode status with SDA still low is a failed
+	 * recovery.
+	 */
+	if (readl_relaxed(gi2c->se.base + SE_GENI_IOS) & RX_DATA_IN)
+		ret = 0;
+	else if (!ret)
+		ret = -EBUSY;
+
+	pm_runtime_put_autosuspend(gi2c->se.dev);
+
+	return ret;
+}
+
 static int geni_i2c_xfer(struct i2c_adapter *adap,
 			 struct i2c_msg msgs[],
 			 int num)
@@ -981,6 +1066,25 @@ static int geni_i2c_xfer(struct i2c_adapter *adap,
 	else
 		ret = geni_i2c_fifo_xfer(gi2c, msgs, num);
 
+	if (!gi2c->gpi_mode &&
+	    (ret == -EPROTO || ret == -ETIMEDOUT || ret == -EAGAIN)) {
+		/*
+		 * Only attempt recovery if SDA is stuck low. -EPROTO and
+		 * -ETIMEDOUT indicate bus errors where the target may be
+		 * holding SDA low. ARB_LOST (-EAGAIN) on a single-controller
+		 * bus indicates a stuck target, not a real arbitration loss.
+		 * GPI DMA mode extends this trigger separately.
+		 */
+		if (!(readl_relaxed(gi2c->se.base + SE_GENI_IOS) & RX_DATA_IN)) {
+			int recovery_ret = i2c_recover_bus(adap);
+
+			if (recovery_ret)
+				dev_err(gi2c->se.dev,
+					"bus recovery failed: %d (xfer error: %d)\n",
+					recovery_ret, ret);
+		}
+	}
+
 	pm_runtime_put_autosuspend(gi2c->se.dev);
 	gi2c->cur = NULL;
 	gi2c->err = 0;
@@ -1188,6 +1292,11 @@ static int geni_i2c_probe(struct platform_device *pdev)
 	if (ret < 0)
 		return ret;
 
+	if (!gi2c->gpi_mode) {
+		gi2c->rinfo.recover_bus = geni_i2c_recover_bus;
+		gi2c->adap.bus_recovery_info = &gi2c->rinfo;
+	}
+
 	ret = i2c_add_adapter(&gi2c->adap);
 	if (ret)
 		return dev_err_probe(dev, ret, "Error adding i2c adapter\n");

-- 
2.34.1

Re: [PATCH v1 2/3] i2c: qcom-geni: Add bus recovery support for FIFO mode

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Compilation error and broken synchronization logic in the command abort path.

Pre-existing issues:
- [High] A NACK interrupt during bus recovery causes a fatal NULL pointer dereference in the interrupt handler.
--

commit de102ddbb8af25ab8831cc82b3f8ba2ab217ef7a
Author: Aniket Randive <aniket.randive@oss.qualcomm.com>

i2c: qcom-geni: Add bus recovery support for FIFO mode

This commit adds bus recovery support using I2C_BUS_CLEAR and
I2C_STOP_ON_BUS hardware opcodes to restore a stuck bus to an idle
state in FIFO mode. It automatically triggers recovery on bus errors
and registers a recovery callback via i2c_bus_recovery_info.

> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c

[ ... ]

> @@ -956,6 +957,90 @@ static int geni_i2c_fifo_xfer(struct geni_i2c_dev *gi2c,
>  	return num;
>  }
>  
> +static int geni_i2c_fifo_bus_cmd(struct geni_i2c_dev *gi2c, u32 cmd)
> +{
> +	unsigned long time_left;
> +	unsigned long flags;
> +
> +	/*
> +	 * Clear cur so the IRQ handler does not attempt FIFO watermark
> +	 * filling or draining while the recovery opcode is in flight.
> +	 * cur and err are shared with geni_i2c_irq(), which reads cur and
> +	 * writes err under gi2c->lock, so take the lock around this reset.
> +	 */
> +	spin_lock_irqsave(&gi2c->lock, flags);
> +	gi2c->cur = NULL;
> +	gi2c->err = 0;
> +	spin_unlock_irqrestore(&gi2c->lock, flags);

[Severity: High]
This is a pre-existing issue, but by explicitly clearing gi2c->cur here,
can a NACK interrupt during bus recovery cause a NULL pointer dereference
in the interrupt handler?

If a NACK interrupt occurs during this sequence, geni_i2c_irq() routes
it to geni_i2c_check_addr_data_nack() which unconditionally checks the
flags on the current message:

drivers/i2c/busses/i2c-qcom-geni.c:geni_i2c_check_addr_data_nack() {
    ...
    else if (!(gi2c->cur->flags & I2C_M_RD))
        geni_i2c_err(gi2c, DATA_NACK);
    ...
}

> +	geni_se_select_mode(&gi2c->se, GENI_SE_FIFO);
> +	reinit_completion(&gi2c->done);
> +
> +	geni_se_setup_m_cmd(&gi2c->se, cmd, 0);
> +	time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT);
> +	if (!time_left) {
> +		dev_err(gi2c->se.dev, "timeout waiting for bus cmd %u\n", cmd);
> +		gi2c->abort_done = false;
> +		geni_se_abort_m_cmd(&gi2c->se);
> +		time_left = ABORT_TIMEOUT;
> +		do {
> +			time_left = wait_for_completion_timeout(&gi2c->done, time_left);
> +		} while (!gi2c->abort_done && time_left);

[Severity: High]
Is gi2c->abort_done declared as a struct completion? If so, assigning false
to it and checking it with a logical NOT will cause a compilation error.

Additionally, does this wait on the correct completion variable? The loop
calls wait_for_completion_timeout(&gi2c->done, time_left) but then checks
!gi2c->abort_done. It appears it should wait on &gi2c->abort_done instead.

Lastly, does the hardware abort command geni_se_abort_m_cmd() need to be
issued while holding gi2c->lock to prevent racing with the concurrent
interrupt handler?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826-i2c_bus_recovery-v1-0-203f0ec76f84@oss.qualcomm.com?part=2

Re: [PATCH v1 2/3] i2c: qcom-geni: Add bus recovery support for FIFO mode

[Jyothi Kumar Seerapu]

On 8/26/2026 3:21 PM, Aniket Randive wrote:
> I2C transfers in FIFO mode can fail with -EPROTO, -ETIMEDOUT or
> -EAGAIN when a target holds SDA low, leaving the bus stuck and
> preventing subsequent transactions.
> 
> Add bus recovery support using the I2C_BUS_CLEAR and I2C_STOP_ON_BUS
> hardware opcodes to restore the bus to an idle state. Check the SDA
> line state through SE_GENI_IOS.RX_DATA_IN and skip recovery when the
> bus is already free. Trigger recovery automatically from
> geni_i2c_xfer() on -EPROTO, -ETIMEDOUT and -EAGAIN (arbitration-lost)
> errors, and register the recovery callback through i2c_bus_recovery_info
> to allow recovery via i2c_recover_bus().
> 
> This adds bus recovery support for FIFO mode only.
> 
> Signed-off-by: Aniket Randive <aniket.randive@oss.qualcomm.com>
> ---
>   drivers/i2c/busses/i2c-qcom-geni.c | 109 +++++++++++++++++++++++++++++++++++++
>   1 file changed, 109 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 658636c1ee0e..9fa1a8ac400c 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -138,6 +138,7 @@ struct geni_i2c_dev {
>   	u32 num_msgs;
>   	struct geni_i2c_gpi_multi_desc_xfer i2c_multi_desc_config;
>   	const struct geni_i2c_desc *dev_data;
> +	struct i2c_bus_recovery_info rinfo;
>   };
>   
>   struct geni_i2c_err_log {
> @@ -956,6 +957,90 @@ static int geni_i2c_fifo_xfer(struct geni_i2c_dev *gi2c,
>   	return num;
>   }
>   
> +static int geni_i2c_fifo_bus_cmd(struct geni_i2c_dev *gi2c, u32 cmd)
> +{
> +	unsigned long time_left;
> +	unsigned long flags;
> +
> +	/*
> +	 * Clear cur so the IRQ handler does not attempt FIFO watermark
> +	 * filling or draining while the recovery opcode is in flight.
> +	 * cur and err are shared with geni_i2c_irq(), which reads cur and
> +	 * writes err under gi2c->lock, so take the lock around this reset.
> +	 */
> +	spin_lock_irqsave(&gi2c->lock, flags);
> +	gi2c->cur = NULL;
> +	gi2c->err = 0;
> +	spin_unlock_irqrestore(&gi2c->lock, flags);
> +	geni_se_select_mode(&gi2c->se, GENI_SE_FIFO);
> +	reinit_completion(&gi2c->done);
> +
> +	geni_se_setup_m_cmd(&gi2c->se, cmd, 0);
> +	time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT);
> +	if (!time_left) {
> +		dev_err(gi2c->se.dev, "timeout waiting for bus cmd %u\n", cmd);
> +		gi2c->abort_done = false;
> +		geni_se_abort_m_cmd(&gi2c->se);
> +		time_left = ABORT_TIMEOUT;
> +		do {
> +			time_left = wait_for_completion_timeout(&gi2c->done, time_left);
> +		} while (!gi2c->abort_done && time_left);
> +
> +		if (!time_left)
> +			dev_err(gi2c->se.dev, "abort timed out for bus cmd %u\n", cmd);
> +
> +		return -ETIMEDOUT;
> +	}
> +
> +	/*
> +	 * ARB_LOST and BUS_PROTO interrupts may be reported while the bus
> +	 * transitions from stuck to idle during the recovery sequence.
> +	 * The opcode completed successfully so treat these as success.
> +	 */
> +	if (gi2c->err == -EAGAIN || gi2c->err == -EPROTO)
> +		return 0;
> +
> +	return gi2c->err;
> +}
> +
> +static int geni_i2c_recover_bus(struct i2c_adapter *adap)
> +{
> +	struct geni_i2c_dev *gi2c = i2c_get_adapdata(adap);
> +	int ret;
> +
> +	ret = pm_runtime_get_sync(gi2c->se.dev);
> +	if (ret < 0) {
> +		dev_err(gi2c->se.dev, "bus recovery failed, error turning SE resources:%d\n", ret);
> +		pm_runtime_put_noidle(gi2c->se.dev);
> +		return ret;
> +	}
> +
> +	/* SDA is high means bus is free */
What about SCL line ?> +	if (readl_relaxed(gi2c->se.base + SE_GENI_IOS) 
& RX_DATA_IN) {
> +		pm_runtime_put_autosuspend(gi2c->se.dev);
> +		return 0;
> +	}
> +
> +	ret = geni_i2c_fifo_bus_cmd(gi2c, I2C_BUS_CLEAR);
> +	if (!ret)
> +		ret = geni_i2c_fifo_bus_cmd(gi2c, I2C_STOP_ON_BUS);
> +
> +	/*
> +	 * Recovery succeeds only once the slave releases SDA, so the bus
> +	 * state is the authority: RX_DATA_IN high means the bus is free,
> +	 * while a clean opcode status with SDA still low is a failed
> +	 * recovery.
> +	 */
> +	if (readl_relaxed(gi2c->se.base + SE_GENI_IOS) & RX_DATA_IN)
> +		ret = 0;
> +	else if (!ret)
> +		ret = -EBUSY;
> +
> +	pm_runtime_put_autosuspend(gi2c->se.dev);
> +
> +	return ret;
> +}
> +
>   static int geni_i2c_xfer(struct i2c_adapter *adap,
>   			 struct i2c_msg msgs[],
>   			 int num)
> @@ -981,6 +1066,25 @@ static int geni_i2c_xfer(struct i2c_adapter *adap,
>   	else
>   		ret = geni_i2c_fifo_xfer(gi2c, msgs, num);
>   
> +	if (!gi2c->gpi_mode &&
> +	    (ret == -EPROTO || ret == -ETIMEDOUT || ret == -EAGAIN)) {
> +		/*
> +		 * Only attempt recovery if SDA is stuck low. -EPROTO and
> +		 * -ETIMEDOUT indicate bus errors where the target may be
> +		 * holding SDA low. ARB_LOST (-EAGAIN) on a single-controller
> +		 * bus indicates a stuck target, not a real arbitration loss.
> +		 * GPI DMA mode extends this trigger separately.
> +		 */
> +		if (!(readl_relaxed(gi2c->se.base + SE_GENI_IOS) & RX_DATA_IN)) {
> +			int recovery_ret = i2c_recover_bus(adap);
> +
> +			if (recovery_ret)
> +				dev_err(gi2c->se.dev,
> +					"bus recovery failed: %d (xfer error: %d)\n",
> +					recovery_ret, ret);
> +		}
Can we move this to a helper function like "geni_i2c_need_bus_recovery" 
?> +	}
> +
>   	pm_runtime_put_autosuspend(gi2c->se.dev);
>   	gi2c->cur = NULL;
>   	gi2c->err = 0;
> @@ -1188,6 +1292,11 @@ static int geni_i2c_probe(struct platform_device *pdev)
>   	if (ret < 0)
>   		return ret;
>   
> +	if (!gi2c->gpi_mode) {
> +		gi2c->rinfo.recover_bus = geni_i2c_recover_bus;
> +		gi2c->adap.bus_recovery_info = &gi2c->rinfo;
> +	}
In geni_i2c_xfer(), geni_i2c_recover_bus() is invoked directly, then why 
do we need this ?> +
>   	ret = i2c_add_adapter(&gi2c->adap);
>   	if (ret)
>   		return dev_err_probe(dev, ret, "Error adding i2c adapter\n");
> 

[PATCH v1 3/3] i2c: qcom-geni: Add bus recovery support for GPI DMA mode

[Aniket Randive]

I2C transfers in GPI DMA mode can fail with -EIO when a target holds
SDA low, leaving the bus stuck and preventing subsequent transactions.
The GPI completion callback reports a NACK and a genuine bus error
alike as -EIO, and the existing recovery path switches the SE to FIFO
mode which is not valid in GPI DMA mode.

Add bus recovery support using the I2C_BUS_CLEAR and I2C_STOP_ON_BUS
opcodes issued via the GPI TX DMA channel. Guard the -EIO recovery
trigger with a SE_GENI_IOS RX_DATA_IN check to skip recovery when SDA
is already released. Extend the recovery callback to dispatch through
the GPI DMA path when gi2c->gpi_mode is set.

Signed-off-by: Aniket Randive <aniket.randive@oss.qualcomm.com>
---
 drivers/i2c/busses/i2c-qcom-geni.c | 104 +++++++++++++++++++++++++++++++------
 1 file changed, 88 insertions(+), 16 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index 9fa1a8ac400c..85cb1367958c 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -34,8 +34,6 @@
 #define I2C_READ		0x2
 #define I2C_WRITE_READ		0x3
 #define I2C_ADDR_ONLY		0x4
-#define I2C_BUS_CLEAR		0x6
-#define I2C_STOP_ON_BUS		0x7
 /* M_CMD params for I2C */
 #define PRE_CMD_DELAY		BIT(0)
 #define TIMESTAMP_BEFORE	BIT(1)
@@ -1003,6 +1001,70 @@ static int geni_i2c_fifo_bus_cmd(struct geni_i2c_dev *gi2c, u32 cmd)
 	return gi2c->err;
 }
 
+static int geni_i2c_gpi_bus_cmd(struct geni_i2c_dev *gi2c, enum i2c_op cmd)
+{
+	const struct geni_i2c_clk_fld *itr = gi2c->clk_fld;
+	struct dma_async_tx_descriptor *desc;
+	struct gpi_i2c_config peripheral = {};
+	struct dma_slave_config config = {};
+	unsigned long time_left;
+	unsigned long flags;
+	dma_cookie_t cookie;
+
+	config.peripheral_config = &peripheral;
+	config.peripheral_size = sizeof(peripheral);
+
+	peripheral.set_config  = 1;
+	peripheral.pack_enable = I2C_PACK_TX | I2C_PACK_RX;
+	peripheral.cycle_count = itr->t_cycle_cnt;
+	peripheral.high_count  = itr->t_high_cnt;
+	peripheral.low_count   = itr->t_low_cnt;
+	peripheral.clk_div     = itr->clk_div;
+	peripheral.op          = cmd;
+
+	if (dmaengine_slave_config(gi2c->tx_c, &config)) {
+		dev_err(gi2c->se.dev, "dma config error for bus cmd %u\n", cmd);
+		return -EIO;
+	}
+
+	desc = dmaengine_prep_slave_single(gi2c->tx_c, 0, 0, DMA_MEM_TO_DEV,
+					   DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+	if (!desc)
+		return -EIO;
+
+	desc->callback_result = i2c_gpi_cb_result;
+	desc->callback_param = gi2c;
+
+	cookie = dmaengine_submit(desc);
+	if (dma_submit_error(cookie))
+		return -EIO;
+
+	/*
+	 * cur and err are shared with geni_i2c_irq() and the GPI callback
+	 * i2c_gpi_cb_result(); both write err (the IRQ handler under
+	 * gi2c->lock). Reset them under the lock before issuing the transfer.
+	 */
+	spin_lock_irqsave(&gi2c->lock, flags);
+	gi2c->cur = NULL;
+	gi2c->err = 0;
+	spin_unlock_irqrestore(&gi2c->lock, flags);
+	reinit_completion(&gi2c->done);
+	dma_async_issue_pending(gi2c->tx_c);
+
+	time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT);
+	if (!time_left) {
+		dev_err(gi2c->se.dev, "timeout waiting for GPI bus cmd %u\n", cmd);
+		dmaengine_terminate_sync(gi2c->tx_c);
+		return -ETIMEDOUT;
+	}
+
+	/* ARB_LOST and BUS_PROTO may be expected during recovery; treat as success */
+	if (gi2c->err == -EAGAIN || gi2c->err == -EPROTO)
+		return 0;
+
+	return gi2c->err;
+}
+
 static int geni_i2c_recover_bus(struct i2c_adapter *adap)
 {
 	struct geni_i2c_dev *gi2c = i2c_get_adapdata(adap);
@@ -1021,15 +1083,23 @@ static int geni_i2c_recover_bus(struct i2c_adapter *adap)
 		return 0;
 	}
 
-	ret = geni_i2c_fifo_bus_cmd(gi2c, I2C_BUS_CLEAR);
-	if (!ret)
-		ret = geni_i2c_fifo_bus_cmd(gi2c, I2C_STOP_ON_BUS);
+	if (gi2c->gpi_mode) {
+		ret = geni_i2c_gpi_bus_cmd(gi2c, I2C_BUS_CLEAR);
+		if (ret)
+			dev_dbg(gi2c->se.dev, "GPI bus clear returned %d, issuing stop anyway\n",
+				ret);
+
+		ret = geni_i2c_gpi_bus_cmd(gi2c, I2C_STOP_ON_BUS);
+	} else {
+		ret = geni_i2c_fifo_bus_cmd(gi2c, I2C_BUS_CLEAR);
+		if (!ret)
+			ret = geni_i2c_fifo_bus_cmd(gi2c, I2C_STOP_ON_BUS);
+	}
 
 	/*
-	 * Recovery succeeds only once the slave releases SDA, so the bus
-	 * state is the authority: RX_DATA_IN high means the bus is free,
-	 * while a clean opcode status with SDA still low is a failed
-	 * recovery.
+	 * Recovery succeeds when SDA is released. RX_DATA_IN high means the
+	 * bus is free; trust this over the opcode return path, which cannot
+	 * flag a benign recovery event in GPI mode.
 	 */
 	if (readl_relaxed(gi2c->se.base + SE_GENI_IOS) & RX_DATA_IN)
 		ret = 0;
@@ -1066,14 +1136,18 @@ static int geni_i2c_xfer(struct i2c_adapter *adap,
 	else
 		ret = geni_i2c_fifo_xfer(gi2c, msgs, num);
 
-	if (!gi2c->gpi_mode &&
-	    (ret == -EPROTO || ret == -ETIMEDOUT || ret == -EAGAIN)) {
+	if (ret == -EPROTO || ret == -ETIMEDOUT || ret == -EAGAIN ||
+	    (gi2c->gpi_mode && ret == -EIO)) {
 		/*
 		 * Only attempt recovery if SDA is stuck low. -EPROTO and
 		 * -ETIMEDOUT indicate bus errors where the target may be
 		 * holding SDA low. ARB_LOST (-EAGAIN) on a single-controller
 		 * bus indicates a stuck target, not a real arbitration loss.
-		 * GPI DMA mode extends this trigger separately.
+		 * In GPI DMA mode the completion callback reports NACK,
+		 * BUS_PROTO and ARB_LOST alike as -EIO, so the SE_GENI_IOS
+		 * RX_DATA_IN check below is what distinguishes a genuinely
+		 * stuck bus from a benign NACK: a NACK leaves SDA released
+		 * (high) after the STOP, so recovery is skipped.
 		 */
 		if (!(readl_relaxed(gi2c->se.base + SE_GENI_IOS) & RX_DATA_IN)) {
 			int recovery_ret = i2c_recover_bus(adap);
@@ -1292,10 +1366,8 @@ static int geni_i2c_probe(struct platform_device *pdev)
 	if (ret < 0)
 		return ret;
 
-	if (!gi2c->gpi_mode) {
-		gi2c->rinfo.recover_bus = geni_i2c_recover_bus;
-		gi2c->adap.bus_recovery_info = &gi2c->rinfo;
-	}
+	gi2c->rinfo.recover_bus = geni_i2c_recover_bus;
+	gi2c->adap.bus_recovery_info = &gi2c->rinfo;
 
 	ret = i2c_add_adapter(&gi2c->adap);
 	if (ret)

-- 
2.34.1

Re: [PATCH v1 3/3] i2c: qcom-geni: Add bus recovery support for GPI DMA mode

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Double free and invalid DMA unmap in geni_i2c_gpi_xfer() due to uninitialized rx_buf across loop iterations.
--

commit 1ca4e6273080dc8f5231d9130b61334c26638357
Author: Aniket Randive <aniket.randive@oss.qualcomm.com>

i2c: qcom-geni: Add bus recovery support for GPI DMA mode

This commit adds bus recovery support for GPI DMA mode in the QCOM GENI I2C
driver. It implements I2C_BUS_CLEAR and I2C_STOP_ON_BUS opcodes via the
GPI TX DMA channel and updates the recovery trigger logic.

> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 9fa1a8ac400c3..85cb1367958c6 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c

[ ... ]

> @@ -1066,14 +1136,18 @@ static int geni_i2c_xfer(struct i2c_adapter *adap,
>  	else
>  		ret = geni_i2c_fifo_xfer(gi2c, msgs, num);

[Severity: Critical]
This is a pre-existing issue, but I noticed a potential double-free and
invalid DMA unmap in geni_i2c_gpi_xfer() during multi-message transfers.

Looking at geni_i2c_gpi_xfer() which is called just above this context,
the rx_buf pointer is initialized outside the message processing loop:

drivers/i2c/busses/i2c-qcom-geni.c:geni_i2c_gpi_xfer() {
    void *tx_buf = NULL, *rx_buf = NULL;
    ...
    for (i = 0; i < num; i++) {
        ...
        if (msgs[i].flags & I2C_M_RD) {
            ret =  geni_i2c_gpi(gi2c, msgs, &config,
                                &rx_addr, &rx_buf, I2C_READ, gi2c->rx_c);
        ...
        if (!gi2c->is_tx_multi_desc_xfer)
            geni_i2c_gpi_unmap(gi2c, &msgs[i], tx_buf, tx_addr, rx_buf, rx_addr);
}

If a multi-message transfer contains a read message followed by a write
message, rx_buf is allocated and set during the read message. During the
subsequent write message, the I2C_M_RD block is skipped, meaning rx_buf
is never reset to NULL.

The stale rx_buf pointer is then passed again to geni_i2c_gpi_unmap() at
the end of the write iteration:

drivers/i2c/busses/i2c-qcom-geni.c:geni_i2c_gpi_unmap() {
    ...
    if (rx_buf) {
        dma_unmap_single(gi2c->se.dev->parent, rx_addr, msg->len, DMA_FROM_DEVICE);
        i2c_put_dma_safe_msg_buf(rx_buf, msg, !gi2c->err);
    }
}

Could this lead to a double-free of the bounce buffer and an invalid DMA
unmap since rx_buf retains its value from the previous read iteration?

> -	if (!gi2c->gpi_mode &&
> -	    (ret == -EPROTO || ret == -ETIMEDOUT || ret == -EAGAIN)) {
> +	if (ret == -EPROTO || ret == -ETIMEDOUT || ret == -EAGAIN ||
> +	    (gi2c->gpi_mode && ret == -EIO)) {
>  		/*
>  		 * Only attempt recovery if SDA is stuck low. -EPROTO and
>  		 * -ETIMEDOUT indicate bus errors where the target may be

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826-i2c_bus_recovery-v1-0-203f0ec76f84@oss.qualcomm.com?part=3

Re: [PATCH v1 3/3] i2c: qcom-geni: Add bus recovery support for GPI DMA mode

[Jyothi Kumar Seerapu]

On 8/26/2026 3:21 PM, Aniket Randive wrote:
> I2C transfers in GPI DMA mode can fail with -EIO when a target holds
> SDA low, leaving the bus stuck and preventing subsequent transactions.
> The GPI completion callback reports a NACK and a genuine bus error
> alike as -EIO, and the existing recovery path switches the SE to FIFO
> mode which is not valid in GPI DMA mode.
> 
> Add bus recovery support using the I2C_BUS_CLEAR and I2C_STOP_ON_BUS
> opcodes issued via the GPI TX DMA channel. Guard the -EIO recovery
> trigger with a SE_GENI_IOS RX_DATA_IN check to skip recovery when SDA
> is already released. Extend the recovery callback to dispatch through
> the GPI DMA path when gi2c->gpi_mode is set.
> 
> Signed-off-by: Aniket Randive <aniket.randive@oss.qualcomm.com>
> ---
>   drivers/i2c/busses/i2c-qcom-geni.c | 104 +++++++++++++++++++++++++++++++------
>   1 file changed, 88 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 9fa1a8ac400c..85cb1367958c 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -34,8 +34,6 @@
>   #define I2C_READ		0x2
>   #define I2C_WRITE_READ		0x3
>   #define I2C_ADDR_ONLY		0x4
> -#define I2C_BUS_CLEAR		0x6
> -#define I2C_STOP_ON_BUS		0x7
>   /* M_CMD params for I2C */
>   #define PRE_CMD_DELAY		BIT(0)
>   #define TIMESTAMP_BEFORE	BIT(1)
> @@ -1003,6 +1001,70 @@ static int geni_i2c_fifo_bus_cmd(struct geni_i2c_dev *gi2c, u32 cmd)
>   	return gi2c->err;
>   }
>   
> +static int geni_i2c_gpi_bus_cmd(struct geni_i2c_dev *gi2c, enum i2c_op cmd)
> +{
> +	const struct geni_i2c_clk_fld *itr = gi2c->clk_fld;
> +	struct dma_async_tx_descriptor *desc;
> +	struct gpi_i2c_config peripheral = {};
> +	struct dma_slave_config config = {};
> +	unsigned long time_left;
> +	unsigned long flags;
> +	dma_cookie_t cookie;
> +
> +	config.peripheral_config = &peripheral;
> +	config.peripheral_size = sizeof(peripheral);
> +
> +	peripheral.set_config  = 1;
> +	peripheral.pack_enable = I2C_PACK_TX | I2C_PACK_RX;
> +	peripheral.cycle_count = itr->t_cycle_cnt;
> +	peripheral.high_count  = itr->t_high_cnt;
> +	peripheral.low_count   = itr->t_low_cnt;
> +	peripheral.clk_div     = itr->clk_div;
> +	peripheral.op          = cmd;
> +
> +	if (dmaengine_slave_config(gi2c->tx_c, &config)) {
> +		dev_err(gi2c->se.dev, "dma config error for bus cmd %u\n", cmd);
> +		return -EIO;
> +	}
> +
> +	desc = dmaengine_prep_slave_single(gi2c->tx_c, 0, 0, DMA_MEM_TO_DEV,
> +					   DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
> +	if (!desc)
> +		return -EIO;
> +
> +	desc->callback_result = i2c_gpi_cb_result;
> +	desc->callback_param = gi2c;
> +
> +	cookie = dmaengine_submit(desc);
> +	if (dma_submit_error(cookie))
> +		return -EIO;
> +
> +	/*
> +	 * cur and err are shared with geni_i2c_irq() and the GPI callback
> +	 * i2c_gpi_cb_result(); both write err (the IRQ handler under
> +	 * gi2c->lock). Reset them under the lock before issuing the transfer.
> +	 */
> +	spin_lock_irqsave(&gi2c->lock, flags);
> +	gi2c->cur = NULL;
> +	gi2c->err = 0;
> +	spin_unlock_irqrestore(&gi2c->lock, flags);
> +	reinit_completion(&gi2c->done);
> +	dma_async_issue_pending(gi2c->tx_c);
> +
> +	time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT);
> +	if (!time_left) {
> +		dev_err(gi2c->se.dev, "timeout waiting for GPI bus cmd %u\n", cmd);
> +		dmaengine_terminate_sync(gi2c->tx_c);
> +		return -ETIMEDOUT;
> +	}
> +
> +	/* ARB_LOST and BUS_PROTO may be expected during recovery; treat as success */
> +	if (gi2c->err == -EAGAIN || gi2c->err == -EPROTO)
> +		return 0;
> +
> +	return gi2c->err;
> +}
> +
>   static int geni_i2c_recover_bus(struct i2c_adapter *adap)
>   {
>   	struct geni_i2c_dev *gi2c = i2c_get_adapdata(adap);
> @@ -1021,15 +1083,23 @@ static int geni_i2c_recover_bus(struct i2c_adapter *adap)
>   		return 0;
>   	}
>   
> -	ret = geni_i2c_fifo_bus_cmd(gi2c, I2C_BUS_CLEAR);
> -	if (!ret)
> -		ret = geni_i2c_fifo_bus_cmd(gi2c, I2C_STOP_ON_BUS);
> +	if (gi2c->gpi_mode) {
> +		ret = geni_i2c_gpi_bus_cmd(gi2c, I2C_BUS_CLEAR);
> +		if (ret)
> +			dev_dbg(gi2c->se.dev, "GPI bus clear returned %d, issuing stop anyway\n",
> +				ret);
> +
> +		ret = geni_i2c_gpi_bus_cmd(gi2c, I2C_STOP_ON_BUS);
> +	} else {
> +		ret = geni_i2c_fifo_bus_cmd(gi2c, I2C_BUS_CLEAR);
> +		if (!ret)
> +			ret = geni_i2c_fifo_bus_cmd(gi2c, I2C_STOP_ON_BUS);
> +	}
>   
>   	/*
> -	 * Recovery succeeds only once the slave releases SDA, so the bus
> -	 * state is the authority: RX_DATA_IN high means the bus is free,
> -	 * while a clean opcode status with SDA still low is a failed
> -	 * recovery.
> +	 * Recovery succeeds when SDA is released. RX_DATA_IN high means the
> +	 * bus is free; trust this over the opcode return path, which cannot
> +	 * flag a benign recovery event in GPI mode.
>   	 */
>   	if (readl_relaxed(gi2c->se.base + SE_GENI_IOS) & RX_DATA_IN)
>   		ret = 0;
> @@ -1066,14 +1136,18 @@ static int geni_i2c_xfer(struct i2c_adapter *adap,
>   	else
>   		ret = geni_i2c_fifo_xfer(gi2c, msgs, num);
>   
> -	if (!gi2c->gpi_mode &&
> -	    (ret == -EPROTO || ret == -ETIMEDOUT || ret == -EAGAIN)) {
> +	if (ret == -EPROTO || ret == -ETIMEDOUT || ret == -EAGAIN ||
> +	    (gi2c->gpi_mode && ret == -EIO)) {
>   		/*
>   		 * Only attempt recovery if SDA is stuck low. -EPROTO and
>   		 * -ETIMEDOUT indicate bus errors where the target may be
>   		 * holding SDA low. ARB_LOST (-EAGAIN) on a single-controller
>   		 * bus indicates a stuck target, not a real arbitration loss.
> -		 * GPI DMA mode extends this trigger separately.
> +		 * In GPI DMA mode the completion callback reports NACK,
> +		 * BUS_PROTO and ARB_LOST alike as -EIO, so the SE_GENI_IOS
> +		 * RX_DATA_IN check below is what distinguishes a genuinely
> +		 * stuck bus from a benign NACK: a NACK leaves SDA released
> +		 * (high) after the STOP, so recovery is skipped.
>   		 */
>   		if (!(readl_relaxed(gi2c->se.base + SE_GENI_IOS) & RX_DATA_IN)) {
>   			int recovery_ret = i2c_recover_bus(adap);
This does not appear to handle scenarios such as an I2C multi-message 
transfer where the stretch bit is set during the first message, but an 
error occurs while preparing the second message (for example, during 
memory allocation or DMA descriptor preparation). In such cases, the SCL 
line may remain asserted low and impact subsequent transfers.
  > @@ -1292,10 +1366,8 @@ static int geni_i2c_probe(struct 
platform_device *pdev)
>   	if (ret < 0)
>   		return ret;
>   
> -	if (!gi2c->gpi_mode) {
> -		gi2c->rinfo.recover_bus = geni_i2c_recover_bus;
> -		gi2c->adap.bus_recovery_info = &gi2c->rinfo;
> -	}
> +	gi2c->rinfo.recover_bus = geni_i2c_recover_bus;
> +	gi2c->adap.bus_recovery_info = &gi2c->rinfo;
>   
>   	ret = i2c_add_adapter(&gi2c->adap);
>   	if (ret)
>