Re: [PATCH AUTOSEL 6.19-6.6] iommu/amd: move wait_on_sem() out of spinlock

Ankit Soni
2026-02-16T04:27:58Z

On Sat, Feb 14, 2026 at 04:23:59PM -0500, Sasha Levin wrote:
> From: Ankit Soni <Ankit.Soni@amd.com>
>
> [ Upstream commit d2a0cac10597068567d336e85fa3cbdbe8ca62bf ]
>
> With iommu.strict=1, the existing completion wait path can cause soft
> lockups under stressed environment, as wait_on_sem() busy-waits under the
> spinlock with interrupts disabled.
>
> Move the completion wait in iommu_completion_wait() out of the spinlock.
> wait_on_sem() only polls the hardware-updated cmd_sem and does not require
> iommu->lock, so holding the lock during the busy wait unnecessarily
> increases contention and extends the time with interrupts disabled.
>
> Signed-off-by: Ankit Soni <Ankit.Soni@amd.com>
> Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>
> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
>
> LLM Generated explanations, may be completely bogus:
>
> Now I understand the full picture. Let me analyze the commit as
> presented in the diff.
>
> ### 3. Bug Being Fixed
>
> **Soft lockup**: `wait_on_sem()` busy-waits for up to ~100ms
> (`LOOP_TIMEOUT=100000` iterations of `udelay(1)`) while holding
> `iommu->lock` with interrupts disabled via `raw_spin_lock_irqsave`.
> Under heavy IOMMU flush activity with `iommu.strict=1`, this can trigger
> soft lockup warnings and starve other CPUs waiting for the same lock.
>
> This is a **real, user-visible stability issue** - soft lockups cause
> watchdog warnings and can lead to system hangs on heavily-loaded systems
> using AMD IOMMU with strict mode.
>
> ### 4. Changes in Detail
>
> **Change A: `wait_on_sem()` comparison logic**
> - Old: `*iommu->cmd_sem != data` — exact equality match
> - New: `(__s64)(READ_ONCE(*iommu->cmd_sem) - data) < 0` — monotonic "not
> yet reached" check
>
> This is critical for correctness when moving `wait_on_sem()` outside the
> lock. With the lock held, commands were strictly serialized. Outside the
> lock, the semaphore value could jump past the expected value if a later
> completion overtakes the check. The monotonic comparison handles this
> correctly. The `READ_ONCE` also prevents compiler optimization issues.
>
> **Change B & C: Moving `wait_on_sem()` outside spinlock**
> In both `iommu_completion_wait()` and `iommu_flush_irt_and_complete()`,
> the spinlock is released immediately after queueing commands, before the
> busy-wait.
>
> ### 5. Concurrency Safety Analysis
>
> The subagent raised a concern about a race condition: the
> `atomic64_inc_return` happens OUTSIDE the lock, so concurrent callers
> could get sequence numbers allocated out of order relative to command
> queue insertion.
>
> **However, the monotonic comparison (`< 0` on signed difference) in the
> updated `wait_on_sem()` makes this safe**:
>
> Consider the scenario:
> - Thread A gets seq=1, Thread B gets seq=2
> - Thread B queues completion wait for 2, Thread A queues completion wait
> for 1
> - IOMMU processes: completion_wait(2) first, then completion_wait(1)
> - cmd_sem is written to 2, then 1
>
> With old `!=` comparison:
> - Thread A waits for `cmd_sem == 1` — would work when IOMMU writes 1
> - Thread B waits for `cmd_sem == 2` — would see cmd_sem jump from 2→1,
> missing the match!
>
> With new `(__s64)(cmd_sem - data) < 0` comparison:
> - Thread B waits for `cmd_sem >= 2` — satisfied when cmd_sem=2
> - Thread A waits for `cmd_sem >= 1` — satisfied when cmd_sem=2 (or 1)
> - **But wait**: cmd_sem is written to 2, then 1. After hardware writes
> 1, the value is 1. Thread B checking `cmd_sem >= 2` would fail because
> cmd_sem=1!
>

Hi,

It’s a good catch and a valid potential issue. However, Srikanth Aithal
already reported this earlier, and a fix has been added.
Please check the link below
https://lore.kernel.org/linux-iommu/c20ac675bae8565ca6307eae65efe8b71345e62d.1769054653.git.Ankit.Soni@amd.com/

Thanks,
Ankit

> Actually, this analysis reveals there IS a potential issue with out-of-
> order sequence numbers being written non-monotonically. But the key
> insight from the commit message is: "cmd_sem holds a monotonically non-
> decreasing completion sequence number." The hardware writes the values
> in command buffer order, and even though the queue insertions might
> reorder, each individual thread's `wait_on_sem()` checks if its own
> value has been reached. With the monotonic comparison, if the hardware
> writes 2 (from Thread B's command) first, Thread A (waiting for 1) would
> see `cmd_sem=2 >= 1` and proceed. Then the hardware writes 1 (from
> Thread A's command), which doesn't regress because... actually it would
> set cmd_sem to 1 after it was 2.
>
> This is actually where the companion patch becomes relevant. However,
> examining more carefully: in the pre-patch code, the
> `atomic64_inc_return` was also done outside the lock! Look at the
> current code at line 1434 — `data =
> atomic64_inc_return(&iommu->cmd_sem_val)` is before
> `raw_spin_lock_irqsave` at line 1437. So the out-of-order sequence
> number issue **already exists** in the current code. This patch doesn't
> make it worse; it actually makes it better by changing from exact
> equality to monotonic comparison.
>
> ### 6. Risk Assessment
>
> - **Files changed**: 1 file
> - **Lines changed**: ~25 lines of actual logic change (moving unlock,
> changing comparison)
> - **Subsystem**: AMD IOMMU — important for AMD server/desktop users
> - **Risk**: LOW — the change is logically sound; `wait_on_sem()` only
> reads a hardware-updated memory location
> - **Regression potential**: LOW — the lock still protects command buffer
> operations; only the polling is moved outside
>
> ### 7. Dependency Check
>
> The commit is self-contained. While a companion commit exists to further
> improve correctness by serializing sequence allocation under the lock,
> this commit stands on its own:
> 1. It fixes the immediate soft lockup problem
> 2. The monotonic comparison improves correctness over the existing `!=`
> check
> 3. The pre-existing race with `atomic64_inc_return` outside the lock is
> not made worse
>
> ### 8. Classification
>
> - **Bug type**: Soft lockup / system hang — HIGH priority for stable
> - **User impact**: Users with AMD IOMMU and `iommu.strict=1` under heavy
> I/O can experience soft lockups
> - **Fix quality**: Reviewed by AMD developer, merged by subsystem
> maintainer
> - **Stable criteria**: Obviously correct, fixes a real bug, small scope,
> no new features
>
> ### Conclusion
>
> This commit fixes a real soft lockup bug that can cause system hangs on
> AMD IOMMU systems under stress with `iommu.strict=1`. The fix is small,
> surgical (moving the busy-wait outside the spinlock), and logically
> sound — `wait_on_sem()` only polls hardware-updated memory and doesn't
> need the lock. The additional improvement to the comparison logic
> (monotonic instead of equality) is also correct and prevents false
> timeouts. The commit is from AMD's own developer, reviewed by another
> AMD developer, and merged by the IOMMU maintainer.
>
> The fix meets all stable kernel criteria: it fixes a real stability
> issue (soft lockups), is small and contained, doesn't add new features,
> and has low regression risk.
>
> **YES**
>
> drivers/iommu/amd/iommu.c | 25 +++++++++++++++++--------
> 1 file changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index 7c12be1b247f4..c8b65d6d971e0 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -1185,7 +1185,12 @@ static int wait_on_sem(struct amd_iommu *iommu, u64 data)
> {
> int i = 0;
>
> - while (*iommu->cmd_sem != data && i < LOOP_TIMEOUT) {
> + /*
> + * cmd_sem holds a monotonically non-decreasing completion sequence
> + * number.
> + */
> + while ((__s64)(READ_ONCE(*iommu->cmd_sem) - data) < 0 &&
> + i < LOOP_TIMEOUT) {
> udelay(1);
> i += 1;
> }
> @@ -1437,14 +1442,13 @@ static int iommu_completion_wait(struct amd_iommu *iommu)
> raw_spin_lock_irqsave(&iommu->lock, flags);
>
> ret = __iommu_queue_command_sync(iommu, &cmd, false);
> + raw_spin_unlock_irqrestore(&iommu->lock, flags);
> +
> if (ret)
> - goto out_unlock;
> + return ret;
>
> ret = wait_on_sem(iommu, data);
>
> -out_unlock:
> - raw_spin_unlock_irqrestore(&iommu->lock, flags);
> -
> return ret;
> }
>
> @@ -3120,13 +3124,18 @@ static void iommu_flush_irt_and_complete(struct amd_iommu *iommu, u16 devid)
> raw_spin_lock_irqsave(&iommu->lock, flags);
> ret = __iommu_queue_command_sync(iommu, &cmd, true);
> if (ret)
> - goto out;
> + goto out_err;
> ret = __iommu_queue_command_sync(iommu, &cmd2, false);
> if (ret)
> - goto out;
> + goto out_err;
> + raw_spin_unlock_irqrestore(&iommu->lock, flags);
> +
> wait_on_sem(iommu, data);
> -out:
> + return;
> +
> +out_err:
> raw_spin_unlock_irqrestore(&iommu->lock, flags);
> + return;
> }
>
> static inline u8 iommu_get_int_tablen(struct iommu_dev_data *dev_data)
> --
> 2.51.0
>