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: Fix deadlock in soft reset sequence
Date: Tue, 10 Mar 2026 12:12:14 +1000
Message-ID: <review-patch1-20260309-fix-soft-reset-v1-1-121113be554f@imgtec.com>
In-Reply-To: <20260309-fix-soft-reset-v1-1-121113be554f@imgtec.com>
References: <20260309-fix-soft-reset-v1-1-121113be554f@imgtec.com> <20260309-fix-soft-reset-v1-1-121113be554f@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 call chain is confirmed:
1. `pvr_device_irq_thread_handler()` (`pvr_device.c:203`) =E2=80=94 threade=
d IRQ handler
2. =E2=86=92 `pvr_fwccb_process()` (`pvr_device.c:217`)
3. =E2=86=92 `process_fwccb_command()` (`pvr_ccb.c:196`)
4. =E2=86=92 `pvr_power_reset(pvr_dev, false)` (`pvr_ccb.c:149`)

Inside `pvr_power_reset`, the original code unconditionally called `disable=
_irq()`, which invokes `synchronize_irq()` internally, deadlocking because =
the threaded handler is waiting for itself to complete.

The fix:
```c
if (hard_reset) {
    disable_irq(pvr_dev->irq);
} else {
    /*
     * Soft reset is triggered as a response to a FW command to the Host an=
d is
     * processed from the threaded IRQ handler. This code cannot (nor needs=
 to)
     * wait for any IRQ processing to complete.
     */
    disable_irq_nosync(pvr_dev->irq);
}
```

This is correct. `disable_irq_nosync()` still increments the IRQ disable de=
pth counter, preventing new IRQs from being delivered, but doesn't call `sy=
nchronize_irq()`. Since we're already running *as* the threaded handler, th=
ere's no concurrent IRQ processing to wait for.

**Soft-to-hard escalation path: Correct.** When a soft reset fails (`pvr_po=
wer.c:559-562`), the code sets `hard_reset =3D true` and loops. The IRQ is =
already disabled (via `_nosync`), and the loop proceeds to do the hard rese=
t with IRQs still disabled. At completion, `enable_irq()` at line 568 re-en=
ables correctly regardless of which `disable_irq` variant was used, since b=
oth increment the same disable depth counter.

**Comment quality: Good.** The comment clearly explains why `disable_irq_no=
sync()` is used and why it's safe.

**Minor observation (non-blocking):** The hard reset callers (`pvr_power.c:=
209` in the watchdog, `pvr_mmu.c:169`) appear to run from non-IRQ context, =
so `disable_irq()` remains correct for that path.

No issues found. The patch is clean, minimal, and correct.

---
Generated by Claude Code Patch Reviewer