From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: rust: add Work::disable_sync Date: Sat, 14 Mar 2026 07:15:46 +1000 Message-ID: In-Reply-To: <20260313091646.16938-3-work@onurozkan.dev> References: <20260313091646.16938-1-work@onurozkan.dev> <20260313091646.16938-3-work@onurozkan.dev> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Minor concern: should this really be `unsafe`?** ```rust + pub unsafe fn disable_sync(&self) { + let ptr: *const Self =3D self; + // SAFETY: `self` points to a valid initialized work. + let raw_work =3D unsafe { Self::raw_get(ptr) }; + // SAFETY: `raw_work` is a valid embedded `work_struct`. + unsafe { bindings::disable_work_sync(raw_work) }; + } ``` The safety doc says: ```rust + /// # Safety + /// + /// Must be called from a sleepable context if the work was last queue= d on a non-BH + /// workqueue. ``` This is reasonable =E2=80=94 `disable_work_sync()` can sleep, so calling it= from atomic context would be a bug. However, this pattern ("must be called= from sleepable context") is very common in kernel APIs and many Rust wrapp= ers handle it differently (e.g., via type-level guarantees or just making i= t safe with a documented precondition, since calling from atomic context is= already UB-adjacent behavior the kernel catches with `might_sleep()` debug= ging). Consider whether making this `unsafe` is the right abstraction level= =E2=80=94 the Rust workqueue `Queue` type's methods like `enqueue` are saf= e despite having their own context requirements. Also, the `# Safety` section is documenting a caller requirement but this i= s a method on `&self` =E2=80=94 the naming convention in kernel Rust is typ= ically to use `# Safety` for `unsafe` preconditions, which this does, but y= ou might also want a `# Panics` or general doc section clarifying when it's= valid to call. Minor style nit. **Missing C helper**: The patch adds a Rust wrapper for `disable_work_sync`= but doesn't add a corresponding C helper in `rust/helpers/workqueue.c`. Lo= oking at the binding, `bindings::disable_work_sync` would need to resolve t= o the C function. Since `disable_work_sync()` in `include/linux/workqueue.h= ` is a regular (non-inline, non-macro) extern function, this should work vi= a direct bindgen without a helper. This is fine. --- --- Generated by Claude Code Patch Reviewer