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

[sashiko-bot@kernel.org]

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

New issues:
- [High] The `mult_frac()` macro used to calculate `bit_usec` suffers from a 32-bit intermediate integer overflow on 32-bit platforms, undermining the author's intent to prevent it.
- [High] The timeout calculated for multi-descriptor (GPI) transfers incorrectly uses the maximum individual message length rather than the sum of all queued message lengths, leading to premature timeouts for batches of small messages.

Pre-existing issues:
- [High] The dynamically allocated `dma_buf` and `dma_addr` arrays for multi-descriptor transfers are leaked permanently on early error paths.
- [High] Concurrent lockless calls to `reinit_completion()` during multi-descriptor transfers can overwrite pending completions, leading to lost wakeups and transfer timeouts.
--

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

This commit introduces a dynamic timeout calculation for I2C transfers based
on message length and bus frequency. It replaces a static 1-second timeout
with a calculated timeout that includes a safety margin and a 300ms floor.
For multi-descriptor transfers, it applies the maximum message length across
the queued messages.

> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 96dbf04138bee..c5c3adc8ec771 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -204,6 +208,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]
Does this mult_frac() calculation suffer from an intermediate integer
overflow on 32-bit platforms?

The size_t type and USEC_PER_SEC are both 32-bit on 32-bit architectures.
Because mult_frac(x, n, d) internally computes (x % d) * n / d, for a
large transfer the remainder term multiplied by 1,000,000 can easily exceed
U32_MAX and silently wrap. This would truncate the calculated timeout to a
value less than the theoretical wire time, causing valid large transfers
to spuriously time out.

> +	size_t xfer_max_usec = (bit_usec * I2C_TIMEOUT_SAFETY_COEFFICIENT) +
> +			       I2C_TIMEOUT_MIN_USEC;
> +
> +	return usecs_to_jiffies(xfer_max_usec);
> +}

[ ... ]

> @@ -601,13 +617,13 @@ 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;
>  	u32 time_left;
>  
>  	for (i = 0; i < multi_xfer->msg_idx_cnt - 1; i++) {
>  		reinit_completion(transfer_comp);

[Severity: High]
This is a pre-existing issue, but can this concurrent lockless call to
reinit_completion() cause lost wakeups?

During multi-descriptor transfers, reinit_completion() is called locklessly
in a loop here. Concurrently, the asynchronous DMA callback i2c_gpi_cb_result()
can call complete(). Since reinit_completion() happens outside the waitqueue
lock, the process thread might write done = 0 and then read a stale irq_cnt,
causing it to evaluate msg_idx_cnt != irq_cnt as true and block on
wait_for_completion_timeout() indefinitely.

>  		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 +752,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 this timeout calculation cause spurious timeouts for batches of small
messages?

When a multi-descriptor transfer is submitted, the process thread blocks
on the first iteration of geni_i2c_gpi_multi_xfer_timeout_handler() for
the entire batch duration. However, the timeout here is calculated using
only the maximum individual message length instead of the sum of all queued
message lengths. If a batch contains many small messages, the actual wire time
can exceed this calculated timeout, leading to premature timeouts.

> +			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,

[ ... ]

A related pre-existing issue exists in the unmap path:

drivers/i2c/busses/i2c-qcom-geni.c:geni_i2c_gpi_multi_desc_unmap() {
    ...
    if (tx_multi_xfer->unmap_msg_cnt == gi2c->num_msgs - 1) {
        kfree(tx_multi_xfer->dma_buf);
        kfree(tx_multi_xfer->dma_addr);
        break;
    }
    ...
}

[Severity: High]
This is a pre-existing issue, but does this logic permanently leak the
dynamically allocated dma_buf and dma_addr arrays on early error paths?

If an error occurs or a timeout happens before all messages are successfully
processed, the loop bounds will exit before unmap_msg_cnt reaches
num_msgs - 1. This causes the kfree() block to be completely skipped.
Since the structure is zeroed out on subsequent runs, the pointers are
permanently lost, which can lead to memory exhaustion.

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