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 <[email protected]> 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/[email protected]?part=1
