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: Wed, 11 Mar 2026 13:25:24 +1000 Message-ID: In-Reply-To: <20260310-drain-irqs-before-suspend-v1-1-bf4f9ed68e75@imgtec.com> References: <20260310-drain-irqs-before-suspend-v1-0-bf4f9ed68e75@imgtec.com> <20260310-drain-irqs-before-suspend-v1-1-bf4f9ed68e75@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: Good** The core fix is adding `synchronize_irq()` in `pvr_power_fw_disable()` when= called from the runtime PM suspend path (`rpm_suspend=3Dtrue`). This ensur= es any in-progress threaded IRQ handler completes before `pvr_fw_stop()` sh= uts down the firmware and the GPU is powered off. The removal of `pvr_power_get()`/`pvr_power_put()` from the safety event ha= ndling in the threaded IRQ handler is correct and necessary. The commit mes= sage explains this well: 1. The threaded IRQ handler is already running because an actual hardware i= nterrupt occurred, so the GPU must be powered on at that point. 2. The `synchronize_irq()` in the suspend path blocks the suspend callback = until the threaded handler finishes, so the GPU stays powered throughout th= e handler's execution. 3. Keeping `pvr_power_get()` in the handler would deadlock: `synchronize_ir= q()` is called from the runtime PM suspend callback (which holds the PM loc= k), and `pvr_power_get()` =E2=86=92 `pm_runtime_resume_and_get()` requires = that same lock. **Minor observation**: The `rpm_suspend` boolean parameter added to `pvr_po= wer_fw_disable` is a reasonable approach, though an alternative would have = been to call `synchronize_irq()` directly in `pvr_power_device_suspend()`. = The chosen approach keeps the synchronization close to the firmware stop, w= hich is arguably the right abstraction level. **Note on the hardirq handler**: The primary hardirq handler (`pvr_device_i= rq_handler`) also accesses GPU registers via `pvr_fw_irq_pending()` and `pv= r_device_safety_irq_pending()`. There is a small theoretical window between= `synchronize_irq()` returning and `pvr_fw_stop()` completing where a new h= ardirq could fire and access registers. Patch 2 addresses this properly. **Stable backport note**: The Fixes/Cc-stable tags and the below-the-fold e= xplanation about which commit affects which kernel version are well done, m= aking backporting easier for the stable maintainers. --- Generated by Claude Code Patch Reviewer