[PATCH 0/3] i2c: qcom-geni: improve transfer error recovery and synchronization

[PATCH 0/3] i2c: qcom-geni: improve transfer error recovery and synchronization

[Praveen Talari]

The GENI I2C driver currently has a few issues in its transfer recovery
and completion handling paths.

The timeout recovery flow directly aborts outstanding commands, while
GENI hardware requires command cancellation to be attempted before
issuing an abort. In addition, multiple operations share a common
completion object, which can allow unrelated events to wake waiters
prematurely and lead to incorrect synchronization.

Address NACK handling can also trigger unnecessary cancel and reset
operations even though the transfer has already terminated during the
address phase.

Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
Praveen Talari (3):
      i2c: qcom-geni: use cancel command before abort on transfer timeout
      i2c: qcom-geni: use dedicated completions for abort and reset events
      i2c: qcom-geni: Avoid unnecessary transfer cancel on address NACK

 drivers/i2c/busses/i2c-qcom-geni.c | 100 ++++++++++++++++++++++++-------------
 1 file changed, 66 insertions(+), 34 deletions(-)
---
base-commit: 2b763db0c2763d6bf73d7d3e69665222d1f377cf
change-id: 20260703-fix_cancel_sequence_on_failure_for_i2c-a868571f77e0

Best regards,
--  
Praveen Talari <praveen.talari@oss.qualcomm.com>

[PATCH 1/3] i2c: qcom-geni: use cancel command before abort on transfer timeout

[Praveen Talari]

The GENI I2C driver currently invokes geni_se_abort_m_cmd() directly when
a transfer times out. However, the GENI hardware command cancellation
flow requires a cancel command to be issued first. An abort should only
be used as a fallback when the cancel operation itself fails to complete.

Introduce a dedicated cancel_done completion that is signalled on
M_CMD_CANCEL_EN. The timeout recovery path waits for cancel completion
and escalates to geni_i2c_abort_xfer() only if the cancel command does
not complete within the expected time.

Co-developed-by: Naresh Maramaina <naresh.maramaina@oss.qualcomm.com>
Signed-off-by: Naresh Maramaina <naresh.maramaina@oss.qualcomm.com>
Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
 drivers/i2c/busses/i2c-qcom-geni.c | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index 96dbf04138be..15403edb355a 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -74,6 +74,7 @@ enum geni_i2c_err_code {
 #define PACKING_BYTES_PW	4
 
 #define ABORT_TIMEOUT		HZ
+#define CANCEL_TIMEOUT		HZ
 #define XFER_TIMEOUT		HZ
 #define RST_TIMEOUT		HZ
 
@@ -112,6 +113,7 @@ struct geni_i2c_dev {
 	int err;
 	struct i2c_adapter adap;
 	struct completion done;
+	struct completion cancel_done;
 	struct i2c_msg *cur;
 	int cur_wr;
 	int cur_rd;
@@ -361,6 +363,8 @@ static irqreturn_t geni_i2c_irq(int irq, void *dev)
 	    dm_tx_st & TX_DMA_DONE || dm_tx_st & TX_RESET_DONE ||
 	    dm_rx_st & RX_DMA_DONE || dm_rx_st & RX_RESET_DONE)
 		complete(&gi2c->done);
+	if (m_stat & M_CMD_CANCEL_EN)
+		complete(&gi2c->cancel_done);
 
 	spin_unlock(&gi2c->lock);
 
@@ -387,6 +391,27 @@ static void geni_i2c_abort_xfer(struct geni_i2c_dev *gi2c)
 		dev_err(gi2c->se.dev, "Timeout abort_m_cmd\n");
 }
 
+static void geni_i2c_cancel_xfer(struct geni_i2c_dev *gi2c)
+{
+	unsigned long time_left = msecs_to_jiffies(CANCEL_TIMEOUT);
+	unsigned long flags;
+
+	reinit_completion(&gi2c->cancel_done);
+
+	spin_lock_irqsave(&gi2c->lock, flags);
+	if (!gi2c->err)
+		geni_i2c_err(gi2c, GENI_TIMEOUT);
+	gi2c->cur = NULL;
+	geni_se_cancel_m_cmd(&gi2c->se);
+	spin_unlock_irqrestore(&gi2c->lock, flags);
+
+	time_left = wait_for_completion_timeout(&gi2c->cancel_done, time_left);
+	if (!time_left) {
+		dev_err(gi2c->se.dev, "Timeout cancel_m_cmd\n");
+		geni_i2c_abort_xfer(gi2c);
+	}
+}
+
 static void geni_i2c_rx_fsm_rst(struct geni_i2c_dev *gi2c)
 {
 	u32 val;
@@ -473,7 +498,7 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
 	cur = gi2c->cur;
 	time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT);
 	if (!time_left)
-		geni_i2c_abort_xfer(gi2c);
+		geni_i2c_cancel_xfer(gi2c);
 
 	geni_i2c_rx_msg_cleanup(gi2c, cur);
 
@@ -515,7 +540,7 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
 	cur = gi2c->cur;
 	time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT);
 	if (!time_left)
-		geni_i2c_abort_xfer(gi2c);
+		geni_i2c_cancel_xfer(gi2c);
 
 	geni_i2c_tx_msg_cleanup(gi2c, cur);
 
@@ -1107,6 +1132,7 @@ static int geni_i2c_probe(struct platform_device *pdev)
 
 	gi2c->adap.algo = &geni_i2c_algo;
 	init_completion(&gi2c->done);
+	init_completion(&gi2c->cancel_done);
 	spin_lock_init(&gi2c->lock);
 	platform_set_drvdata(pdev, gi2c);
 

-- 
2.34.1

Re: [PATCH 1/3] i2c: qcom-geni: use cancel command before abort on transfer timeout

[Mukesh Savaliya]

On 7/8/2026 11:45 AM, Praveen Talari wrote:
> The GENI I2C driver currently invokes geni_se_abort_m_cmd() directly when
> a transfer times out. However, the GENI hardware command cancellation
> flow requires a cancel command to be issued first. An abort should only
> be used as a fallback when the cancel operation itself fails to complete.
> 
> Introduce a dedicated cancel_done completion that is signalled on
Typo :signalled

"signaled when M_CMD_CANCEL_EN is received" With this , rest looks good.
> M_CMD_CANCEL_EN. The timeout recovery path waits for cancel completion
> and escalates to geni_i2c_abort_xfer() only if the cancel command does
> not complete within the expected time.
> 
> Co-developed-by: Naresh Maramaina <naresh.maramaina@oss.qualcomm.com>
> Signed-off-by: Naresh Maramaina <naresh.maramaina@oss.qualcomm.com>
> Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
> ---
Reviewed-by: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>

>   drivers/i2c/busses/i2c-qcom-geni.c | 30 ++++++++++++++++++++++++++++--
>   1 file changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 96dbf04138be..15403edb355a 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -74,6 +74,7 @@ enum geni_i2c_err_code {
>   #define PACKING_BYTES_PW	4
>   
>   #define ABORT_TIMEOUT		HZ
> +#define CANCEL_TIMEOUT		HZ
>   #define XFER_TIMEOUT		HZ
>   #define RST_TIMEOUT		HZ
>   
> @@ -112,6 +113,7 @@ struct geni_i2c_dev {
>   	int err;
>   	struct i2c_adapter adap;
>   	struct completion done;
> +	struct completion cancel_done;
>   	struct i2c_msg *cur;
>   	int cur_wr;
>   	int cur_rd;
> @@ -361,6 +363,8 @@ static irqreturn_t geni_i2c_irq(int irq, void *dev)
>   	    dm_tx_st & TX_DMA_DONE || dm_tx_st & TX_RESET_DONE ||
>   	    dm_rx_st & RX_DMA_DONE || dm_rx_st & RX_RESET_DONE)
>   		complete(&gi2c->done);
> +	if (m_stat & M_CMD_CANCEL_EN)
> +		complete(&gi2c->cancel_done);
>   
>   	spin_unlock(&gi2c->lock);
>   
> @@ -387,6 +391,27 @@ static void geni_i2c_abort_xfer(struct geni_i2c_dev *gi2c)
>   		dev_err(gi2c->se.dev, "Timeout abort_m_cmd\n");
>   }
>   
> +static void geni_i2c_cancel_xfer(struct geni_i2c_dev *gi2c)
> +{
> +	unsigned long time_left = msecs_to_jiffies(CANCEL_TIMEOUT);
> +	unsigned long flags;
> +
> +	reinit_completion(&gi2c->cancel_done);
> +
> +	spin_lock_irqsave(&gi2c->lock, flags);
> +	if (!gi2c->err)
> +		geni_i2c_err(gi2c, GENI_TIMEOUT);
> +	gi2c->cur = NULL;
> +	geni_se_cancel_m_cmd(&gi2c->se);
> +	spin_unlock_irqrestore(&gi2c->lock, flags);
> +
> +	time_left = wait_for_completion_timeout(&gi2c->cancel_done, time_left);
> +	if (!time_left) {
> +		dev_err(gi2c->se.dev, "Timeout cancel_m_cmd\n");
> +		geni_i2c_abort_xfer(gi2c);
> +	}
> +}
> +
>   static void geni_i2c_rx_fsm_rst(struct geni_i2c_dev *gi2c)
>   {
>   	u32 val;
> @@ -473,7 +498,7 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
>   	cur = gi2c->cur;
>   	time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT);
>   	if (!time_left)
> -		geni_i2c_abort_xfer(gi2c);
> +		geni_i2c_cancel_xfer(gi2c);
>   
>   	geni_i2c_rx_msg_cleanup(gi2c, cur);
>   
> @@ -515,7 +540,7 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
>   	cur = gi2c->cur;
>   	time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT);
>   	if (!time_left)
> -		geni_i2c_abort_xfer(gi2c);
> +		geni_i2c_cancel_xfer(gi2c);
>   
>   	geni_i2c_tx_msg_cleanup(gi2c, cur);
>   
> @@ -1107,6 +1132,7 @@ static int geni_i2c_probe(struct platform_device *pdev)
>   
>   	gi2c->adap.algo = &geni_i2c_algo;
>   	init_completion(&gi2c->done);
> +	init_completion(&gi2c->cancel_done);
>   	spin_lock_init(&gi2c->lock);
>   	platform_set_drvdata(pdev, gi2c);
>   
> 

Re: [PATCH 1/3] i2c: qcom-geni: use cancel command before abort on transfer timeout

[Praveen Talari]

Hi Mukesh,

Thank you for review.

On 08-07-2026 13:05, Mukesh Savaliya wrote:
>
>
> On 7/8/2026 11:45 AM, Praveen Talari wrote:
>> The GENI I2C driver currently invokes geni_se_abort_m_cmd() directly 
>> when
>> a transfer times out. However, the GENI hardware command cancellation
>> flow requires a cancel command to be issued first. An abort should only
>> be used as a fallback when the cancel operation itself fails to 
>> complete.
>>
>> Introduce a dedicated cancel_done completion that is signalled on
> Typo :signalled
>
> "signaled when M_CMD_CANCEL_EN is received" With this , rest looks good.

Sure. Will update in next patch.

Thanks,

Praveen Talari

>> M_CMD_CANCEL_EN. The timeout recovery path waits for cancel completion
>> and escalates to geni_i2c_abort_xfer() only if the cancel command does
>> not complete within the expected time.
>>
>> Co-developed-by: Naresh Maramaina <naresh.maramaina@oss.qualcomm.com>
>> Signed-off-by: Naresh Maramaina <naresh.maramaina@oss.qualcomm.com>
>> Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
>> ---
> Reviewed-by: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>
>
>>   drivers/i2c/busses/i2c-qcom-geni.c | 30 ++++++++++++++++++++++++++++--
>>   1 file changed, 28 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c 
>> b/drivers/i2c/busses/i2c-qcom-geni.c
>> index 96dbf04138be..15403edb355a 100644
>> --- a/drivers/i2c/busses/i2c-qcom-geni.c
>> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
>> @@ -74,6 +74,7 @@ enum geni_i2c_err_code {
>>   #define PACKING_BYTES_PW    4
>>     #define ABORT_TIMEOUT        HZ
>> +#define CANCEL_TIMEOUT        HZ
>>   #define XFER_TIMEOUT        HZ
>>   #define RST_TIMEOUT        HZ
>>   @@ -112,6 +113,7 @@ struct geni_i2c_dev {
>>       int err;
>>       struct i2c_adapter adap;
>>       struct completion done;
>> +    struct completion cancel_done;
>>       struct i2c_msg *cur;
>>       int cur_wr;
>>       int cur_rd;
>> @@ -361,6 +363,8 @@ static irqreturn_t geni_i2c_irq(int irq, void *dev)
>>           dm_tx_st & TX_DMA_DONE || dm_tx_st & TX_RESET_DONE ||
>>           dm_rx_st & RX_DMA_DONE || dm_rx_st & RX_RESET_DONE)
>>           complete(&gi2c->done);
>> +    if (m_stat & M_CMD_CANCEL_EN)
>> +        complete(&gi2c->cancel_done);
>>         spin_unlock(&gi2c->lock);
>>   @@ -387,6 +391,27 @@ static void geni_i2c_abort_xfer(struct 
>> geni_i2c_dev *gi2c)
>>           dev_err(gi2c->se.dev, "Timeout abort_m_cmd\n");
>>   }
>>   +static void geni_i2c_cancel_xfer(struct geni_i2c_dev *gi2c)
>> +{
>> +    unsigned long time_left = msecs_to_jiffies(CANCEL_TIMEOUT);
>> +    unsigned long flags;
>> +
>> +    reinit_completion(&gi2c->cancel_done);
>> +
>> +    spin_lock_irqsave(&gi2c->lock, flags);
>> +    if (!gi2c->err)
>> +        geni_i2c_err(gi2c, GENI_TIMEOUT);
>> +    gi2c->cur = NULL;
>> +    geni_se_cancel_m_cmd(&gi2c->se);
>> +    spin_unlock_irqrestore(&gi2c->lock, flags);
>> +
>> +    time_left = wait_for_completion_timeout(&gi2c->cancel_done, 
>> time_left);
>> +    if (!time_left) {
>> +        dev_err(gi2c->se.dev, "Timeout cancel_m_cmd\n");
>> +        geni_i2c_abort_xfer(gi2c);
>> +    }
>> +}
>> +
>>   static void geni_i2c_rx_fsm_rst(struct geni_i2c_dev *gi2c)
>>   {
>>       u32 val;
>> @@ -473,7 +498,7 @@ static int geni_i2c_rx_one_msg(struct 
>> geni_i2c_dev *gi2c, struct i2c_msg *msg,
>>       cur = gi2c->cur;
>>       time_left = wait_for_completion_timeout(&gi2c->done, 
>> XFER_TIMEOUT);
>>       if (!time_left)
>> -        geni_i2c_abort_xfer(gi2c);
>> +        geni_i2c_cancel_xfer(gi2c);
>>         geni_i2c_rx_msg_cleanup(gi2c, cur);
>>   @@ -515,7 +540,7 @@ static int geni_i2c_tx_one_msg(struct 
>> geni_i2c_dev *gi2c, struct i2c_msg *msg,
>>       cur = gi2c->cur;
>>       time_left = wait_for_completion_timeout(&gi2c->done, 
>> XFER_TIMEOUT);
>>       if (!time_left)
>> -        geni_i2c_abort_xfer(gi2c);
>> +        geni_i2c_cancel_xfer(gi2c);
>>         geni_i2c_tx_msg_cleanup(gi2c, cur);
>>   @@ -1107,6 +1132,7 @@ static int geni_i2c_probe(struct 
>> platform_device *pdev)
>>         gi2c->adap.algo = &geni_i2c_algo;
>>       init_completion(&gi2c->done);
>> +    init_completion(&gi2c->cancel_done);
>>       spin_lock_init(&gi2c->lock);
>>       platform_set_drvdata(pdev, gi2c);
>>
>

[PATCH 2/3] i2c: qcom-geni: use dedicated completions for abort and reset events

[Praveen Talari]

The driver uses the shared gi2c->done completion for transfer, abort,
and DMA reset operations. This allows unrelated completion events to
prematurely wake abort and reset waiters, leading to incorrect
synchronization.

Introduce dedicated completions for abort, TX reset, and RX reset
operations, and signal them only from their respective interrupt
events. This removes the dependency on shared completion state and
eliminates the abort_done flag-based synchronization.

Co-developed-by: Naresh Maramaina <naresh.maramaina@oss.qualcomm.com>
Signed-off-by: Naresh Maramaina <naresh.maramaina@oss.qualcomm.com>
Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
 drivers/i2c/busses/i2c-qcom-geni.c | 54 +++++++++++++++++++-------------------
 1 file changed, 27 insertions(+), 27 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index 15403edb355a..9490aee4928c 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -113,7 +113,10 @@ struct geni_i2c_dev {
 	int err;
 	struct i2c_adapter adap;
 	struct completion done;
+	struct completion abort_done;
 	struct completion cancel_done;
+	struct completion tx_reset_done;
+	struct completion rx_reset_done;
 	struct i2c_msg *cur;
 	int cur_wr;
 	int cur_rd;
@@ -127,7 +130,6 @@ struct geni_i2c_dev {
 	struct dma_chan *rx_c;
 	bool no_dma;
 	bool gpi_mode;
-	bool abort_done;
 	bool is_tx_multi_desc_xfer;
 	u32 num_msgs;
 	struct geni_i2c_gpi_multi_desc_xfer i2c_multi_desc_config;
@@ -256,8 +258,6 @@ static void geni_i2c_err(struct geni_i2c_dev *gi2c, int err)
 
 	switch (err) {
 	case GENI_ABORT_DONE:
-		gi2c->abort_done = true;
-		break;
 	case NACK:
 	case GENI_TIMEOUT:
 		dev_dbg(gi2c->se.dev, "%s\n", gi2c_log[err].msg);
@@ -359,12 +359,18 @@ static irqreturn_t geni_i2c_irq(int irq, void *dev)
 		writel_relaxed(dm_rx_st, base + SE_DMA_RX_IRQ_CLR);
 
 	/* if this is err with done-bit not set, handle that through timeout. */
-	if (m_stat & M_CMD_DONE_EN || m_stat & M_CMD_ABORT_EN ||
-	    dm_tx_st & TX_DMA_DONE || dm_tx_st & TX_RESET_DONE ||
-	    dm_rx_st & RX_DMA_DONE || dm_rx_st & RX_RESET_DONE)
+	if (m_stat & M_CMD_DONE_EN ||
+	    dm_tx_st & TX_DMA_DONE ||
+	    dm_rx_st & RX_DMA_DONE)
 		complete(&gi2c->done);
 	if (m_stat & M_CMD_CANCEL_EN)
 		complete(&gi2c->cancel_done);
+	if (m_stat & M_CMD_ABORT_EN)
+		complete(&gi2c->abort_done);
+	if (dm_tx_st & TX_RESET_DONE)
+		complete(&gi2c->tx_reset_done);
+	if (dm_rx_st & RX_RESET_DONE)
+		complete(&gi2c->rx_reset_done);
 
 	spin_unlock(&gi2c->lock);
 
@@ -376,17 +382,13 @@ static void geni_i2c_abort_xfer(struct geni_i2c_dev *gi2c)
 	unsigned long time_left = ABORT_TIMEOUT;
 	unsigned long flags;
 
+	reinit_completion(&gi2c->abort_done);
+
 	spin_lock_irqsave(&gi2c->lock, flags);
-	geni_i2c_err(gi2c, GENI_TIMEOUT);
-	gi2c->cur = NULL;
-	gi2c->abort_done = false;
 	geni_se_abort_m_cmd(&gi2c->se);
 	spin_unlock_irqrestore(&gi2c->lock, flags);
 
-	do {
-		time_left = wait_for_completion_timeout(&gi2c->done, time_left);
-	} while (!gi2c->abort_done && time_left);
-
+	time_left = wait_for_completion_timeout(&gi2c->abort_done, time_left);
 	if (!time_left)
 		dev_err(gi2c->se.dev, "Timeout abort_m_cmd\n");
 }
@@ -414,31 +416,25 @@ static void geni_i2c_cancel_xfer(struct geni_i2c_dev *gi2c)
 
 static void geni_i2c_rx_fsm_rst(struct geni_i2c_dev *gi2c)
 {
-	u32 val;
 	unsigned long time_left = RST_TIMEOUT;
 
+	reinit_completion(&gi2c->rx_reset_done);
 	writel_relaxed(1, gi2c->se.base + SE_DMA_RX_FSM_RST);
-	do {
-		time_left = wait_for_completion_timeout(&gi2c->done, time_left);
-		val = readl_relaxed(gi2c->se.base + SE_DMA_RX_IRQ_STAT);
-	} while (!(val & RX_RESET_DONE) && time_left);
 
-	if (!(val & RX_RESET_DONE))
+	time_left = wait_for_completion_timeout(&gi2c->rx_reset_done, time_left);
+	if (!time_left)
 		dev_err(gi2c->se.dev, "Timeout resetting RX_FSM\n");
 }
 
 static void geni_i2c_tx_fsm_rst(struct geni_i2c_dev *gi2c)
 {
-	u32 val;
 	unsigned long time_left = RST_TIMEOUT;
 
+	reinit_completion(&gi2c->tx_reset_done);
 	writel_relaxed(1, gi2c->se.base + SE_DMA_TX_FSM_RST);
-	do {
-		time_left = wait_for_completion_timeout(&gi2c->done, time_left);
-		val = readl_relaxed(gi2c->se.base + SE_DMA_TX_IRQ_STAT);
-	} while (!(val & TX_RESET_DONE) && time_left);
 
-	if (!(val & TX_RESET_DONE))
+	time_left = wait_for_completion_timeout(&gi2c->tx_reset_done, time_left);
+	if (!time_left)
 		dev_err(gi2c->se.dev, "Timeout resetting TX_FSM\n");
 }
 
@@ -851,6 +847,7 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
 	for (i = 0; i < num; i++) {
 		gi2c->cur = &msgs[i];
 		gi2c->err = 0;
+		reinit_completion(&gi2c->done);
 		dev_dbg(gi2c->se.dev, "msg[%d].len:%d\n", i, gi2c->cur->len);
 
 		peripheral.stretch = 0;
@@ -920,6 +917,8 @@ static int geni_i2c_fifo_xfer(struct geni_i2c_dev *gi2c,
 		m_param |= ((msgs[i].addr << SLV_ADDR_SHFT) & SLV_ADDR_MSK);
 
 		gi2c->cur = &msgs[i];
+		gi2c->err = 0;
+		reinit_completion(&gi2c->done);
 		if (msgs[i].flags & I2C_M_RD)
 			ret = geni_i2c_rx_one_msg(gi2c, &msgs[i], m_param);
 		else
@@ -939,8 +938,6 @@ static int geni_i2c_xfer(struct i2c_adapter *adap,
 	struct geni_i2c_dev *gi2c = i2c_get_adapdata(adap);
 	int ret;
 
-	gi2c->err = 0;
-	reinit_completion(&gi2c->done);
 	ret = pm_runtime_get_sync(gi2c->se.dev);
 	if (ret < 0) {
 		dev_err(gi2c->se.dev, "error turning SE resources:%d\n", ret);
@@ -1132,7 +1129,10 @@ static int geni_i2c_probe(struct platform_device *pdev)
 
 	gi2c->adap.algo = &geni_i2c_algo;
 	init_completion(&gi2c->done);
+	init_completion(&gi2c->abort_done);
 	init_completion(&gi2c->cancel_done);
+	init_completion(&gi2c->tx_reset_done);
+	init_completion(&gi2c->rx_reset_done);
 	spin_lock_init(&gi2c->lock);
 	platform_set_drvdata(pdev, gi2c);
 

-- 
2.34.1

Re: [PATCH 2/3] i2c: qcom-geni: use dedicated completions for abort and reset events

[Mukesh Savaliya]

On 7/8/2026 11:45 AM, Praveen Talari wrote:
> The driver uses the shared gi2c->done completion for transfer, abort,
shared => common completion event for transfer, abort and DMA reset
> and DMA reset operations. This allows unrelated completion events to
> prematurely wake abort and reset waiters, leading to incorrect
> synchronization.
Aren't they in sequence i.e. one after another ?
> 
> Introduce dedicated completions for abort, TX reset, and RX reset
> operations, and signal them only from their respective interrupt
> events. This removes the dependency on shared completion state and
> eliminates the abort_done flag-based synchronization.
> 
> Co-developed-by: Naresh Maramaina <naresh.maramaina@oss.qualcomm.com>
> Signed-off-by: Naresh Maramaina <naresh.maramaina@oss.qualcomm.com>
> Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
> ---
>   drivers/i2c/busses/i2c-qcom-geni.c | 54 +++++++++++++++++++-------------------
>   1 file changed, 27 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 15403edb355a..9490aee4928c 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -113,7 +113,10 @@ struct geni_i2c_dev {
>   	int err;
>   	struct i2c_adapter adap;
>   	struct completion done;
> +	struct completion abort_done;
>   	struct completion cancel_done;
> +	struct completion tx_reset_done;
> +	struct completion rx_reset_done;
tx or rx, any one would be running. Not both right ?
if so, i suggest to keep common dma_reset_done.
>   	struct i2c_msg *cur;
>   	int cur_wr;
>   	int cur_rd;
> @@ -127,7 +130,6 @@ struct geni_i2c_dev {
[..]

Re: [PATCH 2/3] i2c: qcom-geni: use dedicated completions for abort and reset events

[Praveen Talari]

Hi Mukesh,

On 08-07-2026 13:05, Mukesh Savaliya wrote:
>
>
> On 7/8/2026 11:45 AM, Praveen Talari wrote:
>> The driver uses the shared gi2c->done completion for transfer, abort,
> shared => common completion event for transfer, abort and DMA reset
Sure.
>> and DMA reset operations. This allows unrelated completion events to
>> prematurely wake abort and reset waiters, leading to incorrect
>> synchronization.
> Aren't they in sequence i.e. one after another ?

If the IRQ is triggered late, it can impact the subsequent transfer 
because gi2c->done may be cleared before the delayed interrupt is handled.

>>
>> Introduce dedicated completions for abort, TX reset, and RX reset
>> operations, and signal them only from their respective interrupt
>> events. This removes the dependency on shared completion state and
>> eliminates the abort_done flag-based synchronization.
>>
>> Co-developed-by: Naresh Maramaina <naresh.maramaina@oss.qualcomm.com>
>> Signed-off-by: Naresh Maramaina <naresh.maramaina@oss.qualcomm.com>
>> Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
>> ---
>>   drivers/i2c/busses/i2c-qcom-geni.c | 54 
>> +++++++++++++++++++-------------------
>>   1 file changed, 27 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c 
>> b/drivers/i2c/busses/i2c-qcom-geni.c
>> index 15403edb355a..9490aee4928c 100644
>> --- a/drivers/i2c/busses/i2c-qcom-geni.c
>> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
>> @@ -113,7 +113,10 @@ struct geni_i2c_dev {
>>       int err;
>>       struct i2c_adapter adap;
>>       struct completion done;
>> +    struct completion abort_done;
>>       struct completion cancel_done;
>> +    struct completion tx_reset_done;
>> +    struct completion rx_reset_done;
> tx or rx, any one would be running. Not both right ?
> if so, i suggest to keep common dma_reset_done.

As explained above, there is a possibility that a delayed IRQ may occur 
while another reset operation is still in progress, potentially 
affecting the reset sequence.

Thanks,

Praveen Talari

>>       struct i2c_msg *cur;
>>       int cur_wr;
>>       int cur_rd;
>> @@ -127,7 +130,6 @@ struct geni_i2c_dev {
> [..]
>

[PATCH 3/3] i2c: qcom-geni: Avoid unnecessary transfer cancel on address NACK

[Praveen Talari]

When a target does not acknowledge its address phase, the GENI hardware
raises a NACK interrupt. In this case, SE_GENI_M_GP_LENGTH remains zero,
indicating that no data phase has started and the transfer was aborted at
the address stage.

The driver currently treats all transfer errors similarly and always
issues geni_i2c_cancel_xfer() followed by TX/RX FSM resets for DMA
transfers. For address NACKs, the transfer has already terminated in
hardware and issuing an additional cancel/reset sequence is unnecessary.

Track address NACKs separately by checking the transfer progress length
when a NACK interrupt is reported. Skip the transfer cancel operation and
DMA FSM reset paths when the NACK occurred during the address phase.

This avoids redundant cancel/reset operations for a normal address NACK
condition while preserving the existing error recovery flow for all other
transfer failures.

Co-developed-by: Naresh Maramaina <naresh.maramaina@oss.qualcomm.com>
Signed-off-by: Naresh Maramaina <naresh.maramaina@oss.qualcomm.com>
Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
 drivers/i2c/busses/i2c-qcom-geni.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index 9490aee4928c..0448654f2678 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -130,6 +130,7 @@ struct geni_i2c_dev {
 	struct dma_chan *rx_c;
 	bool no_dma;
 	bool gpi_mode;
+	bool addr_nack;
 	bool is_tx_multi_desc_xfer;
 	u32 num_msgs;
 	struct geni_i2c_gpi_multi_desc_xfer i2c_multi_desc_config;
@@ -293,8 +294,12 @@ static irqreturn_t geni_i2c_irq(int irq, void *dev)
 	if (!cur ||
 	    m_stat & (M_CMD_FAILURE_EN | M_CMD_ABORT_EN) ||
 	    dm_rx_st & (DM_I2C_CB_ERR)) {
-		if (m_stat & M_GP_IRQ_1_EN)
+		if (m_stat & M_GP_IRQ_1_EN) {
 			geni_i2c_err(gi2c, NACK);
+			val = readl_relaxed(base + SE_GENI_M_GP_LENGTH);
+			if (!val)
+				gi2c->addr_nack = true;
+		}
 		if (m_stat & M_GP_IRQ_3_EN)
 			geni_i2c_err(gi2c, BUS_PROTO);
 		if (m_stat & M_GP_IRQ_4_EN)
@@ -443,7 +448,7 @@ static void geni_i2c_rx_msg_cleanup(struct geni_i2c_dev *gi2c,
 {
 	gi2c->cur_rd = 0;
 	if (gi2c->dma_buf) {
-		if (gi2c->err)
+		if (gi2c->err && !gi2c->addr_nack)
 			geni_i2c_rx_fsm_rst(gi2c);
 		geni_se_rx_dma_unprep(&gi2c->se, gi2c->dma_addr, gi2c->xfer_len);
 		i2c_put_dma_safe_msg_buf(gi2c->dma_buf, cur, !gi2c->err);
@@ -455,7 +460,7 @@ static void geni_i2c_tx_msg_cleanup(struct geni_i2c_dev *gi2c,
 {
 	gi2c->cur_wr = 0;
 	if (gi2c->dma_buf) {
-		if (gi2c->err)
+		if (gi2c->err && !gi2c->addr_nack)
 			geni_i2c_tx_fsm_rst(gi2c);
 		geni_se_tx_dma_unprep(&gi2c->se, gi2c->dma_addr, gi2c->xfer_len);
 		i2c_put_dma_safe_msg_buf(gi2c->dma_buf, cur, !gi2c->err);
@@ -493,7 +498,7 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
 
 	cur = gi2c->cur;
 	time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT);
-	if (!time_left)
+	if (!time_left || (gi2c->err && !gi2c->addr_nack))
 		geni_i2c_cancel_xfer(gi2c);
 
 	geni_i2c_rx_msg_cleanup(gi2c, cur);
@@ -535,7 +540,7 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
 
 	cur = gi2c->cur;
 	time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT);
-	if (!time_left)
+	if (!time_left || (gi2c->err && !gi2c->addr_nack))
 		geni_i2c_cancel_xfer(gi2c);
 
 	geni_i2c_tx_msg_cleanup(gi2c, cur);
@@ -917,6 +922,7 @@ static int geni_i2c_fifo_xfer(struct geni_i2c_dev *gi2c,
 		m_param |= ((msgs[i].addr << SLV_ADDR_SHFT) & SLV_ADDR_MSK);
 
 		gi2c->cur = &msgs[i];
+		gi2c->addr_nack = false;
 		gi2c->err = 0;
 		reinit_completion(&gi2c->done);
 		if (msgs[i].flags & I2C_M_RD)

-- 
2.34.1

Re: [PATCH 3/3] i2c: qcom-geni: Avoid unnecessary transfer cancel on address NACK

[Mukesh Savaliya]

On 7/8/2026 11:45 AM, Praveen Talari wrote:
> When a target does not acknowledge its address phase, the GENI hardware
> raises a NACK interrupt. In this case, SE_GENI_M_GP_LENGTH remains zero,
> indicating that no data phase has started and the transfer was aborted at
> the address stage.
> 
> The driver currently treats all transfer errors similarly and always
> issues geni_i2c_cancel_xfer() followed by TX/RX FSM resets for DMA
> transfers. For address NACKs, the transfer has already terminated in
> hardware and issuing an additional cancel/reset sequence is unnecessary.
> 
> Track address NACKs separately by checking the transfer progress length
> when a NACK interrupt is reported. Skip the transfer cancel operation and
> DMA FSM reset paths when the NACK occurred during the address phase.
> 
> This avoids redundant cancel/reset operations for a normal address NACK
> condition while preserving the existing error recovery flow for all other
> transfer failures.
> 
When a target does not acknowledge its address phase
The driver currently treats all transfer errors similarly
Track address NACKs separately
avoids redundant cancel/reset operations for a normal address NACK

Looks repeated , in general you are handling address NACK and avoiding 
cancel/reset operation. Differentiate this NACK response against real 
errors.
Try to simplify and shorten it.

> Co-developed-by: Naresh Maramaina <naresh.maramaina@oss.qualcomm.com>
> Signed-off-by: Naresh Maramaina <naresh.maramaina@oss.qualcomm.com>
> Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
> ---
>   drivers/i2c/busses/i2c-qcom-geni.c | 16 +++++++++++-----
>   1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 9490aee4928c..0448654f2678 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -130,6 +130,7 @@ struct geni_i2c_dev {
>   	struct dma_chan *rx_c;
>   	bool no_dma;
>   	bool gpi_mode;
> +	bool addr_nack;
>   	bool is_tx_multi_desc_xfer;
>   	u32 num_msgs;
>   	struct geni_i2c_gpi_multi_desc_xfer i2c_multi_desc_config;
> @@ -293,8 +294,12 @@ static irqreturn_t geni_i2c_irq(int irq, void *dev)
>   	if (!cur ||
>   	    m_stat & (M_CMD_FAILURE_EN | M_CMD_ABORT_EN) ||
>   	    dm_rx_st & (DM_I2C_CB_ERR)) {
> -		if (m_stat & M_GP_IRQ_1_EN)
> +		if (m_stat & M_GP_IRQ_1_EN) {
>   			geni_i2c_err(gi2c, NACK);
So we are handling NACK first and then deciding if its ADDR NACK ?
You can pass ADDR or DATA nack to this function right ? and print the 
error log accordingly inside.

> +			val = readl_relaxed(base + SE_GENI_M_GP_LENGTH);
> +			if (!val)
> +				gi2c->addr_nack = true;
> +		}
>   		if (m_stat & M_GP_IRQ_3_EN)
>   			geni_i2c_err(gi2c, BUS_PROTO);
>   		if (m_stat & M_GP_IRQ_4_EN)
> @@ -443,7 +448,7 @@ static void geni_i2c_rx_msg_cleanup(struct geni_i2c_dev *gi2c,
[...]

Re: [PATCH 0/3] i2c: qcom-geni: improve transfer error recovery and synchronization

[Mukesh Savaliya]

On 7/8/2026 11:45 AM, Praveen Talari wrote:
> The GENI I2C driver currently has a few issues in its transfer recovery
has few issues
> and completion handling paths.
> 
> The timeout recovery flow directly aborts outstanding commands, while
> GENI hardware requires command cancellation to be attempted before
> issuing an abort. In addition, multiple operations share a common
> completion object, which can allow unrelated events to wake waiters
> prematurely and lead to incorrect synchronization.
> 
> Address NACK handling can also trigger unnecessary cancel and reset
cancellation and reset operations
> operations even though the transfer has already terminated during the
> address phase.
> 
> Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
> ---
> Praveen Talari (3):
>        i2c: qcom-geni: use cancel command before abort on transfer timeout
>        i2c: qcom-geni: use dedicated completions for abort and reset events
>        i2c: qcom-geni: Avoid unnecessary transfer cancel on address NACK
> 
>   drivers/i2c/busses/i2c-qcom-geni.c | 100 ++++++++++++++++++++++++-------------
>   1 file changed, 66 insertions(+), 34 deletions(-)
> ---
> base-commit: 2b763db0c2763d6bf73d7d3e69665222d1f377cf
> change-id: 20260703-fix_cancel_sequence_on_failure_for_i2c-a868571f77e0
> 
> Best regards,
> --
> Praveen Talari <praveen.talari@oss.qualcomm.com>
>