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/imagination: Synchronize interrupts before suspending the GPU
Date: Wed, 27 May 2026 15:07:57 +1000
Message-ID: <review-patch1-20260526-sync-irqs-6-12-v2-1-b4306e95194d@imgtec.com>
In-Reply-To: <20260526-sync-irqs-6-12-v2-1-b4306e95194d@imgtec.com>
References: <20260526-sync-irqs-6-12-v2-1-b4306e95194d@imgtec.com> <20260526-sync-irqs-6-12-v2-1-b4306e95194d@imgtec.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

**Correctness of the approach:**

The patch adds a `synchronize_irq()` call in `pvr_power_fw_disable()` when =
called from the runtime PM suspend path (`rpm_suspend =3D=3D true`), placed=
 after the firmware has been told to idle and power off but before `pvr_fw_=
stop()` actually shuts down hardware:

```c
+	if (rpm_suspend) {
+		/* Wait for late processing of GPU or firmware IRQs in other cores */
+		synchronize_irq(pvr_dev->irq);
+	}
+
 	return pvr_fw_stop(pvr_dev);
```

This placement is correct =E2=80=94 by the time we reach this point, the FW=
 has acknowledged the idle and power-off requests, so no new IRQs should be=
 generated, and `synchronize_irq()` ensures any in-flight handler completes=
 before proceeding with `pvr_fw_stop()`.

**API parameter addition:**

The `bool rpm_suspend` parameter is added to `pvr_power_fw_disable()`:

```c
-pvr_power_fw_disable(struct pvr_device *pvr_dev, bool hard_reset)
+pvr_power_fw_disable(struct pvr_device *pvr_dev, bool hard_reset, bool rpm=
_suspend)
```

Both call sites are updated correctly:
- `pvr_power_device_suspend()` passes `true` =E2=80=94 this is the runtime =
PM path that needs the synchronization
- `pvr_power_reset()` passes `false` =E2=80=94 this path already handles IR=
Qs separately (it calls `disable_irq()`/`disable_irq_nosync()` before enter=
ing the reset loop)

**Comparison with upstream:**

The upstream version on drm-next uses `disable_irq()`/`enable_irq()` instea=
d of `synchronize_irq()`, which is stronger =E2=80=94 it not only waits for=
 in-flight handlers but also prevents new IRQs from being delivered. Howeve=
r, for the 6.12 stable context, `synchronize_irq()` is sufficient because:
1. By the time it's called, the FW has acknowledged power-off, so no new IR=
Qs should fire
2. It avoids the need for a matching `enable_irq()` in `pvr_power_fw_enable=
()`, which would require additional changes to a function that doesn't exis=
t in the same form on 6.12

This is a deliberate and reasonable simplification for the stable backport,=
 and the commit message documents this explicitly.

**Minor observations (not blocking):**

- The comment `/* Wait for late processing of GPU or firmware IRQs in other=
 cores */` is clear and explains the "why."
- The `v2` changelog properly notes the only changes were restoring missing=
 upstream trailers, so the core logic is unchanged from v1 which was alread=
y reviewed.

**No issues found.** The patch is a clean, minimal stable backport that cor=
rectly addresses the race condition.

---
Generated by Claude Code Patch Reviewer