Claude review: drm/v3d: Using spin_lock_irqsave instead of spin_lock in the RT kernel

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

**Correctness bug: `v3d_stats_update(global_stats, now)` left unprotected**

The original code wraps *both* the `spin_lock`/`spin_unlock` region *and* the subsequent `v3d_stats_update(global_stats, now)` call inside the `preempt_disable()`/`local_irq_save()` scope:

```c
// original
preempt_disable();            // or local_irq_save
spin_lock(&queue->queue_lock);
  v3d_stats_update(&job->file_priv->stats[q], now);  // per-fd stats
spin_unlock(&queue->queue_lock);
v3d_stats_update(global_stats, now);                  // global stats -- still under preempt_disable
preempt_enable();             // or local_irq_restore
```

After the patch:

```c
spin_lock_irqsave(&queue->queue_lock, flags);
  v3d_stats_update(&job->file_priv->stats[q], now);
spin_unlock_irqrestore(&queue->queue_lock, flags);
v3d_stats_update(global_stats, now);   // <-- NO LONGER under any preemption/irq protection!
```

`v3d_stats_update()` calls `write_seqcount_begin()` which internally asserts that preemption is disabled (`lockdep_assert_preemption_disabled()`). After this patch, the `global_stats` update runs with preemption enabled and interrupts enabled, which will trigger a lockdep splat on non-RT kernels and is a correctness issue. The `global_stats` update must also be protected.

To fix this, the `v3d_stats_update(global_stats, now)` call should be moved *inside* the spin_lock_irqsave/spin_unlock_irqrestore region.

**Incomplete: `v3d_job_start_stats()` has the same pattern**

The function `v3d_job_start_stats()` at `v3d_sched.c:139` uses the identical `IS_ENABLED(CONFIG_LOCKDEP)` + `local_irq_save`/`preempt_disable` pattern to protect its `write_seqcount_begin/end` calls. On PREEMPT_RT, `v3d_job_start_stats()` will have the same class of issues. The commit message and patch only address `v3d_job_update_stats()`, leaving the sibling function unfixed.

**Commit message issues**

The commit message says:
> Disabling interrupts before calling 'spin_lock()' is unnecessary;
> its implementation is only for satisfying lockdep's requirements.

This is partially correct — the `local_irq_save` *is* for lockdep — but the `preempt_disable` is not just for lockdep. It's needed to satisfy `write_seqcount_begin()`'s requirement that preemption is disabled. The commit message conflates the two purposes.

**The `seqcount_t` type itself may be the real problem**

The lock is declared as a bare `seqcount_t` (`v3d_drv.h:50`). On PREEMPT_RT, using `seqcount_spinlock_t` (associated with the `queue_lock` spinlock) would be the more idiomatic fix. This would let lockdep understand the association between the seqcount and its protecting lock, eliminating the need for the `IS_ENABLED(CONFIG_LOCKDEP)` hack entirely, and would be RT-safe by design. The patch should consider whether converting to `seqcount_spinlock_t` is the right approach rather than just shuffling lock calls.

**Summary:** The patch introduces a regression by leaving `v3d_stats_update(global_stats, now)` outside any preemption-disabled region, is incomplete (doesn't fix `v3d_job_start_stats`), and doesn't address the underlying `seqcount_t` vs `seqcount_spinlock_t` design question. Needs a v2.

---
Generated by Claude Code Patch Reviewer