* [PATCH] rust: add this_module macro
@ 2023-01-31 13:08 Martin Rodriguez Reboredo
2023-01-31 13:42 ` Greg KH
0 siblings, 1 reply; 7+ messages in thread
From: Martin Rodriguez Reboredo @ 2023-01-31 13:08 UTC (permalink / raw)
To: linux-kernel
Cc: rust-for-linux, Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho,
Boqun Feng, Gary Guo, Björn Roy Baron
Adds a Rust equivalent to the handy THIS_MODULE macro from C.
Signed-off-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
---
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
+ }
+ };
+}
+
#[cfg(not(any(testlib, test)))]
#[panic_handler]
fn panic(info: &core::panic::PanicInfo<'_>) -> ! {
--
2.39.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] rust: add this_module macro
2023-01-31 13:08 [PATCH] rust: add this_module macro Martin Rodriguez Reboredo
@ 2023-01-31 13:42 ` Greg KH
2023-01-31 15:07 ` Martin Rodriguez Reboredo
0 siblings, 1 reply; 7+ messages in thread
From: Greg KH @ 2023-01-31 13:42 UTC (permalink / raw)
To: Martin Rodriguez Reboredo
Cc: linux-kernel, rust-for-linux, Miguel Ojeda, Alex Gaynor,
Wedson Almeida Filho, Boqun Feng, Gary Guo, Björn Roy Baron
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@gmail.com>
> ---
> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] rust: add this_module macro
2023-01-31 13:42 ` Greg KH
@ 2023-01-31 15:07 ` Martin Rodriguez Reboredo
2023-01-31 15:15 ` Greg KH
0 siblings, 1 reply; 7+ messages in thread
From: Martin Rodriguez Reboredo @ 2023-01-31 15:07 UTC (permalink / raw)
To: gregkh
Cc: alex.gaynor, bjorn3_gh, boqun.feng, gary, linux-kernel, ojeda,
rust-for-linux, wedsonaf, yakoyoku
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@gmail.com>
>> ---
>> 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.
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. @@@
And yes, I should have mentioned that in the commit message, but I let
slip that detail.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] rust: add this_module macro
2023-01-31 15:07 ` Martin Rodriguez Reboredo
@ 2023-01-31 15:15 ` Greg KH
2023-01-31 16:07 ` Martin Rodriguez Reboredo
0 siblings, 1 reply; 7+ messages in thread
From: Greg KH @ 2023-01-31 15:15 UTC (permalink / raw)
To: Martin Rodriguez Reboredo
Cc: alex.gaynor, bjorn3_gh, boqun.feng, gary, linux-kernel, ojeda,
rust-for-linux, wedsonaf
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@gmail.com>
> >> ---
> >> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] rust: add this_module macro
2023-01-31 15:15 ` Greg KH
@ 2023-01-31 16:07 ` Martin Rodriguez Reboredo
2023-01-31 16:59 ` Greg KH
0 siblings, 1 reply; 7+ messages in thread
From: Martin Rodriguez Reboredo @ 2023-01-31 16:07 UTC (permalink / raw)
To: gregkh
Cc: alex.gaynor, bjorn3_gh, boqun.feng, gary, linux-kernel, ojeda,
rust-for-linux, wedsonaf, yakoyoku
On Tue, Jan 31, 2023 at 04:15:51PM +0100, Greg KH wrote:
>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@gmail.com>
>> >> ---
>> >> 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
*kicks can*, at least I can take some ideas out of this, anyways, thanks
for your reviews.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] rust: add this_module macro
2023-01-31 16:07 ` Martin Rodriguez Reboredo
@ 2023-01-31 16:59 ` Greg KH
2023-01-31 20:46 ` Miguel Ojeda
0 siblings, 1 reply; 7+ messages in thread
From: Greg KH @ 2023-01-31 16:59 UTC (permalink / raw)
To: Martin Rodriguez Reboredo
Cc: alex.gaynor, bjorn3_gh, boqun.feng, gary, linux-kernel, ojeda,
rust-for-linux, wedsonaf
On Tue, Jan 31, 2023 at 01:07:28PM -0300, Martin Rodriguez Reboredo wrote:
> On Tue, Jan 31, 2023 at 04:15:51PM +0100, Greg KH wrote:
> >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@gmail.com>
> >> >> ---
> >> >> 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
>
> *kicks can*, at least I can take some ideas out of this, anyways, thanks
> for your reviews.
I don't mean to reject this out-of-hand, it's just that without a real
user, it's impossible to review this and say "this is ok" instead of
"perhaps you should do it this other way"?
Right now the rust framework is just that, a framework. Perhaps we
should not be adding anything else to it until there is a real user of
it? Otherwise this will keep coming up again and again.
Treat this like any other kernel feature/addition, you can't add apis
until you submit a user for it at the same time. That's one way we have
been able to evolve and maintain the kernel source tree for so long.
Without an api user, we have no way to know how it's being used or if
it's even being used at all.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] rust: add this_module macro
2023-01-31 16:59 ` Greg KH
@ 2023-01-31 20:46 ` Miguel Ojeda
0 siblings, 0 replies; 7+ messages in thread
From: Miguel Ojeda @ 2023-01-31 20:46 UTC (permalink / raw)
To: Greg KH
Cc: Martin Rodriguez Reboredo, alex.gaynor, bjorn3_gh, boqun.feng,
gary, linux-kernel, ojeda, rust-for-linux, wedsonaf
On Tue, Jan 31, 2023 at 5:59 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> Right now the rust framework is just that, a framework. Perhaps we
> should not be adding anything else to it until there is a real user of
> it? Otherwise this will keep coming up again and again.
>
> Treat this like any other kernel feature/addition, you can't add apis
> until you submit a user for it at the same time. That's one way we have
> been able to evolve and maintain the kernel source tree for so long.
> Without an api user, we have no way to know how it's being used or if
> it's even being used at all.
Agreed. For this patch, it came independently, so I cannot speak about
its users. For other patch series we have sent, the users are
out-of-tree, but they want to come in-tree as soon as possible. We are
coordinating with them to prioritize the submission of the things they
will depend on.
Since the goal is to submit things piece by piece so that they can be
properly reviewed, what we have been doing to ameliorate things is to
provide enough details, examples and documentation for each function,
type, etc. so that it is hopefully clear how they will be used. If
there are some cases where it may not be clear, we can also link to
the upcoming users.
Cheers,
Miguel
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-01-31 20:47 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-31 13:08 [PATCH] rust: add this_module macro Martin Rodriguez Reboredo
2023-01-31 13:42 ` Greg KH
2023-01-31 15:07 ` Martin Rodriguez Reboredo
2023-01-31 15:15 ` Greg KH
2023-01-31 16:07 ` Martin Rodriguez Reboredo
2023-01-31 16:59 ` Greg KH
2023-01-31 20:46 ` Miguel Ojeda
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).