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 3 potential issue(s) to consider:
- [High] Integer overflow on 32-bit architectures in dynamic timeout calculation causes premature timeouts for large messages.
- [High] Multi-descriptor DMA transfers incorrectly calculate the batched transfer timeout using the maximum single message length rather than the aggregate transfer length, causing premature timeouts.
- [Low] Kernel-doc parameter name mismatch for `timeout_jiffies` in `geni_i2c_gpi_multi_xfer_timeout_handler`.
--

commit 7cdbc3fca002264681a23e9e8f274275a5451975
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.

> 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 architectures?

If size_t is 32-bit, bit_cnt * USEC_PER_SEC can wrap around for large
I2C transfers, such as firmware uploads or EEPROM writes over 8KB. This
would produce a calculated timeout much smaller than the actual wire time,
causing operations to prematurely abort.

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

[ ... ]

>  * 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 does the kernel-doc parameter name match the function
signature?

The comment documents transfer_timeout_msecs, but the function signature
below was changed to take timeout_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)

[ ... ]

> @@ -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 calculate the timeout correctly for multi-descriptor batches?

For multi-descriptor transfers, the hardware processes the entire batch before
raising a single interrupt on the final descriptor. Wait for completion uses a
timeout computed from max_len (the length of the single longest message).

If a batch contains many small messages, their cumulative wire time could
easily exceed the timeout calculated for the single longest message, leading
to premature timeouts. Should this calculate the sum of all message lengths
instead of taking the maximum?

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