[PATCH 0/2] rust: workqueue: remove HasWork::OFFSET

[PATCH 0/2] rust: workqueue: remove HasWork::OFFSET

[Tamir Duberstein]

The bulk of this change occurs in the latter commit, please see its
message for details. The first commit was developed in another series
not yet shared with the list but was picked into this series to allow
`HasWork::work_container_of` to return `container_of!` without the need
to cast from `*const Self` to `*mut Self`.

Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
Tamir Duberstein (2):
      rust: retain pointer mut-ness in `container_of!`
      rust: workqueue: remove HasWork::OFFSET

 rust/kernel/lib.rs       |  5 ++---
 rust/kernel/pci.rs       |  2 +-
 rust/kernel/platform.rs  |  2 +-
 rust/kernel/rbtree.rs    | 23 ++++++++++-------------
 rust/kernel/workqueue.rs | 45 ++++++++++++---------------------------------
 5 files changed, 26 insertions(+), 51 deletions(-)
---
base-commit: ff64846bee0e7e3e7bc9363ebad3bab42dd27e24
change-id: 20250307-no-offset-e463667a72fb

Best regards,
-- 
Tamir Duberstein <tamird@gmail.com>

[PATCH 1/2] rust: retain pointer mut-ness in `container_of!`

[Tamir Duberstein]

Avoid casting the input pointer to `*const _`, allowing the output
pointer to be `*mut` if the input is `*mut`. This allows a number of
`*const` to `*mut` conversions to be removed at the cost of slightly
worse ergonomics when the macro is used with a reference rather than a
pointer; the only example of this was in the macro's own doctest.

Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
 rust/kernel/lib.rs      |  5 ++---
 rust/kernel/pci.rs      |  2 +-
 rust/kernel/platform.rs |  2 +-
 rust/kernel/rbtree.rs   | 23 ++++++++++-------------
 4 files changed, 14 insertions(+), 18 deletions(-)

diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 7697c60b2d1a..9cd6b6864739 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -187,7 +187,7 @@ fn panic(info: &core::panic::PanicInfo<'_>) -> ! {
 /// }
 ///
 /// let test = Test { a: 10, b: 20 };
-/// let b_ptr = &test.b;
+/// let b_ptr: *const _ = &test.b;
 /// // SAFETY: The pointer points at the `b` field of a `Test`, so the resulting pointer will be
 /// // in-bounds of the same allocation as `b_ptr`.
 /// let test_alias = unsafe { container_of!(b_ptr, Test, b) };
@@ -196,9 +196,8 @@ fn panic(info: &core::panic::PanicInfo<'_>) -> ! {
 #[macro_export]
 macro_rules! container_of {
     ($ptr:expr, $type:ty, $($f:tt)*) => {{
-        let ptr = $ptr as *const _ as *const u8;
         let offset: usize = ::core::mem::offset_of!($type, $($f)*);
-        ptr.sub(offset) as *const $type
+        $ptr.byte_sub(offset).cast::<$type>()
     }}
 }
 
diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index 4c98b5b9aa1e..c37476576f02 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -364,7 +364,7 @@ pub unsafe fn from_dev(dev: ARef<device::Device>) -> Self {
     fn as_raw(&self) -> *mut bindings::pci_dev {
         // SAFETY: By the type invariant `self.0.as_raw` is a pointer to the `struct device`
         // embedded in `struct pci_dev`.
-        unsafe { container_of!(self.0.as_raw(), bindings::pci_dev, dev) as _ }
+        unsafe { container_of!(self.0.as_raw(), bindings::pci_dev, dev) }
     }
 
     /// Returns the PCI vendor ID.
diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
index 50e6b0421813..c51617569c01 100644
--- a/rust/kernel/platform.rs
+++ b/rust/kernel/platform.rs
@@ -189,7 +189,7 @@ unsafe fn from_dev(dev: ARef<device::Device>) -> Self {
     fn as_raw(&self) -> *mut bindings::platform_device {
         // SAFETY: By the type invariant `self.0.as_raw` is a pointer to the `struct device`
         // embedded in `struct platform_device`.
-        unsafe { container_of!(self.0.as_raw(), bindings::platform_device, dev) }.cast_mut()
+        unsafe { container_of!(self.0.as_raw(), bindings::platform_device, dev) }
     }
 }
 
diff --git a/rust/kernel/rbtree.rs b/rust/kernel/rbtree.rs
index 0d1e75810664..1fdea2806cfa 100644
--- a/rust/kernel/rbtree.rs
+++ b/rust/kernel/rbtree.rs
@@ -424,7 +424,7 @@ pub fn cursor_lower_bound(&mut self, key: &K) -> Option<Cursor<'_, K, V>>
         while !node.is_null() {
             // SAFETY: By the type invariant of `Self`, all non-null `rb_node` pointers stored in `self`
             // point to the links field of `Node<K, V>` objects.
-            let this = unsafe { container_of!(node, Node<K, V>, links) }.cast_mut();
+            let this = unsafe { container_of!(node, Node<K, V>, links) };
             // SAFETY: `this` is a non-null node so it is valid by the type invariants.
             let this_key = unsafe { &(*this).key };
             // SAFETY: `node` is a non-null node so it is valid by the type invariants.
@@ -496,7 +496,7 @@ fn drop(&mut self) {
             // but it is not observable. The loop invariant is still maintained.
 
             // SAFETY: `this` is valid per the loop invariant.
-            unsafe { drop(KBox::from_raw(this.cast_mut())) };
+            unsafe { drop(KBox::from_raw(this)) };
         }
     }
 }
@@ -761,7 +761,7 @@ pub fn remove_current(self) -> (Option<Self>, RBTreeNode<K, V>) {
         let next = self.get_neighbor_raw(Direction::Next);
         // SAFETY: By the type invariant of `Self`, all non-null `rb_node` pointers stored in `self`
         // point to the links field of `Node<K, V>` objects.
-        let this = unsafe { container_of!(self.current.as_ptr(), Node<K, V>, links) }.cast_mut();
+        let this = unsafe { container_of!(self.current.as_ptr(), Node<K, V>, links) };
         // SAFETY: `this` is valid by the type invariants as described above.
         let node = unsafe { KBox::from_raw(this) };
         let node = RBTreeNode { node };
@@ -806,7 +806,7 @@ fn remove_neighbor(&mut self, direction: Direction) -> Option<RBTreeNode<K, V>>
             unsafe { bindings::rb_erase(neighbor, addr_of_mut!(self.tree.root)) };
             // SAFETY: By the type invariant of `Self`, all non-null `rb_node` pointers stored in `self`
             // point to the links field of `Node<K, V>` objects.
-            let this = unsafe { container_of!(neighbor, Node<K, V>, links) }.cast_mut();
+            let this = unsafe { container_of!(neighbor, Node<K, V>, links) };
             // SAFETY: `this` is valid by the type invariants as described above.
             let node = unsafe { KBox::from_raw(this) };
             return Some(RBTreeNode { node });
@@ -912,7 +912,7 @@ unsafe fn to_key_value_mut<'b>(node: NonNull<bindings::rb_node>) -> (&'b K, &'b
     unsafe fn to_key_value_raw<'b>(node: NonNull<bindings::rb_node>) -> (&'b K, *mut V) {
         // SAFETY: By the type invariant of `Self`, all non-null `rb_node` pointers stored in `self`
         // point to the links field of `Node<K, V>` objects.
-        let this = unsafe { container_of!(node.as_ptr(), Node<K, V>, links) }.cast_mut();
+        let this = unsafe { container_of!(node.as_ptr(), Node<K, V>, links) };
         // SAFETY: The passed `node` is the current node or a non-null neighbor,
         // thus `this` is valid by the type invariants.
         let k = unsafe { &(*this).key };
@@ -1021,7 +1021,7 @@ fn next(&mut self) -> Option<Self::Item> {
 
         // SAFETY: By the type invariant of `IterRaw`, `self.next` is a valid node in an `RBTree`,
         // and by the type invariant of `RBTree`, all nodes point to the links field of `Node<K, V>` objects.
-        let cur = unsafe { container_of!(self.next, Node<K, V>, links) }.cast_mut();
+        let cur = unsafe { container_of!(self.next, Node<K, V>, links) };
 
         // SAFETY: `self.next` is a valid tree node by the type invariants.
         self.next = unsafe { bindings::rb_next(self.next) };
@@ -1216,7 +1216,7 @@ pub fn get_mut(&mut self) -> &mut V {
         // SAFETY:
         // - `self.node_links` is a valid pointer to a node in the tree.
         // - We have exclusive access to the underlying tree, and can thus give out a mutable reference.
-        unsafe { &mut (*(container_of!(self.node_links, Node<K, V>, links).cast_mut())).value }
+        unsafe { &mut (*(container_of!(self.node_links, Node<K, V>, links))).value }
     }
 
     /// Converts the entry into a mutable reference to its value.
@@ -1226,7 +1226,7 @@ pub fn into_mut(self) -> &'a mut V {
         // SAFETY:
         // - `self.node_links` is a valid pointer to a node in the tree.
         // - This consumes the `&'a mut RBTree<K, V>`, therefore it can give out a mutable reference that lives for `'a`.
-        unsafe { &mut (*(container_of!(self.node_links, Node<K, V>, links).cast_mut())).value }
+        unsafe { &mut (*(container_of!(self.node_links, Node<K, V>, links))).value }
     }
 
     /// Remove this entry from the [`RBTree`].
@@ -1239,9 +1239,7 @@ pub fn remove_node(self) -> RBTreeNode<K, V> {
         RBTreeNode {
             // SAFETY: The node was a node in the tree, but we removed it, so we can convert it
             // back into a box.
-            node: unsafe {
-                KBox::from_raw(container_of!(self.node_links, Node<K, V>, links).cast_mut())
-            },
+            node: unsafe { KBox::from_raw(container_of!(self.node_links, Node<K, V>, links)) },
         }
     }
 
@@ -1272,8 +1270,7 @@ fn replace(self, node: RBTreeNode<K, V>) -> RBTreeNode<K, V> {
         // SAFETY:
         // - `self.node_ptr` produces a valid pointer to a node in the tree.
         // - Now that we removed this entry from the tree, we can convert the node to a box.
-        let old_node =
-            unsafe { KBox::from_raw(container_of!(self.node_links, Node<K, V>, links).cast_mut()) };
+        let old_node = unsafe { KBox::from_raw(container_of!(self.node_links, Node<K, V>, links)) };
 
         RBTreeNode { node: old_node }
     }

-- 
2.48.1

Re: [PATCH 1/2] rust: retain pointer mut-ness in `container_of!`

[Benno Lossin]

On Fri Mar 7, 2025 at 10:58 PM CET, Tamir Duberstein wrote:
> Avoid casting the input pointer to `*const _`, allowing the output
> pointer to be `*mut` if the input is `*mut`. This allows a number of
> `*const` to `*mut` conversions to be removed at the cost of slightly
> worse ergonomics when the macro is used with a reference rather than a
> pointer; the only example of this was in the macro's own doctest.
>
> Signed-off-by: Tamir Duberstein <tamird@gmail.com>

Reviewed-by: Benno Lossin <benno.lossin@proton.me>

---
Cheers,
Benno

> ---
>  rust/kernel/lib.rs      |  5 ++---
>  rust/kernel/pci.rs      |  2 +-
>  rust/kernel/platform.rs |  2 +-
>  rust/kernel/rbtree.rs   | 23 ++++++++++-------------
>  4 files changed, 14 insertions(+), 18 deletions(-)

Re: [PATCH 1/2] rust: retain pointer mut-ness in `container_of!`

[Alice Ryhl]

On Fri, Mar 07, 2025 at 04:58:48PM -0500, Tamir Duberstein wrote:
> Avoid casting the input pointer to `*const _`, allowing the output
> pointer to be `*mut` if the input is `*mut`. This allows a number of
> `*const` to `*mut` conversions to be removed at the cost of slightly
> worse ergonomics when the macro is used with a reference rather than a
> pointer; the only example of this was in the macro's own doctest.
> 
> Signed-off-by: Tamir Duberstein <tamird@gmail.com>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

[PATCH 2/2] rust: workqueue: remove HasWork::OFFSET

[Tamir Duberstein]

Implement `HasWork::work_container_of` in `impl_has_work!`, narrowing
the interface of `HasWork` and replacing pointer arithmetic with
`container_of!`. Remove the provided implementation of
`HasWork::get_work_offset` without replacement; an implementation is
already generated in `impl_has_work!`. Remove the `Self: Sized` bound on
`HasWork::work_container_of` which was apparently necessary to access
`OFFSET` as `OFFSET` no longer exists.

A similar API change was discussed on the hrtimer series[1].

Link: https://lore.kernel.org/all/20250224-hrtimer-v3-v6-12-rc2-v9-1-5bd3bf0ce6cc@kernel.org/ [1]
Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
 rust/kernel/workqueue.rs | 45 ++++++++++++---------------------------------
 1 file changed, 12 insertions(+), 33 deletions(-)

diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
index 0cd100d2aefb..0e2e0ecc58a6 100644
--- a/rust/kernel/workqueue.rs
+++ b/rust/kernel/workqueue.rs
@@ -429,51 +429,23 @@ pub unsafe fn raw_get(ptr: *const Self) -> *mut bindings::work_struct {
 ///
 /// # Safety
 ///
-/// The [`OFFSET`] constant must be the offset of a field in `Self` of type [`Work<T, ID>`]. The
-/// methods on this trait must have exactly the behavior that the definitions given below have.
+/// The methods on this trait must have exactly the behavior that the definitions given below have.
 ///
 /// [`impl_has_work!`]: crate::impl_has_work
-/// [`OFFSET`]: HasWork::OFFSET
 pub unsafe trait HasWork<T, const ID: u64 = 0> {
-    /// The offset of the [`Work<T, ID>`] field.
-    const OFFSET: usize;
-
-    /// Returns the offset of the [`Work<T, ID>`] field.
-    ///
-    /// This method exists because the [`OFFSET`] constant cannot be accessed if the type is not
-    /// [`Sized`].
-    ///
-    /// [`OFFSET`]: HasWork::OFFSET
-    #[inline]
-    fn get_work_offset(&self) -> usize {
-        Self::OFFSET
-    }
-
     /// Returns a pointer to the [`Work<T, ID>`] field.
     ///
     /// # Safety
     ///
     /// The provided pointer must point at a valid struct of type `Self`.
-    #[inline]
-    unsafe fn raw_get_work(ptr: *mut Self) -> *mut Work<T, ID> {
-        // SAFETY: The caller promises that the pointer is valid.
-        unsafe { (ptr as *mut u8).add(Self::OFFSET) as *mut Work<T, ID> }
-    }
+    unsafe fn raw_get_work(ptr: *mut Self) -> *mut Work<T, ID>;
 
     /// Returns a pointer to the struct containing the [`Work<T, ID>`] field.
     ///
     /// # Safety
     ///
     /// The pointer must point at a [`Work<T, ID>`] field in a struct of type `Self`.
-    #[inline]
-    unsafe fn work_container_of(ptr: *mut Work<T, ID>) -> *mut Self
-    where
-        Self: Sized,
-    {
-        // SAFETY: The caller promises that the pointer points at a field of the right type in the
-        // right kind of struct.
-        unsafe { (ptr as *mut u8).sub(Self::OFFSET) as *mut Self }
-    }
+    unsafe fn work_container_of(ptr: *mut Work<T, ID>) -> *mut Self;
 }
 
 /// Used to safely implement the [`HasWork<T, ID>`] trait.
@@ -504,8 +476,6 @@ macro_rules! impl_has_work {
         // SAFETY: The implementation of `raw_get_work` only compiles if the field has the right
         // type.
         unsafe impl$(<$($generics)+>)? $crate::workqueue::HasWork<$work_type $(, $id)?> for $self {
-            const OFFSET: usize = ::core::mem::offset_of!(Self, $field) as usize;
-
             #[inline]
             unsafe fn raw_get_work(ptr: *mut Self) -> *mut $crate::workqueue::Work<$work_type $(, $id)?> {
                 // SAFETY: The caller promises that the pointer is not dangling.
@@ -513,6 +483,15 @@ unsafe fn raw_get_work(ptr: *mut Self) -> *mut $crate::workqueue::Work<$work_typ
                     ::core::ptr::addr_of_mut!((*ptr).$field)
                 }
             }
+
+            #[inline]
+            unsafe fn work_container_of(
+                ptr: *mut $crate::workqueue::Work<$work_type $(, $id)?>,
+            ) -> *mut Self {
+                // SAFETY: The caller promises that the pointer points at a field of the right type
+                // in the right kind of struct.
+                unsafe { $crate::container_of!(ptr, Self, $field) }
+            }
         }
     )*};
 }

-- 
2.48.1

Re: [PATCH 2/2] rust: workqueue: remove HasWork::OFFSET

[Benno Lossin]

On Fri Mar 7, 2025 at 10:58 PM CET, Tamir Duberstein wrote:
> Implement `HasWork::work_container_of` in `impl_has_work!`, narrowing
> the interface of `HasWork` and replacing pointer arithmetic with
> `container_of!`. Remove the provided implementation of
> `HasWork::get_work_offset` without replacement; an implementation is
> already generated in `impl_has_work!`. Remove the `Self: Sized` bound on
> `HasWork::work_container_of` which was apparently necessary to access
> `OFFSET` as `OFFSET` no longer exists.
>
> A similar API change was discussed on the hrtimer series[1].
>
> Link: https://lore.kernel.org/all/20250224-hrtimer-v3-v6-12-rc2-v9-1-5bd3bf0ce6cc@kernel.org/ [1]
> Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> ---
>  rust/kernel/workqueue.rs | 45 ++++++++++++---------------------------------
>  1 file changed, 12 insertions(+), 33 deletions(-)

What is the motivation of this change? I didn't follow the discussion,
so if you explained it there, it would be nice if you could also add it
to this commit message.

> diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
> index 0cd100d2aefb..0e2e0ecc58a6 100644
> --- a/rust/kernel/workqueue.rs
> +++ b/rust/kernel/workqueue.rs
> @@ -429,51 +429,23 @@ pub unsafe fn raw_get(ptr: *const Self) -> *mut bindings::work_struct {
>  ///
>  /// # Safety
>  ///
> -/// The [`OFFSET`] constant must be the offset of a field in `Self` of type [`Work<T, ID>`]. The
> -/// methods on this trait must have exactly the behavior that the definitions given below have.
> +/// The methods on this trait must have exactly the behavior that the definitions given below have.
>  ///
>  /// [`impl_has_work!`]: crate::impl_has_work
> -/// [`OFFSET`]: HasWork::OFFSET
>  pub unsafe trait HasWork<T, const ID: u64 = 0> {
> -    /// The offset of the [`Work<T, ID>`] field.
> -    const OFFSET: usize;
> -
> -    /// Returns the offset of the [`Work<T, ID>`] field.
> -    ///
> -    /// This method exists because the [`OFFSET`] constant cannot be accessed if the type is not
> -    /// [`Sized`].
> -    ///
> -    /// [`OFFSET`]: HasWork::OFFSET
> -    #[inline]
> -    fn get_work_offset(&self) -> usize {
> -        Self::OFFSET
> -    }
> -
>      /// Returns a pointer to the [`Work<T, ID>`] field.
>      ///
>      /// # Safety
>      ///
>      /// The provided pointer must point at a valid struct of type `Self`.
> -    #[inline]
> -    unsafe fn raw_get_work(ptr: *mut Self) -> *mut Work<T, ID> {
> -        // SAFETY: The caller promises that the pointer is valid.
> -        unsafe { (ptr as *mut u8).add(Self::OFFSET) as *mut Work<T, ID> }
> -    }
> +    unsafe fn raw_get_work(ptr: *mut Self) -> *mut Work<T, ID>;
>  
>      /// Returns a pointer to the struct containing the [`Work<T, ID>`] field.
>      ///
>      /// # Safety
>      ///
>      /// The pointer must point at a [`Work<T, ID>`] field in a struct of type `Self`.
> -    #[inline]
> -    unsafe fn work_container_of(ptr: *mut Work<T, ID>) -> *mut Self
> -    where
> -        Self: Sized,

This bound is required in order to allow the usage of `dyn HasWork` (ie
object safety), so it should stay.

Maybe add a comment explaining why it's there.

---
Cheers,
Benno

> -    {
> -        // SAFETY: The caller promises that the pointer points at a field of the right type in the
> -        // right kind of struct.
> -        unsafe { (ptr as *mut u8).sub(Self::OFFSET) as *mut Self }
> -    }
> +    unsafe fn work_container_of(ptr: *mut Work<T, ID>) -> *mut Self;
>  }
>  
>  /// Used to safely implement the [`HasWork<T, ID>`] trait.
> @@ -504,8 +476,6 @@ macro_rules! impl_has_work {
>          // SAFETY: The implementation of `raw_get_work` only compiles if the field has the right
>          // type.
>          unsafe impl$(<$($generics)+>)? $crate::workqueue::HasWork<$work_type $(, $id)?> for $self {
> -            const OFFSET: usize = ::core::mem::offset_of!(Self, $field) as usize;
> -
>              #[inline]
>              unsafe fn raw_get_work(ptr: *mut Self) -> *mut $crate::workqueue::Work<$work_type $(, $id)?> {
>                  // SAFETY: The caller promises that the pointer is not dangling.
> @@ -513,6 +483,15 @@ unsafe fn raw_get_work(ptr: *mut Self) -> *mut $crate::workqueue::Work<$work_typ
>                      ::core::ptr::addr_of_mut!((*ptr).$field)
>                  }
>              }
> +
> +            #[inline]
> +            unsafe fn work_container_of(
> +                ptr: *mut $crate::workqueue::Work<$work_type $(, $id)?>,
> +            ) -> *mut Self {
> +                // SAFETY: The caller promises that the pointer points at a field of the right type
> +                // in the right kind of struct.
> +                unsafe { $crate::container_of!(ptr, Self, $field) }
> +            }
>          }
>      )*};
>  }

Re: [PATCH 2/2] rust: workqueue: remove HasWork::OFFSET

[Tamir Duberstein]

On Fri, Mar 14, 2025 at 3:20 PM Benno Lossin <benno.lossin@proton.me> wrote:
>
> On Fri Mar 7, 2025 at 10:58 PM CET, Tamir Duberstein wrote:
> > Implement `HasWork::work_container_of` in `impl_has_work!`, narrowing
> > the interface of `HasWork` and replacing pointer arithmetic with
> > `container_of!`. Remove the provided implementation of
> > `HasWork::get_work_offset` without replacement; an implementation is
> > already generated in `impl_has_work!`. Remove the `Self: Sized` bound on
> > `HasWork::work_container_of` which was apparently necessary to access
> > `OFFSET` as `OFFSET` no longer exists.
> >
> > A similar API change was discussed on the hrtimer series[1].
> >
> > Link: https://lore.kernel.org/all/20250224-hrtimer-v3-v6-12-rc2-v9-1-5bd3bf0ce6cc@kernel.org/ [1]
> > Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> > ---
> >  rust/kernel/workqueue.rs | 45 ++++++++++++---------------------------------
> >  1 file changed, 12 insertions(+), 33 deletions(-)
>
> What is the motivation of this change? I didn't follow the discussion,
> so if you explained it there, it would be nice if you could also add it
> to this commit message.

The motivation is right at the top: it narrows the interface and
replaces pointer arithmetic with an existing macro, and then deletes
unnecessary code.

> > diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
> > index 0cd100d2aefb..0e2e0ecc58a6 100644
> > --- a/rust/kernel/workqueue.rs
> > +++ b/rust/kernel/workqueue.rs
> > @@ -429,51 +429,23 @@ pub unsafe fn raw_get(ptr: *const Self) -> *mut bindings::work_struct {
> >  ///
> >  /// # Safety
> >  ///
> > -/// The [`OFFSET`] constant must be the offset of a field in `Self` of type [`Work<T, ID>`]. The
> > -/// methods on this trait must have exactly the behavior that the definitions given below have.
> > +/// The methods on this trait must have exactly the behavior that the definitions given below have.
> >  ///
> >  /// [`impl_has_work!`]: crate::impl_has_work
> > -/// [`OFFSET`]: HasWork::OFFSET
> >  pub unsafe trait HasWork<T, const ID: u64 = 0> {
> > -    /// The offset of the [`Work<T, ID>`] field.
> > -    const OFFSET: usize;
> > -
> > -    /// Returns the offset of the [`Work<T, ID>`] field.
> > -    ///
> > -    /// This method exists because the [`OFFSET`] constant cannot be accessed if the type is not
> > -    /// [`Sized`].
> > -    ///
> > -    /// [`OFFSET`]: HasWork::OFFSET
> > -    #[inline]
> > -    fn get_work_offset(&self) -> usize {
> > -        Self::OFFSET
> > -    }
> > -
> >      /// Returns a pointer to the [`Work<T, ID>`] field.
> >      ///
> >      /// # Safety
> >      ///
> >      /// The provided pointer must point at a valid struct of type `Self`.
> > -    #[inline]
> > -    unsafe fn raw_get_work(ptr: *mut Self) -> *mut Work<T, ID> {
> > -        // SAFETY: The caller promises that the pointer is valid.
> > -        unsafe { (ptr as *mut u8).add(Self::OFFSET) as *mut Work<T, ID> }
> > -    }
> > +    unsafe fn raw_get_work(ptr: *mut Self) -> *mut Work<T, ID>;
> >
> >      /// Returns a pointer to the struct containing the [`Work<T, ID>`] field.
> >      ///
> >      /// # Safety
> >      ///
> >      /// The pointer must point at a [`Work<T, ID>`] field in a struct of type `Self`.
> > -    #[inline]
> > -    unsafe fn work_container_of(ptr: *mut Work<T, ID>) -> *mut Self
> > -    where
> > -        Self: Sized,
>
> This bound is required in order to allow the usage of `dyn HasWork` (ie
> object safety), so it should stay.
>
> Maybe add a comment explaining why it's there.

I guess a doctest would be better, but I still don't understand why
the bound is needed. Sorry, can you cite something or explain in more
detail please?

Re: [PATCH 2/2] rust: workqueue: remove HasWork::OFFSET

[Benno Lossin]

On Fri Mar 14, 2025 at 9:44 PM CET, Tamir Duberstein wrote:
> On Fri, Mar 14, 2025 at 3:20 PM Benno Lossin <benno.lossin@proton.me> wrote:
>>
>> On Fri Mar 7, 2025 at 10:58 PM CET, Tamir Duberstein wrote:
>> > Implement `HasWork::work_container_of` in `impl_has_work!`, narrowing
>> > the interface of `HasWork` and replacing pointer arithmetic with
>> > `container_of!`. Remove the provided implementation of
>> > `HasWork::get_work_offset` without replacement; an implementation is
>> > already generated in `impl_has_work!`. Remove the `Self: Sized` bound on
>> > `HasWork::work_container_of` which was apparently necessary to access
>> > `OFFSET` as `OFFSET` no longer exists.
>> >
>> > A similar API change was discussed on the hrtimer series[1].
>> >
>> > Link: https://lore.kernel.org/all/20250224-hrtimer-v3-v6-12-rc2-v9-1-5bd3bf0ce6cc@kernel.org/ [1]
>> > Signed-off-by: Tamir Duberstein <tamird@gmail.com>
>> > ---
>> >  rust/kernel/workqueue.rs | 45 ++++++++++++---------------------------------
>> >  1 file changed, 12 insertions(+), 33 deletions(-)
>>
>> What is the motivation of this change? I didn't follow the discussion,
>> so if you explained it there, it would be nice if you could also add it
>> to this commit message.
>
> The motivation is right at the top: it narrows the interface and
> replaces pointer arithmetic with an existing macro, and then deletes
> unnecessary code.
>
>> > diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
>> > index 0cd100d2aefb..0e2e0ecc58a6 100644
>> > --- a/rust/kernel/workqueue.rs
>> > +++ b/rust/kernel/workqueue.rs
>> > @@ -429,51 +429,23 @@ pub unsafe fn raw_get(ptr: *const Self) -> *mut bindings::work_struct {
>> >  ///
>> >  /// # Safety
>> >  ///
>> > -/// The [`OFFSET`] constant must be the offset of a field in `Self` of type [`Work<T, ID>`]. The
>> > -/// methods on this trait must have exactly the behavior that the definitions given below have.
>> > +/// The methods on this trait must have exactly the behavior that the definitions given below have.
>> >  ///
>> >  /// [`impl_has_work!`]: crate::impl_has_work
>> > -/// [`OFFSET`]: HasWork::OFFSET
>> >  pub unsafe trait HasWork<T, const ID: u64 = 0> {
>> > -    /// The offset of the [`Work<T, ID>`] field.
>> > -    const OFFSET: usize;
>> > -
>> > -    /// Returns the offset of the [`Work<T, ID>`] field.
>> > -    ///
>> > -    /// This method exists because the [`OFFSET`] constant cannot be accessed if the type is not
>> > -    /// [`Sized`].
>> > -    ///
>> > -    /// [`OFFSET`]: HasWork::OFFSET
>> > -    #[inline]
>> > -    fn get_work_offset(&self) -> usize {
>> > -        Self::OFFSET
>> > -    }
>> > -
>> >      /// Returns a pointer to the [`Work<T, ID>`] field.
>> >      ///
>> >      /// # Safety
>> >      ///
>> >      /// The provided pointer must point at a valid struct of type `Self`.
>> > -    #[inline]
>> > -    unsafe fn raw_get_work(ptr: *mut Self) -> *mut Work<T, ID> {
>> > -        // SAFETY: The caller promises that the pointer is valid.
>> > -        unsafe { (ptr as *mut u8).add(Self::OFFSET) as *mut Work<T, ID> }
>> > -    }
>> > +    unsafe fn raw_get_work(ptr: *mut Self) -> *mut Work<T, ID>;
>> >
>> >      /// Returns a pointer to the struct containing the [`Work<T, ID>`] field.
>> >      ///
>> >      /// # Safety
>> >      ///
>> >      /// The pointer must point at a [`Work<T, ID>`] field in a struct of type `Self`.
>> > -    #[inline]
>> > -    unsafe fn work_container_of(ptr: *mut Work<T, ID>) -> *mut Self
>> > -    where
>> > -        Self: Sized,
>>
>> This bound is required in order to allow the usage of `dyn HasWork` (ie
>> object safety), so it should stay.
>>
>> Maybe add a comment explaining why it's there.
>
> I guess a doctest would be better, but I still don't understand why
> the bound is needed. Sorry, can you cite something or explain in more
> detail please?

Here is a link: https://doc.rust-lang.org/reference/items/traits.html#dyn-compatibility

But I realized that the trait wasn't object safe to begin with due to
the `OFFSET` associated constant. So I'm not sure we need this. Alice,
do you need `dyn HasWork`?

---
Cheers,
Benno

Re: [PATCH 2/2] rust: workqueue: remove HasWork::OFFSET

[Tamir Duberstein]

On Sat, Mar 15, 2025 at 5:30 AM Benno Lossin <benno.lossin@proton.me> wrote:
>
> On Fri Mar 14, 2025 at 9:44 PM CET, Tamir Duberstein wrote:
> > On Fri, Mar 14, 2025 at 3:20 PM Benno Lossin <benno.lossin@proton.me> wrote:
> >>
> >> On Fri Mar 7, 2025 at 10:58 PM CET, Tamir Duberstein wrote:
> >> > Implement `HasWork::work_container_of` in `impl_has_work!`, narrowing
> >> > the interface of `HasWork` and replacing pointer arithmetic with
> >> > `container_of!`. Remove the provided implementation of
> >> > `HasWork::get_work_offset` without replacement; an implementation is
> >> > already generated in `impl_has_work!`. Remove the `Self: Sized` bound on
> >> > `HasWork::work_container_of` which was apparently necessary to access
> >> > `OFFSET` as `OFFSET` no longer exists.
> >> >
> >> > A similar API change was discussed on the hrtimer series[1].
> >> >
> >> > Link: https://lore.kernel.org/all/20250224-hrtimer-v3-v6-12-rc2-v9-1-5bd3bf0ce6cc@kernel.org/ [1]
> >> > Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> >> > ---
> >> >  rust/kernel/workqueue.rs | 45 ++++++++++++---------------------------------
> >> >  1 file changed, 12 insertions(+), 33 deletions(-)
> >>
> >> What is the motivation of this change? I didn't follow the discussion,
> >> so if you explained it there, it would be nice if you could also add it
> >> to this commit message.
> >
> > The motivation is right at the top: it narrows the interface and
> > replaces pointer arithmetic with an existing macro, and then deletes
> > unnecessary code.
> >
> >> > diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
> >> > index 0cd100d2aefb..0e2e0ecc58a6 100644
> >> > --- a/rust/kernel/workqueue.rs
> >> > +++ b/rust/kernel/workqueue.rs
> >> > @@ -429,51 +429,23 @@ pub unsafe fn raw_get(ptr: *const Self) -> *mut bindings::work_struct {
> >> >  ///
> >> >  /// # Safety
> >> >  ///
> >> > -/// The [`OFFSET`] constant must be the offset of a field in `Self` of type [`Work<T, ID>`]. The
> >> > -/// methods on this trait must have exactly the behavior that the definitions given below have.
> >> > +/// The methods on this trait must have exactly the behavior that the definitions given below have.
> >> >  ///
> >> >  /// [`impl_has_work!`]: crate::impl_has_work
> >> > -/// [`OFFSET`]: HasWork::OFFSET
> >> >  pub unsafe trait HasWork<T, const ID: u64 = 0> {
> >> > -    /// The offset of the [`Work<T, ID>`] field.
> >> > -    const OFFSET: usize;
> >> > -
> >> > -    /// Returns the offset of the [`Work<T, ID>`] field.
> >> > -    ///
> >> > -    /// This method exists because the [`OFFSET`] constant cannot be accessed if the type is not
> >> > -    /// [`Sized`].
> >> > -    ///
> >> > -    /// [`OFFSET`]: HasWork::OFFSET
> >> > -    #[inline]
> >> > -    fn get_work_offset(&self) -> usize {
> >> > -        Self::OFFSET
> >> > -    }
> >> > -
> >> >      /// Returns a pointer to the [`Work<T, ID>`] field.
> >> >      ///
> >> >      /// # Safety
> >> >      ///
> >> >      /// The provided pointer must point at a valid struct of type `Self`.
> >> > -    #[inline]
> >> > -    unsafe fn raw_get_work(ptr: *mut Self) -> *mut Work<T, ID> {
> >> > -        // SAFETY: The caller promises that the pointer is valid.
> >> > -        unsafe { (ptr as *mut u8).add(Self::OFFSET) as *mut Work<T, ID> }
> >> > -    }
> >> > +    unsafe fn raw_get_work(ptr: *mut Self) -> *mut Work<T, ID>;
> >> >
> >> >      /// Returns a pointer to the struct containing the [`Work<T, ID>`] field.
> >> >      ///
> >> >      /// # Safety
> >> >      ///
> >> >      /// The pointer must point at a [`Work<T, ID>`] field in a struct of type `Self`.
> >> > -    #[inline]
> >> > -    unsafe fn work_container_of(ptr: *mut Work<T, ID>) -> *mut Self
> >> > -    where
> >> > -        Self: Sized,
> >>
> >> This bound is required in order to allow the usage of `dyn HasWork` (ie
> >> object safety), so it should stay.
> >>
> >> Maybe add a comment explaining why it's there.
> >
> > I guess a doctest would be better, but I still don't understand why
> > the bound is needed. Sorry, can you cite something or explain in more
> > detail please?
>
> Here is a link: https://doc.rust-lang.org/reference/items/traits.html#dyn-compatibility
>
> But I realized that the trait wasn't object safe to begin with due to
> the `OFFSET` associated constant. So I'm not sure we need this. Alice,
> do you need `dyn HasWork`?

I wrote a simple test:

diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
index 0e2e0ecc58a6..4f2dd2c1ebcb 100644
--- a/rust/kernel/workqueue.rs
+++ b/rust/kernel/workqueue.rs
@@ -448,6 +448,11 @@ pub unsafe trait HasWork<T, const ID: u64 = 0> {
     unsafe fn work_container_of(ptr: *mut Work<T, ID>) -> *mut Self;
 }

+fn has_work_object_safe<T: HasWork<T>>(has_work: T) {
+    fn _assert_object_safe(_: &dyn HasWork<()>) {}
+    _assert_object_safe(&has_work);
+}
+
 /// Used to safely implement the [`HasWork<T, ID>`] trait.
 ///
 /// # Examples

`HasWork` is not object-safe even before this patch:

> error[E0038]: the trait `workqueue::HasWork` cannot be made into an object
>    --> ../rust/kernel/workqueue.rs:481:25
>     |
> 481 |     _assert_object_safe(&has_work);
>     |                         ^^^^^^^^^ `workqueue::HasWork` cannot be made into an object
>     |
> note: for a trait to be "dyn-compatible" it needs to allow building a vtable to allow the call to be resolvable dynamically; for more information visit <https://doc.rust-lang.org/reference/items/traits.html#object-safety>
>    --> ../rust/kernel/workqueue.rs:439:11
>     |
> 437 | pub unsafe trait HasWork<T, const ID: u64 = 0> {
>     |                  ------- this trait cannot be made into an object...
> 438 |     /// The offset of the [`Work<T, ID>`] field.
> 439 |     const OFFSET: usize;
>     |           ^^^^^^ ...because it contains this associated `const`
> ...
> 458 |     unsafe fn raw_get_work(ptr: *mut Self) -> *mut Work<T, ID> {
>     |               ^^^^^^^^^^^^ ...because associated function `raw_get_work` has no `self` parameter
>     = help: consider moving `OFFSET` to another trait
>     = help: only type `workqueue::ClosureWork<T>` is seen to implement the trait in this crate, consider using it directly instead
>     = note: `workqueue::HasWork` can be implemented in other crates; if you want to support your users passing their own types here, you can't refer to a specific type
> help: consider turning `raw_get_work` into a method by giving it a `&self` argument
>     |
> 458 |     unsafe fn raw_get_work(&self, ptr: *mut Self) -> *mut Work<T, ID> {
>     |                            ++++++
> help: alternatively, consider constraining `raw_get_work` so it does not apply to trait objects
>     |
> 458 |     unsafe fn raw_get_work(ptr: *mut Self) -> *mut Work<T, ID> where Self: Sized {
>     |                                                                +++++++++++++++++
>
> error: aborting due to 3 previous errors

so I don't think adding the Sized bound makes sense - we'd end up
adding it on every item in the trait.

Re: [PATCH 2/2] rust: workqueue: remove HasWork::OFFSET

[Benno Lossin]

On Sat Mar 15, 2025 at 4:37 PM CET, Tamir Duberstein wrote:
> On Sat, Mar 15, 2025 at 5:30 AM Benno Lossin <benno.lossin@proton.me> wrote:
>>
>> On Fri Mar 14, 2025 at 9:44 PM CET, Tamir Duberstein wrote:
>> > On Fri, Mar 14, 2025 at 3:20 PM Benno Lossin <benno.lossin@proton.me> wrote:
>> >>
>> >> On Fri Mar 7, 2025 at 10:58 PM CET, Tamir Duberstein wrote:
>> >> >      /// Returns a pointer to the struct containing the [`Work<T, ID>`] field.
>> >> >      ///
>> >> >      /// # Safety
>> >> >      ///
>> >> >      /// The pointer must point at a [`Work<T, ID>`] field in a struct of type `Self`.
>> >> > -    #[inline]
>> >> > -    unsafe fn work_container_of(ptr: *mut Work<T, ID>) -> *mut Self
>> >> > -    where
>> >> > -        Self: Sized,
>> >>
>> >> This bound is required in order to allow the usage of `dyn HasWork` (ie
>> >> object safety), so it should stay.
>> >>
>> >> Maybe add a comment explaining why it's there.
>> >
>> > I guess a doctest would be better, but I still don't understand why
>> > the bound is needed. Sorry, can you cite something or explain in more
>> > detail please?
>>
>> Here is a link: https://doc.rust-lang.org/reference/items/traits.html#dyn-compatibility
>>
>> But I realized that the trait wasn't object safe to begin with due to
>> the `OFFSET` associated constant. So I'm not sure we need this. Alice,
>> do you need `dyn HasWork`?
>
> I wrote a simple test:

[...]

> so I don't think adding the Sized bound makes sense - we'd end up
> adding it on every item in the trait.

Yeah the `Sized` bound was probably to make the cast work, so let's
remove it.

---
Cheers,
Benno

Re: [PATCH 2/2] rust: workqueue: remove HasWork::OFFSET

[Tamir Duberstein]

On Sat, Mar 15, 2025 at 2:06 PM Benno Lossin <benno.lossin@proton.me> wrote:
>
> On Sat Mar 15, 2025 at 4:37 PM CET, Tamir Duberstein wrote:
> > On Sat, Mar 15, 2025 at 5:30 AM Benno Lossin <benno.lossin@proton.me> wrote:
> >>
> >> On Fri Mar 14, 2025 at 9:44 PM CET, Tamir Duberstein wrote:
> >> > On Fri, Mar 14, 2025 at 3:20 PM Benno Lossin <benno.lossin@proton.me> wrote:
> >> >>
> >> >> On Fri Mar 7, 2025 at 10:58 PM CET, Tamir Duberstein wrote:
> >> >> >      /// Returns a pointer to the struct containing the [`Work<T, ID>`] field.
> >> >> >      ///
> >> >> >      /// # Safety
> >> >> >      ///
> >> >> >      /// The pointer must point at a [`Work<T, ID>`] field in a struct of type `Self`.
> >> >> > -    #[inline]
> >> >> > -    unsafe fn work_container_of(ptr: *mut Work<T, ID>) -> *mut Self
> >> >> > -    where
> >> >> > -        Self: Sized,
> >> >>
> >> >> This bound is required in order to allow the usage of `dyn HasWork` (ie
> >> >> object safety), so it should stay.
> >> >>
> >> >> Maybe add a comment explaining why it's there.
> >> >
> >> > I guess a doctest would be better, but I still don't understand why
> >> > the bound is needed. Sorry, can you cite something or explain in more
> >> > detail please?
> >>
> >> Here is a link: https://doc.rust-lang.org/reference/items/traits.html#dyn-compatibility
> >>
> >> But I realized that the trait wasn't object safe to begin with due to
> >> the `OFFSET` associated constant. So I'm not sure we need this. Alice,
> >> do you need `dyn HasWork`?
> >
> > I wrote a simple test:
>
> [...]
>
> > so I don't think adding the Sized bound makes sense - we'd end up
> > adding it on every item in the trait.
>
> Yeah the `Sized` bound was probably to make the cast work, so let's
> remove it.

It's already removed, right?

Re: [PATCH 2/2] rust: workqueue: remove HasWork::OFFSET

[Tamir Duberstein]

On Sat, Mar 15, 2025 at 2:12 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> On Sat, Mar 15, 2025 at 2:06 PM Benno Lossin <benno.lossin@proton.me> wrote:
> >
> > On Sat Mar 15, 2025 at 4:37 PM CET, Tamir Duberstein wrote:
> > > On Sat, Mar 15, 2025 at 5:30 AM Benno Lossin <benno.lossin@proton.me> wrote:
> > >>
> > >> On Fri Mar 14, 2025 at 9:44 PM CET, Tamir Duberstein wrote:
> > >> > On Fri, Mar 14, 2025 at 3:20 PM Benno Lossin <benno.lossin@proton.me> wrote:
> > >> >>
> > >> >> On Fri Mar 7, 2025 at 10:58 PM CET, Tamir Duberstein wrote:
> > >> >> >      /// Returns a pointer to the struct containing the [`Work<T, ID>`] field.
> > >> >> >      ///
> > >> >> >      /// # Safety
> > >> >> >      ///
> > >> >> >      /// The pointer must point at a [`Work<T, ID>`] field in a struct of type `Self`.
> > >> >> > -    #[inline]
> > >> >> > -    unsafe fn work_container_of(ptr: *mut Work<T, ID>) -> *mut Self
> > >> >> > -    where
> > >> >> > -        Self: Sized,
> > >> >>
> > >> >> This bound is required in order to allow the usage of `dyn HasWork` (ie
> > >> >> object safety), so it should stay.
> > >> >>
> > >> >> Maybe add a comment explaining why it's there.
> > >> >
> > >> > I guess a doctest would be better, but I still don't understand why
> > >> > the bound is needed. Sorry, can you cite something or explain in more
> > >> > detail please?
> > >>
> > >> Here is a link: https://doc.rust-lang.org/reference/items/traits.html#dyn-compatibility
> > >>
> > >> But I realized that the trait wasn't object safe to begin with due to
> > >> the `OFFSET` associated constant. So I'm not sure we need this. Alice,
> > >> do you need `dyn HasWork`?
> > >
> > > I wrote a simple test:
> >
> > [...]
> >
> > > so I don't think adding the Sized bound makes sense - we'd end up
> > > adding it on every item in the trait.
> >
> > Yeah the `Sized` bound was probably to make the cast work, so let's
> > remove it.
>
> It's already removed, right?

Ping. Can you help me understand what change, if any, you think is required?

Re: [PATCH 2/2] rust: workqueue: remove HasWork::OFFSET

[Benno Lossin]

On Sun Mar 16, 2025 at 1:55 PM CET, Tamir Duberstein wrote:
> On Sat, Mar 15, 2025 at 2:12 PM Tamir Duberstein <tamird@gmail.com> wrote:
>>
>> On Sat, Mar 15, 2025 at 2:06 PM Benno Lossin <benno.lossin@proton.me> wrote:
>> >
>> > On Sat Mar 15, 2025 at 4:37 PM CET, Tamir Duberstein wrote:
>> > > On Sat, Mar 15, 2025 at 5:30 AM Benno Lossin <benno.lossin@proton.me> wrote:
>> > >>
>> > >> On Fri Mar 14, 2025 at 9:44 PM CET, Tamir Duberstein wrote:
>> > >> > On Fri, Mar 14, 2025 at 3:20 PM Benno Lossin <benno.lossin@proton.me> wrote:
>> > >> >>
>> > >> >> On Fri Mar 7, 2025 at 10:58 PM CET, Tamir Duberstein wrote:
>> > >> >> >      /// Returns a pointer to the struct containing the [`Work<T, ID>`] field.
>> > >> >> >      ///
>> > >> >> >      /// # Safety
>> > >> >> >      ///
>> > >> >> >      /// The pointer must point at a [`Work<T, ID>`] field in a struct of type `Self`.
>> > >> >> > -    #[inline]
>> > >> >> > -    unsafe fn work_container_of(ptr: *mut Work<T, ID>) -> *mut Self
>> > >> >> > -    where
>> > >> >> > -        Self: Sized,
>> > >> >>
>> > >> >> This bound is required in order to allow the usage of `dyn HasWork` (ie
>> > >> >> object safety), so it should stay.
>> > >> >>
>> > >> >> Maybe add a comment explaining why it's there.
>> > >> >
>> > >> > I guess a doctest would be better, but I still don't understand why
>> > >> > the bound is needed. Sorry, can you cite something or explain in more
>> > >> > detail please?
>> > >>
>> > >> Here is a link: https://doc.rust-lang.org/reference/items/traits.html#dyn-compatibility
>> > >>
>> > >> But I realized that the trait wasn't object safe to begin with due to
>> > >> the `OFFSET` associated constant. So I'm not sure we need this. Alice,
>> > >> do you need `dyn HasWork`?
>> > >
>> > > I wrote a simple test:
>> >
>> > [...]
>> >
>> > > so I don't think adding the Sized bound makes sense - we'd end up
>> > > adding it on every item in the trait.
>> >
>> > Yeah the `Sized` bound was probably to make the cast work, so let's
>> > remove it.
>>
>> It's already removed, right?
>
> Ping. Can you help me understand what change, if any, you think is required?

No change required, with my reply above I intended to take my
complaint away :)

---
Cheers,
Benno

Re: [PATCH 2/2] rust: workqueue: remove HasWork::OFFSET

[Tamir Duberstein]

On Sun, Mar 16, 2025 at 1:43 PM Benno Lossin <benno.lossin@proton.me> wrote:
>
> No change required, with my reply above I intended to take my
> complaint away :)

Cool :) is there something else to be done to earn your RB, or do you
mean to defer to Alice?

Re: [PATCH 2/2] rust: workqueue: remove HasWork::OFFSET

[Benno Lossin]

On Sun Mar 16, 2025 at 7:59 PM CET, Tamir Duberstein wrote:
> On Sun, Mar 16, 2025 at 1:43 PM Benno Lossin <benno.lossin@proton.me> wrote:
>>
>> No change required, with my reply above I intended to take my
>> complaint away :)
>
> Cool :) is there something else to be done to earn your RB, or do you
> mean to defer to Alice?

Ah sorry, I didn't end up adding it (:

Reviewed-by: Benno Lossin <benno.lossin@proton.me>

It would be good if Alice could also take a look.

---
Cheers,
Benno

Re: [PATCH 2/2] rust: workqueue: remove HasWork::OFFSET

[Alice Ryhl]

On Fri, Mar 07, 2025 at 04:58:49PM -0500, Tamir Duberstein wrote:
> Implement `HasWork::work_container_of` in `impl_has_work!`, narrowing
> the interface of `HasWork` and replacing pointer arithmetic with
> `container_of!`. Remove the provided implementation of
> `HasWork::get_work_offset` without replacement; an implementation is
> already generated in `impl_has_work!`. Remove the `Self: Sized` bound on
> `HasWork::work_container_of` which was apparently necessary to access
> `OFFSET` as `OFFSET` no longer exists.
> 
> A similar API change was discussed on the hrtimer series[1].
> 
> Link: https://lore.kernel.org/all/20250224-hrtimer-v3-v6-12-rc2-v9-1-5bd3bf0ce6cc@kernel.org/ [1]
> Signed-off-by: Tamir Duberstein <tamird@gmail.com>

Overall looks good to me, but please CC the WORKQUEUE maintainers on the
next version.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Rust Binder still builds with this change:

Tested-by: Alice Ryhl <aliceryhl@google.com>

> -    where
> -        Self: Sized,

I did have trait object support in mind when I wrote these abstractions,
but I don't actually need it and I don't think I actually got it working
with trait objects.

Alice

Re: [PATCH 2/2] rust: workqueue: remove HasWork::OFFSET

[Tamir Duberstein]

On Mon, Mar 17, 2025 at 7:34 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> On Fri, Mar 07, 2025 at 04:58:49PM -0500, Tamir Duberstein wrote:
> > Implement `HasWork::work_container_of` in `impl_has_work!`, narrowing
> > the interface of `HasWork` and replacing pointer arithmetic with
> > `container_of!`. Remove the provided implementation of
> > `HasWork::get_work_offset` without replacement; an implementation is
> > already generated in `impl_has_work!`. Remove the `Self: Sized` bound on
> > `HasWork::work_container_of` which was apparently necessary to access
> > `OFFSET` as `OFFSET` no longer exists.
> >
> > A similar API change was discussed on the hrtimer series[1].
> >
> > Link: https://lore.kernel.org/all/20250224-hrtimer-v3-v6-12-rc2-v9-1-5bd3bf0ce6cc@kernel.org/ [1]
> > Signed-off-by: Tamir Duberstein <tamird@gmail.com>
>
> Overall looks good to me, but please CC the WORKQUEUE maintainers on the
> next version.
>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
>
> Rust Binder still builds with this change:
>
> Tested-by: Alice Ryhl <aliceryhl@google.com>
>
> > -    where
> > -        Self: Sized,
>
> I did have trait object support in mind when I wrote these abstractions,
> but I don't actually need it and I don't think I actually got it working
> with trait objects.
>
> Alice

Thanks! Does there need to be another version? No changes have been requested.

Re: [PATCH 2/2] rust: workqueue: remove HasWork::OFFSET

[Alice Ryhl]

On Mon, Mar 17, 2025 at 07:35:55AM -0400, Tamir Duberstein wrote:
> On Mon, Mar 17, 2025 at 7:34 AM Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > Overall looks good to me, but please CC the WORKQUEUE maintainers on the
> > next version.
> 
> Thanks! Does there need to be another version? No changes have been requested.

Yes, it needs a new version to fix conflicts with commit 7b948a2af6b5
("rust: pci: fix unrestricted &mut pci::Device") that landed in the
merge window just now. More generally, a new version is also recommended
if maintainers are missing in the CC list.

As an FYI, I'm looking to do some additional work on the workqueue
abstractions to support delayed work items for use by GPU drivers. Since
I agree with this change, I'll base my work on top of your series.

Alice

Re: [PATCH 0/2] rust: workqueue: remove HasWork::OFFSET

[Tamir Duberstein]

Gentle ping on this.