On 01.08.24 17:11, Andreas Hindborg wrote: > "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 am a bit confused, there are two default values here: (1) the value returned by `try_from_param_arg(None)`. (2) the value given by the user to the `module!` macro. I am talking about changing the definition of ModuleParam, so (1). I get the feeling that you are talking about (2), is that correct? > I think we need to drop the default value if we adopt the arg without > value scheme. Yes definitely. I don't see anything in the code doing this currently, right? We could also only allow `Copy` values to be used as Parameters (but then `str` cannot be used as a parameter...), since they can't implement `Drop`. >>> 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. Eh, why do we want to give that up? I don't think it's difficult to do. >>>>>>> + // 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`. Oh then the original code is definitely wrong, since it creates a shared reference. Yeah then you should use `addr_of_mut!`. --- Cheers, Benno