On Mon, Apr 01, 2024 at 10:01:34PM +0000, Benno Lossin wrote:
> On 01.04.24 23:10, Boqun Feng wrote:
> > On Mon, Apr 01, 2024 at 06:52:50PM +0000, Benno Lossin wrote:
> > [...]
> >> + // Double nested modules, since then nobody can access the public items inside.
> >> + mod __module_init {{
> >> + mod __module_init {{
> >> + use super::super::{type_};
> >> +
> >> + /// The \"Rust loadable module\" mark.
> >> + //
> >> + // This may be best done another way later on, e.g. as a new modinfo
> >> + // key or a new section. For the moment, keep it simple.
> >> + #[cfg(MODULE)]
> >> + #[doc(hidden)]
> >> + #[used]
> >> + static __IS_RUST_MODULE: () = ();
> >> +
> >> + static mut __MOD: Option<{type_}> = None;
> >> +
> >> + // SAFETY: `__this_module` is constructed by the kernel at load time and will not be
> >> + // freed until the module is unloaded.
> >> + #[cfg(MODULE)]
> >> + static THIS_MODULE: kernel::ThisModule = unsafe {{
> >> + kernel::ThisModule::from_ptr(&kernel::bindings::__this_module as *const _ as *mut _)
> >
> > While we're at it, probably we want the following as well? I.e. using
> > `Opaque` and extern block, because __this_module is certainly something
> > interior mutable and !Unpin.
> >
> > diff --git a/rust/macros/module.rs b/rust/macros/module.rs
> > index 293beca0a583..8aa4eed6578c 100644
> > --- a/rust/macros/module.rs
> > +++ b/rust/macros/module.rs
> > @@ -219,7 +219,11 @@ mod __module_init {{
> > // freed until the module is unloaded.
> > #[cfg(MODULE)]
> > static THIS_MODULE: kernel::ThisModule = unsafe {{
> > - kernel::ThisModule::from_ptr(&kernel::bindings::__this_module as *const _ as *mut _)
> > + extern \"C\" {{
> > + static __this_module: kernel::types::Opaque<kernel::bindings::module>;
> > + }}
> > +
> > + kernel::ThisModule::from_ptr(__this_module.get())
> > }};
> > #[cfg(not(MODULE))]
> > static THIS_MODULE: kernel::ThisModule = unsafe {{
> >
> > Thoughts?
>
> I am not sure we need it. Bindgen generates
>
> extern "C" {
> pub static mut __this_module: module;
> }
>
> And the `mut` should take care of the "it might be modified by other
> threads".
Hmm.. but there could a C thread modifies some field of __this_module
while Rust code uses it, e.g. struct module has a list_head in it, which
could be used by C code to put another module next to it.
> The only thing that sticks out to me is the borrow, it should probably
> be using `addr_of_mut!` instead. Then we don't need to re-import it
> again manually.
>
> I think it should be a separate patch though.
>
Yes, agreed.
Regards,
Boqun
> --
> Cheers,
> Benno
>
> >
> > Note this requires `Opaque::get` to be `const`, which I will send out
> > shortly, I think it's a good change regardless of the usage here.
> >
> > Regards,
> > Boqun
> >
>
