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: Disable 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-2-bf4f9ed68e75@imgtec.com> References: <20260310-drain-irqs-before-suspend-v1-0-bf4f9ed68e75@imgtec.com> <20260310-drain-irqs-before-suspend-v1-2-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, with a minor issue** This patch upgrades `synchronize_irq()` to `disable_irq()` in the suspend p= ath, which is strictly stronger: it both waits for in-progress handlers AND= prevents new IRQs from being delivered. The corresponding `enable_irq()` i= s added to the resume path. This closes the theoretical window noted above = where a new hardirq could fire after `synchronize_irq()` returns. **IRQ disable depth accounting**: I verified the interaction with `pvr_powe= r_reset()`, which independently calls `disable_irq()`/`enable_irq()` around= its reset loop. During hard reset, it calls `pvr_power_device_suspend()` a= nd `pvr_power_device_resume()`, but because `pvr_dev->fw_dev.booted` is set= to `false` before those calls, both functions skip the `pvr_power_fw_disab= le()`/`pvr_power_fw_enable()` calls, avoiding double disable/enable. The ac= counting is correct. **Error path in `pvr_power_fw_enable`**: The error rollback is correct =E2= =80=94 if `pvr_fw_start()` or `pvr_wait_for_fw_boot()` fails during resume,= `disable_irq()` is called to re-disable the IRQ, keeping the state consist= ent. **Error path in `pvr_power_fw_disable`**: Similarly, if `pvr_fw_stop()` fai= ls during suspend, `enable_irq()` is called to restore the previous state. **Typo in commit message**: > disable IRQs for the while GPU at the kernel level Should be "for the **whole** GPU". **Minor style nit**: The `int err;` declaration is hoisted out of the `if (= !hard_reset)` block to the function scope. This is necessary for the new co= de after the `if` block, but the `err` variable inside the `if (!hard_reset= )` block is now used before the new assignment after the block. This is fin= e functionally, just noting the change in scoping was needed and is correct. **Overall**: Both patches are well-motivated, correctly implemented, and th= e series split makes sense for backporting. The only actionable item is the= "while" =E2=86=92 "whole" typo in patch 2's commit message. --- Generated by Claude Code Patch Reviewer