From: Wedson Almeida Filho <walmeida@microsoft.com>

This allows modules to be initialised in-place in pinned memory, which
enables the usage of pinned types (e.g., mutexes, spinlocks, driver
registrations, etc.) in modules without any extra allocations.

Drivers that don't need this may continue to implement `Module` without
any changes.

Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
---
 rust/kernel/lib.rs    | 25 ++++++++++++++++++++++++-
 rust/macros/module.rs | 18 ++++++------------
 2 files changed, 30 insertions(+), 13 deletions(-)

diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 5c641233e26d..64aee4fbc53b 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -62,7 +62,7 @@
 /// The top level entrypoint to implementing a kernel module.
 ///
 /// For any teardown or cleanup operations, your type may implement [`Drop`].
-pub trait Module: Sized + Sync {
+pub trait Module: Sized + Sync + Send {
     /// Called at module initialization time.
     ///
     /// Use this method to perform whatever setup or registration your module
@@ -72,6 +72,29 @@ pub trait Module: Sized + Sync {
     fn init(module: &'static ThisModule) -> error::Result<Self>;
 }
 
+/// A module that is pinned and initialised in-place.
+pub trait InPlaceModule: Sync + Send {
+    /// Creates an initialiser for the module.
+    ///
+    /// It is called when the module is loaded.
+    fn init(module: &'static ThisModule) -> impl init::PinInit<Self, error::Error>;
+}
+
+impl<T: Module> InPlaceModule for T {
+    fn init(module: &'static ThisModule) -> impl init::PinInit<Self, error::Error> {
+        let initer = move |slot: *mut Self| {
+            let m = <Self as Module>::init(module)?;
+
+            // SAFETY: `slot` is valid for write per the contract with `pin_init_from_closure`.
+            unsafe { slot.write(m) };
+            Ok(())
+        };
+
+        // SAFETY: On success, `initer` always fully initialises an instance of `Self`.
+        unsafe { init::pin_init_from_closure(initer) }
+    }
+}
+
 /// Equivalent to `THIS_MODULE` in the C API.
 ///
 /// C header: [`include/linux/export.h`](srctree/include/linux/export.h)
diff --git a/rust/macros/module.rs b/rust/macros/module.rs
index 27979e582e4b..0b2bb4ec2fba 100644
--- a/rust/macros/module.rs
+++ b/rust/macros/module.rs
@@ -208,7 +208,7 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
             #[used]
             static __IS_RUST_MODULE: () = ();
 
-            static mut __MOD: Option<{type_}> = None;
+            static mut __MOD: core::mem::MaybeUninit<{type_}> = core::mem::MaybeUninit::uninit();
 
             // SAFETY: `__this_module` is constructed by the kernel at load time and will not be
             // freed until the module is unloaded.
@@ -275,23 +275,17 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
             }}
 
             fn __init() -> core::ffi::c_int {{
-                match <{type_} as kernel::Module>::init(&THIS_MODULE) {{
-                    Ok(m) => {{
-                        unsafe {{
-                            __MOD = Some(m);
-                        }}
-                        return 0;
-                    }}
-                    Err(e) => {{
-                        return e.to_errno();
-                    }}
+                let initer = <{type_} as kernel::InPlaceModule>::init(&THIS_MODULE);
+                match unsafe {{ initer.__pinned_init(__MOD.as_mut_ptr()) }} {{
+                    Ok(m) => 0,
+                    Err(e) => e.to_errno(),
                 }}
             }}
 
             fn __exit() {{
                 unsafe {{
                     // Invokes `drop()` on `__MOD`, which should be used for cleanup.
-                    __MOD = None;
+                    __MOD.assume_init_drop();
                 }}
             }}
 
-- 
2.34.1

On 27.03.24 04:23, Wedson Almeida Filho wrote:
> From: Wedson Almeida Filho <walmeida@microsoft.com>
> 
> This allows modules to be initialised in-place in pinned memory, which
> enables the usage of pinned types (e.g., mutexes, spinlocks, driver
> registrations, etc.) in modules without any extra allocations.
> 
> Drivers that don't need this may continue to implement `Module` without
> any changes.
> 
> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>

I have some suggestions below, with those fixed,

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

> ---
>  rust/kernel/lib.rs    | 25 ++++++++++++++++++++++++-
>  rust/macros/module.rs | 18 ++++++------------
>  2 files changed, 30 insertions(+), 13 deletions(-)
> 
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 5c641233e26d..64aee4fbc53b 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -62,7 +62,7 @@
>  /// The top level entrypoint to implementing a kernel module.
>  ///
>  /// For any teardown or cleanup operations, your type may implement [`Drop`].
> -pub trait Module: Sized + Sync {
> +pub trait Module: Sized + Sync + Send {
>      /// Called at module initialization time.
>      ///
>      /// Use this method to perform whatever setup or registration your module
> @@ -72,6 +72,29 @@ pub trait Module: Sized + Sync {
>      fn init(module: &'static ThisModule) -> error::Result<Self>;
>  }
> 
> +/// A module that is pinned and initialised in-place.
> +pub trait InPlaceModule: Sync + Send {
> +    /// Creates an initialiser for the module.
> +    ///
> +    /// It is called when the module is loaded.
> +    fn init(module: &'static ThisModule) -> impl init::PinInit<Self, error::Error>;
> +}
> +
> +impl<T: Module> InPlaceModule for T {
> +    fn init(module: &'static ThisModule) -> impl init::PinInit<Self, error::Error> {
> +        let initer = move |slot: *mut Self| {
> +            let m = <Self as Module>::init(module)?;
> +
> +            // SAFETY: `slot` is valid for write per the contract with `pin_init_from_closure`.
> +            unsafe { slot.write(m) };
> +            Ok(())
> +        };
> +
> +        // SAFETY: On success, `initer` always fully initialises an instance of `Self`.
> +        unsafe { init::pin_init_from_closure(initer) }
> +    }
> +}
> +
>  /// Equivalent to `THIS_MODULE` in the C API.
>  ///
>  /// C header: [`include/linux/export.h`](srctree/include/linux/export.h)
> diff --git a/rust/macros/module.rs b/rust/macros/module.rs
> index 27979e582e4b..0b2bb4ec2fba 100644
> --- a/rust/macros/module.rs
> +++ b/rust/macros/module.rs
> @@ -208,7 +208,7 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
>              #[used]
>              static __IS_RUST_MODULE: () = ();
> 
> -            static mut __MOD: Option<{type_}> = None;
> +            static mut __MOD: core::mem::MaybeUninit<{type_}> = core::mem::MaybeUninit::uninit();

I would prefer `::core::mem::MaybeUninit`, since that prevents
accidentally referring to a module named `core`.

> 
>              // SAFETY: `__this_module` is constructed by the kernel at load time and will not be
>              // freed until the module is unloaded.
> @@ -275,23 +275,17 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
>              }}
> 
>              fn __init() -> core::ffi::c_int {{
> -                match <{type_} as kernel::Module>::init(&THIS_MODULE) {{
> -                    Ok(m) => {{
> -                        unsafe {{
> -                            __MOD = Some(m);
> -                        }}
> -                        return 0;
> -                    }}
> -                    Err(e) => {{
> -                        return e.to_errno();
> -                    }}
> +                let initer = <{type_} as kernel::InPlaceModule>::init(&THIS_MODULE);

Ditto with `::kernel::InPlaceModule`.

> +                match unsafe {{ initer.__pinned_init(__MOD.as_mut_ptr()) }} {{

This requires that the `PinInit` trait is in scope, I would use:

     match unsafe {{ ::kernel::init::PinInit::__pinned_init(initer, __MOD.as_mut_ptr()) }} {{

-- 
Cheers,
Benno

> +                    Ok(m) => 0,
> +                    Err(e) => e.to_errno(),
>                  }}
>              }}
> 
>              fn __exit() {{
>                  unsafe {{
>                      // Invokes `drop()` on `__MOD`, which should be used for cleanup.
> -                    __MOD = None;
> +                    __MOD.assume_init_drop();
>                  }}
>              }}
> 
> --
> 2.34.1
> 

On Wed, 27 Mar 2024 at 13:16, Benno Lossin <benno.lossin@proton.me> wrote:
>
> On 27.03.24 04:23, Wedson Almeida Filho wrote:
> > From: Wedson Almeida Filho <walmeida@microsoft.com>
> >
> > This allows modules to be initialised in-place in pinned memory, which
> > enables the usage of pinned types (e.g., mutexes, spinlocks, driver
> > registrations, etc.) in modules without any extra allocations.
> >
> > Drivers that don't need this may continue to implement `Module` without
> > any changes.
> >
> > Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
>
> I have some suggestions below, with those fixed,
>
> Reviewed-by: Benno Lossin <benno.lossin@proton.me>
>
> > ---
> >  rust/kernel/lib.rs    | 25 ++++++++++++++++++++++++-
> >  rust/macros/module.rs | 18 ++++++------------
> >  2 files changed, 30 insertions(+), 13 deletions(-)
> >
> > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> > index 5c641233e26d..64aee4fbc53b 100644
> > --- a/rust/kernel/lib.rs
> > +++ b/rust/kernel/lib.rs
> > @@ -62,7 +62,7 @@
> >  /// The top level entrypoint to implementing a kernel module.
> >  ///
> >  /// For any teardown or cleanup operations, your type may implement [`Drop`].
> > -pub trait Module: Sized + Sync {
> > +pub trait Module: Sized + Sync + Send {
> >      /// Called at module initialization time.
> >      ///
> >      /// Use this method to perform whatever setup or registration your module
> > @@ -72,6 +72,29 @@ pub trait Module: Sized + Sync {
> >      fn init(module: &'static ThisModule) -> error::Result<Self>;
> >  }
> >
> > +/// A module that is pinned and initialised in-place.
> > +pub trait InPlaceModule: Sync + Send {
> > +    /// Creates an initialiser for the module.
> > +    ///
> > +    /// It is called when the module is loaded.
> > +    fn init(module: &'static ThisModule) -> impl init::PinInit<Self, error::Error>;
> > +}
> > +
> > +impl<T: Module> InPlaceModule for T {
> > +    fn init(module: &'static ThisModule) -> impl init::PinInit<Self, error::Error> {
> > +        let initer = move |slot: *mut Self| {
> > +            let m = <Self as Module>::init(module)?;
> > +
> > +            // SAFETY: `slot` is valid for write per the contract with `pin_init_from_closure`.
> > +            unsafe { slot.write(m) };
> > +            Ok(())
> > +        };
> > +
> > +        // SAFETY: On success, `initer` always fully initialises an instance of `Self`.
> > +        unsafe { init::pin_init_from_closure(initer) }
> > +    }
> > +}
> > +
> >  /// Equivalent to `THIS_MODULE` in the C API.
> >  ///
> >  /// C header: [`include/linux/export.h`](srctree/include/linux/export.h)
> > diff --git a/rust/macros/module.rs b/rust/macros/module.rs
> > index 27979e582e4b..0b2bb4ec2fba 100644
> > --- a/rust/macros/module.rs
> > +++ b/rust/macros/module.rs
> > @@ -208,7 +208,7 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
> >              #[used]
> >              static __IS_RUST_MODULE: () = ();
> >
> > -            static mut __MOD: Option<{type_}> = None;
> > +            static mut __MOD: core::mem::MaybeUninit<{type_}> = core::mem::MaybeUninit::uninit();
>
> I would prefer `::core::mem::MaybeUninit`, since that prevents
> accidentally referring to a module named `core`.

Makes sense, changed in v3.

I also added a patch to v3 that adds `::` to all cases in the code
generated by the macro.

>
> >
> >              // SAFETY: `__this_module` is constructed by the kernel at load time and will not be
> >              // freed until the module is unloaded.
> > @@ -275,23 +275,17 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
> >              }}
> >
> >              fn __init() -> core::ffi::c_int {{
> > -                match <{type_} as kernel::Module>::init(&THIS_MODULE) {{
> > -                    Ok(m) => {{
> > -                        unsafe {{
> > -                            __MOD = Some(m);
> > -                        }}
> > -                        return 0;
> > -                    }}
> > -                    Err(e) => {{
> > -                        return e.to_errno();
> > -                    }}
> > +                let initer = <{type_} as kernel::InPlaceModule>::init(&THIS_MODULE);
>
> Ditto with `::kernel::InPlaceModule`.
>
> > +                match unsafe {{ initer.__pinned_init(__MOD.as_mut_ptr()) }} {{
>
> This requires that the `PinInit` trait is in scope, I would use:
>
>      match unsafe {{ ::kernel::init::PinInit::__pinned_init(initer, __MOD.as_mut_ptr()) }} {{

Also changed in v3.

> --
> Cheers,
> Benno
>
> > +                    Ok(m) => 0,
> > +                    Err(e) => e.to_errno(),
> >                  }}
> >              }}
> >
> >              fn __exit() {{
> >                  unsafe {{
> >                      // Invokes `drop()` on `__MOD`, which should be used for cleanup.
> > -                    __MOD = None;
> > +                    __MOD.assume_init_drop();
> >                  }}
> >              }}
> >
> > --
> > 2.34.1
> >
>

> This allows modules to be initialised in-place in pinned memory, which
> enables the usage of pinned types (e.g., mutexes, spinlocks, driver
> registrations, etc.) in modules without any extra allocations.
>
> Drivers that don't need this may continue to implement `Module` without
> any changes.
>
> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
> ---
>  rust/kernel/lib.rs    | 25 ++++++++++++++++++++++++-
>  rust/macros/module.rs | 18 ++++++------------
>  2 files changed, 30 insertions(+), 13 deletions(-)
>
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 5c641233e26d..64aee4fbc53b 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -62,7 +62,7 @@
>  /// The top level entrypoint to implementing a kernel module.
>  ///
>  /// For any teardown or cleanup operations, your type may implement [`Drop`].
> -pub trait Module: Sized + Sync {
> +pub trait Module: Sized + Sync + Send {

This does not compile with `CONFIG_AX88796B_RUST_PHY=y || m` (or the
phylib abstractions' doctests) since the module `Registration` is not
`Send`.

I remember Trevor raising the question whether we want to require modules
to be `Send`. I am not aware of any examples of `!Send` modules but I guess
it would be possible to write code that is only correct under the
assumption that it is loaded/unloaded in the same context.

@Trevor: Are you aware of any modules with that requirement?

I have been using this patch for quite a while with my TCP CCAs now
(without the `Send` bound) and did not experience any other issues; thus
offering:
	Tested-by: Valentin Obst <kernel@valentinobst.de>

	- Best Valentin

>      /// Called at module initialization time.
>      ///
>      /// Use this method to perform whatever setup or registration your module
> @@ -72,6 +72,29 @@ pub trait Module: Sized + Sync {
>      fn init(module: &'static ThisModule) -> error::Result<Self>;
>  }
>
> +/// A module that is pinned and initialised in-place.
> +pub trait InPlaceModule: Sync + Send {
> +    /// Creates an initialiser for the module.
> +    ///
> +    /// It is called when the module is loaded.
> +    fn init(module: &'static ThisModule) -> impl init::PinInit<Self, error::Error>;
> +}
> +
> +impl<T: Module> InPlaceModule for T {
> +    fn init(module: &'static ThisModule) -> impl init::PinInit<Self, error::Error> {
> +        let initer = move |slot: *mut Self| {
> +            let m = <Self as Module>::init(module)?;
> +
> +            // SAFETY: `slot` is valid for write per the contract with `pin_init_from_closure`.
> +            unsafe { slot.write(m) };
> +            Ok(())
> +        };
> +
> +        // SAFETY: On success, `initer` always fully initialises an instance of `Self`.
> +        unsafe { init::pin_init_from_closure(initer) }
> +    }
> +}
> +
>  /// Equivalent to `THIS_MODULE` in the C API.
>  ///
>  /// C header: [`include/linux/export.h`](srctree/include/linux/export.h)
> diff --git a/rust/macros/module.rs b/rust/macros/module.rs
> index 27979e582e4b..0b2bb4ec2fba 100644
> --- a/rust/macros/module.rs
> +++ b/rust/macros/module.rs
> @@ -208,7 +208,7 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
>              #[used]
>              static __IS_RUST_MODULE: () = ();
>
> -            static mut __MOD: Option<{type_}> = None;
> +            static mut __MOD: core::mem::MaybeUninit<{type_}> = core::mem::MaybeUninit::uninit();
>
>              // SAFETY: `__this_module` is constructed by the kernel at load time and will not be
>              // freed until the module is unloaded.
> @@ -275,23 +275,17 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
>              }}
>
>              fn __init() -> core::ffi::c_int {{
> -                match <{type_} as kernel::Module>::init(&THIS_MODULE) {{
> -                    Ok(m) => {{
> -                        unsafe {{
> -                            __MOD = Some(m);
> -                        }}
> -                        return 0;
> -                    }}
> -                    Err(e) => {{
> -                        return e.to_errno();
> -                    }}
> +                let initer = <{type_} as kernel::InPlaceModule>::init(&THIS_MODULE);
> +                match unsafe {{ initer.__pinned_init(__MOD.as_mut_ptr()) }} {{
> +                    Ok(m) => 0,
> +                    Err(e) => e.to_errno(),
>                  }}
>              }}
>
>              fn __exit() {{
>                  unsafe {{
>                      // Invokes `drop()` on `__MOD`, which should be used for cleanup.
> -                    __MOD = None;
> +                    __MOD.assume_init_drop();
>                  }}
>              }}
>
> --

On Wed, 27 Mar 2024 at 05:13, Valentin Obst <kernel@valentinobst.de> wrote:
>
> > This allows modules to be initialised in-place in pinned memory, which
> > enables the usage of pinned types (e.g., mutexes, spinlocks, driver
> > registrations, etc.) in modules without any extra allocations.
> >
> > Drivers that don't need this may continue to implement `Module` without
> > any changes.
> >
> > Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
> > ---
> >  rust/kernel/lib.rs    | 25 ++++++++++++++++++++++++-
> >  rust/macros/module.rs | 18 ++++++------------
> >  2 files changed, 30 insertions(+), 13 deletions(-)
> >
> > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> > index 5c641233e26d..64aee4fbc53b 100644
> > --- a/rust/kernel/lib.rs
> > +++ b/rust/kernel/lib.rs
> > @@ -62,7 +62,7 @@
> >  /// The top level entrypoint to implementing a kernel module.
> >  ///
> >  /// For any teardown or cleanup operations, your type may implement [`Drop`].
> > -pub trait Module: Sized + Sync {
> > +pub trait Module: Sized + Sync + Send {
>
> This does not compile with `CONFIG_AX88796B_RUST_PHY=y || m` (or the
> phylib abstractions' doctests) since the module `Registration` is not
> `Send`.

Thanks for the heads up. I thought I had enabled all rust code but
indeed I was missing this. I will fix it in v2.

> I remember Trevor raising the question whether we want to require modules
> to be `Send`. I am not aware of any examples of `!Send` modules but I guess
> it would be possible to write code that is only correct under the
> assumption that it is loaded/unloaded in the same context.

It might be possible in the future, but I don't believe it is now
because all rust modules support unloading. And there is no guarantee
that the thread unloading (and therefore calling module_exit) is the
same that loaded (and called module_init), so a module must be Send to
properly handle drop being called from a different thread.

Not requiring Send on the original Module trait was an oversight that
I don't want to repeat in InPlaceModule.

>
> @Trevor: Are you aware of any modules with that requirement?
>
> I have been using this patch for quite a while with my TCP CCAs now
> (without the `Send` bound) and did not experience any other issues; thus
> offering:
>         Tested-by: Valentin Obst <kernel@valentinobst.de>

Thanks!

>
>         - Best Valentin
>
> >      /// Called at module initialization time.
> >      ///
> >      /// Use this method to perform whatever setup or registration your module
> > @@ -72,6 +72,29 @@ pub trait Module: Sized + Sync {
> >      fn init(module: &'static ThisModule) -> error::Result<Self>;
> >  }
> >
> > +/// A module that is pinned and initialised in-place.
> > +pub trait InPlaceModule: Sync + Send {
> > +    /// Creates an initialiser for the module.
> > +    ///
> > +    /// It is called when the module is loaded.
> > +    fn init(module: &'static ThisModule) -> impl init::PinInit<Self, error::Error>;
> > +}
> > +
> > +impl<T: Module> InPlaceModule for T {
> > +    fn init(module: &'static ThisModule) -> impl init::PinInit<Self, error::Error> {
> > +        let initer = move |slot: *mut Self| {
> > +            let m = <Self as Module>::init(module)?;
> > +
> > +            // SAFETY: `slot` is valid for write per the contract with `pin_init_from_closure`.
> > +            unsafe { slot.write(m) };
> > +            Ok(())
> > +        };
> > +
> > +        // SAFETY: On success, `initer` always fully initialises an instance of `Self`.
> > +        unsafe { init::pin_init_from_closure(initer) }
> > +    }
> > +}
> > +
> >  /// Equivalent to `THIS_MODULE` in the C API.
> >  ///
> >  /// C header: [`include/linux/export.h`](srctree/include/linux/export.h)
> > diff --git a/rust/macros/module.rs b/rust/macros/module.rs
> > index 27979e582e4b..0b2bb4ec2fba 100644
> > --- a/rust/macros/module.rs
> > +++ b/rust/macros/module.rs
> > @@ -208,7 +208,7 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
> >              #[used]
> >              static __IS_RUST_MODULE: () = ();
> >
> > -            static mut __MOD: Option<{type_}> = None;
> > +            static mut __MOD: core::mem::MaybeUninit<{type_}> = core::mem::MaybeUninit::uninit();
> >
> >              // SAFETY: `__this_module` is constructed by the kernel at load time and will not be
> >              // freed until the module is unloaded.
> > @@ -275,23 +275,17 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
> >              }}
> >
> >              fn __init() -> core::ffi::c_int {{
> > -                match <{type_} as kernel::Module>::init(&THIS_MODULE) {{
> > -                    Ok(m) => {{
> > -                        unsafe {{
> > -                            __MOD = Some(m);
> > -                        }}
> > -                        return 0;
> > -                    }}
> > -                    Err(e) => {{
> > -                        return e.to_errno();
> > -                    }}
> > +                let initer = <{type_} as kernel::InPlaceModule>::init(&THIS_MODULE);
> > +                match unsafe {{ initer.__pinned_init(__MOD.as_mut_ptr()) }} {{
> > +                    Ok(m) => 0,
> > +                    Err(e) => e.to_errno(),
> >                  }}
> >              }}
> >
> >              fn __exit() {{
> >                  unsafe {{
> >                      // Invokes `drop()` on `__MOD`, which should be used for cleanup.
> > -                    __MOD = None;
> > +                    __MOD.assume_init_drop();
> >                  }}
> >              }}
> >
> > --

On 27.03.24 15:23, Wedson Almeida Filho wrote:
> On Wed, 27 Mar 2024 at 05:13, Valentin Obst <kernel@valentinobst.de> wrote:
>>
>>> This allows modules to be initialised in-place in pinned memory, which
>>> enables the usage of pinned types (e.g., mutexes, spinlocks, driver
>>> registrations, etc.) in modules without any extra allocations.
>>>
>>> Drivers that don't need this may continue to implement `Module` without
>>> any changes.
>>>
>>> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
>>> ---
>>>   rust/kernel/lib.rs    | 25 ++++++++++++++++++++++++-
>>>   rust/macros/module.rs | 18 ++++++------------
>>>   2 files changed, 30 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
>>> index 5c641233e26d..64aee4fbc53b 100644
>>> --- a/rust/kernel/lib.rs
>>> +++ b/rust/kernel/lib.rs
>>> @@ -62,7 +62,7 @@
>>>   /// The top level entrypoint to implementing a kernel module.
>>>   ///
>>>   /// For any teardown or cleanup operations, your type may implement [`Drop`].
>>> -pub trait Module: Sized + Sync {
>>> +pub trait Module: Sized + Sync + Send {
>>
>> This does not compile with `CONFIG_AX88796B_RUST_PHY=y || m` (or the
>> phylib abstractions' doctests) since the module `Registration` is not
>> `Send`.
> 
> Thanks for the heads up. I thought I had enabled all rust code but
> indeed I was missing this. I will fix it in v2.
> 
>> I remember Trevor raising the question whether we want to require modules
>> to be `Send`. I am not aware of any examples of `!Send` modules but I guess
>> it would be possible to write code that is only correct under the
>> assumption that it is loaded/unloaded in the same context.
> 
> It might be possible in the future, but I don't believe it is now
> because all rust modules support unloading. And there is no guarantee
> that the thread unloading (and therefore calling module_exit) is the
> same that loaded (and called module_init), so a module must be Send to
> properly handle drop being called from a different thread.
> 
> Not requiring Send on the original Module trait was an oversight that
> I don't want to repeat in InPlaceModule.

I think that this change should go to the stable tree, can you split it
into its own patch?

-- 
Cheers,
Benno

> 
>>
>> @Trevor: Are you aware of any modules with that requirement?
>>
>> I have been using this patch for quite a while with my TCP CCAs now
>> (without the `Send` bound) and did not experience any other issues; thus
>> offering:
>>          Tested-by: Valentin Obst <kernel@valentinobst.de>
> 
> Thanks!
> 
>>
>>          - Best Valentin
>>

On Wed, 27 Mar 2024 at 12:56, Benno Lossin <benno.lossin@proton.me> wrote:
>
> On 27.03.24 15:23, Wedson Almeida Filho wrote:
> > On Wed, 27 Mar 2024 at 05:13, Valentin Obst <kernel@valentinobst.de> wrote:
> >>
> >>> This allows modules to be initialised in-place in pinned memory, which
> >>> enables the usage of pinned types (e.g., mutexes, spinlocks, driver
> >>> registrations, etc.) in modules without any extra allocations.
> >>>
> >>> Drivers that don't need this may continue to implement `Module` without
> >>> any changes.
> >>>
> >>> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
> >>> ---
> >>>   rust/kernel/lib.rs    | 25 ++++++++++++++++++++++++-
> >>>   rust/macros/module.rs | 18 ++++++------------
> >>>   2 files changed, 30 insertions(+), 13 deletions(-)
> >>>
> >>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> >>> index 5c641233e26d..64aee4fbc53b 100644
> >>> --- a/rust/kernel/lib.rs
> >>> +++ b/rust/kernel/lib.rs
> >>> @@ -62,7 +62,7 @@
> >>>   /// The top level entrypoint to implementing a kernel module.
> >>>   ///
> >>>   /// For any teardown or cleanup operations, your type may implement [`Drop`].
> >>> -pub trait Module: Sized + Sync {
> >>> +pub trait Module: Sized + Sync + Send {
> >>
> >> This does not compile with `CONFIG_AX88796B_RUST_PHY=y || m` (or the
> >> phylib abstractions' doctests) since the module `Registration` is not
> >> `Send`.
> >
> > Thanks for the heads up. I thought I had enabled all rust code but
> > indeed I was missing this. I will fix it in v2.
> >
> >> I remember Trevor raising the question whether we want to require modules
> >> to be `Send`. I am not aware of any examples of `!Send` modules but I guess
> >> it would be possible to write code that is only correct under the
> >> assumption that it is loaded/unloaded in the same context.
> >
> > It might be possible in the future, but I don't believe it is now
> > because all rust modules support unloading. And there is no guarantee
> > that the thread unloading (and therefore calling module_exit) is the
> > same that loaded (and called module_init), so a module must be Send to
> > properly handle drop being called from a different thread.
> >
> > Not requiring Send on the original Module trait was an oversight that
> > I don't want to repeat in InPlaceModule.
>
> I think that this change should go to the stable tree, can you split it
> into its own patch?

Sure, I split it off in v2.

Note that you'll also need the [new] patch to `rust/kernel/net/phy.rs`.

> --
> Cheers,
> Benno
>
> >
> >>
> >> @Trevor: Are you aware of any modules with that requirement?
> >>
> >> I have been using this patch for quite a while with my TCP CCAs now
> >> (without the `Send` bound) and did not experience any other issues; thus
> >> offering:
> >>          Tested-by: Valentin Obst <kernel@valentinobst.de>
> >
> > Thanks!
> >
> >>
> >>          - Best Valentin
> >>
>