From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/amdgpu: use spin_lock_irqsave for PASID IDR lock
Date: Tue, 31 Mar 2026 17:36:41 +1000
Message-ID: <review-patch2-20260330053025.19203-3-mikhail.v.gavrilov@gmail.com>
In-Reply-To: <20260330053025.19203-3-mikhail.v.gavrilov@gmail.com>
References: <20260330053025.19203-1-mikhail.v.gavrilov@gmail.com> <20260330053025.19203-3-mikhail.v.gavrilov@gmail.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

**Status: Good**

Converts all three lock sites to `spin_lock_irqsave`/`spin_unlock_irqrestor=
e`:

1. **`amdgpu_pasid_alloc()`** =E2=80=94 process context allocator
2. **`amdgpu_pasid_free()`** =E2=80=94 called from both process and hardirq=
 context (via `amdgpu_pasid_free_cb`)
3. **`amdgpu_pasid_mgr_cleanup()`** =E2=80=94 module cleanup

All three conversions are correct and follow the same pattern:
```c
+	unsigned long flags;
 ...
-	spin_lock(&amdgpu_pasid_idr_lock);
+	spin_lock_irqsave(&amdgpu_pasid_idr_lock, flags);
 ...
-	spin_unlock(&amdgpu_pasid_idr_lock);
+	spin_unlock_irqrestore(&amdgpu_pasid_idr_lock, flags);
```

**Minor nit**: In `amdgpu_pasid_free()`, the `flags` variable declaration i=
s placed before the `trace_amdgpu_pasid_freed()` call with no blank line se=
parating declarations from statements:

```c
void amdgpu_pasid_free(u32 pasid)
{
+	unsigned long flags;
 	trace_amdgpu_pasid_freed(pasid);
```

Kernel coding style typically expects a blank line between variable declara=
tions and the first statement. Looking at the applied result, it does have =
the declaration immediately followed by the trace call with no blank line. =
This is a cosmetic nit =E2=80=94 some maintainers care, some don't.

**Correctness note on `amdgpu_pasid_mgr_cleanup()`**: Using `spin_lock_irqs=
ave` here is technically unnecessary since this runs during module unload w=
here no concurrent IRQ-context users should exist, but it's the right thing=
 to do for consistency =E2=80=94 it ensures the lock annotation is uniform =
across all call sites, preventing future lockdep warnings if the call order=
ing ever changes.

---
Generated by Claude Code Patch Reviewer