Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues: - [High] Dynamic timeout calculation causes an integer overflow on 32-bit architectures for large transfers, leading to premature timeouts. - [High] Multi-descriptor transfer timeout calculation uses `max_len` instead of total transfer length, causing premature timeouts for batches. - [Low] Mismatch between kernel-doc comment and function signature for `geni_i2c_gpi_multi_xfer_timeout_handler`. Pre-existing issues: - [High] Data race and missed wakeup in `geni_i2c_gpi_multi_xfer_timeout_handler` due to lockless `reinit_completion` without memory barriers. -- --- Patch [1]: [PATCH v3] i2c: qcom-geni: Add dynamic transfer timeout based on transfer length and frequency --- commit d129d3fb4c6f62c77ad085c820aa98f9027086bf 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 transfer timeout with a dynamically computed timeout based on message length and bus frequency. It aims to reduce unnecessary delays on error paths while adding a safety margin and a 300ms floor. For multi-descriptor transfers, it computes the per-completion timeout using the maximum message length in the batch. > 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 on 32-bit architectures? Since size_t and USEC_PER_SEC are 32-bit values on these platforms, does a transfer length of 478 bytes or more silently wrap around and result in a prematurely small timeout? > + size_t xfer_max_usec = (bit_usec * I2C_TIMEOUT_SAFETY_COEFFICIENT) + > + I2C_TIMEOUT_MIN_USEC; > + > + return usecs_to_jiffies(xfer_max_usec); > +} > + [ ... ] > @@ -591,7 +606,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] Is the kernel-doc parameter name mismatched here? The function signature below updates the parameter name to timeout_jiffies. > * @transfer_comp: Completion object of the transfer > * > * This function waits for the completion of each processed transfer messages > @@ -601,7 +616,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) > { > int i; > @@ -612,7 +627,7 @@ static int geni_i2c_gpi_multi_xfer_timeout_handler(struct > device *dev, > > if (multi_xfer->msg_idx_cnt != multi_xfer->irq_cnt) { [Severity: High] This is a pre-existing issue, but does this sequence lack memory barriers? Could the lockless read of multi_xfer->irq_cnt be reordered before the store to done in the preceding reinit_completion(transfer_comp) call, potentially causing a missed wakeup and subsequent hang? > 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 +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); [Severity: High] Since the hardware takes time proportional to the sum of all message lengths, does calculating the timeout based only on max_len cause premature timeouts for batches containing many messages? > 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, > "I2C multi write msg transfer timeout: > %d\n", -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
