Re: [PATCH v2] 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 2 potential issue(s) to consider:
- [High] The calculation of `bit_usec` can overflow `size_t` on 32-bit architectures for large I2C transfers, leading to drastically reduced timeouts.
- [High] GPI multi-descriptor timeout relies on the maximum single message length rather than the total transfer length, causing premature timeouts for batched transfers.
--

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

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

This commit replaces the static 1-second timeout for I2C transfers with a
dynamic calculation based on message length and bus frequency. It uses a 10x
safety margin with a 300ms floor, and applies the maximum message length for
batched multi-descriptor transfers.

> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 96dbf04138bee..d43db77b36781 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 size_t on 32-bit architectures?

Since size_t is 32 bits on 32-bit platforms, the intermediate result of
bit_cnt * USEC_PER_SEC can exceed the 32-bit maximum before the division
takes place.

If a message is larger than 477 bytes, bit_cnt becomes greater than 4293.
Multiplying that by 1,000,000 overflows the 32-bit value, which truncates
the result.

This could lead to drastically reduced timeouts and premature -ETIMEDOUT
failures for large I2C transfers.

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

[ ... ]

> @@ -736,8 +751,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);

[Severity: High]
Does this timeout calculation correctly account for the time required to
transmit all messages in a batched multi-descriptor transfer?

The loop calculates max_len based on the longest single message in the
chain. However, the hardware processes the entire batch of messages
sequentially.

Because DMA_PREP_INTERRUPT is typically only set on the final message
in geni_i2c_gpi(), the completion handler blocks in
wait_for_completion_timeout() while waiting for the entire batch to finish
transmitting.

If a batch contains many messages, the cumulative transmission time might
exceed the 10x safety margin of the single largest message, resulting in a
spurious transfer timeout.

>  			if (ret) {
>  				dev_err(gi2c->se.dev,
>  					"I2C multi write msg transfer timeout: %d\n",

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