Re: [PATCH v5 2/2] i2c: Add Synopsys Advanced I2C (DWC_i2c) driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] i2c: dwc-adv: pre-queued reads in dwc_adv_i2c_xfer_msg() hang
  block reads
- [High] i2c: dwc-adv: broken STOP_DET handling in
  dwc_adv_i2c_process_transfer()
--

commit 661b42138a542f38b7cf29c492367e656df39940
Author: Aniket Limaye <a-limaye@ti.com>

i2c: Add Synopsys Advanced I2C (DWC_i2c) driver

This commit adds a platform driver for the Synopsys Advanced I2C
(DWC_i2c) controller in controller mode, discovering per-block register
offsets at runtime via the block header chain.

> diff --git a/drivers/i2c/busses/i2c-dwc-adv.c b/drivers/i2c/busses/i2c-dwc-adv.c
> new file mode 100644
> index 0000000000000..990026e09bdce
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-dwc-adv.c

[ ... ]

> +static void dwc_adv_i2c_xfer_msg(struct dwc_adv_i2c_dev *dev)
> +{
> +	struct i2c_msg *msgs = dev->msgs;
> +	u32 intr_mask = DWC_ADV_I2C_CTRLR_INTR_MASK;
> +	int tx_limit, rx_limit;
> +	u32 buf_len = dev->tx_buf_len;
> +	u8 *buf = dev->tx_buf;
> +	u32 cmd;
> +	int rx_tl;
> +
> +	for (; dev->msg_write_idx < dev->msgs_num; dev->msg_write_idx++) {
> +		u32 flags = msgs[dev->msg_write_idx].flags;
> +
> +		if (!(dev->status & DWC_ADV_I2C_STATUS_SW_WRITE_IN_PROGRESS)) {
> +			buf = msgs[dev->msg_write_idx].buf;
> +			buf_len = msgs[dev->msg_write_idx].len;
> +		}
> +
> +		tx_limit = dev->tx_fifo_depth - dwc_adv_reg_read(dev, dev->regs.txflr);
> +		rx_limit = dev->rx_fifo_depth - dwc_adv_reg_read(dev, dev->regs.rxflr);
> +
> +		while (buf_len > 0 && tx_limit > 0 && rx_limit > 0) {
> +			/* Avoid rx buffer overrun */
> +			if ((flags & I2C_M_RD) &&
> +			    dev->rx_outstanding >= dev->rx_fifo_depth)
> +				break;
> +
> +			cmd = 0;
> +
> +			/*
> +			 * The bus is held while the TX FIFO is empty unless
> +			 * STOP is requested in IC_DATA_CMD, so always set it
> +			 * on the last byte of the last message.
> +			 *
> +			 * i2c-core always sets the buffer length of
> +			 * I2C_FUNC_SMBUS_BLOCK_DATA to 1. The length will be
> +			 * adjusted when receiving the first byte. Thus we
> +			 * can't stop the transaction here.
> +			 */
> +			if (dev->msg_write_idx == dev->msgs_num - 1 &&
> +			    buf_len == 1 && !(flags & I2C_M_RECV_LEN))
> +				cmd |= DWC_ADV_I2C_DATA_CMD_STOP;
> +
> +			/*
> +			 * A RESTART is only issued automatically when the
> +			 * direction changes. Request it explicitly at the
> +			 * start of each following message.
> +			 */
> +			if (dev->need_restart) {
> +				cmd |= DWC_ADV_I2C_DATA_CMD_RESTART;
> +				dev->need_restart = false;
> +			}
> +
> +			if (msgs[dev->msg_write_idx].flags & I2C_M_RD) {
> +				dwc_adv_reg_write(dev, dev->regs.data_cmd,
> +						  cmd | DWC_ADV_I2C_DATA_CMD_READ);
> +				rx_limit--;
> +				dev->rx_outstanding++;
> +			} else {
> +				dwc_adv_reg_write(dev, dev->regs.data_cmd, cmd | *buf++);
> +			}
> +			tx_limit--;
> +			buf_len--;
> +		}
> +
> +		dev->tx_buf = buf;
> +		dev->tx_buf_len = buf_len;
> +
> +		if (buf_len > 0) {
> +			dev->status |= DWC_ADV_I2C_STATUS_SW_WRITE_IN_PROGRESS;
> +			break;
> +		}
> +
> +		/*
> +		 * Because we don't know the buffer length in the
> +		 * I2C_FUNC_SMBUS_BLOCK_DATA case, we can't stop the
> +		 * transaction here. Also disable the TX_EMPTY IRQ while
> +		 * waiting for the data length byte to avoid a flood of
> +		 * bogus interrupts.
> +		 */
> +		if (flags & I2C_M_RECV_LEN) {
> +			dev->status |= DWC_ADV_I2C_STATUS_SW_WRITE_IN_PROGRESS;
> +			intr_mask &= ~DWC_ADV_I2C_INTR_TX_EMPTY;
> +			break;
> +		}

[Severity: High]
Does dwc_adv_i2c_xfer_msg() pre-queue multiple read commands when an
I2C_M_RECV_LEN message starts with len > 1?

In i2c_smbus_xfer_emulated(), enabling I2C_CLIENT_PEC increments
msg[1].len from 1 to 2 before starting an SMBus block read:

drivers/i2c/i2c-core-smbus.c:i2c_smbus_xfer_emulated() {
    ...
	if (read_write == I2C_SMBUS_READ)
		msg[1].len += 1;	/* Until we know the true length */
    ...
}

Unlike i2c_dw_xfer_msg(), the loop in dwc_adv_i2c_xfer_msg() does not
break after queuing the single length-byte read when I2C_M_RECV_LEN is
set.  When initial len > 1, it queues multiple read commands without
DWC_ADV_I2C_DATA_CMD_STOP and raises dev->rx_outstanding >= 2, which sets
regs.rx_tl >= 1 so RX_FULL does not fire when the length byte arrives.
Once dwc_adv_i2c_recv_len() runs, if the total message length is less
than or equal to the already queued dev->rx_outstanding (such as when the
target returns an invalid block length clamped to count = 1, or when
I2C_RDWR passes a larger initial len), dev->tx_buf_len becomes 0 and no
final read command with DWC_ADV_I2C_DATA_CMD_STOP is ever queued, hanging
the bus until the transfer times out.

Also, checking buf_len > 0 before flags & I2C_M_RECV_LEN means that
breaking out of the loop after queuing only the first byte of a message
with initial len > 1 skips masking DWC_ADV_I2C_INTR_TX_EMPTY.

[ ... ]

> +static u32 dwc_adv_i2c_read_clear_intrbits(struct dwc_adv_i2c_dev *dev)
> +{
> +	u32 stat = dwc_adv_reg_read(dev, dev->regs.intr_stat);
> +
> +	if (stat & DWC_ADV_I2C_INTR_RX_UNDER)
> +		dwc_adv_i2c_ack_intr(dev, DWC_ADV_I2C_INTR_CLR_RX_UNDER);
> +	if (stat & DWC_ADV_I2C_INTR_RX_OVER)
> +		dwc_adv_i2c_ack_intr(dev, DWC_ADV_I2C_INTR_CLR_RX_OVER);
> +	if (stat & DWC_ADV_I2C_INTR_TX_OVER)
> +		dwc_adv_i2c_ack_intr(dev, DWC_ADV_I2C_INTR_CLR_TX_OVER);
> +	if (stat & DWC_ADV_I2C_INTR_RD_REQ)
> +		dwc_adv_i2c_ack_intr(dev, DWC_ADV_I2C_INTR_CLR_RD_REQ);
> +	if (stat & DWC_ADV_I2C_INTR_TX_ABRT) {
> +		dev->abort_source = dwc_adv_reg_read(dev, dev->regs.tx_abrt_source);
> +		dwc_adv_i2c_ack_intr(dev, DWC_ADV_I2C_INTR_CLR_TX_ABRT);
> +	}
> +	if (stat & DWC_ADV_I2C_INTR_RX_DONE)
> +		dwc_adv_i2c_ack_intr(dev, DWC_ADV_I2C_INTR_CLR_RX_DONE);
> +	if (stat & DWC_ADV_I2C_INTR_ACTIVITY)
> +		dwc_adv_i2c_ack_intr(dev, DWC_ADV_I2C_INTR_CLR_ACTIVITY);
> +	if (stat & DWC_ADV_I2C_INTR_STOP_DET &&
> +	    (dev->rx_outstanding == 0 || (stat & DWC_ADV_I2C_INTR_RX_FULL)))
> +		dwc_adv_i2c_ack_intr(dev, DWC_ADV_I2C_INTR_CLR_STOP_DET);
> +	if (stat & DWC_ADV_I2C_INTR_START_DET)
> +		dwc_adv_i2c_ack_intr(dev, DWC_ADV_I2C_INTR_CLR_START_DET);
> +	if (stat & DWC_ADV_I2C_INTR_GEN_CALL)
> +		dwc_adv_i2c_ack_intr(dev, DWC_ADV_I2C_INTR_CLR_GEN_CALL);
> +
> +	return stat;
> +}
> +
> +static void dwc_adv_i2c_process_transfer(struct dwc_adv_i2c_dev *dev, u32 stat)
> +{
> +	if (stat & DWC_ADV_I2C_INTR_TX_ABRT) {
> +		/* TX_ABRT flushes the TX/RX FIFOs, nothing left to process */
> +		dev->cmd_err |= DWC_ADV_I2C_INTR_TX_ABRT;
> +		dev->status &= ~DWC_ADV_I2C_STATUS_SW_MASK;
> +		dev->rx_outstanding = 0;
> +		dwc_adv_reg_write(dev, dev->regs.intr_mask, 0);
> +		goto tx_aborted;
> +	}
> +
> +	if (stat & DWC_ADV_I2C_INTR_RX_FULL) {
> +		dwc_adv_i2c_read(dev);
> +		/* Re-evaluate RX_TL and TX_EMPTY for the reads still pending */
> +		stat |= DWC_ADV_I2C_INTR_TX_EMPTY;
> +	}
> +
> +	if (stat & DWC_ADV_I2C_INTR_TX_EMPTY)
> +		dwc_adv_i2c_xfer_msg(dev);
> +
> +	/* A STOP in the middle of a read or write is an error */
> +	if ((stat & DWC_ADV_I2C_INTR_STOP_DET) &&
> +	    (dev->status & (DWC_ADV_I2C_STATUS_SW_WRITE_IN_PROGRESS |
> +			     DWC_ADV_I2C_STATUS_SW_READ_IN_PROGRESS))) {
> +		dev_err(dev->dev, "spurious STOP, status 0x%x\n", dev->status);
> +		dev->rx_outstanding = 0;
> +		dev->msg_err = -EIO;
> +	}
> +
> +tx_aborted:
> +	if (((stat & (DWC_ADV_I2C_INTR_TX_ABRT | DWC_ADV_I2C_INTR_STOP_DET)) ||
> +	     dev->msg_err) && dev->rx_outstanding == 0)
> +		complete(&dev->cmd_complete);
> +}

[Severity: High]
Can dwc_adv_i2c_process_transfer() falsely abort a multi-batch read with
-EIO and leave DWC_ADV_I2C_INTR_STOP_DET unacknowledged when STOP_DET
arrives before the final RX_FULL?

dwc_adv_i2c_read_clear_intrbits() skips clearing DWC_ADV_I2C_INTR_STOP_DET
when dev->rx_outstanding > 0 and DWC_ADV_I2C_INTR_RX_FULL is not set, but
still returns stat with DWC_ADV_I2C_INTR_STOP_DET set.  On any read that
takes more than one RX_FULL interrupt (such as a read larger than
rx_fifo_depth / 2 or an I2C_M_RECV_LEN block read), the earlier
dwc_adv_i2c_read() leaves DWC_ADV_I2C_STATUS_SW_READ_IN_PROGRESS set in
dev->status.  If STOP_DET then fires before the final RX_FULL,
dwc_adv_i2c_process_transfer() treats it as a spurious STOP, zeroes
dev->rx_outstanding, sets dev->msg_err = -EIO, and completes
dev->cmd_complete without draining the RX FIFO.

Because dev->msg_err is set after dwc_adv_i2c_xfer_msg() has already run,
the if (dev->msg_err) intr_mask = 0; check in dwc_adv_i2c_xfer_msg() is
also bypassed on that interrupt, leaving interrupts unmasked and STOP_DET
asserted in hardware.  Similarly, if a premature STOP occurs during a
single-batch read after all read commands were queued (so neither
SW_WRITE_IN_PROGRESS nor SW_READ_IN_PROGRESS is set while
dev->rx_outstanding > 0 and RX_FULL is clear), STOP_DET is neither cleared
nor masked, causing an interrupt storm.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-tda54-upstream-i2c-v5-0-280a4a779b24@ti.com?part=2