"Benno Lossin" <benno.lossin@xxxxxxxxx> writes:
> On 01.08.24 15:40, Andreas Hindborg wrote:
>> "Benno Lossin" <benno.lossin@xxxxxxxxx> writes:
>>> On 01.08.24 13:29, Andreas Hindborg wrote:
>>>> "Benno Lossin" <benno.lossin@xxxxxxxxx> writes:
>>>>> On 05.07.24 13:15, Andreas Hindborg wrote:
>>>>>> +
>>>>>> +/// Types that can be used for module parameters.
>>>>>> +///
>>>>>> +/// Note that displaying the type in `sysfs` will fail if
>>>>>> +/// [`core::str::from_utf8`] (as implemented through the [`core::fmt::Display`]
>>>>>> +/// trait) writes more than [`PAGE_SIZE`] bytes (including an additional null
>>>>>> +/// terminator).
>>>>>> +///
>>>>>> +/// [`PAGE_SIZE`]: `bindings::PAGE_SIZE`
>>>>>> +pub trait ModuleParam: core::fmt::Display + core::marker::Sized {
>>>>>> + /// The `ModuleParam` will be used by the kernel module through this type.
>>>>>> + ///
>>>>>> + /// This may differ from `Self` if, for example, `Self` needs to track
>>>>>> + /// ownership without exposing it or allocate extra space for other possible
>>>>>> + /// parameter values. This is required to support string parameters in the
>>>>>> + /// future.
>>>>>> + type Value: ?Sized;
>>>>>> +
>>>>>> + /// Whether the parameter is allowed to be set without an argument.
>>>>>> + ///
>>>>>> + /// Setting this to `true` allows the parameter to be passed without an
>>>>>> + /// argument (e.g. just `module.param` instead of `module.param=foo`).
>>>>>> + const NOARG_ALLOWED: bool;
>>>>>
>>>>> I think, there is a better way of doing this. Instead of this bool, we
>>>>> do the following:
>>>>> 1. have a `const DEFAULT: Option<Self>`
>>>>> 2. change the type of the argument of `try_from_param_arg` to
>>>>> `&'static [u8]`
>>>>>
>>>>> That way we don't have the weird behavior of `try_from_param_arg` that
>>>>> for params that don't have a default value.
>>>>
>>>> Since we have no parameter types for which `NOARG_ALLOWED` is true in
>>>> this patch set, it is effectively dead code. I will remove it.
>>>
>>> Hmm what parameters actually are optional? I looked at the old rust
>>> branch and only `bool` is marked as optional. Are there others?
>>>
>>> If it is used commonly for custom parameters (I could imagine that Rust
>>> modules have enums as parameters and specifying nothing could mean the
>>> default value), then it might be a good idea to just include it now.
>>> (otherwise we might forget the design later)
>>
>> As far as I can tell from the C code, all parameters are able to have
>> the `NOARG` flag set. We get a null pointer in the callback in that
>> case.
>>
>> If we want to handle this now, we could drop the `default` field
>> in the Rust module macro. There is no equivalent in the C macros.
>> And then use an `Option<Option<_>>` to represent the value. `None` would
>> be an unset parameter. `Some(None)` would be a parameter without a
>> value. `Some(Some(_))` would be a set parameter with a value. We could
>> probably fix the types so that only parameters with the `NOARG` flag use
>> the double option, others use a single option.
>
> What did you think of my approach that I detailed above? I would like to
> avoid `Option<Option<_>>` if we can.
How would you represent the case when the parameter is passed without a
value and a default is given in `module!`?
I think we need to drop the default value if we adopt the arg without
value scheme.
>
>> Or we could just not adopt this feature in the Rust abstractions.
>
> Depends on how common this is and if people need to use it. I think that
> what I proposed above isn't that complex, so it should be easy to
> implement.
Rust modules would just force people to add "my_module.param=1" instead
of just "my_module.param". I think that is reasonable.
>
>>>>>> + param_type.to_string(),
>>>>>> + param_ops_path(¶m_type).to_string(),
>>>>>> + );
>>>>>> +
>>>>>> + self.emit_param("parmtype", ¶m_name, ¶m_kernel_type);
>>>>>
>>>>> Is the spelling intentional? "parmtype"?
>>>>
>>>> This is intentional. I don't think the kernel is ever parsing this, but
>>>> it is parsed by the `modinfo` tool.
>>>
>>> Hmm, why is it not `paramtype`? Does that conflict with something?
>>
>> You would have to take that up with the maintainer(s) of the `modinfo`
>> tool. The name is externally dictated [1].
>
> Thanks for the pointer that's what I wanted to know (is it given from
> somewhere else? or is it a name that we came up with), then it's fine :)
>
>>>>>> + // Note: when we enable r/w parameters, we need to lock here.
>>>>>> +
>>>>>> + // SAFETY: Parameters do not need to be locked because they are
>>>>>> + // read only or sysfs is not enabled.
>>>>>> + unsafe {{
>>>>>> + <{param_type_internal} as kernel::module_param::ModuleParam>::value(
>>>>>> + &__{name}_{param_name}_value
>>>>>> + )
>>>>>> + }}
>>>>>> + }}
>>>>>> + ",
>>>>>> + name = info.name,
>>>>>> + param_name = param_name,
>>>>>> + param_type_internal = param_type_internal,
>>>>>> + );
>>>>>> +
>>>>>> + let kparam = format!(
>>>>>> + "
>>>>>> + kernel::bindings::kernel_param__bindgen_ty_1 {{
>>>>>> + // SAFETY: Access through the resulting pointer is
>>>>>> + // serialized by C side and only happens before module
>>>>>> + // `init` or after module `drop` is called.
>>>>>> + arg: unsafe {{ &__{name}_{param_name}_value }}
>>>>>> + as *const _ as *mut core::ffi::c_void,
>>>>>
>>>>> Here you should use `addr_of[_mut]!` instead of taking a reference.
>>>>
>>>> This is a static initializer, so it would be evaluated in const context.
>>>> At that time, this is going to be the only reference to
>>>> `&__{name}_{param_name}_value` which would be const. So it should be
>>>> fine?
>>>
>>> When compiling this [1] with a sufficiently new Rust version, you will
>>> get an error:
>>>
>>> warning: creating a shared reference to mutable static is discouraged
>>> --> src/main.rs:4:22
>>> |
>>> 4 | let x = unsafe { &foo };
>>> | ^^^^ shared reference to mutable static
>>> |
>>> = note: for more information, see issue #114447 <https://github.com/rust-lang/rust/issues/114447>
>>> = note: this will be a hard error in the 2024 edition
>>> = note: this shared reference has lifetime `'static`, but if the static ever gets mutated, or a mutable reference is created, then any further use of this shared reference is Undefined Behavior
>>> = note: `#[warn(static_mut_refs)]` on by default
>>> help: use `addr_of!` instead to create a raw pointer
>>> |
>>> 4 | let x = unsafe { addr_of!(foo) };
>>> | ~~~~~~~~~~~~~
>>>
>>> [1]: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=c914a438938be6f5fc643ee277efa1d1
>>>
>>> So I think we should start using `addr_of!` for mutable static now.
>>
>> Oh. Thanks for the pointer.
>>
>> Hmm, `addr_of_mut!` still requires the unsafe block. Hopefully that goes
>> away as well with the feature you linked as well.
>
> I think that will take some time until it is gone.
>
>> This also requires `const_mut_refs`, but as I recall that is going to be
>> stabilized soon.
>
> That should only be needed if you need `addr_of_mut!`, but IIUC, you
> only need `addr_of!`, right?
The pointer we create here is the one passed to `free` in
module_param.rs, so it will eventually be used as `&mut T`.
Best regards,
Andreas
