Re: [PATCH V5] 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 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