From: Andreas Hindborg <a.hindborg@kernel.org>
To: "Benno Lossin" <lossin@kernel.org>
Cc: "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>,
"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, 11 Jun 2025 14:24:08 +0200 [thread overview]
Message-ID: <87wm9ixy8n.fsf@kernel.org> (raw)
In-Reply-To: <87plfazi13.fsf@kernel.org> (Andreas Hindborg's message of "Wed, 11 Jun 2025 12:31:20 +0200")
Andreas Hindborg <a.hindborg@kernel.org> writes:
> "Benno Lossin" <lossin@kernel.org> writes:
>
>> 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.
>>
>
> OK.
>
>> [...]
>>
>>> +/// 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.
>
> Yes, you are right. At any rate the wording is wrong, a pointer cannot
> "be an instance", it can point to one.
>
>>
>>> +/// - `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?
>
> Looks like a mistake, I'll change it to "# Note".
>
>>
>>> +///
>>> +/// [`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.
>
> OK.
>
>>
>>> + 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.
>
> OK.
>
>>
>>> + 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.
>
> Will add this.
>
>>
>>> + 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?
>
> Will remove.
>
>>
>>> + /// 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.
>
> OK.
>
Clippy thinks it is useful:
error: missing documentation for a static
--> /home/aeh/src/linux-rust/module-params/rust/kernel/module_param.rs:182:9
|
182 | pub static $ops: $crate::bindings::kernel_param_ops = $crate::bindings::kernel_param_ops {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
...
191 | make_param_ops!(PARAM_OPS_I8, i8);
| --------------------------------- in this macro invocation
|
So either we need to `#[allow(missing_docs)]` or just add the line back. What do you prefer?
Best regards,
Andreas Hindborg
next prev parent reply other threads:[~2025-06-11 12:24 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
2025-06-11 10:31 ` Andreas Hindborg
2025-06-11 12:24 ` Andreas Hindborg [this message]
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=87wm9ixy8n.fsf@kernel.org \
--to=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=lossin@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).