Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] `amd_i2c_dw_xfer_quirk()` returns 0 on success instead of the number of successfully transferred messages, violating the I2C `master_xfer` algorithm contract. - [High] `amd_i2c_dw_xfer_quirk()` fails to read the RX FIFO for read messages that are not the final message in a transaction, leaving client buffers uninitialized. -- commit dd05c8cde6068b0c819673691fc4626bca60cd8f Author: Dmitry Guzman <[email protected]> i2c: designware: use separate i2c_algorithm for AMD_NAVI_GPU This commit creates a separate i2c_algorithm structure for the AMD_NAVI_GPU variant instead of checking flags at runtime. The pointer to amd_i2c_dw_xfer_quirk is placed directly in the new structure, and the function signature is updated to match the master_xfer API. > diff --git a/drivers/i2c/busses/i2c-designware-master.c > b/drivers/i2c/busses/i2c-designware-master.c > index 936b9150754e8..f029aa1eadbf8 100644 > --- a/drivers/i2c/busses/i2c-designware-master.c > +++ b/drivers/i2c/busses/i2c-designware-master.c [ ... ] > @@ -291,8 +291,9 @@ static int i2c_dw_status(struct dw_i2c_dev *dev) > * Initiate and continue master read/write transaction with polling > * based transfer routine afterward write messages into the Tx buffer. > */ > -static int amd_i2c_dw_xfer_quirk(struct dw_i2c_dev *dev, struct i2c_msg > *msgs, int num_msgs) > +int amd_i2c_dw_xfer_quirk(struct i2c_adapter *adap, struct i2c_msg *msgs, > int num_msgs) > { > + struct dw_i2c_dev *dev = i2c_get_adapdata(adap); > int msg_wrt_idx, msg_itr_lmt, buf_len, data_idx; > int cmd = 0, status; > u8 *tx_buf; [Severity: High] This isn't a bug introduced by this patch, but looking at the RX FIFO draining loop later in this function: if (cmd) { regmap_write(dev->map, DW_IC_TX_TL, 2 * (buf_len - 1)); regmap_write(dev->map, DW_IC_RX_TL, 2 * (buf_len - 1)); Does this code fail to read the RX FIFO for read messages that are not the final message in a transaction? When processing a read message that is not the final message, the cmd variable remains 0. As a result, the if (cmd) block is entirely skipped, bypassing the regmap_read loop that drains the FIFO. Will this leave client buffers uninitialized for early read messages and misalign the hardware RX FIFO for subsequent operations? [Severity: High] This is also a pre-existing issue, but looking at the success return path at the end of this function: status = i2c_dw_check_stopbit(dev); if (status) return status; } return 0; Does this code return 0 on success instead of the number of successfully transferred messages? Since the I2C master_xfer algorithm contract requires returning num_msgs on success, I2C client drivers that check if (ret != num_msgs) might treat the transfer as incomplete or failed. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=11
