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