Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues: - [High] The `mult_frac()` macro used to calculate `bit_usec` suffers from a 32-bit intermediate integer overflow on 32-bit platforms, undermining the author's intent to prevent it. - [High] The timeout calculated for multi-descriptor (GPI) transfers incorrectly uses the maximum individual message length rather than the sum of all queued message lengths, leading to premature timeouts for batches of small messages. Pre-existing issues: - [High] The dynamically allocated `dma_buf` and `dma_addr` arrays for multi-descriptor transfers are leaked permanently on early error paths. - [High] Concurrent lockless calls to `reinit_completion()` during multi-descriptor transfers can overwrite pending completions, leading to lost wakeups and transfer timeouts. -- --- Patch [1]: [PATCH V4] i2c: qcom-geni: Add dynamic transfer timeout based on transfer length and frequency --- commit 6638d80f15522cd0e8e676c42f903f9ce0d7a55c Author: Aniket Randive <[email protected]> Subject: i2c: qcom-geni: Add dynamic transfer timeout based on transfer length and frequency This commit introduces a dynamic timeout calculation for I2C transfers based on message length and bus frequency. It replaces a static 1-second timeout with a calculated timeout that includes a safety margin and a 300ms floor. For multi-descriptor transfers, it applies the maximum message length across the queued messages. > diff --git a/drivers/i2c/busses/i2c-qcom-geni.c > b/drivers/i2c/busses/i2c-qcom-geni.c > index 96dbf04138bee..c5c3adc8ec771 100644 > --- a/drivers/i2c/busses/i2c-qcom-geni.c > +++ b/drivers/i2c/busses/i2c-qcom-geni.c > @@ -204,6 +208,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] Does this mult_frac() calculation suffer from an intermediate integer overflow on 32-bit platforms? The size_t type and USEC_PER_SEC are both 32-bit on 32-bit architectures. Because mult_frac(x, n, d) internally computes (x % d) * n / d, for a large transfer the remainder term multiplied by 1,000,000 can easily exceed U32_MAX and silently wrap. This would truncate the calculated timeout to a value less than the theoretical wire time, causing valid large transfers to spuriously time out. > + size_t xfer_max_usec = (bit_usec * I2C_TIMEOUT_SAFETY_COEFFICIENT) + > + I2C_TIMEOUT_MIN_USEC; > + > + return usecs_to_jiffies(xfer_max_usec); > +} [ ... ] > @@ -601,13 +617,13 @@ 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; > u32 time_left; > > for (i = 0; i < multi_xfer->msg_idx_cnt - 1; i++) { > reinit_completion(transfer_comp); [Severity: High] This is a pre-existing issue, but can this concurrent lockless call to reinit_completion() cause lost wakeups? During multi-descriptor transfers, reinit_completion() is called locklessly in a loop here. Concurrently, the asynchronous DMA callback i2c_gpi_cb_result() can call complete(). Since reinit_completion() happens outside the waitqueue lock, the process thread might write done = 0 and then read a stale irq_cnt, causing it to evaluate msg_idx_cnt != irq_cnt as true and block on wait_for_completion_timeout() indefinitely. > if (multi_xfer->msg_idx_cnt != multi_xfer->irq_cnt) { > time_left = wait_for_completion_timeout(transfer_comp, > - > transfer_timeout_msecs); > + > timeout_jiffies); > if (!time_left) { > dev_err(dev, "%s: Transfer timeout\n", > __func__); > return -ETIMEDOUT; [ ... ] > @@ -736,8 +752,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 this timeout calculation cause spurious timeouts for batches of small messages? When a multi-descriptor transfer is submitted, the process thread blocks on the first iteration of geni_i2c_gpi_multi_xfer_timeout_handler() for the entire batch duration. However, the timeout here is calculated using only the maximum individual message length instead of the sum of all queued message lengths. If a batch contains many small messages, the actual wire time can exceed this calculated timeout, leading to premature timeouts. > + ret = > geni_i2c_gpi_multi_xfer_timeout_handler(gi2c->se.dev, gi2c_gpi_xfer, > - > XFER_TIMEOUT, &gi2c->done); > + timeout, > &gi2c->done); > if (ret) { > dev_err(gi2c->se.dev, [ ... ] A related pre-existing issue exists in the unmap path: 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; } ... } [Severity: High] This is a pre-existing issue, but does this logic permanently leak the dynamically allocated dma_buf and dma_addr arrays on early error paths? If an error occurs or a timeout happens before all messages are successfully processed, the loop bounds will exit before unmap_msg_cnt reaches num_msgs - 1. This causes the kfree() block to be completely skipped. Since the structure is zeroed out on subsequent runs, the pointers are permanently lost, which can lead to memory exhaustion. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
