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/v3d: Using spin_lock_irqsave instead of spin_lock in the RT kernel Date: Thu, 05 Mar 2026 13:49:50 +1000 Message-ID: In-Reply-To: <20260304031724.246404-1-xiaolei.wang@windriver.com> References: <20260304031724.246404-1-xiaolei.wang@windriver.com> <20260304031724.246404-1-xiaolei.wang@windriver.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 bug: `v3d_stats_update(global_stats, now)` left unprotected** The original code wraps *both* the `spin_lock`/`spin_unlock` region *and* t= he subsequent `v3d_stats_update(global_stats, now)` call inside the `preemp= t_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 -- st= ill 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 preemptio= n/irq protection! ``` `v3d_stats_update()` calls `write_seqcount_begin()` which internally assert= s that preemption is disabled (`lockdep_assert_preemption_disabled()`). Aft= er this patch, the `global_stats` update runs with preemption enabled and i= nterrupts 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 identica= l `IS_ENABLED(CONFIG_LOCKDEP)` + `local_irq_save`/`preempt_disable` pattern= to protect its `write_seqcount_begin/end` calls. On PREEMPT_RT, `v3d_job_s= tart_stats()` will have the same class of issues. The commit message and pa= tch only address `v3d_job_update_stats()`, leaving the sibling function unf= ixed. **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 =E2=80=94 the `local_irq_save` *is* for lockdep = =E2=80=94 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) w= ould be the more idiomatic fix. This would let lockdep understand the assoc= iation between the seqcount and its protecting lock, eliminating the need f= or 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 `se= qcount_t` vs `seqcount_spinlock_t` design question. Needs a v2. --- Generated by Claude Code Patch Reviewer