Re: [PATCH v1 2/3] i2c: qcom-slave: Add driver for Qualcomm I2C slave controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [Critical] Initialization and teardown ordering leaves the interrupt handler active while the interconnect bandwidth path is disabled, risking fatal bus aborts.
- [Critical] The TX_FIFO_EMPTY interrupt is permanently enabled but never masked when the FIFO is empty, leading to an immediate interrupt storm.
- [High] The driver implements an I2C slave controller by hijacking the master `smbus_xfer` API instead of using the standard Linux I2C slave framework.
- [High] The interrupt handler resets the controller on fatal errors without holding the driver's spinlock, racing with concurrent userspace read/write operations.
- [High] The driver writes directly to the hardware TX FIFO without checking if there is available space, allowing userspace to overflow it.
- [High] Received data from multiple separate I2C transactions is appended without resetting the buffer, destroying message boundaries and wedging the RX FIFO.
- [High] Error recovery performs a read-modify-write on a command register and immediately overwrites it, potentially canceling recovery.
- [High] Error recovery resets the controller but fails to restore the programmed I2C slave address.
--

commit 99a7e48b114fde46339ce7f708bbacadb9e1ea19
Author: Viken Dadhaniya <viken.dadhaniya@oss.qualcomm.com>
Subject: i2c: qcom-slave: Add driver for Qualcomm I2C slave controller

This commit adds support for the dedicated Qualcomm I2C slave controller
found on QDU1000 and related SoCs. It registers the controller as an SMBus
adapter and handles read/write requests, IRQ events, and power management.

> diff --git a/drivers/i2c/busses/i2c-qcom-slave.c b/drivers/i2c/busses/i2c-qcom-slave.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-qcom-slave.c
[ ... ]
> +static void qcom_i2c_slave_write_fifo(struct qcom_i2c_slave *slave)
> +{
> +	int i;
> +
> +	if (!slave->tx_count) {
> +		dev_err(slave->dev, "TX FIFO write count is zero\n");
> +		return;
> +	}
> +
> +	for (i = 0; i < slave->tx_count; i++) {
> +		dev_dbg(slave->dev, "Data to TX FIFO: 0x%x\n",
> +			slave->tx_msg_buf[i]);
> +		writel(slave->tx_msg_buf[i], slave->base + I2C_S_TX_FIFO);
> +	}

[Severity: High]
Does this loop write to the TX FIFO without verifying available hardware
capacity? If userspace provides a payload larger than the FIFO depth, could
this overflow the hardware FIFO? 

Should this check TX_FIFO_COUNT_MASK in I2C_S_FIFOS_STATUS first?

> +
> +	slave->tx_count = 0;
> +}
[ ... ]
> +static void qcom_i2c_slave_read_fifo(struct qcom_i2c_slave *slave)
> +{
[ ... ]
> +	for (i = 0; i < rx_data_count &&
> +	     slave->rx_count < I2C_SLAVE_MAX_MSG_SIZE; i++) {
> +		slave->rx_msg_buf[slave->rx_count] =
> +			readl_relaxed(slave->base + I2C_S_RX_FIFO);
> +		dev_dbg(slave->dev, "Data from RX FIFO: 0x%x\n",
> +			slave->rx_msg_buf[slave->rx_count]);
> +		slave->rx_count++;
> +	}
> +}

[Severity: High]
Does this append data from multiple separate I2C transactions together without
resetting the buffer on message boundaries? 

Also, if slave->rx_count reaches I2C_SLAVE_MAX_MSG_SIZE, will this stop
draining the hardware FIFO and leave the controller permanently asserting
RX interrupts?

[ ... ]
> +static void qcom_i2c_slave_hw_init(struct qcom_i2c_slave *slave)
> +{
> +	writel(CLEAR_TX_FIFO | CLEAR_RX_FIFO, slave->base + I2C_S_CONTROL);
> +	writel(QCOM_I2C_SLAVE_ALL_IRQ, slave->base + I2C_S_IRQ_EN);

[Severity: High]
Is the TX_FIFO_EMPTY interrupt permanently enabled here via
QCOM_I2C_SLAVE_ALL_IRQ? 

If the FIFO is initially empty, will this cause an immediate interrupt
storm upon probe since the handler doesn't mask it when there is no data
to transmit?

> +	writel(slave->slave_addr, slave->base + I2C_S_DEVICE_ADDR);
> +	writel(CORE_EN, slave->base + I2C_S_CONFIG);
> +}
[ ... ]
> +static irqreturn_t qcom_i2c_slave_interrupt(int irq, void *dev_id)
> +{
> +	struct qcom_i2c_slave *slave = dev_id;
> +	u32 irq_stat;
> +
> +	irq_stat = readl_relaxed(slave->base + I2C_S_IRQ_STATUS);
> +	if (!irq_stat)
> +		return IRQ_NONE;
> +
> +	dev_dbg(slave->dev, "IRQ status: 0x%x\n", irq_stat);
> +
> +	/*
> +	 * ERR_CONDITION and CLOCK_LOW_TIMEOUT require full recovery.
> +	 * Return early after handling to avoid processing stale irq_stat bits.
> +	 */
> +	if (irq_stat & (BIT(ERR_CONDITION) | BIT(CLOCK_LOW_TIMEOUT))) {
> +		enum qcom_i2c_slave_irq irq_type = (irq_stat & BIT(ERR_CONDITION)) ?
> +						    ERR_CONDITION : CLOCK_LOW_TIMEOUT;
> +		dev_err(slave->dev, "%s\n", qcom_i2c_slave_irq_names[irq_type]);
> +		qcom_i2c_slave_dump_regs(slave);
> +		qcom_i2c_slave_set_bits(slave, I2C_S_SW_RESET_REG, SW_RESET);

[Severity: High]
Is this software reset performed without holding slave->lock? If a concurrent
thread is executing qcom_i2c_slave_write() and modifying the TX FIFO, could
this cause hardware state corruption?

[Severity: High]
Does SW_RESET clear the hardware registers, including I2C_S_DEVICE_ADDR?
If so, does this recovery path leave the controller without its configured
slave address, causing it to drop off the bus?

> +		qcom_i2c_slave_clear_irq(slave, QCOM_I2C_SLAVE_ALL_IRQ);
> +		writel(QCOM_I2C_SLAVE_ALL_IRQ, slave->base + I2C_S_IRQ_EN);
> +		qcom_i2c_slave_set_bits(slave, I2C_S_CONTROL,
> +					CLEAR_TX_FIFO | CLEAR_RX_FIFO);
> +		qcom_i2c_slave_set_bits(slave, I2C_S_CONFIG, CORE_EN);
> +		writel(NACK, slave->base + I2C_S_CONTROL);

[Severity: High]
Does this writel() immediately overwrite the I2C_S_CONTROL register that was
just modified by qcom_i2c_slave_set_bits()? Since it only writes the NACK
bit, will it unintentionally clear the FIFO command bits and other control
state before the hardware can process them?

> +		return IRQ_HANDLED;
> +	}
[ ... ]
> +static int qcom_i2c_slave_xfer(struct i2c_adapter *adap, u16 addr,
> +			       unsigned short flags, char read_write,
> +			       u8 command, int protocol,
> +			       union i2c_smbus_data *data)
> +{
[ ... ]
> +static u32 qcom_i2c_slave_func(struct i2c_adapter *adap)
> +{
> +	return I2C_FUNC_SMBUS_BYTE      |
> +	       I2C_FUNC_SMBUS_BYTE_DATA |
> +	       I2C_FUNC_SMBUS_WORD_DATA |
> +	       I2C_FUNC_SMBUS_BLOCK_DATA;
> +}
> +
> +static const struct i2c_algorithm qcom_i2c_slave_algo = {
> +	.smbus_xfer	= qcom_i2c_slave_xfer,
> +	.functionality	= qcom_i2c_slave_func,
> +};

[Severity: High]
Does this driver register a master SMBus algorithm (smbus_xfer) for a purely
slave IP? 

Would this break interoperability with standard I2C slave backends (like
i2c-slave-eeprom) which expect the standard Linux I2C slave framework instead
of intercepting master ioctls?

[ ... ]
> +static int qcom_i2c_slave_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	ret = devm_request_irq(dev, slave->irq, qcom_i2c_slave_interrupt, 0,
> +			       dev_name(dev), slave);
> +	if (ret) {
> +		dev_err(dev, "request_irq failed for IRQ %d: %d\n",
> +			slave->irq, ret);
> +		return ret;
> +	}
> +
> +	ret = qcom_i2c_slave_icc_init(slave);
> +	if (ret)
> +		return ret;

[Severity: High]
Is the interrupt requested before the interconnect bandwidth path is
initialized? 

If an external master sends a transaction in this window, could the interrupt
fire and access hardware registers over a disabled interconnect, leading to
a bus abort?

[ ... ]
> +static void qcom_i2c_slave_remove(struct platform_device *pdev)
> +{
> +	struct qcom_i2c_slave *slave = platform_get_drvdata(pdev);
> +
> +	i2c_del_adapter(&slave->adap);
> +	icc_disable(slave->icc_path);

[Severity: High]
Does this disable the interconnect while the devm-managed interrupt and
hardware are still active? 

If an interrupt fires after icc_disable() but before devres cleanup, could
this trigger a panic?

> +	/* clocks are disabled automatically by devm */
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260628-i2c-qcom-slave-v1-0-8b0a5c01f9f6@oss.qualcomm.com?part=2