From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: Documentation: drm: Add entry for removing spsc_queue to TODO list Date: Thu, 04 Jun 2026 15:59:15 +1000 Message-ID: In-Reply-To: <20260529215207.115513-2-phasta@kernel.org> References: <20260529215207.115513-2-phasta@kernel.org> <20260529215207.115513-2-phasta@kernel.org> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Factual error =E2=80=94 `ACCESS_ONCE()` reference is wrong:** The patch states: ``` +This queue utilizes: + +- preempt_disable() +- atomic instructions +- memory barriers +- ACCESS_ONCE() ``` `ACCESS_ONCE()` was removed from the kernel years ago and replaced with `RE= AD_ONCE()`/`WRITE_ONCE()`. The current `spsc_queue.h` uses `READ_ONCE()` an= d `WRITE_ONCE()` exclusively. This should be corrected to `READ_ONCE() / WR= ITE_ONCE()`. **Minor: "has to take a lock in some situations" claim is misleading:** The patch says: ``` +Moreover, drm_sched_entity_push_job(), the only user of spsc_queue_push(),= has +to take a lock in some situations anyways and calls to it are often serial= ized +with a driver lock. ``` Looking at the current code, `drm_sched_entity_push_job()` itself does not = take any lock internally. Rather, its documentation states that callers sho= uld hold a "common lock" externally to guarantee ordering with `drm_sched_j= ob_arm()`. This is an important distinction =E2=80=94 the function relies o= n external serialization, but doesn't itself take any lock "in some situati= ons." The wording could be made more precise to say something like "calls t= o it are typically externally serialized by a driver lock" to better reflec= t reality. **Level classification:** The task is marked as "Beginner" level. This seems reasonable for the repla= cement itself, though verifying correctness with benchmarks (especially on = amdgpu) may push it toward Intermediate. The existing TODO entry immediatel= y above it (about locking in `drm_sched_rq`) is marked Intermediate for sim= ilar work, so there's a minor inconsistency, but this is a judgment call. **Structural nit =E2=80=94 contact formatting:** The existing TODO entries in the file use a single-line `Contact:` format l= ike: ``` Contact: Philipp Stanner ``` This patch uses a multi-line list format: ``` +Contact: + +- Philipp Stanner +- Christian K=C3=B6nig ``` The list format is actually fine since there are two contacts, but worth no= ting the slight style difference. No change needed. **Summary:** The patch is a reasonable TODO addition. The main actionable i= tem is correcting the `ACCESS_ONCE()` reference to `READ_ONCE() / WRITE_ONC= E()`, and optionally clarifying the locking claim about `drm_sched_entity_p= ush_job()`. --- Generated by Claude Code Patch Reviewer