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 <[email protected]>
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/[email protected]?part=2
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.