Linux-i3c Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH AUTOSEL 6.18-5.15] i3c: mipi-i3c-hci: Tolerate i3c_master_add_i3c_dev_locked() failures in DAA
       [not found] <20260831133314.4125787-1-sashal@kernel.org>
@ 2026-08-31 13:30 ` Sasha Levin
  2026-08-31 17:36   ` sashiko-bot
  2026-08-31 13:31 ` [PATCH AUTOSEL 6.18] i3c: master: svc: Prevent IRQ storm from false SLVSTART on NPCM845 Sasha Levin
  1 sibling, 1 reply; 4+ messages in thread
From: Sasha Levin @ 2026-08-31 13:30 UTC (permalink / raw)
  To: patches, stable
  Cc: Adrian Hunter, Frank Li, Alexandre Belloni, Sasha Levin,
	linux-i3c, linux-kernel

From: Adrian Hunter <adrian.hunter@intel.com>

[ Upstream commit c236563c8a84239d31a1e6ec4444887a7b5ed98f ]

i3c_master_add_i3c_dev_locked() no longer leaves the address marked as
free on failure, so aborting the DAA sequence on its error is unnecessary.

Failure to register a discovered device does not invalidate the entire
Dynamic Address Assignment (DAA) procedure.  Align with the behavior of
other I3C master drivers by ignoring errors from
i3c_master_add_i3c_dev_locked() and continuing enumeration.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Link: https://patch.msgid.link/20260612080107.11606-5-adrian.hunter@intel.com
Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---

LLM Generated explanations, may be completely bogus:

## Phase 1: Commit Message Forensics

### Step 1.1: Subject Line
**Record:** `[i3c: mipi-i3c-hci]` `[Tolerate]` — Stop aborting DAA when
`i3c_master_add_i3c_dev_locked()` fails; continue enumeration instead.

### Step 1.2: Tags
**Record:**
- **Signed-off-by:** Adrian Hunter `<adrian.hunter@intel.com>` (author)
- **Reviewed-by:** Frank Li `<Frank.Li@nxp.com>` (NXP I3C maintainer)
- **Link:** https://patch.msgid.link/20260612080107.11606-5-
  adrian.hunter@intel.com
- **Signed-off-by:** Alexandre Belloni `<alexandre.belloni@bootlin.com>`
  (I3C subsystem maintainer)
- No Fixes:, Reported-by:, Tested-by:, Cc: stable, or syzbot tags
- Notable: Reviewed by subsystem expert; part of V4 4/7 series

### Step 1.3: Body Analysis
**Record:**
- **Bug:** `mipi-i3c-hci` aborts the entire DAA loop when
  `i3c_master_add_i3c_dev_locked()` fails for one device.
- **Symptom:** Remaining I3C devices on the bus are never
  enumerated/registered after a single device-add failure.
- **Root cause (per author):** After commit `38d3d33` ("Prevent reuse of
  dynamic address on device add failure"), failed registration no longer
  frees the address slot, so aborting DAA is unnecessary and harmful.
- **Fix approach:** Ignore the return value and continue DAA, matching
  `svc-i3c-master`, `cdns`, `renesas`, `dw`, and `adi` drivers.
- No explicit kernel version range in message.

### Step 1.4: Hidden Bug Fix Detection
**Record:** Yes — described as alignment/cleanup, but it fixes a real
logic bug: premature DAA termination leaves devices undiscovered. Same
class of bug fixed in `svc-i3c-master` (commit `3b2ac810`, Cc: stable).

---

## Phase 2: Diff Analysis

### Step 2.1: Inventory
**Record:**
- `drivers/i3c/master/mipi-i3c-hci/cmd_v1.c`: −3 lines (remove ret check
  + break)
- `drivers/i3c/master/mipi-i3c-hci/cmd_v2.c`: −3 lines (same)
- Functions: `hci_cmd_v1_daa()`, `hci_cmd_v2_daa()`
- Scope: single-file surgical fix in one driver (2 command variants)

### Step 2.2: Code Flow Change
**Record:**
- **Hunk 1 (`cmd_v1.c`):** Before: assign address via hardware DAA →
  call `i3c_master_add_i3c_dev_locked()` → on error, `break` out of DAA
  loop. After: call function without checking return; loop continues to
  next device.
- **Hunk 2 (`cmd_v2.c`):** Identical behavioral change in v2 DAA path.
- Affected path: normal DAA enumeration loop during bus probe / hot-
  join.

### Step 2.3: Bug Mechanism
**Record:**
- **Category:** Logic/correctness fix (error-path handling)
- **Mechanism:** Treating a per-device registration failure as fatal to
  the entire multi-device DAA sequence. With prerequisite `38d3d33`, the
  address is retained on failure, so continuing is safe. Aborting
  prevents registration of subsequently discovered devices.

### Step 2.4: Fix Quality
**Record:** Obviously correct — matches established pattern in five
other I3C master drivers. Minimal change. Low regression risk: only
removes an overly aggressive early-exit; real bus/transfer errors still
break the loop via `RESP_STATUS` checks.

---

## Phase 3: Git History Investigation

### Step 3.1: Blame
**Record:** Buggy `if (ret) break;` pattern introduced in
`9ad9a52cce282` (Nov 2020, "i3c/master: introduce the mipi-i3c-hci
driver"). Present since driver introduction.

### Step 3.2: Fixes: Tag
**Record:** N/A — no Fixes: tag. Prerequisite identified from commit
message: `38d3d33bf42c2` / upstream `b3ba8383da4d0` ("Prevent reuse of
dynamic address on device add failure"), which changes
`i3c_master_add_i3c_dev_locked()` to mark addresses as occupied on
failure via `err_prevent_addr_reuse`. **Confirmed present in this tree**
(`git merge-base --is-ancestor` passes).

### Step 3.3: Related File History
**Record:** Recent related commits in tree:
- `38d3d33` — prerequisite (already backported to 6.18.y)
- `3b2ac810` — svc driver: identical "don't check return value" fix (in
  tree, Cc: stable)
- Fix commit `c236563c8a842` is in mainline but **not yet in this
  6.18.44 checkout**

### Step 3.4: Author Context
**Record:** Adrian Hunter is an active Intel I3C contributor. Multiple
related fixes in `drivers/i3c/` around DAA, hot-join, and address
management (Jun 2026 series).

### Step 3.5: Dependencies
**Record:**
- **Hard dependency:** `38d3d33` (already in tree) — without it,
  continuing DAA after failure could reassign addresses.
- **Not required:** Patches 1/7 (race fix), 2/7 (DISEC), 5–7/7 (return-
  void API change + reconciliation — **not merged to mainline**).
- **Standalone:** Yes, for stable purposes, given prerequisite is
  present.

---

## Phase 4: Mailing List and External Research

### Step 4.1: Original Discussion
**Record:**
- **URL:** https://patch.msgid.link/20260612080107.11606-5-
  adrian.hunter@intel.com
- **Series:** V1→V4, patch 4/7; applied version is latest (V4)
- **Cover letter (V4 0/7):** "Patches 3-7 fix address management
  issues... when DAA does not complete cleanly"
- **Reviewer feedback:** "Applied, thanks!" from maintainer on cover
  letter
- No explicit stable nomination found in mbox for this specific patch

### Step 4.2: Reviewers
**Record:** CC'd: `alexandre.belloni@bootlin.com`, `Frank.Li@nxp.com`,
`linux-i3c@lists.infradead.org`, `linux-kernel@vger.kernel.org`.
Reviewed-by Frank Li (NXP).

### Step 4.3: Bug Report
**Record:** No external bug report or syzbot link. Bug identified
through code review / series development. Precedent: `3b2ac810`
documented identical failure mode for svc driver with explicit I3C spec
violation scenario.

### Step 4.4: Related Patches
**Record:** Part of 7-patch V4 series. Patches 5–7 (API change to void
return + post-DAA reconciliation) were **not** merged upstream. This
patch was merged standalone with patch 3.

### Step 4.5: Stable List History
**Record:** No stable-list discussion found for this specific patch.
Prerequisite `38d3d33` was backported to this tree (has upstream-commit
marker and Greg K commit).

---

## Phase 5: Code Semantic Analysis

### Step 5.1: Key Functions
**Record:** `hci_cmd_v1_daa()`, `hci_cmd_v2_daa()`, called via
`i3c_hci_daa()` in `core.c`.

### Step 5.2: Callers
**Record:** `i3c_hci_daa()` → registered as `master->ops.do_daa` →
invoked by `i3c_master_do_daa()` / `i3c_master_do_daa_ext()` during bus
initialization and hot-join DAA. Called during device probe, not a hot
syscall path.

### Step 5.3: Callees
**Record:** `i3c_master_add_i3c_dev_locked()` — allocates device,
retrieves CCC info, attaches to bus. On failure (with `38d3d33`): logs
error, marks address slot occupied, returns error code.

### Step 5.4: Reachability
**Record:** Triggered during I3C bus enumeration on systems using
`mipi-i3c-hci` (Intel and other MIPI HCI platforms). Multi-device buses
are common (sensors, PMICs, etc.). Failure of one device's registration
is plausible (transient CCC errors, firmware quirks).

### Step 5.5: Similar Patterns
**Record:** All other I3C master drivers ignore
`i3c_master_add_i3c_dev_locked()` return during DAA:

```1224:1225:drivers/i3c/master/svc-i3c-master.c
        for (i = 0; i < dev_nb; i++)
                i3c_master_add_i3c_dev_locked(m, addrs[i]);
```

Same pattern in `renesas-i3c.c`, `i3c-master-cdns.c`, `dw-i3c-master.c`,
`adi-i3c-master.c`. `mipi-i3c-hci` is the only outlier.

---

## Phase 6: Cross-Reference Against Local Tree

### Step 6.1: Buggy Code Exists?
**Record:** **Yes.** Local tree is **linux-6.18.y** (`v6.18.44`). Both
`cmd_v1.c:365-367` and `cmd_v2.c:303-305` still have `ret =
i3c_master_add_i3c_dev_locked(...); if (ret) break;`. Bug present since
driver introduction (2020).

### Step 6.2: Backport Complications
**Record:** **Clean apply.** `git cherry-pick --no-commit c236563c8a842`
auto-merged both files without conflicts.

### Step 6.3: Related Fixes Already Present?
**Record:**
- Prerequisite `38d3d33` — **present**
- Svc driver equivalent fix `3b2ac810` — **present**
- This specific mipi-i3c-hci fix — **not present**

---

## Phase 7: Subsystem and Maintainer Context

### Step 7.1: Subsystem Criticality
**Record:** `drivers/i3c/master/mipi-i3c-hci/` — **IMPORTANT** (bus
driver affecting all I3C peripherals on HCI-based platforms, but
hardware-specific).

### Step 7.2: Subsystem Activity
**Record:** Actively maintained — multiple mipi-i3c-hci fixes in 6.18.y
(hot-join, DMA, IRQ handling).

---

## Phase 8: Impact and Risk Assessment

### Step 8.1: Who Is Affected
**Record:** Users of `CONFIG_I3C` with `mipi-i3c-hci` hardware and
multiple I3C devices on the bus.

### Step 8.2: Trigger Conditions
**Record:** DAA discovers ≥2 devices; `i3c_master_add_i3c_dev_locked()`
fails for an early device (allocation failure, CCC retrieval error,
duplicate handling, etc.). Not timing-dependent. Requires `CONFIG_I3C` +
mipi-i3c-hci. Unprivileged users cannot directly trigger, but affects
system hardware functionality at boot/probe.

### Step 8.3: Failure Mode Severity
**Record:** Subsequent I3C devices fail to register —
sensors/peripherals missing, potential bus state inconsistency (hardware
has address assigned, kernel unaware). **Severity: MEDIUM-HIGH** for
affected hardware (functional failure, not kernel crash). Pre-`38d3d33`,
could also cause address conflicts on hot-join (documented in svc fix);
that part is fixed, but partial enumeration remains.

### Step 8.4: Risk-Benefit
**Record:**
- **Benefit:** HIGH for affected platforms — restores multi-device
  enumeration; completes behavior change started by `38d3d33` backport
- **Risk:** VERY LOW — 6-line total removal, matches five other drivers,
  prerequisite already in tree
- **Ratio:** Strong benefit, minimal risk

---

## Phase 9: Final Synthesis

### Step 9.1: Evidence Summary

**FOR backport:**
- Real functional bug with documented precedent (`3b2ac810`, Cc: stable)
- Prerequisite `38d3d33` already in 6.18.y
- Surgical 6-line fix, reviewed by NXP maintainer
- Applies cleanly
- Aligns mipi-i3c-hci with all other I3C master drivers
- Completes address-management fix series partially backported to stable

**AGAINST backport:**
- No crash, security issue, or data corruption
- Hardware-specific (mipi-i3c-hci only)
- Requires `i3c_master_add_i3c_dev_locked()` failure (not everyday, but
  plausible)
- Part of larger series (but this patch is self-contained with
  prerequisite)

**Unresolved:** No user bug report or syzbot reproduction; impact
inferred from code analysis and svc-driver precedent.

### Step 9.2: Stable Rules Checklist
1. Obviously correct and tested? **PASS** — matches established driver
   pattern; Reviewed-by Frank Li
2. Fixes a real bug? **PASS** — premature DAA abort leaves devices
   undiscovered
3. Important issue? **PASS** — functional hardware failure on multi-
   device I3C buses (MEDIUM-HIGH for affected users)
4. Small and contained? **PASS** — 2 files, 6 lines removed
5. No new features/APIs? **PASS** — behavior alignment only
6. Can apply to local tree? **PASS** — clean cherry-pick; prerequisite
   present

### Step 9.3: Exception Categories
**Record:** N/A — not a device ID, quirk, DT, build, or docs fix.
Standard driver bug fix.

### Step 9.4: Decision Rationale
This fix should be backported to **linux-6.18.y** because the
prerequisite commit changing `i3c_master_add_i3c_dev_locked()` failure
semantics is already in the tree, but `mipi-i3c-hci` still aborts DAA on
registration failure — leaving subsequent devices undiscovered. The
identical issue was fixed and stable-nominated for the svc driver. This
is the companion fix for the HCI driver, small and low-risk.

---

## Verification

- [Phase 1] `git show c236563c8a842`: parsed subject, tags, body
- [Phase 2] Diff: 2 files, `hci_cmd_v1_daa`/`hci_cmd_v2_daa`, 6 lines
  removed
- [Phase 3] `git blame cmd_v1.c:365-367`: introduced `9ad9a52` (2020)
- [Phase 3] `git merge-base --is-ancestor 38d3d33bf42c2 HEAD`:
  prerequisite present
- [Phase 3] `git show 38d3d33`: `err_prevent_addr_reuse` handler
  confirmed
- [Phase 3] `git show 3b2ac810`: svc driver identical fix with Cc:
  stable
- [Phase 3] `git merge-base --is-ancestor 3b2ac810 HEAD`: svc fix in
  tree
- [Phase 4] `b4 dig -c c236563c8a842`: lore URL found
- [Phase 4] `b4 dig -a`: V1–V4 series, patch 4/7
- [Phase 4] `b4 dig -w`: maintainers CC'd
- [Phase 4] `/tmp/i3c_daa.mbox`: cover letter, patches 1–7 subjects;
  patch 7 not merged upstream
- [Phase 5] `grep i3c_master_add_i3c_dev_locked drivers/i3c/master/`: 5
  other drivers ignore return
- [Phase 5] `core.c:266-270`: `i3c_hci_daa` → `perform_daa` call chain
- [Phase 6] `git describe HEAD`: v6.18.44 (linux-6.18.y)
- [Phase 6] Read `cmd_v1.c:365-367`, `cmd_v2.c:303-305`: buggy code
  present
- [Phase 6] `git cherry-pick --no-commit c236563c8a842`: clean apply
- [Phase 6] `git log HEAD..c236563c8a842 --
  drivers/i3c/master/mipi-i3c-hci/`: fix not in tree
- [Phase 7] `git log -20 -- drivers/i3c/master/mipi-i3c-hci/`: active
  subsystem
- [Phase 8] Read `svc-i3c-master.c:1205-1225`: documented failure mode

**YES**

 drivers/i3c/master/mipi-i3c-hci/cmd_v1.c | 4 +---
 drivers/i3c/master/mipi-i3c-hci/cmd_v2.c | 4 +---
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/i3c/master/mipi-i3c-hci/cmd_v1.c b/drivers/i3c/master/mipi-i3c-hci/cmd_v1.c
index efb7a1f92641c..b6ba18ccdd2d3 100644
--- a/drivers/i3c/master/mipi-i3c-hci/cmd_v1.c
+++ b/drivers/i3c/master/mipi-i3c-hci/cmd_v1.c
@@ -362,9 +362,7 @@ static int hci_cmd_v1_daa(struct i3c_hci *hci)
 		 * TODO: Extend the subsystem layer to allow for registering
 		 * new device and provide BCR/DCR/PID at the same time.
 		 */
-		ret = i3c_master_add_i3c_dev_locked(&hci->master, next_addr);
-		if (ret)
-			break;
+		i3c_master_add_i3c_dev_locked(&hci->master, next_addr);
 	}
 
 	if (dat_idx >= 0)
diff --git a/drivers/i3c/master/mipi-i3c-hci/cmd_v2.c b/drivers/i3c/master/mipi-i3c-hci/cmd_v2.c
index 5fc2e4c55ebb0..d7999d341a227 100644
--- a/drivers/i3c/master/mipi-i3c-hci/cmd_v2.c
+++ b/drivers/i3c/master/mipi-i3c-hci/cmd_v2.c
@@ -300,9 +300,7 @@ static int hci_cmd_v2_daa(struct i3c_hci *hci)
 		 * TODO: Extend the subsystem layer to allow for registering
 		 * new device and provide BCR/DCR/PID at the same time.
 		 */
-		ret = i3c_master_add_i3c_dev_locked(&hci->master, next_addr);
-		if (ret)
-			break;
+		i3c_master_add_i3c_dev_locked(&hci->master, next_addr);
 	}
 
 	hci_free_xfer(xfer, 2);
-- 
2.53.0


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [PATCH AUTOSEL 6.18] i3c: master: svc: Prevent IRQ storm from false SLVSTART on NPCM845
       [not found] <20260831133314.4125787-1-sashal@kernel.org>
  2026-08-31 13:30 ` [PATCH AUTOSEL 6.18-5.15] i3c: mipi-i3c-hci: Tolerate i3c_master_add_i3c_dev_locked() failures in DAA Sasha Levin
@ 2026-08-31 13:31 ` Sasha Levin
  2026-08-31 17:40   ` sashiko-bot
  1 sibling, 1 reply; 4+ messages in thread
From: Sasha Levin @ 2026-08-31 13:31 UTC (permalink / raw)
  To: patches, stable
  Cc: Stanley Chu, Frank Li, Alexandre Belloni, Sasha Levin,
	miquel.raynal, linux-i3c, imx, linux-kernel

From: Stanley Chu <yschu@nuvoton.com>

[ Upstream commit 1effa3adfe53cb2bb28bf5640a676b791d5ab405 ]

On NPCM845, when a target on the I3C bus gets stuck holding SDA low,
the controller reports a false Master Request (MR) in-band interrupt
event. The driver handles this by emitting a STOP condition to restore
the bus.

However, the hardware quirk SVC_I3C_QUIRK_FALSE_SLVSTART indicates that
emitting a STOP condition may spuriously set the SLVSTART interrupt
status bit. In the Master Request case, this creates a feedback loop:
the STOP triggers a new SLVSTART event, the IRQ handler fires again,
the controller still reports an MR type, another STOP is emitted, and
the cycle repeats indefinitely, resulting in an IRQ storm that can lock
up the CPU.

Clear the SLVSTART status bit explicitly after emitting the STOP in the
Master Request IBI handler when the SVC_I3C_QUIRK_FALSE_SLVSTART quirk
is set. This breaks the feedback loop without affecting normal SLVSTART
processing, which is already guarded in the top-level IRQ handler by
checking that MSTATUS is in SLVREQ state.

Signed-off-by: Stanley Chu <yschu@nuvoton.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Link: https://patch.msgid.link/20260413005040.1211107-3-yschu@nuvoton.com
Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---

LLM Generated explanations, may be completely bogus:

# Stable Backport Analysis: i3c: master: svc: Prevent IRQ storm from
false SLVSTART on NPCM845

**Local tree:** Linux 6.18.43 (`v6.18.43-1-gc7f0dac02d232`)

---

## PHASE 1: COMMIT MESSAGE FORENSICS

### Step 1.1: Subject Line
**Record:** `[i3c: master: svc]` `[Prevent]` — Prevent IRQ storm from
false SLVSTART on NPCM845 (Nuvoton BMC I3C master driver hardware quirk
workaround).

### Step 1.2: Tags
**Record:**
- **Signed-off-by:** Stanley Chu `<yschu@nuvoton.com>` (author)
- **Reviewed-by:** Frank Li `<Frank.Li@nxp.com>` (NXP I3C reviewer)
- **Link:**
  https://patch.msgid.link/20260413005040.1211107-3-yschu@nuvoton.com
- **Signed-off-by:** Alexandre Belloni `<alexandre.belloni@bootlin.com>`
  (I3C maintainer)
- No Fixes:, Reported-by:, Tested-by:, Cc: stable@vger.kernel.org
- Notable: Reviewed by subsystem reviewer; maintainer applied the
  series. No syzbot report (hardware-specific quirk).

### Step 1.3: Body Analysis
**Record:**
- **Bug:** On NPCM845, when an I3C target holds SDA low (bus stuck), the
  controller reports a false Master Request (MR) IBI. The driver emits
  STOP to recover the bus, but STOP spuriously sets the SLVSTART status
  bit (known `SVC_I3C_QUIRK_FALSE_SLVSTART` behavior).
- **Symptom:** Feedback loop — STOP → spurious SLVSTART → IRQ handler →
  MR again → STOP → … → **IRQ storm that can lock up the CPU**.
- **Root cause:** MR handler emits STOP without clearing the spurious
  SLVSTART bit afterward; top-level quirk guard (SLVREQ state check)
  does not break this specific MR+stuck-SDA loop.
- **Fix:** After STOP in the `MASTER_REQUEST` IBI path, explicitly clear
  SLVSTART when the quirk is set.
- **Version info:** NPCM845-specific; no kernel version range stated.

### Step 1.4: Hidden Bug Fix Detection
**Record:** Not disguised — explicitly a bug fix for IRQ storm / CPU
lockup. Falls under hardware quirk/workaround exception category.

---

## PHASE 2: DIFF ANALYSIS

### Step 2.1: Inventory
**Record:**
- **File:** `drivers/i3c/master/svc-i3c-master.c` (+9 lines, 0 removed)
- **Function modified:** `svc_i3c_master_ibi_isr()`
- **Scope:** Single-file, surgical fix in one `switch` case
  (`SVC_I3C_MSTATUS_IBITYPE_MASTER_REQUEST`)

### Step 2.2: Code Flow Change
**Record:**
- **Hunk (MASTER_REQUEST case):**
  - **Before:** `svc_i3c_master_emit_stop(master); break;`
  - **After:** Same STOP, then if `SVC_I3C_QUIRK_FALSE_SLVSTART` quirk
    is set, `writel(SVC_I3C_MINT_SLVSTART, master->regs +
    SVC_I3C_MSTATUS)` to clear spurious SLVSTART.
- **Path affected:** IRQ-driven IBI handler, non-critical task section,
  MR event only, only when quirk bit is set (NPCM845).

### Step 2.3: Bug Mechanism
**Record:** **Category:** Hardware quirk workaround / IRQ storm
prevention (synchronization with hardware interrupt status).
- STOP on NPCM845 spuriously sets SLVSTART interrupt status.
- In MR+stuck-SDA scenario, top-level handler's SLVREQ guard does not
  prevent re-entry into MR handling.
- Explicit status clear after STOP breaks the feedback loop.

### Step 2.4: Fix Quality
**Record:**
- **Obviously correct:** Uses the same `writel(SVC_I3C_MINT_SLVSTART,
  ...)` pattern already used in `svc_i3c_master_irq_handler()` at line
  626.
- **Minimal:** Quirk-gated, only in MR path.
- **Regression risk:** Very low — only affects NPCM845
  (`npcm845_drvdata` sets `SVC_I3C_QUIRK_FALSE_SLVSTART`). Normal
  SLVSTART processing remains guarded by SLVREQ check in the top-level
  IRQ handler.

---

## PHASE 3: GIT HISTORY INVESTIGATION

### Step 3.1: Blame
**Record:** Lines 609–611 (`MASTER_REQUEST` STOP without clear) blamed
to `19eef1d98eeda` (kernel import). The MR+STOP path predates this
series; the missing clear is a gap in the original
`SVC_I3C_QUIRK_FALSE_SLVSTART` handling from March 2025.

### Step 3.2: Fixes: Tag
**Record:** No Fixes: tag. N/A.

### Step 3.3: Related File History
**Record:** Related commits in this tree on `svc-i3c-master.c`:
- `466c7f87de52d` — Fix missed IBI after false SLVSTART (series patch
  1/2, **present**)
- `98ddff8a90f82` — Initialize `dev` to NULL in
  `svc_i3c_master_ibi_isr()`
- `8ddff9989f06a` — Prevent incomplete IBI transaction
- Quirk introduced via code present since kernel import;
  `SVC_I3C_QUIRK_FALSE_SLVSTART` and `npcm845_drvdata` confirmed in
  tree.

### Step 3.4: Author Context
**Record:** Stanley Chu (Nuvoton) authored NPCM845 I3C fixes. Frank Li
(NXP) reviewed. Alexandre Belloni (I3C maintainer) committed. Author has
multiple related svc-i3c-master fixes in this tree.

### Step 3.5: Dependencies
**Record:**
- **Prerequisite:** Patch 1/2 (`466c7f87de52d` — re-read MSTATUS in IRQ
  handler) is **already in this tree**.
- **Required infrastructure:** `SVC_I3C_QUIRK_FALSE_SLVSTART`,
  `svc_has_quirk()`, `npcm845_drvdata` — all **present**.
- **Standalone:** This patch (2/2) is self-contained; applies cleanly on
  top of current tree (`git apply --check` passed).
- Upstream commit: `1effa3adfe53c`; **not yet in this 6.18.43 tree**.

---

## PHASE 4: MAILING LIST AND EXTERNAL RESEARCH

### Step 4.1: Original Discussion
**Record:**
- **b4 dig -c 1effa3adfe53c:**
  https://patch.msgid.link/20260413005040.1211107-3-yschu@nuvoton.com
- **Series:** v1, 2 patches: (1) Fix missed IBI, (2) Prevent IRQ storm
- **Maintainer response:** Alexandre Belloni: "Applied, thanks!" — both
  patches applied to i3c tree.
- **Stable nomination:** None found in thread.
- **NAKs/concerns:** None found.

### Step 4.2: Reviewers
**Record:** CC'd: frank.li@nxp.com, miquel.raynal@bootlin.com,
alexandre.belloni@bootlin.com, linux-i3c@lists.infradead.org, Nuvoton
engineers. Reviewed-by: Frank Li.

### Step 4.3: Bug Report
**Record:** No external bug report or syzbot link. Hardware quirk
described by Nuvoton driver author; credible for embedded BMC platform.

### Step 4.4: Series Context
**Record:** 2-patch series addressing false SLVSTART quirk. Patch 1
fixes missed IBI (race); patch 2 fixes IRQ storm (feedback loop). Both
are complementary; patch 1 already in this tree; patch 2 is still
missing.

### Step 4.5: Stable List History
**Record:** Not searched on lore stable list; no stable nomination found
in patch thread. Absence is not a negative signal per instructions.

---

## PHASE 5: CODE SEMANTIC ANALYSIS

### Step 5.1: Key Functions
**Record:** `svc_i3c_master_ibi_isr()` (modified),
`svc_i3c_master_irq_handler()` (caller, unmodified).

### Step 5.2: Callers
**Record:**
- `svc_i3c_master_irq_handler()` → `svc_i3c_master_ibi_isr()` (line 646)
- IRQ registered via `devm_request_irq()` at line 1944
- **Context:** Hard IRQ context on I3C SLVSTART interrupt — hot path for
  all IBI events on NPCM845.

### Step 5.3: Callees
**Record:** `svc_i3c_master_emit_stop()`, `svc_has_quirk()`, `writel()`
to hardware MSTATUS register.

### Step 5.4: Reachability
**Record:**
- Triggered when I3C bus target holds SDA low (hardware fault or
  misbehaving device).
- IRQ-driven, runs on every spurious SLVSTART in the MR feedback loop.
- Not directly userspace-triggerable, but bus faults on BMC/server
  platforms are realistic production scenarios.
- **Impact when triggered:** Continuous IRQ processing → CPU lockup.

### Step 5.5: Similar Patterns
**Record:** Top-level IRQ handler already clears SLVSTART and has quirk
guard. IBI and HOT_JOIN cases also emit STOP but do not need this extra
clear (commit explains MR-specific loop). Same
`writel(SVC_I3C_MINT_SLVSTART, ...)` idiom used elsewhere in file.

---

## PHASE 6: CROSS-REFERENCING AGAINST LOCAL TREE

### Step 6.1: Buggy Code Exists?
**Record:** **YES.** Current tree at lines 609–611:

```609:611:drivers/i3c/master/svc-i3c-master.c
        case SVC_I3C_MSTATUS_IBITYPE_MASTER_REQUEST:
                svc_i3c_master_emit_stop(master);
                break;
```

No SLVSTART clear after STOP. `SVC_I3C_QUIRK_FALSE_SLVSTART` and
`npcm845_drvdata` are present (lines 154, 2056–2059). Prerequisite patch
`466c7f87de52d` is present. Upstream fix `1effa3adfe53c` is **not** in
this tree.

### Step 6.2: Backport Complications
**Record:** **Clean apply** — `git apply --check` against upstream diff
succeeded with no conflicts. No rework needed.

### Step 6.3: Related Fixes Already Present?
**Record:** Patch 1/2 (`466c7f87de52d`) present. IRQ storm fix
(`1effa3adfe53c`) absent. No alternate fix for this issue found.

---

## PHASE 7: SUBSYSTEM AND MAINTAINER CONTEXT

### Step 7.1: Subsystem Criticality
**Record:** **Subsystem:** `drivers/i3c/master/` — I3C bus master driver
(Silvaco/Vayavya Labs SVC IP, Nuvoton NPCM845). **Criticality:**
IMPORTANT/PERIPHERAL — affects NPCM845 BMC platforms specifically, but
IRQ storm is a system-wide CPU lockup.

### Step 7.2: Subsystem Activity
**Record:** I3C subsystem actively maintained in 6.18.y with recent svc
and mipi-i3c-hci fixes. NPCM845 support and quirk infrastructure are
established in this tree.

---

## PHASE 8: IMPACT AND RISK ASSESSMENT

### Step 8.1: Who Is Affected
**Record:** Users of Nuvoton NPCM845 I3C controller
(`"nuvoton,npcm845-i3c"` DT compatible). Primarily embedded BMC/server
platforms. Config-specific (driver + hardware present).

### Step 8.2: Trigger Conditions
**Record:** I3C target stuck holding SDA low → false MR IBI → STOP
recovery loop. Requires bus fault or misbehaving device — uncommon but
realistic. Not unprivileged-userspace-direct, but can freeze the system
when it occurs.

### Step 8.3: Failure Mode Severity
**Record:** **IRQ storm → CPU lockup.** Severity: **CRITICAL** (system
becomes unresponsive).

### Step 8.4: Risk-Benefit
**Record:**
- **Benefit:** HIGH for NPCM845 users — prevents system lockup on bus
  fault.
- **Risk:** VERY LOW — 9 lines, quirk-gated, same register write pattern
  as existing code, zero impact on non-NPCM845 platforms.
- **Ratio:** Strongly favors backport.

---

## PHASE 9: FINAL SYNTHESIS

### Step 9.1: Evidence Summary

**FOR backporting:**
- Fixes real IRQ storm causing CPU lockup (CRITICAL severity)
- Small, surgical, quirk-gated hardware workaround
- Reviewed by NXP reviewer; applied by I3C maintainer
- Prerequisites present in 6.18.43 tree; applies cleanly
- Complements already-backported patch 1/2 in the series
- Matches stable exception category: hardware quirk/workaround

**AGAINST backporting:**
- NPCM845-specific (limited audience) — but stable routinely takes
  hardware quirk fixes
- Requires bus fault to trigger — but consequence is system lockup
- No syzbot/user bug report — but hardware quirk from silicon vendor is
  credible

**Unresolved:** None affecting the decision.

### Step 9.2: Stable Rules Checklist
1. Obviously correct and tested? **PASS** — minimal register clear,
   reviewed, maintainer-applied
2. Fixes a real bug? **PASS** — IRQ storm on NPCM845
3. Important issue? **PASS** — CPU lockup (CRITICAL)
4. Small and contained? **PASS** — 9 lines, one case branch
5. No new features/APIs? **PASS** — quirk workaround only
6. Can apply to local tree? **PASS** — clean apply, prerequisites
   present

### Step 9.3: Exception Category
**Record:** Hardware quirk/workaround for NPCM845 I3C controller.

### Step 9.4: Decision Rationale
This commit closes a gap in the existing `SVC_I3C_QUIRK_FALSE_SLVSTART`
handling. The 6.18.43 tree already has NPCM845 support, the quirk flag,
and series patch 1/2, but lacks this IRQ storm fix. When an I3C bus
fault causes a target to hold SDA low, the driver can enter an infinite
IRQ loop that locks the CPU. The fix is minimal, quirk-gated, reviewed,
and applies cleanly. This is appropriate stable material.

---

## Verification

- [Phase 1] Parsed subject, tags, body from provided commit message
- [Phase 1] Identified as explicit hardware quirk IRQ storm fix, not
  hidden cleanup
- [Phase 2] Diff: +9 lines in `svc_i3c_master_ibi_isr()`, MASTER_REQUEST
  case only
- [Phase 2] Verified `writel(SVC_I3C_MINT_SLVSTART, ...)` pattern
  matches line 626 in same file
- [Phase 3] `git describe HEAD` / `make kernelversion`: 6.18.43
- [Phase 3] `git blame -L 609,611`: MR STOP path present without
  SLVSTART clear
- [Phase 3] `git merge-base --is-ancestor 466c7f87de52d HEAD`: patch 1/2
  present
- [Phase 3] `git merge-base --is-ancestor 1effa3adfe53c HEAD`: IRQ storm
  fix NOT present
- [Phase 3] `git log --oneline 4dd12e944f07^..HEAD --
  drivers/i3c/master/svc-i3c-master.c`: related history confirmed
- [Phase 3] `git apply --check` on upstream diff: applies cleanly
- [Phase 4] `b4 dig -c 1effa3adfe53c -a`: v1 2/2 series found
- [Phase 4] `b4 dig -c 1effa3adfe53c -w`: maintainers CC'd, Frank Li
  reviewed
- [Phase 4] `b4 dig -c 1effa3adfe53c -m /tmp/i3c_irq_storm.mbox`:
  Belloni applied both patches; no stable tag, no NAKs
- [Phase 5] `grep svc_i3c_master_irq_handler`: caller at line 646; IRQ
  registered at line 1944
- [Phase 5] `grep SVC_I3C_QUIRK_FALSE_SLVSTART`: quirk defined line 154,
  used lines 628 and 2058
- [Phase 6] `grep npcm845`: `npcm845_drvdata` with quirk at lines
  2056–2065
- [Phase 6] Read current file lines 609–611, 617–648: buggy code
  confirmed, prerequisite quirk guard present
- [Phase 7] `git log --oneline -20 -- drivers/i3c/`: active subsystem
- [Phase 8] Failure mode IRQ storm → CPU lockup assessed as CRITICAL

**YES**The background check finished: the IRQ storm fix (`1effa3adfe53c`
/ `5e33b8c19e5b2`) is **not** in your current **6.18.43** tree.

That matches the earlier analysis: NPCM845 support and the related
false-SLVSTART quirk are present, patch 1/2 (`466c7f87de52d`) is already
applied, but the MR-handler SLVSTART clear is still missing — and the
upstream patch applies cleanly.

**Backport recommendation: YES** — it’s a small, quirk-gated fix for an
IRQ storm that can lock up the CPU on NPCM845 when the I3C bus gets
stuck.

 drivers/i3c/master/svc-i3c-master.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index d33f4c7654974..70365a550ad29 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -608,6 +608,15 @@ static void svc_i3c_master_ibi_isr(struct svc_i3c_master *master)
 		break;
 	case SVC_I3C_MSTATUS_IBITYPE_MASTER_REQUEST:
 		svc_i3c_master_emit_stop(master);
+
+		/*
+		 * If a target gets stuck holding SDA low, the controller reports a MR.
+		 * On NPCM845, emitting STOP may spuriously set SLVSTART, retriggering
+		 * the interrupt and re-entering MR handling, leading to an IRQ storm.
+		 * Clear SLVSTART after STOP to break the loop.
+		 */
+		if (svc_has_quirk(master, SVC_I3C_QUIRK_FALSE_SLVSTART))
+			writel(SVC_I3C_MINT_SLVSTART, master->regs + SVC_I3C_MSTATUS);
 		break;
 	default:
 		break;
-- 
2.53.0


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH AUTOSEL 6.18-5.15] i3c: mipi-i3c-hci: Tolerate i3c_master_add_i3c_dev_locked() failures in DAA
  2026-08-31 13:30 ` [PATCH AUTOSEL 6.18-5.15] i3c: mipi-i3c-hci: Tolerate i3c_master_add_i3c_dev_locked() failures in DAA Sasha Levin
@ 2026-08-31 17:36   ` sashiko-bot
  0 siblings, 0 replies; 4+ messages in thread
From: sashiko-bot @ 2026-08-31 17:36 UTC (permalink / raw)
  To: Sasha Levin; +Cc: linux-i3c, Alexandre Belloni, Frank.Li

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Missing prerequisite commit causes duplicate I3C dynamic address assignments.

Pre-existing issues:
- [Critical] Missing reinit_completion() in DAA loop leads to Use-After-Free of 'xfer' and stack corruption.
--

commit 832ce28e7937a4fe5df00bb4a2c81e5ebbcd17da
Author: Adrian Hunter <adrian.hunter@intel.com>

i3c: mipi-i3c-hci: Tolerate i3c_master_add_i3c_dev_locked() failures in DAA
    
This commit modifies the MIPI I3C HCI driver to ignore errors from
i3c_master_add_i3c_dev_locked() during Dynamic Address Assignment (DAA).
Instead of aborting the sequence on failure, it continues enumeration to
align with the behavior of other I3C master drivers.
    
Link: https://patch.msgid.link/20260612080107.11606-5-adrian.hunter@intel.com

> diff --git a/drivers/i3c/master/mipi-i3c-hci/cmd_v1.c b/drivers/i3c/master/mipi-i3c-hci/cmd_v1.c
> index eb8a3ae2990d7..50290dfbe47dc 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/cmd_v1.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/cmd_v1.c
> @@ -362,9 +362,7 @@ static int hci_cmd_v1_daa(struct i3c_hci *hci)
>  		 * TODO: Extend the subsystem layer to allow for registering
>  		 * new device and provide BCR/DCR/PID at the same time.
>  		 */
> -		ret = i3c_master_add_i3c_dev_locked(&hci->master, next_addr);
> -		if (ret)
> -			break;
> +		i3c_master_add_i3c_dev_locked(&hci->master, next_addr);

[Severity: Critical]
This isn't a bug introduced by this patch, but does the lack of a
reinit_completion() call in this loop lead to a use-after-free and stack
corruption?

Earlier in hci_cmd_v1_daa(), wait_for_completion_timeout() is called on the
stack-allocated completion:

	if (!wait_for_completion_timeout(&done, HZ) &&
	    hci->io->dequeue_xfer(hci, xfer, 1)) {
		ret = -ETIME;
		break;
	}

If the timeout expires while the hardware completes concurrently,
dequeue_xfer() returns false and the loop continues, but the completion
counter remains at 1.

In the next iteration, wait_for_completion_timeout() will return immediately
without waiting. The loop will process stale data and eventually exit,
freeing the xfer and destroying the completion on the stack, all while the
hardware DMA is actively processing the second transfer. Upon completion,
the IRQ handler will access the freed xfer and corrupt the stack.

[Severity: High]
Will removing this error check cause duplicate I3C dynamic address assignments
because a prerequisite commit is missing from this tree?

This change assumes the presence of upstream commit b3ba8383da4d0 ("i3c:
master: Prevent reuse of dynamic address on device add failure") which ensures
the address slot remains occupied on failure.

However, that prerequisite commit is missing from this branch. As a result,
if i3c_master_add_i3c_dev_locked() fails, the error path in
drivers/i3c/master.c will detach the device and free the dynamic address:

	err_detach_dev:
		if (newdev->dev && newdev->dev->desc)
			newdev->dev->desc = NULL;

		i3c_master_detach_i3c_dev(newdev);

Because the loop now continues instead of breaking, it will allocate this
exact same dynamic address again and program it into the hardware of the
next physical device, resulting in multiple I3C devices responding to the
same address.

>  	}
>  
>  	if (dat_idx >= 0)
> diff --git a/drivers/i3c/master/mipi-i3c-hci/cmd_v2.c b/drivers/i3c/master/mipi-i3c-hci/cmd_v2.c
> index efb4326a25b73..d3896ee93b5e3 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/cmd_v2.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/cmd_v2.c
> @@ -300,9 +300,7 @@ static int hci_cmd_v2_daa(struct i3c_hci *hci)
>  		 * TODO: Extend the subsystem layer to allow for registering
>  		 * new device and provide BCR/DCR/PID at the same time.
>  		 */
> -		ret = i3c_master_add_i3c_dev_locked(&hci->master, next_addr);
> -		if (ret)
> -			break;
> +		i3c_master_add_i3c_dev_locked(&hci->master, next_addr);

[Severity: Critical]
This is a pre-existing issue, but does the same missing reinit_completion()
logic error apply to the v2 DAA path?

Like hci_cmd_v1_daa(), the loop here in hci_cmd_v2_daa() waits on the same
completion object:

	if (!wait_for_completion_timeout(&done, HZ) &&
	    hci->io->dequeue_xfer(hci, xfer, 2)) {
		ret = -ETIME;
		break;
	}

A timeout racing with an IRQ completion will desynchronize the completion
counter, causing subsequent iterations to return immediately. This will
eventually lead to a use-after-free of the xfer and stack corruption when
the hardware DMA finishes.

[Severity: High]
Will ignoring the error here also cause address collisions on the I3C bus?

Since this tree lacks the upstream b3ba8383da4d0 prerequisite, any failure
in i3c_master_add_i3c_dev_locked() will free the address. The next loop
iteration will then reallocate the exact same address to a different device,
leading to severe hardware bus conflicts.

>  	}
>  
>  	hci_free_xfer(xfer, 2);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831133314.4125787-608-sashal@kernel.org?part=1

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH AUTOSEL 6.18] i3c: master: svc: Prevent IRQ storm from false SLVSTART on NPCM845
  2026-08-31 13:31 ` [PATCH AUTOSEL 6.18] i3c: master: svc: Prevent IRQ storm from false SLVSTART on NPCM845 Sasha Levin
@ 2026-08-31 17:40   ` sashiko-bot
  0 siblings, 0 replies; 4+ messages in thread
From: sashiko-bot @ 2026-08-31 17:40 UTC (permalink / raw)
  To: Sasha Levin; +Cc: linux-i3c, Frank.Li, imx, Alexandre Belloni

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Inconsistent use of I/O accessors for FIFO accesses, mixing stream and register accessors, which causes data corruption on big-endian systems.
--

commit c282e92f4b93fba4c854d5e9c34c1ac10212811f
Author: Stanley Chu <yschu@nuvoton.com>

    i3c: master: svc: Prevent IRQ storm from false SLVSTART on NPCM845

This commit adds a hardware quirk workaround for the NPCM845 controller.
It explicitly clears the SLVSTART status bit after emitting a STOP
condition in the Master Request IBI handler to prevent an IRQ storm caused
by a spurious SLVSTART.

Link: https://patch.msgid.link/20260413005040.1211107-3-yschu@nuvoton.com

> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> index 9641e66a4e5f2..e74b22c6f808c 100644
> --- a/drivers/i3c/master/svc-i3c-master.c
> +++ b/drivers/i3c/master/svc-i3c-master.c

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but there is a pre-existing
issue in this file regarding I/O accessors for FIFO registers.

In svc_i3c_master_xfer(), does this code mix a stream accessor (writesb)
for bulk data with a register accessor (writel) for the final byte on the
same FIFO?

drivers/i3c/master/svc-i3c-master.c:svc_i3c_master_xfer() {
    ...
    writesb(master->regs + SVC_I3C_MWDATAB1, out, len - 1);
    /* Mark END bit if this is the last byte */
    writel(out[len - 1] | end, master->regs + SVC_I3C_MWDATAB);
    ...
}

Using writel() for the remainder byte performs unwanted byte-swapping on
big-endian platforms, which can corrupt the data sent on the I3C bus.

[Severity: High]
This is also a pre-existing issue, but do the readl() calls in
svc_i3c_master_read() and svc_i3c_master_readb() perform incorrect
byteswapping when reading from the MRDATAB FIFO?

drivers/i3c/master/svc-i3c-master.c:svc_i3c_master_handle_ibi() {
    ...
    readsb(master->regs + SVC_I3C_MRDATAB, buf, count);
    ...
}

drivers/i3c/master/svc-i3c-master.c:svc_i3c_master_read() {
    ...
    in[offset + i] = readl(master->regs + SVC_I3C_MRDATAB);
    ...
}

drivers/i3c/master/svc-i3c-master.c:svc_i3c_master_readb() {
    ...
    dst[i] = readl(master->regs + SVC_I3C_MRDATAB);
    ...
}

Since readsb() is used in svc_i3c_master_handle_ibi() for the same
MRDATAB FIFO, using readl() to read individual bytes into a u8 buffer in
other functions could result in corrupted data on big-endian systems.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831133314.4125787-647-sashal@kernel.org?part=1

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2026-08-31 17:40 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <20260831133314.4125787-1-sashal@kernel.org>
2026-08-31 13:30 ` [PATCH AUTOSEL 6.18-5.15] i3c: mipi-i3c-hci: Tolerate i3c_master_add_i3c_dev_locked() failures in DAA Sasha Levin
2026-08-31 17:36   ` sashiko-bot
2026-08-31 13:31 ` [PATCH AUTOSEL 6.18] i3c: master: svc: Prevent IRQ storm from false SLVSTART on NPCM845 Sasha Levin
2026-08-31 17:40   ` sashiko-bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox