Re: [PATCH] rust: add this_module macro

[Greg KH]

On Tue, Jan 31, 2023 at 12:07:45PM -0300, Martin Rodriguez Reboredo wrote:
> On Tue, Jan 31, 2023 at 02:42:08PM +0100, Greg KH wrote:
> >On Tue, Jan 31, 2023 at 10:08:41AM -0300, Martin Rodriguez Reboredo wrote:
> >> Adds a Rust equivalent to the handy THIS_MODULE macro from C.
> >> 
> >> Signed-off-by: Martin Rodriguez Reboredo <yakoyoku@xxxxxxxxx>
> >> ---
> >>  rust/kernel/lib.rs | 12 ++++++++++++
> >>  1 file changed, 12 insertions(+)
> >> 
> >> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> >> index e0b0e953907d..afb6b0390426 100644
> >> --- a/rust/kernel/lib.rs
> >> +++ b/rust/kernel/lib.rs
> >> @@ -80,6 +80,18 @@ impl ThisModule {
> >>      }
> >>  }
> >>  
> >> +/// Returns the current module.
> >> +#[macro_export]
> >> +macro_rules! this_module {
> >> +    () => {
> >> +        if cfg!(MODULE) {
> >> +            Some(unsafe { $crate::ThisModule::from_ptr(&mut $crate::bindings::__this_module) })
> >> +        } else {
> >> +            None
> >> +        }
> >> +    };
> >> +}
> >
> >While this is handy, what exactly will it be used for?  The C
> >wrappers/shim/whatever should probably handle this for you already when
> >you save this pointer into a structure right?
> >
> >Surely you aren't trying to increment your own module's reference count,
> >right?  That just doesn't work :)
> >
> >thanks,
> >
> >greg k-h
> 
> This was meant for setting the owner field of a file_operations struct
> or the cra_owner field of crypto_alg and many other structs.

But shouldn't the macro kernel::declare_file_operations() do this for
you automagically?  You should never have to manually say "this module!"
to any structure or function call if we do things right.

Yes, many "old school" structures in the kernel do this, but we have
learned from the 1990's, see the fun wrappers around simple things like
usb_register_driver(); as an example of how the driver author themselves
should never see a module pointer anywhere.

> I know that increfing a module without a good reason is dead dumb, so
> I'm not trying to send things in a downwards spiral. @@@

That's good, but let's not add housekeeping requirements when we do not
have to do so if at all possible please.

thanks,

greg k-h