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: Prepare the scheduler logic for FW events in IRQ context
Date: Tue, 05 May 2026 11:44:53 +1000
Message-ID: <review-patch6-20260429-panthor-signal-from-irq-v1-6-4b92ae4142d2@collabora.com>
In-Reply-To: <20260429-panthor-signal-from-irq-v1-6-4b92ae4142d2@collabora.com>
References: <20260429-panthor-signal-from-irq-v1-0-4b92ae4142d2@collabora.com> <20260429-panthor-signal-from-irq-v1-6-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 issues that need fixing.**

This is the largest and most complex patch. It introduces `events_lock` (spinlock) to protect CSG slot access during event processing, moves fence signalling inline (removing the `sync_upd_work` and `fw_events_work` work items), and makes the fence signalling path IRQ-safe.

**Issue 1: `update_fdinfo_stats()` locking:**

```c
static void update_fdinfo_stats(struct panthor_job *job)
{
	...
	scoped_guard(spinlock, &group->fdinfo.lock) {
```

This uses `spinlock` (not `spinlock_irqsave`), but `update_fdinfo_stats()` is now called from `queue_check_job_completion()` -> `list_for_each_entry_safe(job, ...)` -> `update_fdinfo_stats()`. Since `queue_check_job_completion` itself can be called from `csg_slot_sync_update_locked` -> `group_check_job_completion`, which runs under `events_lock` (an irqsave spinlock), and ultimately from hard IRQ context (after patch 9), this `spinlock` guard will deadlock if `fdinfo.lock` is taken in process context and an interrupt fires. This should be `spinlock_irqsave`.

**Issue 2: Nested `dma_fence_begin_signalling`:**

```c
static void group_check_job_completion(struct panthor_group *group)
{
	bool cookie;
	...
	cookie = dma_fence_begin_signalling();
	for (queue_idx = 0; queue_idx < group->queue_count; queue_idx++) {
		...
		queue_check_job_completion(queue);
	}
	dma_fence_end_signalling(cookie);
}
```

But `queue_check_job_completion()` also calls `dma_fence_begin_signalling()` internally. This results in nested fence signalling critical sections, which will trigger lockdep annotations/warnings.

**Issue 3: Lock ordering documentation:**

The relationship between `scheduler->lock` (mutex), `events_lock` (spinlock), and `fence_ctx.lock` (spinlock) should be documented. The nesting is: `lock` -> `events_lock` -> `fence_ctx.lock`, and this ordering constraint should be explicit.

**Observation:** The conversion of `queue_suspend_timeout` and `queue_resume_timeout` to use `guard(spinlock_irqsave)` is correct and necessary since these can now be called from IRQ context via `queue_check_job_completion`.

---

---
Generated by Claude Code Patch Reviewer