[PATCH V5] i2c: qcom-geni: Add dynamic transfer timeout based on transfer length and frequency

[PATCH V5] i2c: qcom-geni: Add dynamic transfer timeout based on transfer length and frequency

[Aniket Randive]

The driver uses a static XFER_TIMEOUT of HZ (1 second) for all transfers
regardless of message length or bus frequency, causing unnecessary
delays on error paths.

Compute the timeout dynamically from message length and bus frequency
with a 10x safety margin over the theoretical wire time. Add a 300 ms
floor to account for I2C clock stretching and other situations where a
slave may keep SCL asserted for an extended period, including faulty
devices holding the bus.

This replaces the fixed 1-second timeout with a transfer-specific
timeout while preserving sufficient margin for software overheads and
bus-level delays.

For GPI multi-descriptor transfers, use the maximum message length
across all queued messages as the per-completion timeout.

Signed-off-by: Aniket Randive <aniket.randive@oss.qualcomm.com>
---

Changes in V5:
- Updated the commit message as per the review comments.

Changes in V4:
- As per konrad suggestion used mult_frac() for bit_usec to avoid
 intermediate overflow on 32-bit targets.
- Updated the commit message and added a driver comment explaining the
 rationale for the 0.3-second minimum timeout floor value.

 drivers/i2c/busses/i2c-qcom-geni.c | 47 +++++++++++++++++++++++-------
 1 file changed, 37 insertions(+), 10 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index 96dbf04138be..f011d2564cd2 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -74,9 +74,14 @@ enum geni_i2c_err_code {
 #define PACKING_BYTES_PW	4
 
 #define ABORT_TIMEOUT		HZ
-#define XFER_TIMEOUT		HZ
 #define RST_TIMEOUT		HZ
 
+/* 9 bits per byte (8 data + 1 ACK), 10x safety margin */
+#define I2C_TIMEOUT_SAFETY_COEFFICIENT	10
+
+/* 300ms floor: budget for clock stretching; slave may hold SCL low indefinitely */
+#define I2C_TIMEOUT_MIN_USEC		300000
+
 struct geni_i2c_desc {
 	bool no_dma_support;
 	unsigned int tx_fifo_depth;
@@ -204,6 +209,16 @@ static int geni_i2c_clk_map_idx(struct geni_i2c_dev *gi2c)
 	return -EINVAL;
 }
 
+static unsigned long geni_i2c_xfer_timeout(struct geni_i2c_dev *gi2c, size_t len)
+{
+	size_t bit_cnt = len * 9;
+	size_t bit_usec = mult_frac(bit_cnt, USEC_PER_SEC, gi2c->clk_freq_out);
+	size_t xfer_max_usec = (bit_usec * I2C_TIMEOUT_SAFETY_COEFFICIENT) +
+			       I2C_TIMEOUT_MIN_USEC;
+
+	return usecs_to_jiffies(xfer_max_usec);
+}
+
 static int qcom_geni_i2c_conf(struct geni_se *se, unsigned long freq)
 {
 	struct geni_i2c_dev *gi2c = dev_get_drvdata(se->dev);
@@ -445,7 +460,7 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
 				u32 m_param)
 {
 	dma_addr_t rx_dma = 0;
-	unsigned long time_left;
+	unsigned long time_left, timeout;
 	void *dma_buf;
 	struct geni_se *se = &gi2c->se;
 	size_t len = msg->len;
@@ -470,8 +485,9 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
 		gi2c->dma_buf = dma_buf;
 	}
 
+	timeout = geni_i2c_xfer_timeout(gi2c, len);
 	cur = gi2c->cur;
-	time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT);
+	time_left = wait_for_completion_timeout(&gi2c->done, timeout);
 	if (!time_left)
 		geni_i2c_abort_xfer(gi2c);
 
@@ -484,7 +500,7 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
 				u32 m_param)
 {
 	dma_addr_t tx_dma = 0;
-	unsigned long time_left;
+	unsigned long time_left, timeout;
 	void *dma_buf;
 	struct geni_se *se = &gi2c->se;
 	size_t len = msg->len;
@@ -512,8 +528,9 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
 	if (!dma_buf) /* Get FIFO IRQ */
 		writel_relaxed(1, se->base + SE_GENI_TX_WATERMARK_REG);
 
+	timeout = geni_i2c_xfer_timeout(gi2c, len);
 	cur = gi2c->cur;
-	time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT);
+	time_left = wait_for_completion_timeout(&gi2c->done, timeout);
 	if (!time_left)
 		geni_i2c_abort_xfer(gi2c);
 
@@ -591,7 +608,7 @@ static void geni_i2c_gpi_multi_desc_unmap(struct geni_i2c_dev *gi2c, struct i2c_
  * geni_i2c_gpi_multi_xfer_timeout_handler() - Handles multi message transfer timeout
  * @dev: Pointer to the corresponding dev node
  * @multi_xfer: Pointer to the geni_i2c_gpi_multi_desc_xfer
- * @transfer_timeout_msecs: Timeout value in milliseconds
+ * @transfer_timeout_msecs: Per-message completion timeout in jiffies
  * @transfer_comp: Completion object of the transfer
  *
  * This function waits for the completion of each processed transfer messages
@@ -601,7 +618,7 @@ static void geni_i2c_gpi_multi_desc_unmap(struct geni_i2c_dev *gi2c, struct i2c_
  */
 static int geni_i2c_gpi_multi_xfer_timeout_handler(struct device *dev,
 						   struct geni_i2c_gpi_multi_desc_xfer *multi_xfer,
-						   u32 transfer_timeout_msecs,
+						   unsigned long timeout_jiffies,
 						   struct completion *transfer_comp)
 {
 	int i;
@@ -612,7 +629,7 @@ static int geni_i2c_gpi_multi_xfer_timeout_handler(struct device *dev,
 
 		if (multi_xfer->msg_idx_cnt != multi_xfer->irq_cnt) {
 			time_left = wait_for_completion_timeout(transfer_comp,
-								transfer_timeout_msecs);
+								timeout_jiffies);
 			if (!time_left) {
 				dev_err(dev, "%s: Transfer timeout\n", __func__);
 				return -ETIMEDOUT;
@@ -736,8 +753,16 @@ static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[],
 		dma_async_issue_pending(gi2c->tx_c);
 
 		if ((msg_idx == (gi2c->num_msgs - 1)) || flags & DMA_PREP_INTERRUPT) {
+			unsigned long timeout;
+			size_t max_len = 0;
+			int j;
+
+			for (j = 0; j < gi2c->num_msgs; j++)
+				max_len = max_t(size_t, max_len, msgs[j].len);
+
+			timeout = geni_i2c_xfer_timeout(gi2c, max_len);
 			ret = geni_i2c_gpi_multi_xfer_timeout_handler(gi2c->se.dev, gi2c_gpi_xfer,
-								      XFER_TIMEOUT, &gi2c->done);
+								      timeout, &gi2c->done);
 			if (ret) {
 				dev_err(gi2c->se.dev,
 					"I2C multi write msg transfer timeout: %d\n",
@@ -851,8 +876,10 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
 		}
 
 		if (!gi2c->is_tx_multi_desc_xfer) {
+			unsigned long timeout = geni_i2c_xfer_timeout(gi2c, msgs[i].len);
+
 			dma_async_issue_pending(gi2c->tx_c);
-			time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT);
+			time_left = wait_for_completion_timeout(&gi2c->done, timeout);
 			if (!time_left) {
 				dev_err(gi2c->se.dev, "%s:I2C timeout\n", __func__);
 				gi2c->err = -ETIMEDOUT;
-- 
2.34.1

Re: [PATCH V5] i2c: qcom-geni: Add dynamic transfer timeout based on transfer length and frequency

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] A 32-bit integer overflow in `mult_frac()` causes the calculated timeout to be severely truncated on 32-bit targets for moderately large I2C transfers, leading to premature aborts.
- [High] For multi-descriptor transfers, the timeout is incorrectly calculated based solely on the maximum single message length rather than the sum of all message lengths in the batch.
- [Low] The kernel-doc block for `geni_i2c_gpi_multi_xfer_timeout_handler` has an outdated parameter name, causing a kernel-doc warning.
--

--- Patch [1]: [PATCH V5] i2c: qcom-geni: Add dynamic transfer timeout based on transfer length and frequency ---
commit 99a3ffd517ce0554f8d902961d8c4336195d5457
Author: Aniket Randive <aniket.randive@oss.qualcomm.com>

i2c: qcom-geni: Add dynamic transfer timeout based on transfer length and frequency

This patch replaces the static 1-second transfer timeout with a dynamic 
timeout calculated from the message length and bus frequency. It adds a 
300 ms floor for clock stretching and updates the GPI multi-descriptor 
transfer to use a maximum message length timeout.

> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 96dbf04138bee..f011d2564cd20 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -204,6 +209,16 @@ static int geni_i2c_clk_map_idx(struct geni_i2c_dev *gi2c)
>  	return -EINVAL;
>  }
>  
> +static unsigned long geni_i2c_xfer_timeout(struct geni_i2c_dev *gi2c, size_t len)
> +{
> +	size_t bit_cnt = len * 9;
> +	size_t bit_usec = mult_frac(bit_cnt, USEC_PER_SEC, gi2c->clk_freq_out);

[Severity: High]
Can this cause an integer overflow on 32-bit architectures?

Looking at mult_frac(x, n, d), it calculates r * n_ / d_ where r is the
remainder of bit_cnt % gi2c->clk_freq_out and n_ is USEC_PER_SEC (1,000,000).

If size_t is 32-bit, r * USEC_PER_SEC will exceed U32_MAX for remainders
larger than 4294. This wraps around and results in a truncated timeout
that might prematurely abort valid large I2C transfers.

[ ... ]

> @@ -591,7 +608,7 @@ static void geni_i2c_gpi_multi_desc_unmap(struct geni_i2c_dev *gi2c, struct i2c_
>   * geni_i2c_gpi_multi_xfer_timeout_handler() - Handles multi message transfer timeout
>   * @dev: Pointer to the corresponding dev node
>   * @multi_xfer: Pointer to the geni_i2c_gpi_multi_desc_xfer
> - * @transfer_timeout_msecs: Timeout value in milliseconds
> + * @transfer_timeout_msecs: Per-message completion timeout in jiffies

[Severity: Low]
This isn't a bug, but the kernel-doc parameter name @transfer_timeout_msecs
was not updated to match the new function argument timeout_jiffies below,
which will cause a kernel-doc warning during compilation.

>   * @transfer_comp: Completion object of the transfer
>   *
>   * This function waits for the completion of each processed transfer messages
> @@ -601,7 +618,7 @@ static void geni_i2c_gpi_multi_desc_unmap(struct geni_i2c_dev *gi2c, struct i2c_
>   */
>  static int geni_i2c_gpi_multi_xfer_timeout_handler(struct device *dev,
>  						   struct geni_i2c_gpi_multi_desc_xfer *multi_xfer,
> -						   u32 transfer_timeout_msecs,
> +						   unsigned long timeout_jiffies,
>  						   struct completion *transfer_comp)

[ ... ]

> @@ -736,8 +753,16 @@ static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[],
>  		dma_async_issue_pending(gi2c->tx_c);
>  
>  		if ((msg_idx == (gi2c->num_msgs - 1)) || flags & DMA_PREP_INTERRUPT) {
> +			unsigned long timeout;
> +			size_t max_len = 0;
> +			int j;
> +
> +			for (j = 0; j < gi2c->num_msgs; j++)
> +				max_len = max_t(size_t, max_len, msgs[j].len);
> +
> +			timeout = geni_i2c_xfer_timeout(gi2c, max_len);

[Severity: High]
Will computing the timeout based on max_len cause multi-descriptor batches
to time out prematurely?

Since this is a Block Event Interrupt (BEI) DMA transfer, intermediate
interrupts are suppressed and the hardware will take time proportional to
the total sum of all message lengths in the batch. 

Calculating the timeout using only the maximum single message length rather
than the cumulative length might result in a wait timeout that is shorter
than the physical wire time needed to transmit the entire batch.

>  			ret = geni_i2c_gpi_multi_xfer_timeout_handler(gi2c->se.dev, gi2c_gpi_xfer,
> -								      XFER_TIMEOUT, &gi2c->done);
> +								      timeout, &gi2c->done);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715101805.3615166-1-aniket.randive@oss.qualcomm.com?part=1

Re: [PATCH V5] i2c: qcom-geni: Add dynamic transfer timeout based on transfer length and frequency

[Dmitry Guzman]

On Wed, 15 Jul 2026 15:48:05 +0530
Aniket Randive <aniket.randive@oss.qualcomm.com> wrote:

> This replaces the fixed 1-second timeout with a transfer-specific
> timeout while preserving sufficient margin for software overheads and
> bus-level delays.
> 

The dynamic timeout may be useful for any I2C bus controller, not only
for qcom-geni. Structure i2c_adapter already has field "timeout". Isn't
it worth to move the timeout calculation to i2c-core, so that the core
updates the timeout field in i2c_adapter structure, and the controller
driver just uses this value, instead of duplicating the calculation in
every driver that will use this feature?

Also, such parameters as I2C_TIMEOUT_SAFETY_COEFFICIENT and
I2C_TIMEOUT_MIN_USEC may be configurable by user (for example, in
device tree) for more flexibility.

Best regards,
-- 
Dmitry Guzman <Dmitry.Guzman@mobileye.com>

Re: [PATCH V5] i2c: qcom-geni: Add dynamic transfer timeout based on transfer length and frequency

[Mukesh Savaliya]

On 7/15/2026 5:15 PM, Dmitry Guzman wrote:
> On Wed, 15 Jul 2026 15:48:05 +0530
> Aniket Randive <aniket.randive@oss.qualcomm.com> wrote:
> 
>> This replaces the fixed 1-second timeout with a transfer-specific
>> timeout while preserving sufficient margin for software overheads and
>> bus-level delays.
>>
> 
> The dynamic timeout may be useful for any I2C bus controller, not only
> for qcom-geni. Structure i2c_adapter already has field "timeout". Isn't
This is good, thanks for pointing. For geni driver, please review 
gi2c->adap.timeout (client->adapter->timeout) can be used and you may 
explore retries also in case you want to retry.
> it worth to move the timeout calculation to i2c-core, so that the core
> updates the timeout field in i2c_adapter structure, and the controller
> driver just uses this value, instead of duplicating the calculation in
> every driver that will use this feature?
> 
> Also, such parameters as I2C_TIMEOUT_SAFETY_COEFFICIENT and
> I2C_TIMEOUT_MIN_USEC may be configurable by user (for example, in
> device tree) for more flexibility.
> 
I think it's good to keep it within driver, not to expose as 
configurable. This would keep changing per user/system load, but ideal 
timing is theoretical + some offset, hence can be better managed by driver.

> Best regards,