From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/imagination: Synchronize interrupts before suspending the GPU Date: Mon, 25 May 2026 18:34:26 +1000 Message-ID: In-Reply-To: <20260522-sync-irqs-6-12-v1-1-b0ecc9675078@imgtec.com> References: <20260522-sync-irqs-6-12-v1-1-b0ecc9675078@imgtec.com> <20260522-sync-irqs-6-12-v1-1-b0ecc9675078@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 fix:** The core change is sound. Adding `synchronize_irq()` between the firmware p= ower-off request and `pvr_fw_stop()` ensures any in-flight threaded IRQ han= dler completes before the GPU is actually stopped: ```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); ``` `synchronize_irq()` waits for both hardirq and threaded IRQ handlers to com= plete, which is exactly what's needed given the crash trace shows `pvr_devi= ce_irq_thread_handler` as the offending function. **Placement is correct:** The call is placed after `pvr_power_request_pwr_o= ff()` (which tells the firmware to stop generating new interrupts) but befo= re `pvr_fw_stop()` (which actually powers off the GPU). This ordering ensur= es: (1) no new IRQs will be generated by the firmware, and (2) any already-= in-flight handler finishes before hardware access is revoked. **Boolean parameter addition:** ```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) ``` The two call sites are updated correctly: - `pvr_power_device_suspend()` passes `true` =E2=80=94 this is the runtime = PM path that needs synchronization - `pvr_power_reset()` passes `false` =E2=80=94 this path already handles IR= Qs differently (via `disable_irq`/`disable_irq_nosync` earlier in the reset= sequence, which on 6.12 would be just `disable_irq`) **Minor observation =E2=80=94 `synchronize_irq()` vs upstream's `disable_ir= q()`:** The upstream drm-next version uses `disable_irq()` instead of `synchronize_= irq()`, which is strictly stronger =E2=80=94 it both synchronizes *and* pre= vents new IRQs from being delivered while the GPU is off. This backport's u= se of `synchronize_irq()` alone leaves a small theoretical window: a new IR= Q could arrive (e.g., a spurious interrupt or shared IRQ line) after `synch= ronize_irq()` returns but before `pvr_fw_stop()` completes. In practice thi= s is unlikely to be a problem since: (1) the firmware has already been told= to power off, so it shouldn't generate new IRQs, and (2) the IRQ handler s= hould handle spurious interrupts gracefully. The comment in the patch descr= iption acknowledges that this is a subset of the upstream fix, which is acc= eptable for a minimal stable backport. **Commit message quality:** Good. Clearly explains the race condition, why = it's different from the upstream commit, and provides a concrete crash sign= ature. The `Fixes:` tag and `Cc: stable` are appropriate. **No issues found.** The patch is a clean, minimal, and correct stable back= port. --- Generated by Claude Code Patch Reviewer