"Benno Lossin" <benno.lossin@xxxxxxxxx> writes: > On 02.08.24 12:27, Andreas Hindborg wrote: >> "Benno Lossin" <benno.lossin@xxxxxxxxx> writes: >>> 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 confused myself as well I think. I am talking about (1). Let me try >> again. >> >> If we support `NOARG_ALLOWED` (`KERNEL_PARAM_OPS_FL_NOARG` in C. I >> should change the flag name in Rust), modules can optionally support >> some parameters where users can pass parameters either as >> `my_module.param=value` or `my_module.param`. Thus, at the level of >> `try_from_param_arg`, we need to represent two cases: parameter passed >> without value, and parameter passed with value. A third case, parameter >> not passed at all, is equivalent to `try_from_param_arg` never being >> called. In C this is undetectable for the predefined parameter types. I >> wanted the double option to detect this. But I guess it does not make >> sense. > > My idea was to have an `const DEFAULT: Option<Self>` to represent the > following: > (1) if `DEFAULT == None`, then `KERNEL_PARAM_OPS_FL_NOARG` is not set > and thus either the `module!` user-specified default value is used, > or it is specified via `my_module.param=value` and > `try_from_param_arg` is called. > (2) if `DEFAULT == Some(d)`, then `KERNEL_PARAM_OPS_FL_NOARG` is set and > when `NULL` is given to `kernel_param_ops.set`, the parameter value > is set to `d`, otherwise `try_from_param_arg` is called. > > But I think I agree with you removing `NOARG_ALLOWED`, see below. Great! > >> At a higher level where the bindings supply the parsing functions, we >> can decide that passing an argument without a value yields a default >> parameter value. C does this for the predefined `bool` type. The >> predefined integer types does not support omitting the value. >> >> This patch only supports the higher level predefined parameter types, >> and does not allow modules to supply their own parameter parsing >> functions. None of the types we implement in this patch support passing >> the argument without a value. This is intentional to mirror the C >> implementation. >> >> To that end, I removed `NOARG_ALLOWED`, and changed the parsing function >> trait to: >> >> fn try_from_param_arg(arg: &'static [u8]) -> Result<Self>; >> >> If/when we start supporting types like `bool` or custom parsing >> functions provided by the module, we will have to update the signature >> to take an `Option` to represent the case where the user passed an >> argument without a value. However, to mimic C, the function must always >> return a value if successful, even if the user did not supply a value to >> the argument. >> >> Two different default values are in flight here. 1) the value that the >> parameter will have before the kernel calls `try_from_param_arg` via >> `set_param` and 2) the value to return from `try_from_param_arg` if the >> user did not pass a value with the argument. >> >> For a `bool` 1) would usually be `false` and 2) would always be `true`. >> >> For predefined types the module would not customize 2), but 1) is useful >> to customize. For custom types where the module supplies the parsing >> function, 2) would be implicitly given by the module in the parsing >> function. >> >> In this patch set, we only have 1 default value, namely 1). We do not >> need 2) because we do not support parameters without values. > > I am not sure that putting the default value of `my_module.param` into > the `ModuleParam` trait is a good idea. It feels more correct to me to > add an optional field to the part in `module!` that can be set to denote > this default value -- we might also want to change the name of > `default`, what do you think of `default_inactive` and `default_active`? For all the predefined parameter types, the module code would never set the `default_active` value. It should be part of the data parsing specification for the predefined argument types. For custom parsing functions implemented in the module, it makes sense specifying this value in the trait. > Since one might want an option by default be `true` and when one writes > `my_module.param`, it should be `false`. I think this would be confusing, since the default C parameter types do not have this semantic. Specifying a default true boolean as an argument does not make it false in C land. TBH this also feels like the most sane thing to do. > Also as the C side shows, having default values for integer types is not > really a good idea, since you might want a non-zero default value. > If one does not specify the `default_active` value, then the > `KERNEL_PARAM_OPS_FL_NOARG` is not set. I would rather predefine `KERNEL_PARAM_OPS_FL_NOARG` in the trait implementation like C does. We would set the flag for `bool` but not for integer types. For custom implementations of the trait, the developer can do whatever they want. > If you don't want to implement this (which I can fully understand, since > we might get `syn` before anyone needs params with default values), then > we should write this idea down (maybe in an issue?). But regardless, I > would like to know your opinion on this topic. We can create he issue if this series is accepted without the feature. > >>>> 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? >> >> The current patch uses the default value given in the `module!` macro to >> initialize the parameter value. > > But what drops the default value, when an actual value is specified via > `my_module.param=value`? (or is the default value only assigned when > nothing is specified?) Some more confusion maybe. When I wrote "drop the default value", I meant remove it from the code, not specify it. I take it back though. We would use the value given in the `module!` macro `default` field as `default_inactive`. `default_active` is not required for integer types, since they always require a value for the argument. But the drop would happen in `set_param` when the return value of `core::ptr::replace` is dropped. > >>> 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`. >> >> We should plan on eventually supporting `String`. > > Yes. > >>>>>> 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. >> >> I just don't see the point. Just have the user pass `my_module.param=1` >> instead of omitting the value. Having multiple ways of specifying for >> instance a true boolean just leads to confusion. Some boolean parameters >> have a default value of `true`, for instance `nvme.use_cmb_sqes`. In >> this case specifying `nvme.use_cmb_sqes` has no effect, even though one >> might think it has. > > This just shows to me that a "global" default in `ModuleParam` is wrong, > since for `use_cmb_sqes` one could either have a negated flag, or no > default value, forcing the user to write `nvme.use_cmb_sqes=false`. I do not think it is a good idea to be able to override the value assigned to a parameter when it is given without a value. The more predictable a user interface is, the better. Best regards, Andreas