[PATCH AUTOSEL 6.18] i2c: qup: Propagate clock enable failures
From: Sasha Levin
Date: Mon Oct 19 2026 - 10:16:29 EST
From: Pengpeng Hou <pengpeng@xxxxxxxxxxx>
[ Upstream commit a477626332800942f463d76aa48db0c47f1dfd18 ]
The QUP I2C driver treats the core and iface clocks as required
resources, but qup_i2c_enable_clocks() ignores clk_prepare_enable()
failures. Probe can then continue to register the I2C adapter, and
runtime/system resume can return success, even when a required clock
transition failed.
Make the helper return an error, unwind a partially enabled clock, and
propagate failures from probe and resume paths.
Signed-off-by: Pengpeng Hou <pengpeng@xxxxxxxxxxx>
Reviewed-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxxxxxxxx>
Signed-off-by: Andi Shyti <andi.shyti@xxxxxxxxxx>
Link: https://lore.kernel.org/r/20260624055237.29977-1-pengpeng@xxxxxxxxxxx
Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx>
---
LLM Generated explanations, may be completely bogus:
The local checkout is **v7.3-rc4**. The driver and the ignored-clock-
enable bug exist in this lineage (since v3.15); the fix is a small,
self-contained error-path change that belongs in this tree.
---
## PHASE 1: COMMIT MESSAGE FORENSICS
**Step 1.1: PARSE THE SUBJECT LINE**
Record: **[i2c: qup]** **[Propagate]** — make `clk_prepare_enable()`
failures visible instead of swallowing them.
**Step 1.2: PARSE ALL COMMIT MESSAGE TAGS**
Record:
- **Fixes:** none (expected)
- **Reported-by:** none
- **Tested-by:** none
- **Reviewed-by:** Konrad Dybcio (Qualcomm / linux-arm-msm)
- **Acked-by:** none
- **Link:**
https://lore.kernel.org/r/20260624055237.29977-1-pengpeng@xxxxxxxxxxx
- **Cc: stable:** none (expected)
- **Signed-off-by:** Pengpeng Hou (author); Andi Shyti (I2C maintainer)
- No syzbot / kernel test robot tags
**Step 1.3: ANALYZE THE COMMIT BODY TEXT**
Record:
- **Bug:** `qup_i2c_enable_clocks()` treats core and iface clocks as
required, but ignores `clk_prepare_enable()` return values.
- **Symptoms:** probe still registers the I2C adapter; runtime/system
resume still return success after a required clock failed.
- **Version info:** none in the message. Git shows the ignore-return
pattern since the driver was added.
- **Root cause:** void helper with unchecked clock enables; callers
cannot fail.
**Step 1.4: DETECT HIDDEN BUG FIXES**
Record: Not disguised. It is an explicit error-path fix: check returns,
unwind a half-enabled clock, propagate from probe and resume.
---
## PHASE 2: DIFF ANALYSIS
**Step 2.1: INVENTORY THE CHANGES**
Record:
- **File:** `drivers/i2c/busses/i2c-qup.c` — **+24 / −7** (31 lines)
- **Functions:** `qup_i2c_enable_clocks`, `qup_i2c_probe`,
`qup_i2c_pm_resume_runtime`, `qup_i2c_resume`
- **Scope:** single-file surgical fix
**Step 2.2: UNDERSTAND THE CODE FLOW CHANGE**
Record:
1. **Helper:** void, two unchecked `clk_prepare_enable()` → `int`, abort
on first failure, disable core clk if iface enable fails.
2. **Probe (DT path only):** ignore enable → `ret = …; if (ret) goto
fail_dma`. ACPI path still skips this helper.
3. **Runtime resume:** always `return 0` → return the helper’s error.
4. **System resume:** always `pm_request_autosuspend()` after resume →
skip autosuspend request if clocks failed.
**Step 2.3: IDENTIFY THE BUG MECHANISM**
Record: **Error-path / ignored-return + resource leak.**
- Broken: required clocks can fail; driver still MMIO-resets the QUP
(`writel` to `QUP_SW_RESET` immediately after enable in probe) and
later runs I2C with clocks off; iface-fail leaves core clk enabled.
- Fix: fail probe/resume, unwind partial enable.
**Step 2.4: ASSESS THE FIX QUALITY**
Record: Obviously correct and matches other I2C host drivers. `goto
fail_dma` is the right label: `fail:` would call
`qup_i2c_disable_clocks()` and MMIO/unprepare clocks that were never (or
no longer) enabled. Regression risk is low: a real clock failure now
fails probe/resume instead of continuing on dead hardware.
---
## PHASE 3: GIT HISTORY INVESTIGATION
**Step 3.1: BLAME THE CHANGED LINES**
Record: Ignored enables introduced in **`10c5a8425968`** (“i2c: qup: New
bus driver…”, Bjorn Andersson, 2014). First contained in **v3.15-rc1**.
`git log -L :qup_i2c_enable_clocks:` shows only that commit and this
fix.
**Step 3.2: FOLLOW THE FIXES: TAG**
Record: No `Fixes:` tag. Original driver commit is an ancestor of HEAD.
**Step 3.3: CHECK FILE HISTORY**
Record: Recent related work is unrelated (`pm_runtime_mark_last_busy`,
timeout loop, interconnect vote). Older related fix **`3cf357dfb40b`**
already routed some probe errors to `fail` so clocks get released — this
patch fills the missing enable-check. Standalone, not `X/Y`.
**Step 3.4: CHECK THE AUTHOR'S OTHER COMMITS**
Record: Pengpeng Hou has only this commit under `drivers/i2c/`. Reviewer
is a Qualcomm platform maintainer; committer is the I2C maintainer.
**Step 3.5: CHECK FOR DEPENDENT/PREREQUISITE COMMITS**
Record: No new APIs or structs. Applies standalone on this tree. No
follow-ups on `i2c-qup.c` after `a477626332800`.
---
## PHASE 4: MAILING LIST AND EXTERNAL RESEARCH
**Step 4.1: FIND THE ORIGINAL PATCH DISCUSSION**
Record:
- **b4 dig:**
https://patch.msgid.link/20260624055237.29977-1-pengpeng@xxxxxxxxxxx
- **Revisions:** v1 only (no v2/v3)
- **Review:** Konrad Dybcio `Reviewed-by` the same day; Andi Shyti
merged to `i2c/i2c` (~2026-07-28). No NAK, no stable nomination in-
thread.
- lore.kernel.org fetch returned 403; thread confirmed via gnuweeb,
patchew, and openwall mirrors.
**Step 4.2: CHECK WHO REVIEWED THE PATCH**
Record (`b4 dig -w`): To Andi Shyti; Cc linux-arm-msm, linux-i2c, linux-
kernel. Konrad was not in the original To/Cc but reviewed from linux-
arm-msm.
**Step 4.3: SEARCH FOR THE BUG REPORT**
Record: No bugzilla/syzbot/user report. Issue is from code review of
ignored returns.
**Step 4.4: CHECK FOR RELATED PATCHES AND SERIES**
Record: Single standalone patch, not a series.
**Step 4.5: CHECK STABLE MAILING LIST HISTORY**
Record: No discussion of this subject on stable@ found
(lore.kernel.org/stable 403; web search did not hit this patch).
---
## PHASE 5: CODE SEMANTIC ANALYSIS
**Step 5.1: KEY FUNCTIONS**
Record: `qup_i2c_enable_clocks`, `qup_i2c_probe`,
`qup_i2c_pm_resume_runtime`, `qup_i2c_resume`. (semcode MCP not
available.)
**Step 5.2: TRACE CALLERS**
Record: Only three call sites in this file:
- `qup_i2c_probe` (DT/clk path, boot / module load)
- `qup_i2c_pm_resume_runtime` (runtime PM; also used as `RUNTIME_PM_OPS`
resume)
- `qup_i2c_resume` → that runtime helper (system sleep)
Not interrupt context. Reachable on every QUP I2C device probe and every
resume.
**Step 5.3: TRACE CALLEES**
Record: `clk_prepare_enable` / `clk_disable_unprepare` only. Probe then
`clk_get_rate` and `writel(QUP_SW_RESET)`.
**Step 5.4: FOLLOW THE CALL CHAIN**
Record: Platform probe / PM resume. Userspace does not call this
directly, but every Qualcomm QUP I2C bus (touch, sensors, PMICs, cameras
on many phones and boards) goes through it. Unclocked MMIO after a
failed enable is a hang path.
**Step 5.5: SEARCH FOR SIMILAR PATTERNS**
Record: Most I2C hosts already check `clk_prepare_enable`
(`i2c-designware-common.c`, `i2c-exynos5.c`, `i2c-img-scb.c`, etc.). A
few still ignore it (`i2c-mv64xxx.c`, `i2c-hix5hd2.c`). This brings QUP
in line with the common pattern.
---
## PHASE 6: CROSS-REFERENCING AGAINST THE LOCAL TREE
**Step 6.1: DOES THE BUGGY CODE EXIST IN THIS TREE?**
Record: **Yes.** Tree is **v7.3-rc4** (`7.3.0-rc4`, `f0100363d8c37`).
Introducer `10c5a8425968` and fix `a477626332800` are both ancestors of
HEAD. Pre-fix helper in the parent of the fix is still the 2014 void
ignore-return version. Driver: `drivers/i2c/busses/i2c-qup.c`.
**Step 6.2: CHECK FOR BACKPORT COMPLICATIONS**
Record: Clean apply on this tree (same file, same functions). No extra
prerequisites.
**Step 6.3: CHECK IF RELATED FIXES ARE ALREADY HERE**
Record: No alternate fix for ignored enable. `3cf357dfb40b` only fixes a
later probe unwind, not enable checking. No post-fix regressions on this
file.
---
## PHASE 7: SUBSYSTEM AND MAINTAINER CONTEXT
**Step 7.1: SUBSYSTEM AND CRITICALITY**
Record: **I2C host driver** (`drivers/i2c/busses/i2c-qup.c`),
`CONFIG_I2C_QUP` (`ARCH_QCOM || COMPILE_TEST`). **IMPORTANT** for
Qualcomm platforms (common on phones, boards, IoT); not core mm/VFS.
**Step 7.2: SUBSYSTEM ACTIVITY**
Record: Mature driver (2014), still getting occasional correctness
fixes. Not a new subsystem.
---
## PHASE 8: IMPACT AND RISK ASSESSMENT
**Step 8.1: WHO IS AFFECTED**
Record: Qualcomm SoC users with QUP I2C (`qcom,i2c-qup-v1.1.1` /
`v2.1.1` / `v2.2.1`). Driver- and config-specific, but a large device
population.
**Step 8.2: TRIGGER CONDITIONS**
Record: `clk_prepare_enable()` failure on core or iface (probe, runtime
resume, or system resume). Unprivileged users cannot poke the helper,
but resume is a normal path. Failures are uncommon on healthy clocks;
they do happen with power-domain / RPM / -ENOMEM issues. No public
reproducer.
**Step 8.3: FAILURE MODE SEVERITY**
Record: Continuing after a failed enable then `writel`/`readl` on the
QUP can hang the interconnect; resume success with clocks off breaks
later transfers; iface-fail leaks a prepared core clk. **HIGH** when it
triggers (hang / stuck I2C), **MEDIUM** overall because triggers are
rare. Not a demonstrated CVE.
**Step 8.4: RISK-BENEFIT RATIO**
Record:
- **Benefit:** avoid unclocked MMIO and false-success resume on QUP I2C.
- **Risk:** very low (~24 insertions, error paths only, standard clk
unwind).
- **Ratio:** favorable.
---
## PHASE 9: FINAL SYNTHESIS
**Step 9.1: EVIDENCE**
**For:**
- Real ignored-error bug on required clocks.
- Probe immediately MMIO-resets the block; clocks-off access can hang.
- Resume used to report success with clocks still off.
- Partial enable leaked the core clock.
- 24 insertions in one file; obviously correct; no new API.
- Reviewed by Konrad Dybcio, merged by Andi Shyti.
- Bug present since v3.15; no extra dependencies.
- Matches the usual I2C host-driver pattern.
**Against:**
- No user/syzbot report in ~12 years.
- Clock enable rarely fails on working hardware.
- No in-thread `Cc: stable`.
**Unresolved:** Direct lore.kernel.org/stable search 403; no evidence
this was rejected for stable.
**Step 9.2: STABLE RULES CHECKLIST**
1. Obviously correct and tested? **PASS** — standard clk check/unwind;
maintainer review/merge. No extra Tested-by.
2. Fixes a real bug that affects users? **PASS** — ignored required-
clock failure on probe/resume.
3. Important issue? **PASS** — hang / dead I2C / clock leak on that
path, not a style cleanup.
4. Small and contained? **PASS** — +24/−7, one file.
5. No new features or APIs? **PASS**.
6. Can apply to the local tree? **PASS** — driver and call sites match.
**Step 9.3: EXCEPTION CATEGORIES**
Record: None (not a device ID, quirk, DT, build, or doc fix). Qualifies
as a normal error-path bugfix.
**Step 9.4: DECISION**
This is a small, correct fix for ignored required-clock errors. On this
v7.3-rc4 tree the buggy helper and the QUP probe/resume paths are
present; the change prevents probe/resume from continuing into unclocked
MMIO. That is enough for stable, even without a field report.
---
### What it solves for this tree
`qup_i2c_enable_clocks()` used to ignore `clk_prepare_enable()`. Probe
still registered the adapter and then `writel`’d `QUP_SW_RESET`; resume
returned 0. If enable actually failed, that is unclocked MMIO (possible
SoC hang) plus a clock leak when only the iface clock failed. The patch
fails those paths and unwinds.
### Stable rules / risk
Meets the rules: real bug, small, no features, applies here. Benefit is
avoiding hangs and false-success PM on Qualcomm QUP I2C; risk is low.
### Concerns
No `Fixes:` / user report (not a reason to reject). `fail_dma` on enable
failure is correct. No follow-up fix found.
---
### Verification
- **[Phase 1]** Tags: Reviewed-by Konrad Dybcio; SOB Andi Shyti; Link
lore msgid `20260624055237.29977-1`; no Fixes/Reported-by/Cc:stable.
- **[Phase 2]** Diff: +24/−7 in `i2c-qup.c`; helper becomes `int`;
probe/resume propagate; partial unwind on iface fail.
- **[Phase 2]** Pre-fix parent: void helper, probe
`qup_i2c_enable_clocks(qup)` then `writel(QUP_SW_RESET)`; resume
always 0.
- **[Phase 2]** Post-fix probe uses `goto fail_dma` (not `fail`), so
`qup_i2c_disable_clocks()` is not run on a failed enable.
- **[Phase 3]** `git log -L :qup_i2c_enable_clocks:` → `10c5a8425968`
(v3.15-rc1) then `a477626332800`.
- **[Phase 3]** `git merge-base --is-ancestor` both SHAs vs HEAD: both
in this tree.
- **[Phase 3]** Author: only this I2C commit; no later commits on this
file after the fix.
- **[Phase 4]** `b4 dig -c a477626332800`: matched msgid; `-a`: v1 only;
`-w`: Andi + linux-i2c/arm-msm.
- **[Phase 4]** Review: Konrad Reviewed-by; Andi “merged to i2c/i2c”; no
NAK.
- **[Phase 4]** lore.kernel.org 403; gnuweeb/patchew/openwall used
instead. Stable-list search: no hits for this subject.
- **[Phase 5]** Callers: probe, runtime resume, system resume only.
Other I2C hosts already check `clk_prepare_enable`.
- **[Phase 6]** `git describe HEAD` → `v7.3-rc4-27-gf0100363d8c37`;
`make kernelversion` → `7.3.0-rc4`. Buggy code existed; driver
present.
- **[Phase 7]** `CONFIG_I2C_QUP` depends on `ARCH_QCOM`; I2C maintainer
Andi Shyti.
- **[Phase 8]** Failure mode: unclocked MMIO / false resume success;
severity HIGH if triggered, uncommon trigger.
- **UNVERIFIED:** Could not fetch lore.kernel.org HTML directly (403).
Thread content verified via b4 + mirrors.
**YES**
drivers/i2c/busses/i2c-qup.c | 31 ++++++++++++++++++++++++-------
1 file changed, 24 insertions(+), 7 deletions(-)
diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
index a0e076fc5f363..ee7915ee2ba2f 100644
--- a/drivers/i2c/busses/i2c-qup.c
+++ b/drivers/i2c/busses/i2c-qup.c
@@ -1657,10 +1657,21 @@ static const struct i2c_adapter_quirks qup_i2c_quirks_v2 = {
.flags = I2C_AQ_NO_ZERO_LEN,
};
-static void qup_i2c_enable_clocks(struct qup_i2c_dev *qup)
+static int qup_i2c_enable_clocks(struct qup_i2c_dev *qup)
{
- clk_prepare_enable(qup->clk);
- clk_prepare_enable(qup->pclk);
+ int ret;
+
+ ret = clk_prepare_enable(qup->clk);
+ if (ret)
+ return ret;
+
+ ret = clk_prepare_enable(qup->pclk);
+ if (ret) {
+ clk_disable_unprepare(qup->clk);
+ return ret;
+ }
+
+ return 0;
}
static void qup_i2c_disable_clocks(struct qup_i2c_dev *qup)
@@ -1823,7 +1834,9 @@ static int qup_i2c_probe(struct platform_device *pdev)
ret = PTR_ERR(qup->pclk);
goto fail_dma;
}
- qup_i2c_enable_clocks(qup);
+ ret = qup_i2c_enable_clocks(qup);
+ if (ret)
+ goto fail_dma;
src_clk_freq = clk_get_rate(qup->clk);
}
qup->src_clk_freq = src_clk_freq;
@@ -1975,8 +1988,7 @@ static int qup_i2c_pm_resume_runtime(struct device *device)
struct qup_i2c_dev *qup = dev_get_drvdata(device);
dev_dbg(device, "pm_runtime: resuming...\n");
- qup_i2c_enable_clocks(qup);
- return 0;
+ return qup_i2c_enable_clocks(qup);
}
static int qup_i2c_suspend(struct device *device)
@@ -1988,7 +2000,12 @@ static int qup_i2c_suspend(struct device *device)
static int qup_i2c_resume(struct device *device)
{
- qup_i2c_pm_resume_runtime(device);
+ int ret;
+
+ ret = qup_i2c_pm_resume_runtime(device);
+ if (ret)
+ return ret;
+
pm_request_autosuspend(device);
return 0;
}
--
2.53.0