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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] i2c: dwc-adv: IRQ storm and read deadlocks in dwc_adv_i2c_xfer_msg()
- [High] i2c: dwc-adv: lost repeated START on reads in dwc_adv_i2c_xfer_msg()
- [High] i2c: dwc-adv: false spurious STOP error in dwc_adv_i2c_process_transfer()
- [High] i2c: dwc-adv: unclocked MMIO access in dwc_adv_i2c_isr() when PM disabled
- [High] i2c: dwc-adv: unclocked MMIO access in dwc_adv_i2c_prepare_recovery()
--

commit 65be539e602e2870e8d704876155ca1ec922d703
Author: Aniket Limaye <a-limaye@ti.com>

i2c: Add Synopsys Advanced I2C (DWC_i2c) driver

This commit adds a new platform driver for the Synopsys Advanced I2C
(DWC_i2c) controller in controller mode, discovering block register offsets
at runtime by walking 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..1c5b7b154bed8
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-dwc-adv.c
> @@ -0,0 +1,1333 @@

[ ... ]

> +static void dwc_adv_i2c_init(struct dwc_adv_i2c_dev *dev)
> +{
> +	__dwc_adv_i2c_disable(dev);
> +
> +	dwc_adv_i2c_write_timings(dev);
> +
> +	if (dev->sda_hold_time)
> +		dwc_adv_reg_write(dev, dev->regs.sda_hold, dev->sda_hold_time);
> +
> +	dwc_adv_reg_write(dev, dev->regs.tx_tl, dev->tx_fifo_depth / 2);
> +	dwc_adv_reg_write(dev, dev->regs.rx_tl, 0);
> +	dwc_adv_reg_write(dev, dev->regs.con, dev->ctrlr_cfg);
> +}

[ ... ]

> +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) {
> +			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) {
> +				/* Avoid rx buffer overrun */
> +				if (dev->rx_outstanding >= dev->rx_fifo_depth)
> +					break;

[Severity: High]
Can dwc_adv_i2c_xfer_msg() lose the repeated start condition between
consecutive I2C_M_RD messages when dev->rx_outstanding reaches
dev->rx_fifo_depth?

When a message finishes queueing (buf_len == 0), dwc_adv_i2c_xfer_msg()
sets dev->need_restart = true and advances dev->msg_write_idx to the next
message. If the next message is an I2C_M_RD message and at least one
previously queued command has been popped from the TX FIFO into the shift
register (tx_limit > 0) while the RX FIFO is not yet full (rx_limit > 0),
execution enters the inner while loop:

    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) {
        /* Avoid rx buffer overrun */
        if (dev->rx_outstanding >= dev->rx_fifo_depth)
            break;

If dev->rx_outstanding >= dev->rx_fifo_depth, the loop breaks before
writing cmd to dev->regs.data_cmd, discarding cmd after dev->need_restart
has already been cleared to false. When RX_FULL later drains the RX FIFO
and dwc_adv_i2c_xfer_msg() resumes queueing the first byte of that read
message, dev->need_restart is false, so DWC_ADV_I2C_DATA_CMD_RESTART is
never issued on the bus.

Should the dev->rx_outstanding >= dev->rx_fifo_depth check happen before
consuming dev->need_restart?

> +				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;
> +
> +		/*
> +		 * 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;
> +		}
> +
> +		if (buf_len > 0) {
> +			dev->status |= DWC_ADV_I2C_STATUS_SW_WRITE_IN_PROGRESS;
> +			break;
> +		}
> +		dev->status &= ~DWC_ADV_I2C_STATUS_SW_WRITE_IN_PROGRESS;
> +		dev->need_restart = true;
> +	}
> +
> +	/*
> +	 * If i2c_msg index search is completed, we don't need TX_EMPTY
> +	 * interrupt any more.
> +	 */
> +	if (dev->msg_write_idx == dev->msgs_num)
> +		intr_mask &= ~DWC_ADV_I2C_INTR_TX_EMPTY;
> +
> +	if (dev->msg_err)
> +		intr_mask = 0;
> +
> +	/*
> +	 * Size the RX FIFO threshold to the reads already queued, so the
> +	 * controller raises one RX_FULL for the whole burst instead of one per
> +	 * received byte. While messages remain to be queued, cap it so RX_FULL
> +	 * still arrives in time to drain the FIFO and let the next TX_EMPTY
> +	 * queue the rest.
> +	 */
> +	rx_tl = dev->rx_outstanding;
> +	if (dev->msg_write_idx < dev->msgs_num)
> +		rx_tl = min_t(int, rx_tl, dev->rx_fifo_depth / 2);
> +	dwc_adv_reg_write(dev, dev->regs.rx_tl, rx_tl ? rx_tl - 1 : 0);
> +
> +	dwc_adv_reg_write(dev, dev->regs.intr_mask, intr_mask);
> +}

[Severity: High]
Can the FIFO threshold and DWC_ADV_I2C_INTR_TX_EMPTY handling in
dwc_adv_i2c_xfer_msg() and dwc_adv_i2c_read() cause a hardirq storm on
reads larger than rx_fifo_depth, or a transfer timeout on I2C_M_RECV_LEN
reads?

There appear to be three related issues here:

1. On a read transfer larger than dev->rx_fifo_depth (32 bytes on
ti,tda54-i2c), dwc_adv_i2c_init() sets dev->regs.tx_tl to
dev->tx_fifo_depth / 2 (16), so DWC_ADV_I2C_INTR_TX_EMPTY asserts whenever
txflr <= 16. Meanwhile, while dev->msg_write_idx < dev->msgs_num,
dwc_adv_i2c_xfer_msg() caps rx_tl to dev->rx_fifo_depth / 2 (16) and writes
rx_tl - 1 (15) to dev->regs.rx_tl, so DWC_ADV_I2C_INTR_RX_FULL only asserts
when rxflr >= 16.

After the initial TX_EMPTY interrupt queues 32 read commands
(dev->rx_outstanding = 32), the controller pops the 16th read command from
the TX FIFO into the shift register at the start of clocking that byte on
the bus. At that point txflr drops to 16 and asserts TX_EMPTY, while only
15 bytes have arrived in the RX FIFO (rxflr == 15, or fewer if write bytes
preceded the read), so RX_FULL is not set and dwc_adv_i2c_read() does not
run. dwc_adv_i2c_xfer_msg() then sees dev->rx_outstanding >=
dev->rx_fifo_depth, breaks out of the loop without writing to
dev->regs.data_cmd, and leaves DWC_ADV_I2C_INTR_TX_EMPTY unmasked in
dev->regs.intr_mask. Because txflr remains <= 16, the level-triggered
TX_EMPTY interrupt will re-fire in a tight hardirq loop until the 16th byte
finishes shifting in.

2. In dwc_adv_i2c_xfer_msg(), flags & I2C_M_RECV_LEN is checked before
buf_len > 0:

    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;
    }

    if (buf_len > 0) {
        dev->status |= DWC_ADV_I2C_STATUS_SW_WRITE_IN_PROGRESS;
        break;
    }

If an I2C_M_RECV_LEN message is reached when tx_limit == 0, rx_limit == 0,
or dev->rx_outstanding >= dev->rx_fifo_depth (for example, after preceding
messages filled the FIFO), the inner while loop does not execute and
buf_len remains 1. Checking I2C_M_RECV_LEN before buf_len > 0 masks
DWC_ADV_I2C_INTR_TX_EMPTY before the initial 1-byte length read command has
even been queued into the TX FIFO, stalling the transfer until timeout.

3. When an I2C_M_RECV_LEN message has its initial 1-byte read command
queued after earlier read bytes such that dev->rx_outstanding > 16 (for
example, a 20-byte read message followed by an I2C_M_RECV_LEN message,
giving dev->rx_outstanding == 21), dwc_adv_i2c_xfer_msg() masks
DWC_ADV_I2C_INTR_TX_EMPTY and caps dev->regs.rx_tl to 15. When the first 16
bytes arrive, RX_FULL fires and dwc_adv_i2c_read() drains 16 bytes,
reducing dev->rx_outstanding to 5 and returning early because the first
message still has 4 bytes remaining. Because TX_EMPTY is masked and
dwc_adv_i2c_read() does not update dev->regs.rx_tl, dev->regs.rx_tl stays
at 15 while only 5 bytes remain in flight, so RX_FULL never fires again to
reach dwc_adv_i2c_recv_len() and re-enable 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_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);
> +
> +	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;
> +	}

[Severity: High]
Won't dwc_adv_i2c_process_transfer() falsely abort valid multi-batch read
transfers with -EIO when DWC_ADV_I2C_INTR_STOP_DET is raised before the
final DWC_ADV_I2C_INTR_RX_FULL?

In dwc_adv_i2c_read_clear_intrbits(), DWC_ADV_I2C_INTR_STOP_DET is kept
pending in hardware when dev->rx_outstanding > 0 and
DWC_ADV_I2C_INTR_RX_FULL is not set, because STOP_DET can assert at the end
of a read transfer before the final byte triggers RX_FULL. However,
dwc_adv_i2c_read_clear_intrbits() still returns stat with
DWC_ADV_I2C_INTR_STOP_DET set.

On a read transfer where an earlier partial RX FIFO drain in
dwc_adv_i2c_read() set DWC_ADV_I2C_STATUS_SW_READ_IN_PROGRESS in
dev->status (such as a read larger than rx_fifo_depth / 2 or an
I2C_M_RECV_LEN block read), if STOP_DET fires before RX_FULL is set in
stat, dwc_adv_i2c_read() is not called, so
DWC_ADV_I2C_STATUS_SW_READ_IN_PROGRESS remains set.
dwc_adv_i2c_process_transfer() then sees stat & DWC_ADV_I2C_INTR_STOP_DET
with DWC_ADV_I2C_STATUS_SW_READ_IN_PROGRESS set, logs "spurious STOP",
sets dev->rx_outstanding = 0 and dev->msg_err = -EIO, and completes the
transfer early with an error instead of waiting for RX_FULL to drain the
remaining bytes.

> +
> +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);
> +}

[ ... ]

> +static irqreturn_t dwc_adv_i2c_isr(int irq, void *dev_id)
> +{
> +	struct dwc_adv_i2c_dev *dev = dev_id;
> +	irqreturn_t ret;
> +	int pm;
> +
> +	pm = pm_runtime_get_if_active(dev->dev);
> +	if (!pm)
> +		return IRQ_NONE;
> +
> +	ret = dwc_adv_i2c_handle_irq(dev);
> +
> +	if (pm > 0)
> +		pm_runtime_put_autosuspend(dev->dev);
> +
> +	return ret;
> +}

[Severity: High]
Should dwc_adv_i2c_isr() check if (pm <= 0) instead of if (!pm)?

pm_runtime_get_if_active() returns -EINVAL when runtime PM is disabled
(dev->power.disable_depth > 0). Because if (!pm) only checks for 0, a
-EINVAL return falls through to dwc_adv_i2c_handle_irq(), which performs
MMIO reads on dev->base while the clocks are disabled.

This can happen during system suspend/resume between dwc_adv_i2c_suspend()
(which runs in suspend_late and calls pm_runtime_force_suspend(), disabling
runtime PM and gating pclk and clk) and noirq IRQ disable/enable, or during
driver teardown because dwc_adv_i2c_probe() registers devm_request_irq()
before devm_add_action_or_reset(..., dwc_adv_i2c_pm_teardown, ...), so
pm_runtime_disable() runs before the IRQ is freed.

[ ... ]

> +static void dwc_adv_i2c_prepare_recovery(struct i2c_adapter *adap)
> +{
> +	struct dwc_adv_i2c_dev *dev = i2c_get_adapdata(adap);
> +
> +	__dwc_adv_i2c_disable(dev);
> +	reset_control_assert(dev->rst);
> +	dwc_adv_i2c_prepare_clk(dev, false);
> +}
> +
> +static void dwc_adv_i2c_unprepare_recovery(struct i2c_adapter *adap)
> +{
> +	struct dwc_adv_i2c_dev *dev = i2c_get_adapdata(adap);
> +	int ret;
> +
> +	ret = dwc_adv_i2c_prepare_clk(dev, true);
> +	if (ret) {
> +		dev_err(dev->dev, "failed to enable clock for recovery: %d\n", ret);
> +		reset_control_deassert(dev->rst);
> +		return;
> +	}
> +
> +	reset_control_deassert(dev->rst);
> +	dwc_adv_i2c_init(dev);
> +}

[Severity: High]
Can I2C bus recovery trigger unclocked MMIO accesses in dwc_adv_i2c_isr()
or in callers of i2c_recover_bus()?

1. When i2c_recover_bus() is called from dwc_adv_i2c_wait_bus_not_busy() or
dwc_adv_i2c_xfer_one_part(), the device is held in RPM_ACTIVE by
dwc_adv_i2c_xfer(). dwc_adv_i2c_prepare_recovery() asserts dev->rst and
disables dev->clk and dev->pclk, without masking interrupts in
dev->regs.intr_mask or disabling the IRQ. Because the device remains
RPM_ACTIVE throughout i2c_generic_scl_recovery(), pm_runtime_get_if_active()
in dwc_adv_i2c_isr() returns 1, so any interrupt firing during recovery will
call dwc_adv_i2c_handle_irq() and read MMIO registers while pclk is disabled
and reset is asserted. (Similarly, in dwc_adv_i2c_probe(),
devm_request_irq() is called before dwc_adv_i2c_init() disables the
controller and while TX_EMPTY is unmasked at reset.)

2. In dwc_adv_i2c_unprepare_recovery(), if dwc_adv_i2c_prepare_clk(dev, true)
fails, it returns early with dev->clk and dev->pclk disabled. However, both
callers of i2c_recover_bus() immediately access MMIO registers afterwards:

dwc_adv_i2c_wait_bus_not_busy():
    i2c_recover_bus(&dev->adap);
    status = dwc_adv_reg_read(dev, dev->regs.status);

dwc_adv_i2c_xfer_one_part():
    i2c_recover_bus(&dev->adap);
    dwc_adv_i2c_init(dev);

In addition, because the device is still marked RPM_ACTIVE, subsequent
dwc_adv_i2c_runtime_suspend() will call dwc_adv_i2c_prepare_clk(dev, false)
and unbalance the clock enable count.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027-tda54-upstream-i2c-v4-1-ebeeaf87d181@ti.com?part=2