Re: [PATCH v3 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] Calling usleep_range() from the hardirq handler will cause a 'scheduling while atomic' kernel panic.
- [High] Risk of synchronous external abort due to clocks being disabled before the IRQ is freed in the remove path.
- [High] The driver calls sleeping functions (clock APIs and icc_set_bw) from a `noirq` power management context.
- [High] Premature clock stretch release during a Repeated Start condition.
- [Medium] Missing cleanup of clocks and interconnect bandwidth vote on probe error paths.
--

--- Patch [2]: [PATCH v3 2/2] i2c: qcom-target: Add driver for Qualcomm I2C target controller ---
commit 1029beb1e1d38e9f7984ad5331b9a78f18027431
Author: Viken Dadhaniya <viken.dadhaniya@oss.qualcomm.com>
Subject: i2c: qcom-target: Add driver for Qualcomm I2C target controller

QDU1000 and related Qualcomm SoCs include a dedicated I2C target
controller that operates exclusively in target mode. This patch registers
the controller with the Linux I2C slave framework and handles IRQ events,
clocks, and power management.

> diff --git a/drivers/i2c/busses/i2c-qcom-target.c b/drivers/i2c/busses/i2c-qcom-target.c
> new file mode 100644
> index 0000000000000..277ea944eedb6
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-qcom-target.c

[ ... ]

> @@ -187,15 +187,15 @@ static int qcom_i2c_target_drain_rx_fifo(struct qcom_i2c_target *target)
>  static void qcom_i2c_target_hw_reset(struct qcom_i2c_target *target)
>  {
>  	/* Clear error bits before SW_RESET; the reset may not be instantaneous */
>  	writel(BIT(ERR_CONDITION) | BIT(CLOCK_LOW_TIMEOUT),
>  	       target->base + I2C_S_IRQ_CLR);
>  	writel(SW_RESET, target->base + I2C_S_SW_RESET_REG);
>  	/*
>  	 * I2C_S_SW_RESET_REG is write-only so completion cannot be polled.
>  	 * Use a conservative delay to allow the reset to finish before
>  	 * reconfiguring the controller.
>  	 */
>  	usleep_range(10, 20);

[Severity: High]
Does calling usleep_range() here cause a scheduling while atomic kernel
panic?

qcom_i2c_target_hw_reset() is called from qcom_i2c_target_handle_error(),
which is invoked directly from the hardirq handler qcom_i2c_target_irq()
when an error condition or clock low timeout occurs.

>  	qcom_i2c_target_hw_init(target);
>  	writel(target->slave->addr, target->base + I2C_S_DEVICE_ADDR);
>  	writel(I2C_S_CORE_EN, target->base + I2C_S_CONFIG);
>  }

[ ... ]

> @@ -279,15 +279,15 @@ static irqreturn_t qcom_i2c_target_irq(int irq, void *dev)
>  	if (!READ_ONCE(target->slave)) {
>  		writel(irq_stat, target->base + I2C_S_IRQ_CLR);
>  		return IRQ_HANDLED;
>  	}
>  
>  	if (irq_stat & (BIT(ERR_CONDITION) | BIT(CLOCK_LOW_TIMEOUT)))
>  		return qcom_i2c_target_handle_error(target, irq_stat);
>  
>  	if (irq_stat & BIT(STOP_DETECTED))
>  		return qcom_i2c_target_handle_stop(target, irq_stat);
>  
>  	if (irq_stat & BIT(RESTART_DETECTED)) {
>  		dev_dbg(target->dev, "Repeated start bit detected\n");
>  		target->status = 0;
>  		writel(ACK_RESUME, target->base + I2C_S_CONTROL);

[Severity: High]
If a repeated start occurs and STRCH_RD is co-asserted, does writing
ACK_RESUME here release the clock stretch prematurely?

Releasing it here would happen before the STRCH_RD handler below has a
chance to write the required payload to I2C_S_TX_FIFO, potentially corrupting
the data sent to the I2C master.

>  		writel(BIT(RESTART_DETECTED), target->base + I2C_S_IRQ_CLR);
>  	}
>  
>  	if (irq_stat & BIT(STRCH_RD))
>  		qcom_i2c_target_handle_strch_rd(target);

[ ... ]

> @@ -482,15 +482,15 @@ static int qcom_i2c_target_probe(struct platform_device *pdev)
>  	target->ahb_clk = devm_clk_get(dev, "ahb");
>  	if (IS_ERR(target->ahb_clk))
>  		return dev_err_probe(dev, PTR_ERR(target->ahb_clk),
>  				     "failed to get AHB clock\n");
>  
>  	ret = clk_prepare_enable(target->xo_clk);
>  	if (ret)
>  		return dev_err_probe(dev, ret, "failed to enable XO clock\n");
>  
>  	ret = clk_prepare_enable(target->ahb_clk);
>  	if (ret) {
>  		clk_disable_unprepare(target->xo_clk);
>  		return dev_err_probe(dev, ret, "failed to enable AHB clock\n");
>  	}
>  
>  	target->irq = platform_get_irq(pdev, 0);
>  	if (target->irq < 0)
>  		return target->irq;

[Severity: Medium]
Are xo_clk and ahb_clk leaked here if platform_get_irq() fails?

It appears clk_disable_unprepare() is missing for both clocks in this error
path, as well as in the subsequent qcom_i2c_target_icc_init() and
devm_request_irq() error paths.

>  
>  	ret = qcom_i2c_target_icc_init(target);
>  	if (ret)
>  		return ret;

[ ... ]

> @@ -511,11 +511,11 @@ 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);
>  	icc_set_bw(target->icc_path, 0, 0);
>  	clk_disable_unprepare(target->xo_clk);
>  	clk_disable_unprepare(target->ahb_clk);

[Severity: High]
Since the IRQ is devm-managed, can it fire after the clocks are disabled here?

If the interrupt fires during this window, the handler will read
I2C_S_IRQ_STATUS while the clocks are off, which could cause a synchronous
external abort.

>  }
>  
>  static int qcom_i2c_target_suspend(struct device *dev)

[ ... ]

> @@ -561,11 +561,11 @@ static int qcom_i2c_target_resume(struct device *dev)
>  	return 0;
>  
>  err_disable_xo:
>  	clk_disable_unprepare(target->xo_clk);
>  err_disable_ahb:
>  	clk_disable_unprepare(target->ahb_clk);
>  	return ret;
>  }
>  
>  static const struct dev_pm_ops qcom_i2c_target_pm_ops = {
>  	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(qcom_i2c_target_suspend,
>  				      qcom_i2c_target_resume)

[Severity: High]
Does using the noirq PM macros here cause a scheduling while atomic panic
during suspend and resume?

SET_NOIRQ_SYSTEM_SLEEP_PM_OPS binds the callbacks to the suspend_noirq and
resume_noirq phases, which run with local interrupts disabled. However,
qcom_i2c_target_suspend() and qcom_i2c_target_resume() both call
clk_disable_unprepare() or clk_prepare_enable(), and icc_set_bw(), which
can sleep.

>  };
>  
>  static const struct of_device_id qcom_i2c_target_dt_match[] = {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260813-i2c-qcom-slave-v3-0-1d3742e2ad47@oss.qualcomm.com?part=2