Andreas Hindborg <a.hindb...@kernel.org> writes:
> "Benno Lossin" <los...@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.pa...@suse.com> # from modules perspective
>>> Tested-by: Daniel Gomez <da.go...@samsung.com>
>>> Reviewed-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>
>>> Signed-off-by: Andreas Hindborg <a.hindb...@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
