[PATCH v2] i2c: qcom-geni: Add dynamic transfer timeout based on transfer length and frequency
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 and a 300ms floor. 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 v2: - Remove accidental defconfig change.
drivers/i2c/busses/i2c-qcom-geni.c | 37 +++++++++++++++++++++++------- 1 file changed, 29 insertions(+), 8 deletions(-)
diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c index 96dbf04138be..43ae2121f01c 100644 --- a/drivers/i2c/busses/i2c-qcom-geni.c +++ b/drivers/i2c/busses/i2c-qcom-geni.c @@ -74,9 +74,12 @@ 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, 300ms floor */ +#define I2C_TIMEOUT_SAFETY_COEFFICIENT 10 +#define I2C_TIMEOUT_MIN_USEC 300000 + struct geni_i2c_desc { bool no_dma_support; unsigned int tx_fifo_depth; @@ -204,6 +207,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 = (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); @@ -471,7 +484,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); + time_left = wait_for_completion_timeout(&gi2c->done, geni_i2c_xfer_timeout(gi2c, len)); if (!time_left) geni_i2c_abort_xfer(gi2c);
@@ -513,7 +526,7 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg, writel_relaxed(1, se->base + SE_GENI_TX_WATERMARK_REG);
cur = gi2c->cur; - time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT); + time_left = wait_for_completion_timeout(&gi2c->done, geni_i2c_xfer_timeout(gi2c, len)); if (!time_left) geni_i2c_abort_xfer(gi2c);
@@ -591,7 +604,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 +614,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 +625,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 +749,15 @@ 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) { + 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); + ret = geni_i2c_gpi_multi_xfer_timeout_handler(gi2c->se.dev, gi2c_gpi_xfer, - XFER_TIMEOUT, &gi2c->done); + geni_i2c_xfer_timeout( + gi2c, max_len), &gi2c->done); if (ret) { dev_err(gi2c->se.dev, "I2C multi write msg transfer timeout: %d\n", @@ -852,7 +872,8 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
if (!gi2c->is_tx_multi_desc_xfer) { 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, geni_i2c_xfer_timeout( + gi2c, msgs[i].len)); if (!time_left) { dev_err(gi2c->se.dev, "%s:I2C timeout\n", __func__); gi2c->err = -ETIMEDOUT;
On 7/5/26 3:57 PM, Aniket Randive wrote:
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 and a 300ms floor. For GPI multi-descriptor transfers, use the maximum message length across all queued messages as the per-completion timeout.
What's the reason for a 0.3 s floor?
Why a 10x safety margin specifically?
[...]
+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 = (bit_cnt * USEC_PER_SEC) / gi2c->clk_freq_out;
mult_frac()
Konrad
Thanks Konrad for the review.
Sorry, I missed your comments on the v2 patch and ended up posting v3. I'm happy to post a v4 incorporating your feedback once the discussion concludes.
On 7/6/2026 5:11 PM, Konrad Dybcio wrote:
On 7/5/26 3:57 PM, Aniket Randive wrote:
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 and a 300ms floor. For GPI multi-descriptor transfers, use the maximum message length across all queued messages as the per-completion timeout.
What's the reason for a 0.3 s floor?
The floor accounts for I2C clock stretching. The spec allows slaves to hold SCL low indefinitely during internal processing. A dynamically computed xfer time alone gives no time for that. 300ms value covers worst-case stretching while still detecting real hangs 3x faster than the old 1s static timeout.
Thanks, Aniket
Why a 10x safety margin specifically?
[...]
The multiplier covers the gap between theoretical xfer time and actual completion time (DMA descriptor setup, interrupt latency, and scheduling jitter on a loaded system) Without it, short transfers would have almost no extra time before a spurious timeout.
Thanks, Aniket
+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 = (bit_cnt * USEC_PER_SEC) / gi2c->clk_freq_out;
mult_frac()
Konrad
Good catch. I'll switch the calculation to mult_frac() as suggested in the next v4 patch.
Thanks, Aniket
On 7/9/26 8:28 AM, Aniket RANDIVE wrote:
Thanks Konrad for the review.
Sorry, I missed your comments on the v2 patch and ended up posting v3. I'm happy to post a v4 incorporating your feedback once the discussion concludes.
On 7/6/2026 5:11 PM, Konrad Dybcio wrote:
On 7/5/26 3:57 PM, Aniket Randive wrote:
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 and a 300ms floor. For GPI multi-descriptor transfers, use the maximum message length across all queued messages as the per-completion timeout.
What's the reason for a 0.3 s floor?
The floor accounts for I2C clock stretching. The spec allows slaves to hold SCL low indefinitely during internal processing. A dynamically computed xfer time alone gives no time for that. 300ms value covers worst-case stretching while still detecting real hangs 3x faster than the old 1s static timeout.
Please put that in the commit message and possibly in the code as a comment
Thanks, Aniket
Why a 10x safety margin specifically?
[...]
The multiplier covers the gap between theoretical xfer time and actual completion time (DMA descriptor setup, interrupt latency, and scheduling jitter on a loaded system) Without it, short transfers would have almost no extra time before a spurious timeout.
Likewise
(Should there be a constant safety margin added to account for all that? Keep in mind this driver will run on a turbofast Glymur and on a notsofast Agatti so any numbers that depend on the processor's speed must be reasonable for both)
Konrad
On 7/10/2026 2:57 PM, Konrad Dybcio wrote:
On 7/9/26 8:28 AM, Aniket RANDIVE wrote:
Thanks Konrad for the review.
Sorry, I missed your comments on the v2 patch and ended up posting v3. I'm happy to post a v4 incorporating your feedback once the discussion concludes.
On 7/6/2026 5:11 PM, Konrad Dybcio wrote:
On 7/5/26 3:57 PM, Aniket Randive wrote:
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 and a 300ms floor. For GPI multi-descriptor transfers, use the maximum message length across all queued messages as the per-completion timeout.
What's the reason for a 0.3 s floor?
The floor accounts for I2C clock stretching. The spec allows slaves to hold SCL low indefinitely during internal processing. A dynamically computed xfer time alone gives no time for that. 300ms value covers worst-case stretching while still detecting real hangs 3x faster than the old 1s static timeout.
Please put that in the commit message and possibly in the code as a comment
Sure. I will update the commit message accordingly and add a corresponding comment in the driver as well. Thanks, Aniket
Thanks, Aniket
Why a 10x safety margin specifically?
[...]
The multiplier covers the gap between theoretical xfer time and actual completion time (DMA descriptor setup, interrupt latency, and scheduling jitter on a loaded system) Without it, short transfers would have almost no extra time before a spurious timeout.
Likewise
(Should there be a constant safety margin added to account for all that? Keep in mind this driver will run on a turbofast Glymur and on a notsofast Agatti so any numbers that depend on the processor's speed must be reasonable for both)
Konrad
The 10x multiplier was chosen as a conservative guard band to account for the gap between theoretical transfer time and actual completion time, including DMA setup, interrupt handling, and scheduling delays. Although the value is not derived from a formal worst-case latency analysis, it provides sufficient tolerance to accommodate runtime variability and helps avoid spurious timeouts across a wide range of platforms and system load conditions.
Thanks, Aniket
On 7/5/2026 7:27 PM, Aniket Randive wrote: [...]
static int qcom_geni_i2c_conf(struct geni_se *se, unsigned long freq) { struct geni_i2c_dev *gi2c = dev_get_drvdata(se->dev); @@ -471,7 +484,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);
- time_left = wait_for_completion_timeout(&gi2c->done, geni_i2c_xfer_timeout(gi2c, len));
Simplify with timeout = geni_i2c_xfer_timeout(gi2c, len) and use as an arg ?
if (!time_left) geni_i2c_abort_xfer(gi2c); @@ -513,7 +526,7 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg, writel_relaxed(1, se->base + SE_GENI_TX_WATERMARK_REG); cur = gi2c->cur;
- time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT);
- time_left = wait_for_completion_timeout(&gi2c->done, geni_i2c_xfer_timeout(gi2c, len)); if (!time_left) geni_i2c_abort_xfer(gi2c);
@@ -591,7 +604,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 +614,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)@@ -612,7 +625,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 +749,15 @@ 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) {
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);ret = geni_i2c_gpi_multi_xfer_timeout_handler(gi2c->se.dev, gi2c_gpi_xfer,
XFER_TIMEOUT, &gi2c->done);
geni_i2c_xfer_timeout(gi2c, max_len), &gi2c->done);
Simplify with timeout = geni_i2c_xfer_timeout(gi2c, len) and use as an arg ?
if (ret) { dev_err(gi2c->se.dev, "I2C multi write msg transfer timeout: %d\n",@@ -852,7 +872,8 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i if (!gi2c->is_tx_multi_desc_xfer) { 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, geni_i2c_xfer_timeout(gi2c, msgs[i].len)); if (!time_left) { dev_err(gi2c->se.dev, "%s:I2C timeout\n", __func__); gi2c->err = -ETIMEDOUT;
Thanks Mukesh for the review.
On 7/7/2026 5:37 PM, Mukesh Savaliya wrote:
On 7/5/2026 7:27 PM, Aniket Randive wrote: [...]
static int qcom_geni_i2c_conf(struct geni_se *se, unsigned long freq) { struct geni_i2c_dev *gi2c = dev_get_drvdata(se->dev); @@ -471,7 +484,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); + time_left = wait_for_completion_timeout(&gi2c->done, geni_i2c_xfer_timeout(gi2c, len));
Simplify with timeout = geni_i2c_xfer_timeout(gi2c, len) and use as an arg ?
Yes. we can add extra variable. I though to avoid extra variable.
if (!time_left) geni_i2c_abort_xfer(gi2c); @@ -513,7 +526,7 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg, writel_relaxed(1, se->base + SE_GENI_TX_WATERMARK_REG); cur = gi2c->cur; - time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT); + time_left = wait_for_completion_timeout(&gi2c->done, geni_i2c_xfer_timeout(gi2c, len)); if (!time_left) geni_i2c_abort_xfer(gi2c); @@ -591,7 +604,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 +614,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 +625,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 +749,15 @@ 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) { + 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);
ret = geni_i2c_gpi_multi_xfer_timeout_handler(gi2c-
se.dev, gi2c_gpi_xfer,
- XFER_TIMEOUT, &gi2c->done); + geni_i2c_xfer_timeout( + gi2c, max_len), &gi2c->done);
Simplify with timeout = geni_i2c_xfer_timeout(gi2c, len) and use as an arg ?
if (ret) { dev_err(gi2c->se.dev, "I2C multi write msg transfer timeout: %d\n", @@ -852,7 +872,8 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i if (!gi2c->is_tx_multi_desc_xfer) { 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, geni_i2c_xfer_timeout( + gi2c, msgs[i].len)); if (!time_left) { dev_err(gi2c->se.dev, "%s:I2C timeout\n", __func__); gi2c->err = -ETIMEDOUT;
linaro-mm-sig@lists.linaro.org
-
Aniket RANDIVE -
Aniket Randive
-
Konrad Dybcio -
Mukesh Savaliya