Re: [PATCH v2 11/12] i2c: designware: use separate `i2c_algorithm` for AMD_NAVI_GPU

[sashiko-bot@kernel.org]

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 <Dmitry.Guzman@mobileye.com>

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/20260903-i2c-fault-reporting-v2-0-fedeb91792e6@mobileye.com?part=11