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/panthor: Process FW events in IRQ context
Date: Tue, 05 May 2026 11:44:54 +1000
Message-ID: <review-patch9-20260429-panthor-signal-from-irq-v1-9-4b92ae4142d2@collabora.com>
In-Reply-To: <20260429-panthor-signal-from-irq-v1-9-4b92ae4142d2@collabora.com>
References: <20260429-panthor-signal-from-irq-v1-0-4b92ae4142d2@collabora.com> <20260429-panthor-signal-from-irq-v1-9-4b92ae4142d2@collabora.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0

Patch Review

**Verdict: Has a bug.**

This is the key patch that switches the job IRQ to use a custom raw handler that processes events in hard IRQ context.

**Bug: `drm_WARN_ON_ONCE` misuse:**

```c
static irqreturn_t panthor_job_irq_threaded_handler(int irq, void *data)
{
	struct panthor_irq *pirq = data;

	/* We never return IRQ_WAKE_THREAD, so we're not supposed to be called. */
	drm_WARN_ON_ONCE(&pirq->ptdev->base,
			 "threaded IRQ handler should never be called.");
	return IRQ_NONE;
}
```

`drm_WARN_ON_ONCE(drm, x)` expects a boolean condition as the second argument. A string literal `"threaded IRQ handler should never be called."` is a non-NULL pointer, so the condition is **always true**, and this will fire every time the threaded handler is called (which should be never, but if it is, the warning will be misleading). This should be:

```c
drm_WARN_ON_ONCE(&pirq->ptdev->base, true);
```

Or better, use `drm_WARN_ONCE()` with a custom format string.

**Note:** After patch 10 introduces coalescing, the threaded handler *can* actually be called (when `IRQ_WAKE_THREAD` is returned from the raw handler), so patch 10 overrides this handler anyway. But between patches 9 and 10, the warning is incorrectly coded.

**Observation on the raw handler:**

```c
static irqreturn_t panthor_job_irq_raw_handler(int irq, void *data)
{
	...
	panthor_job_irq_handler(pirq, gpu_read(pirq->iomem, INT_RAWSTAT));

	scoped_guard(spinlock_irqsave, &pirq->mask_lock) {
		if (pirq->state == PANTHOR_IRQ_STATE_PROCESSING)
			pirq->state = PANTHOR_IRQ_STATE_ACTIVE;
	}

	return IRQ_HANDLED;
```

The INT_MASK is never written back to `pirq->mask` here -- the handler reads `INT_RAWSTAT`, processes, and transitions back to ACTIVE without re-enabling the mask register. This works because in the raw handler path, `INT_MASK` was never cleared (unlike the threaded path where it's cleared to prevent re-entry). The handler is re-entrant through the spinlock. However, this means events can fire repeatedly while processing -- which is what the coalescing in patch 10 aims to mitigate.

---

---
Generated by Claude Code Patch Reviewer