Re: [PATCH RFC v3 14/14] i2c: designware: add support for I2C_XFER_V2 - detailed fault reporting

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] i2c: designware: stale rx_buf_len used in i2c_dw_fault_report()
- [High] i2c: designware: stale msgs_cplt across retries in i2c_dw_fault_report()
- [Medium] i2c: designware: missing outer loop break on abort in i2c_dw_xfer_msg()
- [Medium] i2c: designware: unread DW_IC_TXFLR on early stop in __i2c_dw_xfer_one_part()
- [Low] i2c: designware: unaddressed v2 review feedback on style and loop index type
--

commit 99b998b5107033a19225ae77e0693b6c68191b43
Author: Dmitry Guzman <Dmitry.Guzman@mobileye.com>

i2c: designware: add support for I2C_XFER_V2 - detailed fault reporting

This commit adds i2c_dw_xfer_v2() and fault reporting support to the
DesignWare I2C master driver using the ABORT_SOURCE FLUSH_CNT field,
TXFLR register, and per-byte abort checks in the FIFO read and write
loops.

> diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
> index 14350c53a5241..c7efb8551aa30 100644
> --- a/drivers/i2c/busses/i2c-designware-master.c
> +++ b/drivers/i2c/busses/i2c-designware-master.c
> @@ -11,6 +11,7 @@
>  
>  #define DEFAULT_SYMBOL_NAMESPACE	"I2C_DW"
>  
> +#include <linux/bitfield.h>
>  #include <linux/delay.h>
>  #include <linux/err.h>
>  #include <linux/errno.h>
> @@ -33,6 +34,14 @@
>  #define AMD_TIMEOUT_MAX_US	250
>  #define AMD_MASTERCFG_MASK	GENMASK(15, 0)
>  
> +static bool i2c_dw_check_abort_flag(struct dw_i2c_dev *dev)
> +{
> +	u32 stat;
> +
> +	regmap_read(dev->map, DW_IC_RAW_INTR_STAT, &stat);
> +	return (!!(stat & DW_IC_INTR_TX_ABRT));
> +}
> +
>  static int i2c_dw_set_timings_master(struct dw_i2c_dev *dev)
>  {
>  	unsigned int comp_param1;
> @@ -198,6 +207,8 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
>  
>  	i2c_dw_set_mode(dev, DW_IC_MASTER);
>  
> +	dev->rx_buf_len = 0;
> +
>  	/* If the slave address is ten bit address, enable 10BITADDR */
>  	if (msgs[dev->msg_write_idx].flags & I2C_M_TEN) {
>  		ic_con = DW_IC_CON_10BITADDR_MASTER;
> @@ -459,6 +470,21 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
>  					     cmd | *buf++);
>  			}
>  			tx_limit--; buf_len--;
> +
> +			/* If precise fault reporting is required, check if the transfer
> +			 * is aborted after writing each byte.
> +			 * Otherwise, if it is aborted during filling FIFO, there is no way
> +			 * to know how many bytes was written to FIFO after transfer abort
> +			 * and thus are not counted in FLUSH_CNT.
> +			 * If we are checking abort flag after each byte, we can lose only 1 byte.
> +			 */
> +			if (dev->need_precise_report) {
> +				if (i2c_dw_check_abort_flag(dev)) {
> +					dev_warn_ratelimited(dev->dev, "Transfer aborted during FIFO writing. Report may be imprecise.");
> +					break;
> +				}
> +				dev->bytes_written++;
> +			}
>  		}

[Severity: Medium]
In i2c_dw_xfer_msg(), buf_len is decremented before checking
i2c_dw_check_abort_flag(dev), and the break only exits the inner while
loop:

	tx_limit--; buf_len--;
	if (dev->need_precise_report) {
		if (i2c_dw_check_abort_flag(dev)) {
			...
			break;
		}
		dev->bytes_written++;
	}

After the inner while loop, the outer loop over dev->msg_write_idx only
breaks when flags & I2C_M_RECV_LEN or buf_len > 0:

	if (flags & I2C_M_RECV_LEN) {
		dev->status |= STATUS_WRITE_IN_PROGRESS;
		intr_mask &= ~DW_IC_INTR_TX_EMPTY;
		break;
	} else if (buf_len > 0) {
		/* more bytes to be written */
		dev->status |= STATUS_WRITE_IN_PROGRESS;
		break;
	} else
		dev->status &= ~STATUS_WRITE_IN_PROGRESS;

If the abort flag is detected while writing the final byte of a message
(where buf_len was 1 and has just been decremented to 0), won't buf_len > 0
be false, causing the outer for loop to clear STATUS_WRITE_IN_PROGRESS,
advance dev->msg_write_idx, and continue writing subsequent messages into
DW_IC_DATA_CMD after the abort?

[ ... ]

> @@ -784,6 +819,8 @@ __i2c_dw_xfer_one_part(struct dw_i2c_dev *dev, struct i2c_msg *msgs, size_t num)
>  	dev->status = 0;
>  	dev->abort_source = 0;
>  	dev->rx_outstanding = 0;
> +	dev->bytes_written = 0;
> +	dev->tx_flushed = 0;
>  
>  	ret = i2c_dw_wait_bus_not_busy(dev);
>  	if (ret < 0)
> @@ -795,6 +832,9 @@ __i2c_dw_xfer_one_part(struct dw_i2c_dev *dev, struct i2c_msg *msgs, size_t num)
>  	/* Wait for tx to complete */
>  	ret = i2c_dw_wait_transfer(dev);
>  	if (ret) {
> +		if (dev->need_precise_report)
> +			regmap_read(dev->map, DW_IC_TXFLR, &dev->tx_flushed);
> +
>  		dev_err(dev->dev, "controller timed out\n");
>  		/* i2c_dw_init() implicitly disables the adapter */
>  		i2c_recover_bus(&dev->adapter);

[Severity: Medium]
As reported by Sashiko on v2 [1], __i2c_dw_xfer_one_part() only reads
DW_IC_TXFLR into dev->tx_flushed when i2c_dw_wait_transfer() times out, and
i2c_dw_fault_report() only reads FLUSH_CNT when dev->abort_source is
non-zero.

However, __i2c_dw_xfer_one_part() can also fail with -EIO without a timeout
or TX_ABRT when i2c_dw_process_transfer() detects a spurious STOP
mid-transfer (setting dev->msg_err = -EIO) or when dev->status remains
non-zero on early termination:

	if (dev->msg_err)
		return dev->msg_err;
	...
	if (dev->status)
		dev_err(dev->dev,
			"transfer terminated early - interrupt latency too high?\n");

	return -EIO;

In addition, in i2c_dw_process_transfer(), if DW_IC_INTR_TX_EMPTY and
DW_IC_INTR_STOP_DET are both set in stat, i2c_dw_xfer_msg() refills the TX
FIFO and increments dev->bytes_written before the spurious STOP check sets
dev->msg_err = -EIO. Because __i2c_dw_xfer_one_part() then disables or
reinitializes the controller before returning, the hardware FIFO is flushed
while both dev->abort_source and dev->tx_flushed remain 0.

When i2c_dw_fault_report() runs for these -EIO errors, won't unsent bytes
still sitting in the TX FIFO be counted as transmitted on the wire?

> @@ -886,13 +926,72 @@ i2c_dw_msg_is_valid(struct dw_i2c_dev *dev, const struct i2c_msg *msgs, size_t i
>  	return true;
>  }
>  
> +static int i2c_dw_fault_report(struct dw_i2c_dev *dev, struct i2c_transfer_report *report)
> +{
> +	unsigned int idx;
> +
> +	/*
> +	 * Get the number of bytes that were put into TX FIFO but were not sent on wire.
> +	 * If abort IRQ was received, this number is stored in abort_source register.
> +	 * Otherwise, if ic_dw_wait_transfer() returned timeout, use TXFLR register value
> +	 * read before the controller was reset.
> +	 */
> +	if (dev->abort_source)
> +		dev->tx_flushed = FIELD_GET(DW_IC_TX_ABRT_SOURCE_FLUSH_CNT_MASK, dev->abort_source);
> +
> +	if (dev->tx_flushed <= dev->bytes_written)
> +		dev->bytes_written -= dev->tx_flushed;
> +	else
> +		dev->bytes_written = 0;
> +
> +	/*
> +	 * The last byte that the transmission was interrupted on is not counted as flushed.
> +	 * We should not report it as transferred successfully, so decrement the counter.
> +	 */
> +	if (dev->bytes_written)
> +		dev->bytes_written--;
> +
> +	for (idx = 0; idx < dev->msg_write_idx; idx++) {
> +		if (dev->msgs[idx].len <= dev->bytes_written)
> +			dev->bytes_written -= dev->msgs[idx].len;
> +		else
> +			break;
> +	}
> +
> +	report->fault_msg_idx = idx;
> +	for (int i = dev->msg_read_idx; i <= idx && i < dev->msgs_num; i++) {

[Severity: Low]
As reported by Andy Shevchenko on v2 [2], there are a few style and type
issues remaining from the previous revision:

1. In i2c_dw_fault_report(), idx was changed to unsigned int, but the loop
   counter i in for (int i = dev->msg_read_idx; i <= idx && i <
   dev->msgs_num; i++) is still a signed int compared against unsigned int
   idx.
2. In i2c_dw_check_abort_flag(), return (!!(stat & DW_IC_INTR_TX_ABRT));
   still uses redundant outer parentheses and double negation in a function
   returning bool.
3. In i2c_dw_xfer_msg(), the multi-line comment starting with "/* If
   precise fault reporting is required..." still places text on the opening
   /* line instead of starting with a separate /* line.

Could these be cleaned up to match the feedback from v2?

> +		if (dev->msgs[i].flags & I2C_M_RD) {
> +			report->msgs_cplt = i;
> +			if (!(dev->rx_buf_len))
> +				report->bytes_cplt = 0;
> +			else
> +				report->bytes_cplt = dev->rx_buf - dev->msgs[i].buf;

[Severity: High]
Can i2c_dw_fault_report() use a stale dev->rx_buf_len and dangling
dev->rx_buf pointer from an earlier transfer if i2c_dw_wait_bus_not_busy()
fails?

When an earlier I2C_M_RD transfer is interrupted in i2c_dw_read() while
len > 0, i2c_dw_read() saves dev->rx_buf_len = len and dev->rx_buf = buf
(pointing into that transfer's message buffer) and returns.

On the next transfer (or a subsequent part in a multi-part transfer),
__i2c_dw_xfer_one_part() calls i2c_dw_wait_bus_not_busy(dev) before calling
i2c_dw_xfer_init(dev), which is where dev->rx_buf_len = 0 was placed:

	ret = i2c_dw_wait_bus_not_busy(dev);
	if (ret < 0)
		return ret;

	/* Start the transfers */
	i2c_dw_xfer_init(dev);

If i2c_dw_wait_bus_not_busy(dev) returns a negative error,
__i2c_dw_xfer_one_part() returns early without calling i2c_dw_xfer_init().
i2c_dw_xfer_common_v2() then calls i2c_dw_fault_report(dev, report) with
idx == 0 and dev->msg_read_idx == 0. If the new transfer's msgs[0] has
I2C_M_RD set, dev->rx_buf_len is still non-zero from the previous failed
transfer, so i2c_dw_fault_report() computes:

	report->bytes_cplt = dev->rx_buf - dev->msgs[i].buf;

subtracting the current message's buffer address from the previous
transfer's freed dev->rx_buf pointer. In i2cdev_ioctl_rdwr_v2(),
msgs[msgs_done].len is then set to report.bytes_cplt and passed to
copy_to_user(), which can read out of bounds past msgs[0].buf. Should
dev->rx_buf_len = 0 be initialized at the start of __i2c_dw_xfer_one_part()
alongside dev->msg_read_idx and dev->bytes_written?

> +			if ((i < idx) || (report->bytes_cplt < dev->bytes_written))
> +				dev_warn_ratelimited(dev->dev, "Read data lost due to FIFO flush");
> +			break;
> +		}
> +	}
> +
> +	if (report->msgs_cplt < 0) {
> +		report->msgs_cplt = idx;
> +		report->bytes_cplt = dev->bytes_written;
> +	}
> +
> +	return idx;
> +}

[ ... ]

> @@ -937,16 +1048,34 @@ i2c_dw_xfer_common(struct dw_i2c_dev *dev, struct i2c_msg msgs[], int num)
>  
>  	i2c_dw_release_lock(dev);
>  
> -	if (ret < 0)
> +	if (ret < 0) {
> +		if (report) {
> +			i2c_dw_fault_report(dev, report);
> +			report->msgs_cplt += msgs_in_prev_parts;
> +			report->fault_msg_idx += msgs_in_prev_parts;
> +		}
>  		return ret;
> +	}

[Severity: High]
What happens to report->msgs_cplt and report->bytes_cplt when an attempt
fails with -EAGAIN (for example on DW_IC_TX_ARB_LOST) and __i2c_transfer_v2()
retries the transfer?

In __i2c_transfer_v2(), i2c_transfer_report_init(report) (which sets
report->msgs_cplt = -EOPNOTSUPP) is called only once before the retry loop:

	if (report)
		i2c_transfer_report_init(report);
	...
	for (ret = 0, try = 0; try <= adap->retries; try++) {
		...
		if (report)
			ret = adap->algo->xfer_v2(adap, msgs, num, report);
		...
		if (ret != -EAGAIN)
			break;
	}

On the first failed attempt returning -EAGAIN, i2c_dw_fault_report() sets
report->msgs_cplt to a non-negative index and sets report->bytes_cplt. When
__i2c_transfer_v2() calls i2c_dw_xfer_v2() again for the next try and that
attempt also fails, if there is no I2C_M_RD message in
[dev->msg_read_idx, idx], i2c_dw_fault_report() checks:

	if (report->msgs_cplt < 0) {
		report->msgs_cplt = idx;
		report->bytes_cplt = dev->bytes_written;
	}

Because report->msgs_cplt is already >= 0 from the earlier try, neither
report->msgs_cplt nor report->bytes_cplt is updated. Furthermore, if
msgs_in_prev_parts > 0, i2c_dw_xfer_common_v2() executes
report->msgs_cplt += msgs_in_prev_parts on each failed retry, accumulating
msgs_in_prev_parts onto the stale report->msgs_cplt until it can exceed num.
Should report be reinitialized on entry to i2c_dw_xfer_common_v2() or
i2c_dw_fault_report()?

[1] https://lore.kernel.org/r/20260903-i2c-fault-reporting-v2-12-fedeb91792e6@mobileye.com
[2] https://lore.kernel.org/r/apkl_XHaZreKoipX@ashevche-desk.local

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-i2c-fault-reporting-v3-0-515ad74ac4c9@mobileye.com?part=14