Re: [PATCH v1] 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 dynamic timeout calculation overflows a 32-bit `size_t` for large I2C transfers, resulting in spuriously short timeouts on 32-bit platforms.
- [High] The timeout for GPI multi-descriptor transfers incorrectly uses the maximum single message length instead of the sum of all message lengths, causing batched transfers to time out prematurely.

Pre-existing issues:
- [High] The `dma_buf` and `dma_addr` arrays in multi-descriptor transfers are leaked if a setup error occurs before the last message.
- [High] The multi-descriptor DMA completion wait loop is off-by-one, leading to either early returns and Use-After-Free, or spurious timeouts.
--

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

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.

> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 96dbf04138bee..43ae2121f01c2 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
[ ... ]
> @@ -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;

[Severity: High]
Can this calculation overflow on 32-bit platforms?

If USEC_PER_SEC is 1000000 and size_t is 32-bit, large I2C transfers
(e.g. len >= 478 bytes) could cause bit_cnt * USEC_PER_SEC to exceed
the 32-bit maximum.

This would truncate the upper bits of the result, causing legitimate large
I2C transfers to fail systematically with -ETIMEDOUT because the calculated
timeout becomes much shorter than the actual required wire time.

> +	size_t xfer_max_usec = (bit_usec * I2C_TIMEOUT_SAFETY_COEFFICIENT) +
> +			       I2C_TIMEOUT_MIN_USEC;
> +
> +	return usecs_to_jiffies(xfer_max_usec);
> +}
[ ... ]
> @@ -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);

[Severity: High]
This isn't a bug introduced by this patch, but does the loop bound in this
function cause synchronization issues for multi-message batches?

For a batch of 3 messages, the loop bounded by msg_idx_cnt - 1 executes
exactly twice.

If the DMA hardware is configured for Block Event Interrupts and signals
only once at the very end, the first wait would complete, but the second
iteration could time out because no further interrupts arrive.

Conversely, if it interrupts on every descriptor, returning early after 2
iterations would leave the 3rd message in flight, allowing the caller to
free the message buffers while the DMA is still accessing them.

> +								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);

[Severity: High]
Does this use of a single message's maximum length provide a long enough
timeout for the entire batched transfer?

When using multi-descriptor transfers, the DMA controller executes the batch
sequentially.

The single wait_for_completion_timeout call here will block for the
duration of the entire batch.

If a batch contains many messages (e.g. 50 messages of 100 bytes each),
the combined wire time could easily exceed the timeout calculated for just
the single largest message, causing the batch transfer to time out
prematurely.

[Severity: High]
This is a pre-existing issue, but does early failure in multi-descriptor
setup leak the DMA tracking arrays?

Looking at the unmap function called on early aborts:

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;
    }
    ...
}

If an error is encountered before the last message during intermediate
descriptor preparation (like a failure in dma_map_single), the unmap loop
won't reach gi2c->num_msgs - 1.

This leaves the arrays allocated, and since the tracking pointers are
cleared by a memset() at the start of the next transfer, this memory
is permanently leaked.

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