From: "Benno Lossin" <lossin@kernel.org>
To: "Andreas Hindborg" <a.hindborg@kernel.org>,
"Miguel Ojeda" <ojeda@kernel.org>,
"Alex Gaynor" <alex.gaynor@gmail.com>,
"Boqun Feng" <boqun.feng@gmail.com>,
"Gary Guo" <gary@garyguo.net>,
"Björn Roy Baron" <bjorn3_gh@protonmail.com>,
"Benno Lossin" <benno.lossin@proton.me>,
"Alice Ryhl" <aliceryhl@google.com>,
"Masahiro Yamada" <masahiroy@kernel.org>,
"Nathan Chancellor" <nathan@kernel.org>,
"Luis Chamberlain" <mcgrof@kernel.org>,
"Danilo Krummrich" <dakr@kernel.org>,
"Nicolas Schier" <nicolas.schier@linux.dev>
Cc: "Trevor Gross" <tmgross@umich.edu>,
"Adam Bratschi-Kaye" <ark.email@gmail.com>,
<rust-for-linux@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
<linux-kbuild@vger.kernel.org>,
"Petr Pavlu" <petr.pavlu@suse.com>,
"Sami Tolvanen" <samitolvanen@google.com>,
"Daniel Gomez" <da.gomez@samsung.com>,
"Simona Vetter" <simona.vetter@ffwll.ch>,
"Greg KH" <gregkh@linuxfoundation.org>,
"Fiona Behrens" <me@kloenk.dev>,
"Daniel Almeida" <daniel.almeida@collabora.com>,
<linux-modules@vger.kernel.org>
Subject: Re: [PATCH v12 2/3] rust: add parameter support to the `module!` macro
Date: Wed, 07 May 2025 13:22:59 +0200 [thread overview]
Message-ID: <D9PW1NI2S6FV.8LA53J87VCML@kernel.org> (raw)
In-Reply-To: <20250506-module-params-v3-v12-2-c04d80c8a2b1@kernel.org>
On Tue May 6, 2025 at 3:02 PM CEST, Andreas Hindborg wrote:
> Add support for module parameters to the `module!` macro. Implement read
> only support for integer types without `sysfs` support.
>
> Acked-by: Petr Pavlu <petr.pavlu@suse.com> # from modules perspective
> Tested-by: Daniel Gomez <da.gomez@samsung.com>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
> rust/kernel/lib.rs | 1 +
> rust/kernel/module_param.rs | 204 +++++++++++++++++++++++++++++++++++++++++++
> rust/macros/helpers.rs | 25 ++++++
> rust/macros/lib.rs | 31 +++++++
> rust/macros/module.rs | 195 ++++++++++++++++++++++++++++++++++++-----
> samples/rust/rust_minimal.rs | 10 +++
I know this is already the 12th version, but I think this patch should
be split into the module_param module introduction, proc-macro
modifications and the sample changes.
[...]
> +/// Set the module parameter from a string.
> +///
> +/// Used to set the parameter value at kernel initialization, when loading
> +/// the module or when set through `sysfs`.
> +///
> +/// See `struct kernel_param_ops.set`.
> +///
> +/// # Safety
> +///
> +/// - If `val` is non-null then it must point to a valid null-terminated string.
> +/// The `arg` field of `param` must be an instance of `T`.
This sentence is conveying the same (or at least similar) requirement as
the bullet point below.
> +/// - `param.arg` must be a pointer to valid `*mut T` as set up by the
> +/// [`module!`] macro.
> +///
> +/// # Invariants
> +///
> +/// Currently, we only support read-only parameters that are not readable
> +/// from `sysfs`. Thus, this function is only called at kernel
> +/// initialization time, or at module load time, and we have exclusive
> +/// access to the parameter for the duration of the function.
Why is this an Invariants section?
> +///
> +/// [`module!`]: macros::module
> +unsafe extern "C" fn set_param<T>(
> + val: *const kernel::ffi::c_char,
> + param: *const crate::bindings::kernel_param,
> +) -> core::ffi::c_int
> +where
> + T: ModuleParam,
> +{
> + // NOTE: If we start supporting arguments without values, val _is_ allowed
> + // to be null here.
> + if val.is_null() {
> + // TODO: Use pr_warn_once available.
> + crate::pr_warn!("Null pointer passed to `module_param::set_param`");
> + return EINVAL.to_errno();
> + }
> +
> + // SAFETY: By function safety requirement, val is non-null and
> + // null-terminated. By C API contract, `val` is live and valid for reads
> + // for the duration of this function.
We shouldn't mention "C API contract" here and move the liveness
requirement to the safety requirements of the function. Or change the
safety requirements for this function to only be called from some
specific C API.
> + let arg = unsafe { CStr::from_char_ptr(val) };
> +
> + crate::error::from_result(|| {
> + let new_value = T::try_from_param_arg(arg)?;
> +
> + // SAFETY: `param` is guaranteed to be valid by C API contract
> + // and `arg` is guaranteed to point to an instance of `T`.
Ditto.
> + let old_value = unsafe { (*param).__bindgen_anon_1.arg as *mut T };
> +
> + // SAFETY: `old_value` is valid for writes, as we have exclusive
> + // access. `old_value` is pointing to an initialized static, and
> + // so it is properly initialized.
Why do we have exclusive access? Should be in the safety requirements.
> + unsafe { *old_value = new_value };
> + Ok(0)
> + })
> +}
[...]
> +#[doc(hidden)]
> +#[macro_export]
> +/// Generate a static [`kernel_param_ops`](srctree/include/linux/moduleparam.h) struct.
> +///
> +/// # Examples
> +///
> +/// ```ignore
> +/// make_param_ops!(
> +/// /// Documentation for new param ops.
> +/// PARAM_OPS_MYTYPE, // Name for the static.
> +/// MyType // A type which implements [`ModuleParam`].
> +/// );
> +/// ```
> +macro_rules! make_param_ops {
> + ($ops:ident, $ty:ty) => {
> + ///
Spurious newline?
> + /// Static [`kernel_param_ops`](srctree/include/linux/moduleparam.h)
> + /// struct generated by `make_param_ops`
Is it useful for this fact to be in the docs? I'd remove it.
> + #[doc = concat!("for [`", stringify!($ty), "`].")]
> + pub static $ops: $crate::bindings::kernel_param_ops = $crate::bindings::kernel_param_ops {
> + flags: 0,
> + set: Some(set_param::<$ty>),
> + get: None,
> + free: None,
> + };
> + };
> +}
> +
> +make_param_ops!(PARAM_OPS_I8, i8);
> +make_param_ops!(PARAM_OPS_U8, u8);
> +make_param_ops!(PARAM_OPS_I16, i16);
> +make_param_ops!(PARAM_OPS_U16, u16);
> +make_param_ops!(PARAM_OPS_I32, i32);
> +make_param_ops!(PARAM_OPS_U32, u32);
> +make_param_ops!(PARAM_OPS_I64, i64);
> +make_param_ops!(PARAM_OPS_U64, u64);
> +make_param_ops!(PARAM_OPS_ISIZE, isize);
> +make_param_ops!(PARAM_OPS_USIZE, usize);
> diff --git a/rust/macros/helpers.rs b/rust/macros/helpers.rs
> index a3ee27e29a6f..16d300ad3d3b 100644
> --- a/rust/macros/helpers.rs
> +++ b/rust/macros/helpers.rs
> @@ -10,6 +10,17 @@ pub(crate) fn try_ident(it: &mut token_stream::IntoIter) -> Option<String> {
> }
> }
>
> +pub(crate) fn try_sign(it: &mut token_stream::IntoIter) -> Option<char> {
> + let peek = it.clone().next();
> + match peek {
> + Some(TokenTree::Punct(punct)) if punct.as_char() == '-' => {
Should we also allow a leading `+`?
> + let _ = it.next();
> + Some(punct.as_char())
> + }
> + _ => None,
> + }
> +}
> +
> pub(crate) fn try_literal(it: &mut token_stream::IntoIter) -> Option<String> {
> if let Some(TokenTree::Literal(literal)) = it.next() {
> Some(literal.to_string())
> @@ -86,3 +97,17 @@ pub(crate) fn function_name(input: TokenStream) -> Option<Ident> {
> }
> None
> }
> +
> +/// Parse a token stream of the form `expected_name: "value",` and return the
> +/// string in the position of "value".
> +///
> +/// # Panics
> +///
> +/// - On parse error.
> +pub(crate) fn expect_string_field(it: &mut token_stream::IntoIter, expected_name: &str) -> String {
> + assert_eq!(expect_ident(it), expected_name);
> + assert_eq!(expect_punct(it), ':');
> + let string = expect_string(it);
> + assert_eq!(expect_punct(it), ',');
This won't allow omitting the trailing comma.
> + string
> +}
[...]
> @@ -186,33 +336,35 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
> let info = ModuleInfo::parse(&mut it);
>
> let mut modinfo = ModInfoBuilder::new(info.name.as_ref());
> - if let Some(author) = info.author {
> - modinfo.emit("author", &author);
> + if let Some(author) = &info.author {
> + modinfo.emit("author", author);
> }
> - if let Some(authors) = info.authors {
> + if let Some(authors) = &info.authors {
> for author in authors {
> - modinfo.emit("author", &author);
> + modinfo.emit("author", author);
> }
> }
> - if let Some(description) = info.description {
> - modinfo.emit("description", &description);
> + if let Some(description) = &info.description {
> + modinfo.emit("description", description);
> }
> modinfo.emit("license", &info.license);
> - if let Some(aliases) = info.alias {
> + if let Some(aliases) = &info.alias {
> for alias in aliases {
> - modinfo.emit("alias", &alias);
> + modinfo.emit("alias", alias);
> }
> }
> - if let Some(firmware) = info.firmware {
> + if let Some(firmware) = &info.firmware {
> for fw in firmware {
> - modinfo.emit("firmware", &fw);
> + modinfo.emit("firmware", fw);
I don't like that you have to change all of these. Can't you just take a
`&[Parameter]` argument in `emit_params` instead of the whole
`ModuleInfo` struct?
---
Cheers,
Benno
> }
> }
next prev parent reply other threads:[~2025-05-07 11:23 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-05-06 13:02 [PATCH v12 0/3] rust: extend `module!` macro with integer parameter support Andreas Hindborg
2025-05-06 13:02 ` [PATCH v12 1/3] rust: str: add radix prefixed integer parsing functions Andreas Hindborg
2025-05-07 8:58 ` Benno Lossin
2025-05-07 9:15 ` Andreas Hindborg
2025-05-07 11:29 ` Benno Lossin
2025-05-06 13:02 ` [PATCH v12 2/3] rust: add parameter support to the `module!` macro Andreas Hindborg
2025-05-07 11:22 ` Benno Lossin [this message]
2025-06-11 10:31 ` Andreas Hindborg
2025-06-11 12:24 ` Andreas Hindborg
2025-06-11 12:36 ` Miguel Ojeda
2025-06-11 13:28 ` Andreas Hindborg
2025-06-12 7:52 ` Benno Lossin
2025-06-12 11:05 ` Andreas Hindborg
2025-06-11 12:39 ` Miguel Ojeda
2025-05-06 13:02 ` [PATCH v12 3/3] modules: add rust modules files to MAINTAINERS Andreas Hindborg
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=D9PW1NI2S6FV.8LA53J87VCML@kernel.org \
--to=lossin@kernel.org \
--cc=a.hindborg@kernel.org \
--cc=alex.gaynor@gmail.com \
--cc=aliceryhl@google.com \
--cc=ark.email@gmail.com \
--cc=benno.lossin@proton.me \
--cc=bjorn3_gh@protonmail.com \
--cc=boqun.feng@gmail.com \
--cc=da.gomez@samsung.com \
--cc=dakr@kernel.org \
--cc=daniel.almeida@collabora.com \
--cc=gary@garyguo.net \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kbuild@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-modules@vger.kernel.org \
--cc=masahiroy@kernel.org \
--cc=mcgrof@kernel.org \
--cc=me@kloenk.dev \
--cc=nathan@kernel.org \
--cc=nicolas.schier@linux.dev \
--cc=ojeda@kernel.org \
--cc=petr.pavlu@suse.com \
--cc=rust-for-linux@vger.kernel.org \
--cc=samitolvanen@google.com \
--cc=simona.vetter@ffwll.ch \
--cc=tmgross@umich.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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 read-only IMAP folder(s) and NNTP newsgroup(s).