* [PATCH AUTOSEL 6.18-5.10] ata: ata_generic: Don't store pci_device_id
[not found] <20261019135441.1420968-1-sashal@kernel.org>
@ 2026-10-19 13:45 ` Sasha Levin
2026-10-19 13:46 ` [PATCH AUTOSEL 6.18-6.12] ata: pata_ep93xx: fix PIO fallback when DMA init fails Sasha Levin
2026-10-19 13:47 ` [PATCH AUTOSEL 6.18-5.10] ata: libata-sff: don't busy-wait for PIO data-in command completion Sasha Levin
2 siblings, 0 replies; 4+ messages in thread
From: Sasha Levin @ 2026-10-19 13:45 UTC (permalink / raw)
To: patches, stable
Cc: Gary Guo, Bjorn Helgaas, Danilo Krummrich, Damien Le Moal,
Sasha Levin, cassel, linux-ide, linux-kernel
From: Gary Guo <gary@garyguo.net>
[ Upstream commit 4f3075f629077a8cb3d3455c794a1f4a8fc8b43e ]
pci_device_id is not guaranteed to live longer than probe due to presence
of dynamic ID. All information apart from driver_data can be easily
retrieved from pci_dev, so just store driver_data.
Signed-off-by: Gary Guo <gary@garyguo.net>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Danilo Krummrich <dakr@kernel.org>
Acked-by: Damien Le Moal <dlemoal@kernel.org>
Link: https://patch.msgid.link/20260723-pci_id_fix-v4-1-3580726844e1@garyguo.net
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
LLM Generated explanations, may be completely bogus:
I have the history, the series context, and the UAF path. Next I’ll
write the full nine-phase verdict for this 7.3-rc4 tree.
===============================================================
PHASE 1: COMMIT MESSAGE FORENSICS
===============================================================
**Step 1.1: PARSE THE SUBJECT LINE**
Record: `[ata / ata_generic] [Don't store] [Stop keeping a pointer to
struct pci_device_id in host private_data; keep only driver_data]`
Subsystem prefix is `ata: ata_generic:`. Action is “Don’t store” —
defensive lifetime fix, not a feature.
**Step 1.2: PARSE ALL COMMIT MESSAGE TAGS**
Record:
- **Fixes:** none (expected for this review pipeline)
- **Reported-by:** none on this commit
- **Tested-by:** none
- **Reviewed-by:** Danilo Krummrich `<dakr@kernel.org>` (PCI/driver-
core)
- **Acked-by:** Damien Le Moal `<dlemoal@kernel.org>` (libata
maintainer)
- **Link:** `https://patch.msgid.link/20260723-pci_id_fix-v4-1-
3580726844e1@garyguo.net` (v4 patch 1/9)
- **Cc: stable:** none (not a negative signal)
- **Signed-off-by:** Gary Guo (author); Bjorn Helgaas (PCI maintainer,
committer)
- Ignore any Sasha Levin SOB if present — none on the actual git commit
`4f3075f629077`
Notable: ATA maintainer Ack plus PCI maintainer commit. Series name
`pci_id_fix-v4` — this is patch 1 of 9.
**Step 1.3: ANALYZE THE COMMIT BODY TEXT**
Record:
- **Bug:** `pci_device_id` is not guaranteed to outlive `probe` because
PCI dynamic IDs (`new_id` / `remove_id`) can free that object.
- **Symptom/failure mode:** later dereference of
`ap->host->private_data` as `const struct pci_device_id *` is a use-
after-free.
- **Version info:** none in the message.
- **Root cause:** `ata_generic_init_one()` stored the `id` pointer via
`ata_pci_bmdma_init_one(..., (void *)id, ...)`. `generic_set_mode()`
later reads `id->driver_data`. Only `driver_data` is needed;
vendor/device can come from `pci_dev`.
**Step 1.4: DETECT HIDDEN BUG FIXES**
Record: Not a disguised cleanup. The subject is lifetime hygiene, but
the mechanism is a real UAF if a dynamic ID is freed while the ATA host
still holds the pointer. Cover letter for the series calls storing
`pci_device_id` from probe a “bad pattern” and names a genuine dynamic-
ID UAF (Sashiko report, used for the PCI-core patch). This driver change
is both a standalone dangling-pointer fix and a prerequisite so PCI core
can shrink dyn-ID lifetime.
===============================================================
PHASE 2: DIFF ANALYSIS - LINE BY LINE
===============================================================
**Step 2.1: INVENTORY THE CHANGES**
Record:
- File: `drivers/ata/ata_generic.c` — 3 insertions, 3 deletions (net 0,
6 lines touched)
- Functions: `generic_set_mode()`, `ata_generic_init_one()`
- Scope: single-file surgical fix
**Step 2.2: UNDERSTAND THE CODE FLOW CHANGE**
Hunk 1 (`generic_set_mode`):
- Before: `const struct pci_device_id *id = ap->host->private_data;`
then `id->driver_data & ATA_GEN_FORCE_DMA`
- After: `unsigned long driver_data = (unsigned
long)ap->host->private_data;` then `driver_data & ATA_GEN_FORCE_DMA`
- Path: ATA mode programming, including libata error handling
(`ata_eh_set_mode()` → `ap->ops->set_mode`), not probe-only
Hunk 2 (`ata_generic_init_one`):
- Before: `ata_pci_bmdma_init_one(..., (void *)id, 0)` stores the full
`pci_device_id *`
- After: `ata_pci_bmdma_init_one(..., (void *)id->driver_data, 0)`
stores the integer flags only
- Path: PCI probe / host registration
`ATA_GEN_CLASS_MATCH` and `ATA_GEN_INTEL_IDER` remain consumed from the
live `id` argument during probe only — they are not read from
`private_data`.
**Step 2.3: IDENTIFY THE BUG MECHANISM**
Record:
- **Category:** memory safety / use-after-free (stale pointer)
- **Mechanism:** PCI `remove_id_store()` does `list_del` +
`kfree(dynid)` under `dynids.lock` without unbinding devices. If the
host was bound via that dyn ID, `host->private_data` still pointed at
`dynid->id`. `generic_set_mode()` later loads `id->driver_data`.
- After the later PCI-core change in this same series (`3ffc4c9690c33`),
dyn-ID matches return a **stack copy** valid only during probe
(`include/linux/pci.h`: “The pci_device_id parameter is only valid
during probe.”). Storing that pointer past probe would then be a stack
UAF even without `remove_id`. This driver patch must land first (and
it did: v4-1 before v4-9).
**Step 2.4: ASSESS THE FIX QUALITY**
Record:
- Obviously correct: the only post-probe use was `id->driver_data`; copy
that integer instead of the object.
- Minimal; no unrelated edits; no API change.
- Regression risk: very low. `driver_data` values are small flags
(`1<<0`..`1<<2`). Same `void *` ↔ `unsigned long` trick already used
elsewhere (e.g. `pata_amd.c`).
- This tree already contains the after-state (`4f3075f629077` is an
ancestor of HEAD).
===============================================================
PHASE 3: GIT HISTORY INVESTIGATION
===============================================================
**Step 3.1: BLAME THE CHANGED LINES**
Record:
- Storing the `pci_device_id *` in `host->private_data` was introduced
in `1529c69adce1e` (“ata_generic: implement ATA_GEN_* flags and force
enable DMA on MBP 7,1”), 2010-07-01, contained in `v2.6.35-rc4`.
- That commit is an ancestor of this tree. The pattern is ~16 years old,
not new 7.3 code.
**Step 3.2: FOLLOW THE FIXES: TAG**
Record: No `Fixes:` tag on this commit. The buggy pattern is
`1529c69adce1e`, verified present. Related PCI-core UAF has `Fixes:
0994375e9614` (“PCI: add remove_id sysfs entry”) on `3ffc4c9690c33` —
different patch, same series.
**Step 3.3: CHECK FILE HISTORY FOR RELATED CHANGES**
Record (last commits on `drivers/ata/ata_generic.c`):
- `4f3075f629077` this fix
- `9986ce65bebb9` constify `pci_device_id` (unrelated)
- No other recent functional churn at these lines
Standalone vs series: **patch 1/9** of `pci_id_fix`. Sibling driver
patches (`nsp32`, `tpci200`, `mlxsw`, agp). PCI core is patches 7–9.
**This patch applies standalone**; it does not call new PCI helpers.
**Step 3.4: CHECK THE AUTHOR'S OTHER COMMITS**
Record: Gary Guo is not the ATA maintainer; he is the series author
(Rust/PCI ID work). Nearby same-author commits: `3c28376273e8f` nsp32,
`9a1b9aa5cedf3` tpci200, `1ae2920768f79` mlxsw, plus PCI TOCTOU/UAF.
Damien Le Moal Acked this ATA patch. Bjorn Helgaas committed it.
**Step 3.5: CHECK FOR DEPENDENT/PREREQUISITE COMMITS**
Record: No prerequisites. `ata_pci_bmdma_init_one(..., void *host_priv,
...)` has taken an opaque `host_priv` for a long time (`libata-sff.c`).
This is a caller-side change only. Later PCI patches depend on *this*
class of driver fix, not the reverse.
===============================================================
PHASE 4: MAILING LIST AND EXTERNAL RESEARCH
===============================================================
**Step 4.1: FIND THE ORIGINAL PATCH DISCUSSION**
Record:
- `b4 dig -c 4f3075f629077` matched patch-id; lore URL: `https://patch.m
sgid.link/20260723-pci_id_fix-v4-1-3580726844e1@garyguo.net`
- `b4 dig -a` revisions:
- v1 2026-06-26: PCI match refactor only (this driver fix not yet
present)
- v2 2026-06-30: `[PATCH v2 1/7] ata: don't keep pci_device_id` —
driver stores added
- v3 2026-07-06: `[PATCH v3 1/9] ata: don't store pci_device_id`
- v4 2026-07-23: applied version; cover: “PCI: Fix UAF and TOCTOU
related to dynamic ID”
- v2 changelog: “Fix users which store pci_device_id.” v4: style fixes
from Bjorn. Applied version is latest.
- No NAK on this patch. Sashiko bot: “found no issues.”
- Cover letter: Sashiko reported a genuine dynamic-ID UAF; storing
`pci_device_id` from probe is called out as a bad pattern.
**Step 4.2: CHECK WHO REVIEWED THE PATCH**
Record (`b4 dig -w`): linux-ide, linux-pci, driver-core, linux-scsi,
netdev, dri-devel; Damien Le Moal, Niklas Cassel, Bjorn Helgaas, Danilo
Krummrich, Greg KH. Right maintainers were on CC. Reviewed-by Krummrich,
Acked-by Le Moal, committed by Helgaas.
**Step 4.3: SEARCH FOR THE BUG REPORT**
Record: This specific ATA patch has no `Reported-by`. The series UAF
report is on patch 9: `Reported-by: Sashiko <sashiko-bot@kernel.org>` /
lore `20260619170503.518F61F00A3A@smtp.kernel.org`. Direct WebFetch of
lore returned HTTP 403; thread was recovered via `b4 dig -m`. No user
crash log for `ata_generic` itself.
**Step 4.4: CHECK FOR RELATED PATCHES AND SERIES**
Record: 9-patch series. Patches 1–6 fix drivers that stash or pointer-
arithmetic the ID. Patches 7–9 change PCI matching, dyn-ID TOCTOU, and
concurrent probe vs `remove_id` UAF. This commit is self-contained; it
does not require 7–9 to be correct. If 9 were backported without 1, dyn-
ID probe would hand a stack copy to `ata_generic`, which would then
store it — a worse UAF. Evaluating **this** commit alone: it is
independently correct.
**Step 4.5: CHECK STABLE MAILING LIST HISTORY**
Record: `curl`/`WebFetch` to `lore.kernel.org/stable/` returned 403. The
saved v4 thread contains **no** `Cc: stable` discussion. **UNVERIFIED:**
whether anyone asked for this on the stable list.
===============================================================
PHASE 5: CODE SEMANTIC ANALYSIS
===============================================================
**Step 5.1: IDENTIFY KEY FUNCTIONS**
Record: `generic_set_mode()`, `ata_generic_init_one()`. (semcode MCP not
available; used hunks + grep.)
**Step 5.2: TRACE CALLERS**
Record:
- `ata_generic_init_one` — PCI `.probe` of `ata_generic_pci_driver`
(device bind).
- `generic_set_mode` — `generic_port_ops.set_mode`. Called from
`ata_eh_set_mode()` in `drivers/ata/libata-eh.c` when
`ap->ops->set_mode` is set. That runs from `ata_eh_recover()` when
`ATA_EHI_SETMODE` is set (initial config, reset, revalidate). **After
probe**, disk errors / resume / EH can invoke it.
**Step 5.3: TRACE CALLEES**
Record: `ata_pci_bmdma_init_one()` stores `host_priv` on the `ata_host`.
`generic_set_mode` reads BMDMA status and libata xfer-mask helpers. No
new allocations or locks.
**Step 5.4: FOLLOW THE CALL CHAIN**
Record:
- Common path: PCI probe of a device in the static `ata_generic[]` table
→ `id` points at module `.rodata` for the module’s lifetime → **no
UAF** on static IDs.
- Buggy path: `echo ... > /sys/bus/pci/drivers/ata_generic/new_id`
(typically CAP_SYS_ADMIN) → bind via heap `pci_dynid` → store pointer
→ `remove_id` kfree’s dynid while host still bound → later EH
`generic_set_mode` → UAF.
- Reachable from userspace: yes, via PCI sysfs + later ATA EH. Not a
normal unprivileged syscall path.
**Step 5.5: SEARCH FOR SIMILAR PATTERNS**
Record: Same-series fixes in `nsp32`, `tpci200`, `mlxsw`, agp. Other ATA
drivers store their own `hpriv`, not `pci_device_id *`. Pattern was
systematic across PCI drivers, not unique to ATA.
===============================================================
PHASE 6: CROSS-REFERENCING AGAINST THE LOCAL TREE
===============================================================
**Step 6.1: DOES THE BUGGY CODE EXIST IN THIS TREE?**
Record:
- Local tree: `git describe HEAD` = `v7.3-rc4-27-gf0100363d8c37`; `make
kernelversion` = `7.3.0-rc4` (Linus mainline, detached HEAD).
- Commit `4f3075f629077` **is already an ancestor of HEAD** (`git merge-
base --is-ancestor` succeeded; first appeared in `v7.3-rc1`).
- Current `drivers/ata/ata_generic.c` already stores `id->driver_data`
and reads it as `unsigned long`.
- The **pre-fix** code existed on this lineage from `1529c69adce1e`
(v2.6.35) until that commit. This is not a “bug introduced after this
tree branched” case.
**Step 6.2: CHECK FOR BACKPORT COMPLICATIONS**
Record: Against the pre-fix file (`4f3075f629077^`), the diff is a clean
6-line replace. No refactor of `ata_generic.c` in between. Expected
apply on a tree that still has the old pointer store: **clean**. On
*this* checkout the after-state is already present, so re-applying the
patch would be a no-op/conflict with itself — that is git ancestry, not
a missing-code problem.
**Step 6.3: CHECK IF RELATED FIXES ARE ALREADY HERE**
Record: This exact fix is in HEAD. PCI-core companion `3ffc4c9690c33` is
also an ancestor. No alternate/earlier ata_generic dyn-ID fix found
(`git log --grep=pci_device_id -- drivers/ata/ata_generic.c` only this
commit and an unrelated constify).
===============================================================
PHASE 7: SUBSYSTEM AND MAINTAINER CONTEXT
===============================================================
**Step 7.1: IDENTIFY THE SUBSYSTEM AND ITS CRITICALITY**
Record: `drivers/ata/` — libata generic PCI IDE driver
(`CONFIG_ATA_GENERIC`). **IMPORTANT** for machines that still use BIOS-
configured PATA/IDE (Cenatek FORCE_DMA, Intel IDE-R, various legacy
chips). Not mm/net core, but storage.
**Step 7.2: ASSESS SUBSYSTEM ACTIVITY**
Record: libata is actively maintained (recent EH/LPM/AHCI fixes).
`ata_generic.c` itself is mature; little churn besides this lifetime
fix.
===============================================================
PHASE 8: IMPACT AND RISK ASSESSMENT
===============================================================
**Step 8.1: DETERMINE WHO IS AFFECTED**
Record: Driver-specific (`CONFIG_ATA_GENERIC`), and the UAF only if the
host was bound via a **dynamic** PCI ID. Static-table devices (the usual
case) keep `id` in module memory. Affected population: admins using
`new_id` on `ata_generic`, then `remove_id` or (after PCI stack-copy)
any post-probe `set_mode`.
**Step 8.2: DETERMINE THE TRIGGER CONDITIONS**
Record:
- Trigger: privileged sysfs `new_id`/`remove_id`, then ATA EH
`set_mode`.
- Likelihood on production LTS: **low** (dyn IDs are uncommon for this
driver).
- Unprivileged trigger: **no** (driver `new_id`/`remove_id` are root-
owned sysfs). Still a real kernel UAF, not theoretical pointer math.
**Step 8.3: DETERMINE THE FAILURE MODE SEVERITY**
Record: Use-after-free of `pci_dynid` / dangling ID pointer → KASAN/oops
or worse. **Severity: HIGH** when triggered; **likelihood: LOW**.
**Step 8.4: CALCULATE RISK-BENEFIT RATIO**
Record:
- **Benefit:** Prevents a real UAF in a long-lived storage driver;
3-line change; maintainer-acked. Also keeps the driver correct if the
PCI dyn-ID lifetime fix is applied.
- **Risk:** Very low (tiny, local, no locking/API).
- **Ratio:** favorable. Conservative “rare trigger” concern does not
outweigh UAF + trivial patch.
===============================================================
PHASE 9: FINAL SYNTHESIS
===============================================================
**Step 9.1: COMPILE THE EVIDENCE**
FOR backporting:
- Real UAF: `host->private_data` held `pci_device_id *` beyond probe;
`remove_id` can free dyn IDs; `generic_set_mode` runs from libata EH
after probe.
- Bug present since v2.6.35 (`1529c69adce1e`); code exists on this
lineage.
- 6-line, obviously correct; only `driver_data` was used post-probe.
- Reviewed/Acked by PCI and ATA maintainers; PCI maintainer committed
it.
- No new APIs/features.
- Self-contained; no missing helpers in this tree.
AGAINST:
- Common static-ID path was not buggy; trigger needs dyn-ID sysfs
(root).
- No user crash report / syzbot on this exact driver file.
- Part of a 9-patch series whose headline UAF is PCI-core patch 9; this
was added in v2 as “fix users which store pci_device_id.”
- No `Cc: stable` in the thread (absence is expected).
UNRESOLVED:
- lore.kernel.org HTTP 403 for live pages; used `b4 dig` mbox instead.
- Stable-list discussion UNVERIFIED (403).
**Step 9.2: APPLY THE STABLE RULES CHECKLIST**
1. Obviously correct and tested? **PASS** — logic is local and reviewed;
Sashiko found no issues on v4-1.
2. Fixes a real bug that affects users? **PASS** — dangling
`pci_device_id` after dyn-ID removal / after probe lifetime; users of
`ata_generic` + `new_id` can hit it.
3. Important issue (crash, security, corruption, deadlock)? **PASS** —
UAF → oops/KASAN (HIGH, rare trigger).
4. Small and contained? **PASS** — 6 lines, one file.
5. No new features or APIs? **PASS**.
6. Can apply to the local tree? **PASS** for trees still on the old
pointer store. This 7.3-rc4 checkout already contains the commit;
that does not make the fix unsuitable — it confirms the code and
helpers exist here.
**Step 9.3: CHECK FOR EXCEPTION CATEGORIES**
Record: None (not a device ID / quirk / DT / build / docs exception).
Qualifies as a normal small UAF fix.
**Step 9.4: MAKE YOUR DECISION**
This tree is **Linux 7.3-rc4** (mainline). `ata_generic` is present; the
buggy store-the-`id`-pointer pattern lived here from 2010 until
`4f3075f629077`. The patch is a 3-line lifetime fix for a real UAF, not
a feature or refactor, and it applies in isolation.
Rare privileged trigger is not enough to reject a tiny, maintainer-acked
UAF fix in a storage driver. It meets stable-kernel-rules.
### What the commit solves (for stable users)
`ata_generic` used to keep a pointer to the `pci_device_id` passed into
probe. For IDs added through sysfs `new_id`, that object can be freed by
`remove_id` while the ATA host is still live. `generic_set_mode()` —
including from error handling — then dereferences freed memory. The fix
copies `driver_data` (the only field used later) and drops the pointer.
### Risk vs benefit
Benefit is avoiding a UAF/oops on a long-standing driver. Risk is
negligible. No prerequisites missing in this tree.
---
Verification:
- [Phase 1] Parsed tags on `4f3075f629077`: Reviewed-by Danilo
Krummrich, Acked-by Damien Le Moal, Link to v4-1; no Fixes/Reported-
by/Cc:stable
- [Phase 1] Subject/body: don’t store `pci_device_id`; dyn-ID lifetime
- [Phase 2] Diff: 3+ / 3- in `drivers/ata/ata_generic.c`;
`generic_set_mode` + `ata_generic_init_one`
- [Phase 2] Pre-fix file via `git show
4f3075f629077^:drivers/ata/ata_generic.c`: stored `(void *)id`, later
`id->driver_data & ATA_GEN_FORCE_DMA`
- [Phase 3] `git blame` / `git log -S private_data --
drivers/ata/ata_generic.c`: introduced `1529c69adce1e` (2010,
`v2.6.35-rc4`)
- [Phase 3] `git merge-base --is-ancestor 1529c69adce1e HEAD` — yes;
`4f3075f629077` — yes
- [Phase 3] Series: `3c28376273e8f`, `9a1b9aa5cedf3`, `1ae2920768f79`,
`c967b365d7b51`, `04fde70f782b6`, `3ffc4c9690c33`
- [Phase 4] `b4 dig -c 4f3075f629077`: patch.msgid.link v4-1; `-a`:
v1–v4; `-w`: linux-ide/linux-pci + Le Moal/Helgaas
- [Phase 4] `b4 dig -m`: cover letter genuine dyn-ID UAF; this patch
added in v2; no NAK; Sashiko OK
- [Phase 4] WebFetch lore/patch.msgid.link: HTTP 403; used b4 mbox
instead
- [Phase 4] Stable list: UNVERIFIED (403)
- [Phase 5] Callers: PCI probe; `ata_eh_set_mode()` in `libata-eh.c`
lines 3480–3482 and 3962–3963
- [Phase 5] `remove_id_store()` in `drivers/pci/pci-driver.c` kfree’s
dynid without unbind
- [Phase 5] `include/linux/pci.h` ~989: id only valid during probe
(after series)
- [Phase 6] `git describe HEAD` = v7.3-rc4-27-gf0100363d8c37;
kernelversion 7.3.0-rc4
- [Phase 6] Current file already has the fix (commit in 7.3-rc1)
- [Phase 7] `CONFIG_ATA_GENERIC` in `drivers/ata/Kconfig` — generic BIOS
ATA, PCI+BMDMA
- [Phase 8] Failure mode: UAF in EH `set_mode`; severity HIGH; trigger
privileged dyn-ID; risk of the patch itself very low
- UNVERIFIED: stable@ lore discussion (HTTP 403)
The patch is small, fixes a real use-after-free, and meets stable
criteria for this tree’s `ata_generic` code.
**YES**
drivers/ata/ata_generic.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/ata/ata_generic.c b/drivers/ata/ata_generic.c
index e70b6c089cf17..18ea740ca5829 100644
--- a/drivers/ata/ata_generic.c
+++ b/drivers/ata/ata_generic.c
@@ -51,11 +51,11 @@ enum {
static int generic_set_mode(struct ata_link *link, struct ata_device **unused)
{
struct ata_port *ap = link->ap;
- const struct pci_device_id *id = ap->host->private_data;
+ unsigned long driver_data = (unsigned long)ap->host->private_data;
int dma_enabled = 0;
struct ata_device *dev;
- if (id->driver_data & ATA_GEN_FORCE_DMA) {
+ if (driver_data & ATA_GEN_FORCE_DMA) {
dma_enabled = 0xff;
} else if (ap->ioaddr.bmdma_addr) {
/* Bits 5 and 6 indicate if DMA is active on master/slave */
@@ -206,7 +206,7 @@ static int ata_generic_init_one(struct pci_dev *dev, const struct pci_device_id
return rc;
pcim_pin_device(dev);
}
- return ata_pci_bmdma_init_one(dev, ppi, &generic_sht, (void *)id, 0);
+ return ata_pci_bmdma_init_one(dev, ppi, &generic_sht, (void *)id->driver_data, 0);
}
static const struct pci_device_id ata_generic[] = {
--
2.53.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH AUTOSEL 6.18-6.12] ata: pata_ep93xx: fix PIO fallback when DMA init fails
[not found] <20261019135441.1420968-1-sashal@kernel.org>
2026-10-19 13:45 ` [PATCH AUTOSEL 6.18-5.10] ata: ata_generic: Don't store pci_device_id Sasha Levin
@ 2026-10-19 13:46 ` Sasha Levin
2026-10-19 13:47 ` [PATCH AUTOSEL 6.18-5.10] ata: libata-sff: don't busy-wait for PIO data-in command completion Sasha Levin
2 siblings, 0 replies; 4+ messages in thread
From: Sasha Levin @ 2026-10-19 13:46 UTC (permalink / raw)
To: patches, stable
Cc: Rosen Penev, Damien Le Moal, Sasha Levin, cassel, linux-ide,
linux-kernel
From: Rosen Penev <rosenp@gmail.com>
[ Upstream commit c303c3619a1d5cf7d4b457106062d16724724a80 ]
ep93xx_pata_dma_init() returns an error when dma_request_chan() fails,
which causes ep93xx_pata_probe() to abort entirely. The probe function
already has a PIO fallback path (it checks both channel pointers before
enabling UDMA), so the DMA init should not fail the probe on non-fatal
errors.
Propagate -EPROBE_DEFER, such that we allow the DMA controller driver
to load, in case we got probed before the DMA controller driver.
For all other failures (e.g. -ENODEV when the DMA controller is missing
in the device tree), fall back to PIO.
Assisted-by: Opencode:Big-Pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
LLM Generated explanations, may be completely bogus:
## Phase 1: Commit message forensics
**Step 1.1 — Subject**
Record: `[ata / pata_ep93xx] [fix] restore PIO fallback when DMA init
fails so probe does not abort`
**Step 1.2 — Tags**
Record:
- `Assisted-by: Opencode:Big-Pickle`
- `Signed-off-by: Rosen Penev <rosenp@gmail.com>` (author)
- `Signed-off-by: Damien Le Moal <dlemoal@kernel.org>` (libata
maintainer)
- No `Fixes:`, `Reported-by:`, `Tested-by:`, `Reviewed-by:`, `Acked-
by:`, `Link:`, or `Cc: stable` (absence of Fixes/Cc:stable is expected
here)
- Notable: no user/syzbot report; maintainer SOB is present
**Step 1.3 — Body**
Record:
- **Bug:** `ep93xx_pata_dma_init()` treats any `dma_request_chan()`
failure as fatal. `ep93xx_pata_probe()` then returns that error and
never registers the ATA host.
- **Symptom:** PATA/IDE controller never appears; storage is
unavailable. PIO fallback in probe (channel-pointer check before UDMA)
is dead for this path.
- **Root cause:** after the OF DMA conversion, non-defer errors (e.g.
`-ENODEV`) abort probe instead of continuing without DMA.
- **Intended fix:** propagate `-EPROBE_DEFER`; for other failures, warn
and return 0 so the existing PIO path runs.
- **Version info in message:** none.
**Step 1.4 — Hidden bug fix?**
Record: Not hidden. The subject and body describe a real probe
regression. The diff also NULLs error pointers and stops
`fail_release_rx` from falling through into a second release.
## Phase 2: Diff analysis
**Step 2.1 — Inventory**
Record:
- File: `drivers/ata/pata_ep93xx.c` (+21/−9, 30 lines)
- Function: `ep93xx_pata_dma_init()`
- Scope: single-file surgical error-path change
**Step 2.2 — Code flow**
Record (pre-image from `c303c3619a1d5^`):
- **rx `dma_request_chan` fail:** `dev_err_probe()` and return the
error. Probe aborts. `dma_rx_channel` left as `ERR_PTR`.
- **tx `dma_request_chan` fail:** `dev_err_probe()`, `goto
fail_release_rx`. That released rx, then **fell through** into
`fail_release_dma` → `ep93xx_pata_release_dma()` with rx still non-
NULL and tx still an error pointer.
- **`dmaengine_slave_config` fail:** `dev_err_probe()`, release both,
**return `ret`** → probe aborts.
- **After:** non-defer failures NULL the pointers, warn, return 0.
`-EPROBE_DEFER` still fails probe so the DMA driver can load.
`fail_release_rx` no longer falls through.
**Step 2.3 — Bug mechanism**
Record:
- **Primary:** logic/error-path — optional DMA failure became a hard
probe failure (regression of designed PIO fallback).
- **Secondary:** error-path resource handling — old tx-fail path could
`dma_release_channel()` twice on rx and pass `ERR_PTR` into
`dma_release_channel()` (`chan->client_count` dereference in
`drivers/dma/dmaengine.c`).
**Step 2.4 — Fix quality**
Record: Obviously correct and minimal. Matches pre-v6.12 void `dma_init`
(fail DMA, keep probing). Regression risk is low: happy path unchanged;
`-EPROBE_DEFER` still delays probe; libata only sets `udma_mask` when
both channels are non-NULL, so BMDMA ops are not used in PIO mode. No
API/data-structure change.
## Phase 3: Git history
**Step 3.1 — Blame**
Record: The failing `dma_request_chan()` + `return int` behavior was
introduced by `9963113e3a924` (“ata: pata_ep93xx: add device tree
support”, 2024-09-12), contained in **v6.12-rc1**. Before that,
`ep93xx_pata_dma_init()` was `void` and returned after releasing
channels, leaving pointers NULL. Driver itself dates to `2fff27512600`
(2012).
**Step 3.2 — Fixes: tag**
Record: No `Fixes:` tag. The commit that introduced the bug is
`9963113e3a924`. `git merge-base --is-ancestor 9963113e3a924 HEAD`
succeeds in this tree.
**Step 3.3 — File history**
Record: Between the DT conversion and this fix: pinctrl/DT cleanup,
`remove()` callback switch, COMPILE_TEST/type tweaks. No other DMA-init
semantics change. Not patch X/Y. Standalone.
**Step 3.4 — Author**
Record: Rosen Penev is a regular ATA contributor. Damien Le Moal (libata
maintainer) applied it to `for-7.3`.
**Step 3.5 — Dependencies**
Record: Needs the v6.12 `dma_request_chan()` conversion
(`9963113e3a924`). That is in this tree. No extra helper/API required.
Applies standalone on this code.
## Phase 4: Mailing list / external
**Step 4.1 — Original discussion**
Record:
- Lore/msgid:
https://patch.msgid.link/20260720020229.1972888-1-rosenp@gmail.com
(v2, committed)
- v1: https://patch.msgid.link/20260601190749.20045-1-rosenp@gmail.com
- v1→v2: commit-message only (`v2: fix EPROBE_DEFER description`). Code
unchanged.
- Niklas Cassel on v1: “This patch looks correct to me”, asked not to
call `-EPROBE_DEFER` “fatal”.
- Damien on v2: “Applied to for-7.3. Thanks!”
- No NAK, no stable nomination, no reviewer risk flags.
**Step 4.2 — Recipients (`b4 dig -w`)**
Record: `linux-ide@vger.kernel.org`, Damien Le Moal, Niklas Cassel,
`linux-kernel`. Right maintainers.
**Step 4.3 — Bug report**
Record: No `Reported-by` / bugzilla / syzbot. Bug is from code
inspection of the DT conversion.
**Step 4.4 — Series**
Record: Single standalone patch, two revisions, message-only v2.
**Step 4.5 — stable@**
Record: No stable-list discussion found (lore HTML search was unusable;
v2 mbox/thread had no stable Cc). UNVERIFIED beyond the v1/v2 linux-ide
thread.
## Phase 5: Semantic / call chain
**Step 5.1 — Functions**
Record: `ep93xx_pata_dma_init()` only (callees: `dma_request_chan`,
`dmaengine_slave_config`, `dma_release_channel`,
`ep93xx_pata_release_dma`).
**Step 5.2 — Callers**
Record: Only `ep93xx_pata_probe()` (`err =
ep93xx_pata_dma_init(drv_data); if (err) return err;`). Platform probe /
device enumeration. Not IRQ/hot path.
**Step 5.3 — Callees**
Record: `dma_request_chan()` → `of_dma_request_slave_channel()`. Missing
`dmas` returns `-ENODEV` (“Silently fail if there is not even the dmas
property”). Missing controller while `dmas` is present returns
`-EPROBE_DEFER`.
**Step 5.4 — Reachability**
Record: Reachable when `CONFIG_PATA_EP93XX` is enabled and the
`cirrus,ep9312-pata` platform device probes. Userspace does not trigger
it directly; boot/device probe does. On this tree the SoC dtsi node is
`status = "disabled"` and no in-tree board enables `&ide`.
Binding/example still has no `dmas`, so **any** DT that enables the node
as documented hits `-ENODEV` and, before the fix, never binds.
**Step 5.5 — Similar patterns**
Record: Pre-6.12 driver already implemented this fallback. Probe still
has `if (drv_data->dma_rx_channel && drv_data->dma_tx_channel)` before
setting `udma_mask`.
## Phase 6: This tree (v7.3-rc4)
**Step 6.1 — Buggy code here?**
Record: Yes. Local tree: `git describe HEAD` =
**v7.3-rc4-27-gf0100363d8c37**, `make kernelversion` = **7.3.0-rc4**.
`9963113e3a924` is an ancestor. Pre-fix `ep93xx_pata_dma_init()` in
`c303c3619a1d5^` returns errors from `dma_request_chan()`. The candidate
itself is also an ancestor of HEAD (already in 7.3); that does not
change whether the bug class exists in this lineage.
**Step 6.2 — Backport difficulty**
Record: Clean apply on this tree’s `pata_ep93xx.c` (post-DT
`dma_request_chan` shape). No conflicting local changes after the fix.
**Step 6.3 — Related fixes already here**
Record: This is the fix. No later follow-up on the file. `5c8121262484d`
(“pata_ep93xx: fix deferred probing”) only touches `platform_get_irq()`,
not DMA init.
## Phase 7: Subsystem context
**Step 7.1**
Record: Subsystem `drivers/ata` (libata PATA). Criticality
**PERIPHERAL/IMPORTANT**: storage driver, but only Cirrus EP93xx
(`CONFIG_PATA_EP93XX` depends on `ARCH_EP93XX || COMPILE_TEST`).
**Step 7.2**
Record: File is mature (2012) with a burst of DT work in 6.12; otherwise
low churn.
## Phase 8: Impact and risk
**Step 8.1 — Who is affected**
Record: EP93xx PATA users on DT kernels (6.12+). Not universal. In-tree
boards do not enable the IDE node today; anyone enabling it per the
binding (no `dmas`; `additionalProperties: false`) is affected.
**Step 8.2 — Trigger**
Record: Device probe. Common if the node is enabled: official
dtsi/binding have no DMA properties, so `dma_request_chan("rx")` returns
`-ENODEV` every boot. Unprivileged userspace cannot trigger it. Not a
race.
**Step 8.3 — Failure mode**
Record:
- Main: probe fails → no ATA host → disk missing (boot/rootfs failure if
PATA is the disk). Severity **HIGH** for those boards, not a kernel
oops.
- Secondary (rx OK, tx fail): `dma_release_channel(ERR_PTR)` / double-
release. Severity **HIGH** if hit; needs a DT with a working `rx` name
and failing `tx`. Not the in-tree DT shape.
**Step 8.4 — Risk/benefit**
Record: Benefit medium-high for EP93xx DT PATA (driver actually binds).
Risk very low (error path only, ~20 lines, maintainer-acked). Ratio
favors backport.
## Phase 9: Synthesis
**Step 9.1 — Evidence**
FOR:
- Real v6.12 regression: DMA init errors abort probe; original design
and probe still expect PIO fallback.
- In-tree binding/example/dtsi do not describe DMA, so `-ENODEV` is the
realistic `dma_request_chan()` result if the node is enabled.
- Also closes a bad tx-fail cleanup (ERR_PTR / double-release).
- Small, obviously correct; Niklas: “looks correct”; Damien applied it.
- Prerequisites (`dma_request_chan` conversion) are in this tree.
AGAINST:
- Niche SoC; IDE node disabled in all in-tree boards — limited default
user base.
- No crash/syzbot/user report.
- Not a new feature, but also not a universal core-kernel bug.
UNRESOLVED:
- Whether any production EP93xx DT out of tree enables this node
(unverified).
- No stable@ thread found beyond linux-ide.
**Step 9.2 — Stable rules**
1. Obviously correct and tested? **PASS** — logic matches pre-6.12; v1
reviewed; maintainer applied. No separate hardware Tested-by.
2. Real bug affecting users? **PASS** — probe failure / non-working PATA
when DMA is absent.
3. Important (crash/security/corruption/deadlock/etc.)? **PASS** —
storage controller fails to register (boot-breaking if PATA is the
disk). Not an oops, but functionally serious. Secondary error-path
UAF-class DMA release if tx fails after rx.
4. Small and contained? **PASS** — one function, +21/−9.
5. No new features/APIs? **PASS**.
6. Can apply to this tree? **PASS** — same `dma_request_chan` code; DT
conversion is present.
**Step 9.3 — Exception category**
Record: None (not an ID/quirk/DT/build/doc exception). Ordinary driver
bugfix.
**Step 9.4 — Decision**
This tree is **7.3-rc4**. The v6.12 DT conversion is here, so the bug
exists in this lineage. The patch restores the driver’s intended PIO
fallback, keeps `-EPROBE_DEFER`, and is a small, reviewed error-path
fix. It meets stable criteria for this checkout.
What it solves for stable users: after the 6.12 OF DMA switch, missing
DMA (the documented DT) made `pata_ep93xx` fail probe instead of running
PIO. That can leave the disk missing. Risk is low; no extra commits are
required here.
Verification:
- [Phase 1] Parsed tags: author + Damien SOB only; no Fixes/Reported-
by/Cc:stable
- [Phase 2] Diff: `ep93xx_pata_dma_init()` error paths; return 0 except
`-EPROBE_DEFER`; NULL ERR_PTRs; split `fail_release_rx` from
`fail_release_dma`
- [Phase 2] Parent file: `git show
c303c3619a1d5^:drivers/ata/pata_ep93xx.c` confirms probe abort + fall-
through release
- [Phase 2] `dma_release_channel()` dereferences `chan->client_count` —
ERR_PTR would oops
- [Phase 3] Blame: buggy `dma_request_chan` from `9963113e3a924`
(v6.12-rc1); `git merge-base --is-ancestor 9963113e3a924 HEAD` true
- [Phase 3] Pre-DT `dma_init` was void and already fell back to PIO
- [Phase 4] `b4 dig -c c303c3619a1d5`: v1 2026-06-01, v2 2026-07-19
(message-only)
- [Phase 4] Niklas Cassel: “looks correct”; Damien: “Applied to for-7.3”
- [Phase 5] Only caller: `ep93xx_pata_probe()`;
`of_dma_request_slave_channel` returns `-ENODEV` without `dmas`
- [Phase 6] `git describe HEAD` = v7.3-rc4-27-gf0100363d8c37;
kernelversion 7.3.0-rc4
- [Phase 6] Binding `cirrus,ep9312-pata.yaml`: no dmas,
`additionalProperties: false`; dtsi `ide@800a0000` has no dmas, status
disabled; no `&ide` enable in in-tree boards
- [Phase 8] Failure: probe fail / no disk; not an oops on the common
`-ENODEV` path
- UNVERIFIED: out-of-tree boards enabling this node; stable@ mailing
list beyond linux-ide
**YES**
drivers/ata/pata_ep93xx.c | 30 +++++++++++++++++++++---------
1 file changed, 21 insertions(+), 9 deletions(-)
diff --git a/drivers/ata/pata_ep93xx.c b/drivers/ata/pata_ep93xx.c
index b2b9e0058333e..3f61712af5bbe 100644
--- a/drivers/ata/pata_ep93xx.c
+++ b/drivers/ata/pata_ep93xx.c
@@ -652,14 +652,22 @@ static int ep93xx_pata_dma_init(struct ep93xx_pata_data *drv_data)
* start of new transfer.
*/
drv_data->dma_rx_channel = dma_request_chan(dev, "rx");
- if (IS_ERR(drv_data->dma_rx_channel))
- return dev_err_probe(dev, PTR_ERR(drv_data->dma_rx_channel),
- "rx DMA setup failed\n");
+ if (IS_ERR(drv_data->dma_rx_channel)) {
+ ret = PTR_ERR(drv_data->dma_rx_channel);
+ drv_data->dma_rx_channel = NULL;
+ if (ret == -EPROBE_DEFER)
+ return ret;
+ dev_warn(dev, "rx DMA unavailable, using PIO\n");
+ return 0;
+ }
drv_data->dma_tx_channel = dma_request_chan(&pdev->dev, "tx");
if (IS_ERR(drv_data->dma_tx_channel)) {
- ret = dev_err_probe(dev, PTR_ERR(drv_data->dma_tx_channel),
- "tx DMA setup failed\n");
+ ret = PTR_ERR(drv_data->dma_tx_channel);
+ drv_data->dma_tx_channel = NULL;
+ if (ret == -EPROBE_DEFER)
+ goto fail_release_rx;
+ dev_warn(dev, "tx DMA unavailable, using PIO\n");
goto fail_release_rx;
}
@@ -670,7 +678,7 @@ static int ep93xx_pata_dma_init(struct ep93xx_pata_data *drv_data)
conf.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
ret = dmaengine_slave_config(drv_data->dma_rx_channel, &conf);
if (ret) {
- dev_err_probe(dev, ret, "failed to configure rx dma channel");
+ dev_warn(dev, "failed to configure rx dma channel, using PIO\n");
goto fail_release_dma;
}
@@ -681,7 +689,7 @@ static int ep93xx_pata_dma_init(struct ep93xx_pata_data *drv_data)
conf.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
ret = dmaengine_slave_config(drv_data->dma_tx_channel, &conf);
if (ret) {
- dev_err_probe(dev, ret, "failed to configure tx dma channel");
+ dev_warn(dev, "failed to configure tx dma channel, using PIO\n");
goto fail_release_dma;
}
@@ -689,10 +697,14 @@ static int ep93xx_pata_dma_init(struct ep93xx_pata_data *drv_data)
fail_release_rx:
dma_release_channel(drv_data->dma_rx_channel);
+ drv_data->dma_rx_channel = NULL;
+ if (ret == -EPROBE_DEFER)
+ return ret;
+ return 0;
+
fail_release_dma:
ep93xx_pata_release_dma(drv_data);
-
- return ret;
+ return 0;
}
static void ep93xx_pata_dma_start(struct ata_queued_cmd *qc)
--
2.53.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH AUTOSEL 6.18-5.10] ata: libata-sff: don't busy-wait for PIO data-in command completion
[not found] <20261019135441.1420968-1-sashal@kernel.org>
2026-10-19 13:45 ` [PATCH AUTOSEL 6.18-5.10] ata: ata_generic: Don't store pci_device_id Sasha Levin
2026-10-19 13:46 ` [PATCH AUTOSEL 6.18-6.12] ata: pata_ep93xx: fix PIO fallback when DMA init fails Sasha Levin
@ 2026-10-19 13:47 ` Sasha Levin
2026-10-19 14:42 ` sashiko-bot
2 siblings, 1 reply; 4+ messages in thread
From: Sasha Levin @ 2026-10-19 13:47 UTC (permalink / raw)
To: patches, stable
Cc: Richard Weinberger, Damien Le Moal, Sasha Levin, cassel,
linux-ide, linux-kernel
From: Richard Weinberger <richard@nod.at>
[ Upstream commit 4a2e5409472e8a0e3e2817e149c55c1d5f803742 ]
Unlike PIO data-out, the PIO data-in protocol raises no completion
interrupt, the last interrupt announces the final data block, and once
the host has drained it from the data register the ending status must
be obtained synchronously. ata_sff_hsm_move() does this by spinning
in ata_wait_idle() for up to 10ms.
Usually this is not a big deal unless the device is slow. In my case
it's a CF card which keeps BSY asserted for multiple milliseconds(!)
after the final data block. Since the waiting happens in the
interrupt handler, under the port lock with interrupts disabled, the
CPU is hogged for milliseconds on every read command.
To improve the situation, bound the inline wait to ~100us. If the
device is still busy after that, mark the command ATA_TFLAG_POLLING,
so the interrupt handler won't race for it, and obtain the ending
status via ata_sff_pio_task(), which sleeps between status checks
instead of spinning with the lock held.
Since ata_sff_pio_task() may now finish a data-in command, it must
wait for both BSY and DRQ to clear at HSM_ST_LAST, matching what
ata_wait_idle() enforced.
Signed-off-by: Richard Weinberger <richard@nod.at>
Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
LLM Generated explanations, may be completely bogus:
# Stable backport analysis: `ata: libata-sff: don't busy-wait for PIO
data-in command completion`
Local tree: **v7.3-rc4** (`7.3.0-rc4`, HEAD `f0100363d8c37`). Candidate:
`4a2e5409472e8` (Damien Le Moal, 2026-07-21). The buggy PIO completion
wait has been in `drivers/ata/libata-sff.c` since 2008; this change is a
small HSM latency fix for that path.
---
## PHASE 1: COMMIT MESSAGE FORENSICS
**Step 1.1: PARSE THE SUBJECT LINE**
- Subsystem: `ata: libata-sff`
- Action verb: **don't busy-wait** (prevent / stop incorrect spinning)
- Intent: stop spinning in the SFF interrupt handler for PIO data-in
completion
Record: `[ata/libata-sff] [don't busy-wait] [cap IRQ-context wait for
PIO data-in; defer remaining wait to sleeping pio task]`
**Step 1.2: PARSE ALL COMMIT MESSAGE TAGS**
- `Signed-off-by: Richard Weinberger <richard@nod.at>` (author)
- `Signed-off-by: Damien Le Moal <dlemoal@kernel.org>` (libata
maintainer; applied to for-7.3)
- No `Fixes:`
- No `Reported-by:` / `Tested-by:` / `Reviewed-by:` / `Acked-by:`
- No `Link:` / `Cc: stable`
- No syzbot / sanitizer tags
Absence of `Fixes:` / `Cc: stable` is expected for this review and is
not a negative signal.
Record: `[tags: two SOBs (author + ATA maintainer); no Fixes/Reported-
by/Link/stable Cc]`
**Step 1.3: ANALYZE THE COMMIT BODY TEXT**
PIO data-in does not raise a completion IRQ. The last IRQ means “final
data block”; after the host drains it, ending status must be read
synchronously. `ata_sff_hsm_move()` used `ata_wait_idle()`, spinning up
to **10ms**.
On a CompactFlash card that keeps BSY asserted for several milliseconds
after the last block, that wait runs in the **interrupt handler**, under
the **port lock**, with **IRQs disabled**, on **every read**.
Fix: bound the inline wait to ~100µs (`ata_sff_busy_wait(..., 10)`). If
still busy, set `ATA_TFLAG_POLLING` and finish via `ata_sff_pio_task()`,
which sleeps between checks. That task must wait for both BSY and DRQ at
`HSM_ST_LAST`, matching `ata_wait_idle()`.
Record: `[Bug: IRQ-context 10ms busy-wait on PIO data-in] [Symptom: CPU
hogged for ms per read, lock held, IRQs off] [Root cause:
ata_wait_idle() in hsm_move after last PIO-in block] [Trigger: slow
CF/PATA that holds BSY after last DRQ]`
**Step 1.4: DETECT HIDDEN BUG FIXES**
Wording is “improve the situation,” but the failure is a real IRQ-
context stall, not a cleanup. That matches the “oh, that's not good”
class in `Documentation/process/stable-kernel-rules.rst`, and the
hardware-quirk / interactivity class for slow CF.
Record: `[Hidden bug fix: yes — IRQ busy-wait / latency stall, not style
cleanup]`
---
## PHASE 2: DIFF ANALYSIS
**Step 2.1: INVENTORY THE CHANGES**
- File: `drivers/ata/libata-sff.c` only
- Stat: **17 insertions, 7 deletions** (24 lines)
- Functions: `ata_sff_hsm_move()`, `ata_sff_pio_task()`
- Scope: single-file surgical HSM change
Record: `[1 file, +17/-7] [ata_sff_hsm_move, ata_sff_pio_task]
[surgical]`
**Step 2.2: UNDERSTAND THE CODE FLOW CHANGE**
Hunk 1 (`ata_sff_hsm_move`, `HSM_ST`, PIO data-in last block):
- Before: `status = ata_wait_idle(ap)` (up to 1000×10µs = 10ms), then
`goto fsm_start`.
- After: `ata_sff_busy_wait(ap, ATA_BUSY|ATA_DRQ, 10)` (~100µs). If
still busy and status ≠ 0xff: set `ATA_TFLAG_POLLING`,
`ata_sff_queue_pio_task(link, 0)`, return 0. Else same `goto
fsm_start`.
Hunk 2 (`ata_sff_pio_task`):
- Before: always wait only for `ATA_BUSY`.
- After: `wait_mask = ATA_BUSY`, and at `HSM_ST_LAST` also `ATA_DRQ`.
Both short spins and the “still busy → requeue” test use `wait_mask`.
Record: `[IRQ path: 10ms spin → 100µs then optional poll work] [wq path:
wait BSY, or BSY|DRQ at LAST]`
**Step 2.3: IDENTIFY THE BUG MECHANISM**
Category: **logic/latency in IRQ** (busy-wait with IRQs disabled), plus
**hardware delay on slow CF**.
Mechanism: last PIO-in IRQ still needs BSY/DRQ to drop. Spinning in
hardirq with the port lock held stalls the CPU. The fix uses the
existing polling workqueue, which already drops the lock and
`ata_msleep()`s.
`ATA_TFLAG_POLLING` is the existing “IRQ must not complete this qc”
flag. `__ata_sff_interrupt()` skips `port_intr()` when it is set.
Record: `[category: IRQ busy-wait / latency] [fix: short wait + existing
pio_task polling; LAST waits BSY|DRQ]`
**Step 2.4: ASSESS THE FIX QUALITY**
Verified against current code:
- `ata_wait_idle()` is `ata_sff_busy_wait(..., ATA_BUSY|ATA_DRQ, 1000)`
with a debug print. New max=10 is the same bits, 100× shorter.
- Status 0xff (no device) is **not** deferred; it still goes to
`fsm_start`, same as a disconnected `ata_wait_idle()` result.
- `ata_hsm_ok_in_wq()` returns 1 when `ATA_TFLAG_POLLING` is set, so the
`WARN_ON_ONCE(in_wq != ata_hsm_ok_in_wq())` stays consistent when the
workqueue later calls `ata_sff_hsm_move(..., 1)`.
- `pio_task` clears `sff_pio_task_link` **before** `hsm_move`. If the
IRQ path queued work and the wq path hits the new wait, requeue via
`ata_sff_queue_pio_task` is the existing delayed-work pattern.
- Fast devices that drop BSY/DRQ within ~100µs still complete in IRQ;
only slow devices take the workqueue.
Regression risk is real but contained: HSM is sensitive, and the patch
does not call `ata_qc_set_polling()` (no nIEN). For PIO data-in that is
acceptable: the protocol does not raise a completion IRQ after the last
block, which is why the synchronous wait existed.
Record: `[quality: small, uses existing polling HSM] [risk: low-medium
HSM timing; nIEN not set, which matches this protocol]`
---
## PHASE 3: GIT HISTORY INVESTIGATION
**Step 3.1: BLAME THE CHANGED LINES**
- Surrounding `if (hsm_task_state == HSM_ST_LAST && !WRITE)` /
`ata_wait_idle()`: **`624d5c514eed18`** (Tejun Heo, 2008-03-25),
“libata: reorganize SFF related stuff” — move-only; the wait was
already the SFF HSM.
- `git describe 624d5c514eed18`: **v2.6.25-18**; contained in
**v2.6.26-rc1**.
- New lines blamed on `4a2e5409472e8` (this commit).
Record: `[buggy wait present since ~v2.6.26 (2008)] [long-lived, all
LTS-class trees have it]`
**Step 3.2: FOLLOW THE FIXES: TAG**
No `Fixes:` tag. Not applicable beyond that. The wait itself was not
introduced by a recent buggy commit.
Record: `[no Fixes tag; bug is decades-old HSM behavior]`
**Step 3.3: CHECK FILE HISTORY FOR RELATED CHANGES**
Recent `libata-sff.c` (path-limited): this commit is the latest on the
file. No later follow-up on this wait. Older related HSM work
(`ce7514526742c`, `8eee1d3ed5b6f`) is already in this tree. Not part of
a numbered series.
Record: `[standalone] [no post-commit fix on this file] [no missing HSM
locking prereq]`
**Step 3.4: CHECK THE AUTHOR'S OTHER COMMITS**
Path-limited `git log --author="Richard Weinberger" -- drivers/ata/`:
only this commit. Author is a long-time embedded/UML maintainer (sigma
star); CF is typical for that hardware. Damien Le Moal is the libata
maintainer and applied it.
Record: `[author: one-off ATA contributor, real CF user] [applied by
subsystem maintainer]`
**Step 3.5: CHECK FOR DEPENDENT/PREREQUISITE COMMITS**
Uses existing `ata_sff_busy_wait`, `ATA_TFLAG_POLLING`,
`ata_sff_queue_pio_task`, `ata_sff_pio_task`, `ATA_SHORT_PAUSE`. No new
helpers. Parent hunks match 2008-era HSM structure.
Record: `[no extra dependencies; standalone apply]`
---
## PHASE 4: MAILING LIST AND EXTERNAL RESEARCH
**Step 4.1: FIND THE ORIGINAL PATCH DISCUSSION**
`b4 dig -c 4a2e5409472e8`:
- https://patch.msgid.link/20260717090404.857078-1-richard@nod.at
- Single revision: `[PATCH] [RFC] libata: Don't busy-wait...`
- Damien: “Looks OK to me, but the patch title should be: ata: libata-
sff: …”
- Richard: agreed; Damien applied with the title change.
- No NAK, no stable nomination, no race objections in the retrieved
thread (openwall/lkml/patchew). lore.kernel.org fetch returned 403;
discussion recovered from patchew, openwall, and lkml.
Record: `[lore/msgid: 20260717090404.857078-1-richard@nod.at] [v1 RFC
only] [maintainer OK, title-only change] [no NAK]`
**Step 4.2: CHECK WHO REVIEWED THE PATCH**
`b4 dig -w` recipients: Richard Weinberger, linux-ide, linux-kernel,
`upstream+linux@sigma-star.at`, `cassel@kernel.org` (Niklas Cassel),
`dlemoal@kernel.org`.
Visible review: Damien only (“Looks OK” / Applied). No Niklas reply
found. No `Reviewed-by`.
Record: `[CC: linux-ide + Cassel + Le Moal] [reviewer: Damien Le Moal
applied]`
**Step 4.3: SEARCH FOR THE BUG REPORT**
No bugzilla/syzbot. Author is the reporter: CF card, BSY held for
milliseconds after last data block, IRQ busy-wait on every read.
Record: `[one real-hardware report (author, CF)] [no distro bugzilla]`
**Step 4.4: CHECK FOR RELATED PATCHES AND SERIES**
Standalone RFC, not `m/n`. No sibling patches required.
Record: `[standalone series]`
**Step 4.5: CHECK STABLE MAILING LIST HISTORY**
Web search of lore.kernel.org/stable for this subject found **no**
stable-list discussion.
Record: `[no stable@ discussion found]`
---
## PHASE 5: CODE SEMANTIC ANALYSIS
**Step 5.1: IDENTIFY KEY FUNCTIONS**
`ata_sff_hsm_move`, `ata_sff_pio_task`. Helpers used:
`ata_sff_busy_wait`, `ata_sff_queue_pio_task`, `ata_pio_sectors`.
Record: `[modified: ata_sff_hsm_move, ata_sff_pio_task]`
**Step 5.2: TRACE CALLERS**
`ata_sff_hsm_move`:
- `ata_sff_pio_task` (workqueue, `in_wq=1`)
- `__ata_sff_port_intr` → `ata_sff_port_intr` / `ata_sff_interrupt`
(hardirq, `in_wq=0`) — **this is the spinning path**
- `sata_sil.c`, `sata_dwc_460ex.c`, `pata_octeon_cf.c` (CF)
`ata_sff_pio_task`: delayed work on `ata_sff_wq`, queued from
`ata_sff_qc_issue` (PIO polling / writes) and now from the new data-in
completion path.
Record: `[IRQ HSM is the hot path for interrupt-driven PIO-in] [also
used by octeon CF]`
**Step 5.3: TRACE CALLEES**
`ata_pio_sectors` → `ata_pio_sector` sets `HSM_ST_LAST` on last sector.
Then the wait. `ata_sff_queue_pio_task` queues delayed work (delay 0).
`ata_hsm_qc_complete` from wq calls `ata_sff_irq_on` +
`ata_qc_complete`.
Record: `[callees: existing PIO xfer + pio_task + qc complete]`
**Step 5.4: FOLLOW THE CALL CHAIN**
Userspace read on PATA/CF/PIO → SCSI/libata → `ata_sff_qc_issue` (PIO-
in, `HSM_ST`) → device IRQ → `ata_sff_interrupt` → `ata_sff_hsm_move` →
last `ata_pio_sectors` → **old: 10ms `ata_wait_idle` in hardirq**.
Reachable from ordinary block reads on PIO ATA. Unprivileged if the
device is mounted/readable.
Record: `[reachable from userspace I/O on PIO/CF] [common on embedded,
rare on AHCI DMA desktops]`
**Step 5.5: SEARCH FOR SIMILAR PATTERNS**
Error path at lines 1083–1086 still uses `ata_wait_idle(ap)` for PIO-in
+ ERR/DF. Unchanged. Other drivers (`pata_octeon_cf`, `pata_via`, …)
still use `ata_wait_idle` in tf_load / issue, not this HSM completion
wait.
Record: `[no sibling copy of this exact last-block wait; error-path
wait_idle left as-is]`
---
## PHASE 6: CROSS-REFERENCING AGAINST THE LOCAL TREE
**Step 6.1: DOES THE BUGGY CODE EXIST IN THIS TREE?**
Yes in this lineage. Pre-commit parent still has `status =
ata_wait_idle(ap)` after last PIO-in. Current HEAD already contains
`4a2e5409472e8` (`git merge-base --is-ancestor` succeeded).
`ata_sff_hsm_move` / `ata_sff_pio_task` / `ata_wait_idle` all exist.
Record: `[bug introduced ~v2.6.26; this 7.3-rc4 tree has the fix; pre-
fix code is the long-standing HSM wait]`
**Step 6.2: CHECK FOR BACKPORT COMPLICATIONS**
The HSM last-block `ata_wait_idle` hunk is unchanged from 2008 until
this commit. No conflicting refactor in this file after the candidate.
Expected apply on older trees that still have that hunk: **clean**.
Record: `[clean apply on trees that still have ata_wait_idle in that HSM
branch]`
**Step 6.3: CHECK IF RELATED FIXES ARE ALREADY HERE**
This exact commit is already an ancestor of HEAD. That is not used as a
reason to reject: the pipeline forbids “already selected” circular
reasoning. No *different* fix for the same IRQ wait.
Record: `[no alternate fix; this is the fix]`
---
## PHASE 7: SUBSYSTEM AND MAINTAINER CONTEXT
**Step 7.1: IDENTIFY THE SUBSYSTEM AND ITS CRITICALITY**
`drivers/ata/libata-sff.c` — libata SFF (legacy IDE/PATA/BMDMA/CF PIO).
Record: `[libata SFF] [IMPORTANT for PATA/CF/PIO; not core mm/net;
unused on typical AHCI-only machines]`
**Step 7.2: ASSESS SUBSYSTEM ACTIVITY**
Mature. HSM wait logic sat still for ~18 years. Recent ATA work is
EH/SCSI, not this completion wait.
Record: `[mature/stable HSM; bug is old, not a new-subsystem issue]`
---
## PHASE 8: IMPACT AND RISK ASSESSMENT
**Step 8.1: DETERMINE WHO IS AFFECTED**
PIO data-in on SFF: CompactFlash, PATA in PIO, some embedded `pata_*` /
`pata_octeon_cf`, fallback PIO. Not AHCI DMA.
Record: `[driver/hardware-specific: SFF PIO-in, especially slow CF]`
**Step 8.2: DETERMINE THE TRIGGER CONDITIONS**
Every PIO read whose device holds BSY/DRQ > ~100µs after the last block.
Author: multiple milliseconds, every read. Unprivileged I/O on that
device is enough. Not a race.
Record: `[trigger: PIO read on slow SFF/CF] [per-command] [userspace
I/O]`
**Step 8.3: DETERMINE THE FAILURE MODE SEVERITY**
Hardirq + port lock + IRQs off for up to 10ms **per command**.
Sequential 512-byte reads can stall the CPU for long stretches (lost
IRQs, timer/network/audio latency; possible watchdog on a very slow
device). Not a documented oops/UAF/corruption.
Official stable text also allows an **“oh, that's not good”** issue and
notable interactivity problems. This is IRQ stall, not a micro-
optimization.
Record: `[failure: IRQ/CPU stall and latency] [severity: HIGH for
CF/PATA PIO; LOW for AHCI-only]`
**Step 8.4: CALCULATE RISK-BENEFIT RATIO**
- Benefit: stops multi-ms IRQ spin on every slow PIO-in read; real CF
report; embedded LTS users still have this hardware.
- Risk: ~17 lines, existing polling flag + workqueue; fast devices
unchanged (~100µs still in IRQ); ~2 months in mainline with no follow-
up revert/fix on this file.
Record: `[benefit: high for CF/PATA] [risk: low-medium] [ratio:
favorable]`
---
## PHASE 9: FINAL SYNTHESIS
**Step 9.1: COMPILE THE EVIDENCE**
FOR:
- Real hardware: CF holds BSY for milliseconds; author hit it.
- Busy-wait is in hardirq, port lock held, IRQs disabled — system-wide
latency, not a comment-only cleanup.
- Small (+17/-7), one file, existing `ATA_TFLAG_POLLING` +
`ata_sff_pio_task`.
- Maintainer applied after review (“Looks OK”).
- Bug present since v2.6.26; no extra prereqs.
- Fast path unchanged if the device idles within ~100µs.
AGAINST:
- Worded as an improvement, not a crash/CVE.
- One reporter; no syzbot/bugzilla; no `Cc: stable`.
- Performance/interactivity bar in stable-rules prefers distro
maintainers + bugzilla; this did not go that route.
- HSM changes can regress odd controllers (residual IRQs because nIEN is
not set).
- Affects a shrinking hardware set (PATA/CF), not all machines.
UNRESOLVED:
- lore.kernel.org HTTP 403; thread reconstructed from b4 + mirrors.
- No public confirmation from a second user.
- No lockup-detector trace in the message (severity inferred from 10ms×N
in hardirq, not from a posted stack).
**Step 9.2: APPLY THE STABLE RULES CHECKLIST**
1. Obviously correct and tested? **PASS** — matches `ata_wait_idle`
bits; author tested on CF; maintainer applied.
2. Fixes a real bug that affects users? **PASS** — IRQ CPU hog on real
CF reads.
3. Important issue? **PASS** — “oh, that's not good” / interactivity:
multi-ms hardirq spin per read is a stall, not a cosmetic tweak. Not
a UAF/panic.
4. Small and contained? **PASS** — 24 lines, one file.
5. No new features or APIs? **PASS**.
6. Can apply to the local tree? **PASS** — same HSM functions and flags;
pre-fix hunk is the classic `ata_wait_idle` sequence.
**Step 9.3: CHECK FOR EXCEPTION CATEGORIES**
Not a device ID, DT, build, or doc patch. Closest: **hardware
workaround** for slow CF BSY, implemented as a bounded IRQ wait +
existing polling.
Record: `[quasi-quirk: slow device BSY after last PIO-in block]`
**Step 9.4: MAKE YOUR DECISION**
This tree’s libata-sff HSM still has (and had for ~18 years) the 10ms
IRQ wait. The patch is a small, maintainer-acked correction that stops
hogging the CPU with IRQs off on every slow PIO-in completion. That is
in-scope for stable: real user-visible stall, surgical, no new API, no
missing prereqs.
---
### What the commit solves for stable users
On interrupt-driven SFF PIO reads, after the last data block the host
must wait for BSY/DRQ to clear with no completion IRQ. The kernel spun
in `ata_wait_idle()` for up to 10ms in hardirq under the port lock. Slow
CF (and similar) keeps BSY for milliseconds, so **every read** stalls
the CPU. The fix waits ~100µs in IRQ, then uses the existing PIO polling
workqueue, which sleeps instead of spinning.
### Stable rules
Meets “obviously correct,” real user-visible bug, small scope, no new
API, applies to this codebase. Importance is IRQ stall / interactivity,
which stable-rules explicitly allow as “oh, that's not good,” not only
oops/corruption.
### Risk vs benefit
Benefit is high for CF/PATA PIO (embedded, industrial, some laptops).
Risk is a modest HSM timing change, mitigated by reusing
`ATA_TFLAG_POLLING` (IRQ handler already ignores those commands) and by
leaving fast devices on the old complete-in-IRQ path.
### Concerns
No `Fixes:`/second reporter; nIEN not set when flipping to polling
(acceptable for this protocol); HSM always has some controller risk.
None of those outweigh the stall fix.
---
Verification:
- [Phase 1] Subject/body/tags from `git log -1 --format=fuller
4a2e5409472e8`: two SOBs, no Fixes/Reported-by/Link/stable.
- [Phase 1] Body describes CF + multi-ms BSY + IRQ/`ata_wait_idle` 10ms
spin.
- [Phase 2] `git show --stat`: `drivers/ata/libata-sff.c` +17/-7; hunks
in `ata_sff_hsm_move` and `ata_sff_pio_task`.
- [Phase 2] `ata_wait_idle` in `include/linux/libata.h`:
`ata_sff_busy_wait(..., ATA_BUSY|ATA_DRQ, 1000)` = 10ms; new wait uses
max=10 (~100µs).
- [Phase 2] `__ata_sff_interrupt` skips `port_intr` when
`ATA_TFLAG_POLLING` is set (verified in `libata-sff.c`).
- [Phase 2] `ata_hsm_ok_in_wq()` returns 1 if `ATA_TFLAG_POLLING` — WARN
consistency checked.
- [Phase 3] `git blame` on wait site: Tejun `624d5c514eed18` (2008);
`git describe` → v2.6.25-18 / contained in v2.6.26-rc1.
- [Phase 3] `git log HEAD -- drivers/ata/libata-sff.c`: no later commit
on this file after 4a2e5409472e8.
- [Phase 3] Path-limited author log: only this ATA commit.
- [Phase 4] `b4 dig -c 4a2e5409472e8`: msgid
20260717090404.857078-1-richard@nod.at.
- [Phase 4] `b4 dig -a`: v1 RFC only.
- [Phase 4] `b4 dig -w`: linux-ide, Cassel, Le Moal CCd.
- [Phase 4] Damien “Looks OK” / “Applied to for-7.3” via lkml/openwall;
no NAK.
- [Phase 4] lore.kernel.org WebFetch: 403. Thread taken from b4 +
patchew/openwall/lkml.
- [Phase 4] No stable@ hits for this subject.
- [Phase 5] Callers of `ata_sff_hsm_move`: `ata_sff_pio_task`,
`__ata_sff_port_intr`, `sata_sil.c`, `sata_dwc_460ex.c`,
`pata_octeon_cf.c`.
- [Phase 6] `git describe HEAD` / `make kernelversion` / Makefile:
**7.3.0-rc4**.
- [Phase 6] `git merge-base --is-ancestor 4a2e5409472e8 HEAD`: yes.
- [Phase 6] Parent file still has `status = ata_wait_idle(ap)` in the
last PIO-in branch.
- [Phase 8] Failure mode: hardirq busy-wait up to 10ms per PIO-in, not
UAF/panic.
- UNVERIFIED: second independent user report; live NMI/softlockup log
(not in the message).
The patch is small, correct against the SFF HSM, and fixes a real IRQ-
context stall on hardware that still shows up on stable/embedded
systems.
**YES**
drivers/ata/libata-sff.c | 24 +++++++++++++++++-------
1 file changed, 17 insertions(+), 7 deletions(-)
diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index 1e2a2c33cdc80..416e1239ed56d 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -1114,8 +1114,14 @@ int ata_sff_hsm_move(struct ata_port *ap, struct ata_queued_cmd *qc,
if (ap->hsm_task_state == HSM_ST_LAST &&
(!(qc->tf.flags & ATA_TFLAG_WRITE))) {
- /* all data read */
- status = ata_wait_idle(ap);
+ status = ata_sff_busy_wait(ap,
+ ATA_BUSY | ATA_DRQ, 10);
+ if (status != 0xff &&
+ (status & (ATA_BUSY | ATA_DRQ))) {
+ qc->tf.flags |= ATA_TFLAG_POLLING;
+ ata_sff_queue_pio_task(link, 0);
+ return 0;
+ }
goto fsm_start;
}
}
@@ -1213,7 +1219,7 @@ static void ata_sff_pio_task(struct work_struct *work)
container_of(work, struct ata_port, sff_pio_task.work);
struct ata_link *link = ap->sff_pio_task_link;
struct ata_queued_cmd *qc;
- u8 status;
+ u8 status, wait_mask;
int poll_next;
spin_lock_irq(ap->lock);
@@ -1229,6 +1235,10 @@ static void ata_sff_pio_task(struct work_struct *work)
fsm_start:
WARN_ON_ONCE(ap->hsm_task_state == HSM_ST_IDLE);
+ wait_mask = ATA_BUSY;
+ if (ap->hsm_task_state == HSM_ST_LAST)
+ wait_mask |= ATA_DRQ;
+
/*
* This is purely heuristic. This is a fast path.
* Sometimes when we enter, BSY will be cleared in
@@ -1236,14 +1246,14 @@ static void ata_sff_pio_task(struct work_struct *work)
* or something. Snooze for a couple msecs, then
* chk-status again. If still busy, queue delayed work.
*/
- status = ata_sff_busy_wait(ap, ATA_BUSY, 5);
- if (status & ATA_BUSY) {
+ status = ata_sff_busy_wait(ap, wait_mask, 5);
+ if (status & wait_mask) {
spin_unlock_irq(ap->lock);
ata_msleep(ap, 2);
spin_lock_irq(ap->lock);
- status = ata_sff_busy_wait(ap, ATA_BUSY, 10);
- if (status & ATA_BUSY) {
+ status = ata_sff_busy_wait(ap, wait_mask, 10);
+ if (status & wait_mask) {
ata_sff_queue_pio_task(link, ATA_SHORT_PAUSE);
goto out_unlock;
}
--
2.53.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH AUTOSEL 6.18-5.10] ata: libata-sff: don't busy-wait for PIO data-in command completion
2026-10-19 13:47 ` [PATCH AUTOSEL 6.18-5.10] ata: libata-sff: don't busy-wait for PIO data-in command completion Sasha Levin
@ 2026-10-19 14:42 ` sashiko-bot
0 siblings, 0 replies; 4+ messages in thread
From: sashiko-bot @ 2026-10-19 14:42 UTC (permalink / raw)
To: Sasha Levin; +Cc: linux-ide, dlemoal, cassel
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ata: libata-sff: race between ata_sff_hsm_move() and ata_sff_flush_pio_task()
--
commit ffe3d0726a492588a046ac4c8f5eafd55e0c371b
Author: Richard Weinberger <richard@nod.at>
ata: libata-sff: don't busy-wait for PIO data-in command completion
This commit bounds the inline busy-wait for PIO data-in completion in
ata_sff_hsm_move() to ~100us. If the device remains busy after draining the
final data block, it sets ATA_TFLAG_POLLING and queues ata_sff_pio_task() to
poll for completion in process context.
> diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
> index 1e2a2c33cdc80..416e1239ed56d 100644
> --- a/drivers/ata/libata-sff.c
> +++ b/drivers/ata/libata-sff.c
> @@ -1114,8 +1114,14 @@ int ata_sff_hsm_move(struct ata_port *ap, struct ata_queued_cmd *qc,
>
> if (ap->hsm_task_state == HSM_ST_LAST &&
> (!(qc->tf.flags & ATA_TFLAG_WRITE))) {
> - /* all data read */
> - status = ata_wait_idle(ap);
> + status = ata_sff_busy_wait(ap,
> + ATA_BUSY | ATA_DRQ, 10);
> + if (status != 0xff &&
> + (status & (ATA_BUSY | ATA_DRQ))) {
> + qc->tf.flags |= ATA_TFLAG_POLLING;
> + ata_sff_queue_pio_task(link, 0);
> + return 0;
> + }
> goto fsm_start;
> }
> }
[Severity: High]
Can queuing ap->sff_pio_task from the interrupt handler here race with
ata_sff_flush_pio_task() during timeout recovery and trigger a BUG_ON()?
Previously, ap->sff_pio_task was only initially queued at command issue time
and re-queued by ata_sff_pio_task() itself, so cancel_delayed_work_sync() in
ata_sff_flush_pio_task() guaranteed the work item would not be re-armed
before ap->hsm_task_state was reset to HSM_ST_IDLE:
drivers/ata/libata-sff.c:ata_sff_flush_pio_task() {
...
cancel_delayed_work_sync(&ap->sff_pio_task);
spin_lock_irq(ap->lock);
ap->hsm_task_state = HSM_ST_IDLE;
spin_unlock_irq(ap->lock);
ap->sff_pio_task_link = NULL;
}
When an interrupt-driven PIO data-in command times out (via
ata_scsi_cmd_error_handler() -> ata_sff_flush_pio_task() or
ata_exec_internal_sg() -> ata_sff_flush_pio_task()), ap->sff_pio_task is not
yet pending, so cancel_delayed_work_sync() returns immediately.
If the final data block interrupt arrives on another CPU before
ata_sff_flush_pio_task() acquires ap->lock:
CPU0 CPU1 (IRQ)
ata_sff_flush_pio_task()
cancel_delayed_work_sync()
__ata_sff_port_intr()
ata_sff_hsm_move()
ata_pio_sectors(qc)
qc->tf.flags |= ATA_TFLAG_POLLING;
ata_sff_queue_pio_task(link, 0);
spin_lock_irq(ap->lock);
ap->hsm_task_state = HSM_ST_IDLE;
spin_unlock_irq(ap->lock);
ap->sff_pio_task_link = NULL;
When ata_sff_pio_task() then executes on ata_sff_wq, wouldn't it hit
BUG_ON(ap->sff_pio_task_link == NULL), or enter fsm_start with
ap->hsm_task_state == HSM_ST_IDLE while error handling is active?
[ ... ]
>
endasi
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261019135441.1420968-182-sashal@kernel.org?part=1
^ permalink raw reply [flat|nested] 4+ messages in thread