Re: [PATCH v4 2/2] i2c: qcom-target: Add driver for Qualcomm I2C target controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] i2c: missing adapter owner initialization allows use-after-free
- [High] i2c: incorrect IRQ dispatch order corrupts state on Repeated Start
- [High] i2c: inconsistent clock teardown and resume ordering causes bus hangs
- [High] i2c: fatal unclocked MMIO access hazard in resume error paths
- [Low] i2c: unused PM sleep callbacks trigger warnings without CONFIG_PM_SLEEP
--

commit c5134b7c0f90bc099584b542a4a86588eb8e6053
Author: Viken Dadhaniya <viken.dadhaniya@oss.qualcomm.com>
i2c: qcom-target: Add driver for Qualcomm I2C target controller

This commit introduces a new driver for the Qualcomm I2C target controller
found on QDU1000 and related SoCs. It registers with the Linux I2C slave
framework to allow standard slave backends to attach.

> diff --git a/drivers/i2c/busses/i2c-qcom-target.c b/drivers/i2c/busses/i2c-qcom-target.c
> index 0000000000000..5cf4bad1b601a
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-qcom-target.c

[ ... ]

> +static irqreturn_t qcom_i2c_target_irq(int irq, void *dev)
> +{
> +	struct qcom_i2c_target *target = dev;
> +	u32 irq_stat, rx_bits;

[ ... ]

> +	if (irq_stat & BIT(RESTART_DETECTED)) {
> +		dev_dbg(target->dev, "Repeated start bit detected\n");
> +		target->status = 0;
> +		/*
> +		 * Keep clock stretch asserted when STRCH_RD is co-asserted so
> +		 * qcom_i2c_target_handle_strch_rd() can fill TX FIFO first.
> +		 */
> +		if (!(irq_stat & BIT(STRCH_RD)))
> +			writel(ACK_RESUME, target->base + I2C_S_CONTROL);
> +		writel(BIT(RESTART_DETECTED), target->base + I2C_S_IRQ_CLR);
> +	}
> +
> +	if (irq_stat & BIT(STRCH_RD))
> +		qcom_i2c_target_handle_strch_rd(target);
> +
> +	/*
> +	 * Coalesce all write-phase Rx bits into a single drain+ACK. When the
> +	 * RX FIFO fills at threshold, RX_FIFO_FULL, RX_DATA_AVAIL and STRCH_WR
> +	 * can all assert in the same irq_stat snapshot. Pass the combined mask
> +	 * so ACK_RESUME is written exactly once and all bits are cleared together.
> +	 */
> +	rx_bits = irq_stat & (BIT(RX_FIFO_FULL) | BIT(RX_DATA_AVAIL) |
> +			      BIT(STRCH_WR));
> +	if (rx_bits)
> +		qcom_i2c_target_handle_rx_data(target, rx_bits);

[Severity: High]
Is there a state corruption issue here if a Repeated Start occurs immediately
following a write phase?

If the hardware asserts both RESTART_DETECTED and RX_DATA_AVAIL in the same
interrupt, target->status is cleared to 0 first, which wipes the
WRITE_IN_PROGRESS flag. When qcom_i2c_target_handle_rx_data() is called right
after, could this cause the backend to receive an erroneous
I2C_SLAVE_WRITE_REQUESTED event and interpret the trailing data from the
previous write as the start of a brand new transaction?

[ ... ]

> +static int qcom_i2c_target_adap_init(struct qcom_i2c_target *target)
> +{
> +	target->adap.algo = &qcom_i2c_target_algo;
> +	target->adap.dev.parent = target->dev;
> +	target->adap.dev.of_node = target->dev->of_node;
> +	strscpy(target->adap.name, "qcom-i2c-target",
> +		sizeof(target->adap.name));
> +	i2c_set_adapdata(&target->adap, target);
> +
> +	return i2c_add_adapter(&target->adap);
> +}

[Severity: High]
Is the adapter missing the .owner initialization?

Without setting target->adap.owner = THIS_MODULE, the I2C core won't be able
to properly manage module refcounts. Can this lead to a use-after-free if the
module is removed while an unprivileged userspace process holds the character
device open?

[ ... ]

> +static void qcom_i2c_target_remove(struct platform_device *pdev)
> +{
> +	struct qcom_i2c_target *target = platform_get_drvdata(pdev);
> +
> +	writel(0, target->base + I2C_S_CONFIG);
> +	i2c_del_adapter(&target->adap);
> +	devm_free_irq(&pdev->dev, target->irq, target);
> +	icc_set_bw(target->icc_path, 0, 0);
> +	clk_disable_unprepare(target->xo_clk);
> +	clk_disable_unprepare(target->ahb_clk);
> +}

[Severity: High]
Does this code tear down the clocks in the correct order?

In qcom_i2c_target_probe(), xo_clk is enabled before ahb_clk. Here in remove
(and similarly in suspend below), xo_clk is disabled before ahb_clk. Does
this sequence risk a bus lockup or controller hang?

> +static int qcom_i2c_target_suspend(struct device *dev)
> +{
> +	struct qcom_i2c_target *target = dev_get_drvdata(dev);
> +	int ret;
> +
> +	disable_irq(target->irq);
> +	writel(0, target->base + I2C_S_IRQ_EN);
> +	writel(0, target->base + I2C_S_CONFIG);
> +
> +	ret = icc_set_bw(target->icc_path, 0, 0);
> +	if (ret)
> +		dev_err(dev, "icc_set_bw failed on suspend: %d\n", ret);
> +
> +	clk_disable_unprepare(target->xo_clk);
> +	clk_disable_unprepare(target->ahb_clk);
> +
> +	return 0;
> +}

[Severity: Low]
Could these PM callbacks cause unused function warnings?

When CONFIG_PM_SLEEP is disabled, SET_SYSTEM_SLEEP_PM_OPS becomes empty,
rendering qcom_i2c_target_suspend and qcom_i2c_target_resume unreferenced in
the source. This might trigger compiler warnings that break builds using
CONFIG_WERROR.

> +static int qcom_i2c_target_resume(struct device *dev)
> +{
> +	struct qcom_i2c_target *target = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = clk_prepare_enable(target->ahb_clk);
> +	if (ret) {
> +		dev_err(dev, "failed to enable AHB clock\n");
> +		enable_irq(target->irq);
> +		return ret;
> +	}

[Severity: High]
There appear to be two issues in this error path.

First, does resume enable the clocks in the opposite order compared to the
probe sequence? It enables ahb_clk before xo_clk, which might contradict
standard hardware initialization sequences.

Second, does enable_irq() unmask the interrupt while the bus clocks are
disabled? If the IRQ line is asserted, the CPU will jump to
qcom_i2c_target_irq(), which immediately executes a readl_relaxed() on
I2C_S_IRQ_STATUS. Could this cause a fatal bus error (synchronous external
abort) due to an unclocked MMIO access?

[ ... ]

> +err_disable_ahb:
> +	clk_disable_unprepare(target->ahb_clk);
> +	enable_irq(target->irq);
> +
> +	return ret;
> +}

[Severity: High]
Similar to the observation above, does enabling the IRQ here while clocks are
completely disabled risk an unclocked MMIO access and a resulting kernel panic
if the interrupt fires?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260920-i2c-qcom-slave-v4-0-f7e1020f3bb4@oss.qualcomm.com?part=2