Re: [RFC v2] soc: qcom: socinfo.rs: Add Rust Socinfo Driver implementation

[amartora@codeaurora.org]

Just CC'ing the recipients who were missed, and thank you for all the 
feedback
  its much appreciated.

Cheers,
Antonio Martorana

On 2021-08-20 15:18, Miguel Ojeda wrote:
> Hi Antonio,
> 
> Thanks again for working on this.
> 
> On Fri, Aug 20, 2021 at 9:39 PM Antonio Martorana
> <amartora@codeaurora.org> wrote:
>> 
>> V2: Addressed formating issues, added conditional compilation to 
>> debug_fs
>> variables, fixed Kconfig, and still working on removing references to 
>> bindings::
>> by abstracting FFI functions through rust/ .
> 
> Normally the changelog goes outside the commit message, i.e. after 
> `---`.
> 
>> +/// SoC version type with major number in the upper 16 bits and minor
>> +/// number in the lower 16 bits.
>> +
>> +const fn socinfo_major(ver: u32) -> u32 {
> 
> Doc comments should "touch" the function they document. But using
> `///` like shown here would document only `socinfo_major`, and what
> the comment says is not really the function documentation. What I
> meant in v1 is that perhaps you can find a better way to document all
> of them, or none.
> 
> Given `socinfo_version` is not used, I would remove that one, and put
> "// The upper 16 bits are the major." and "// The lower 16 bits are
> the minor." as normal comments inside the function body. Even better,
> you could create a `SocInfoVersion` type.
> 
>> +    ///
>> +    /// SMEM Image table indices
>> +    ///
> 
> No empty comment lines before/after, please. In addition, `///` is a
> doc comment, not a normal comment. You are trying to give a heading
> for a set of `const`s, not document the first one.
> 
> We should add a lint for the former, and probably we could also try
> one for the latter, too...
> 
>> +#[cfg(CONFIG_DEBUG_FS)]
>> +const PMIC_MODELS: [&str; 37] = [
> 
> This is fine, but since you have created a `config_debug_fs` module,
> perhaps you could move everything inside it.
> 
> Related: perhaps the name for that module could be something simpler,
> like `debugfs`.
> 
>> +    unsafe {
>> +        let ref_mut_seq_priv = seq_private.as_mut().unwrap();
>> +    }
> 
> The `// SAFETY: ` comments in several places are missing.
> 
>> +fn qcom_show_pmic_model(seq: &mut bindings::seq_file, p: &mut 
>> c_types::c_void) -> Result {
> 
> Several of these functions cannot fail -- so why `Result`?
> 
> ...ah, I see, you are panicking now -- but then you don't need the
> `Result`. If this was userspace and you were not expecting a failure,
> then yes, panic might have been a good idea. However, here we are
> supposed to return the error like you did in v1. When I mentioned you
> should use `Result`, it means you should return the `EINVAL` as such
> (using the pre-defined error constants we have in
> `rust/kernel/error.rs`, which are in the prelude for ease-of-use, see
> e.g. 
> https://rust-for-linux.github.io/docs/kernel/prelude/struct.Error.html).
> 
>> +    // SAFETY: `socinfo` is guaranteed to be:
>> +    // - A valid (and instalized), non-null pointer
>> +    // - Is properly aligned
>> +    // - Adheres to aliasing rules
> 
> The `// SAFETY: ` comment should explain why the callee's
> preconditions hold, not just state them (which does not help much a
> future reader searching for UB).
> 
>> +    unsafe {
>> +        let mut_socinfo = socinfo.as_mut().unwrap();
> 
> As mentioned in v1, `unsafe` blocks should be split and reduced as
> much as possible.
> 
>> +        let test_model = socinfo_minor((*socinfo).pmic_model);
>> +        model = test_model;
> 
> Why is `test_model` there?
> 
>> +    let mut check_valid_model: bool = false;
>> +    if !PMIC_MODELS[model as usize].is_empty() {
>> +        check_valid_model = true;
>> +    }
> 
> Try to avoid mutability where possible, e.g.:
> 
>     let valid_model = !PMIC_MODELS[model as usize].is_empty();
> 
>> +    if model < PMIC_MODELS.len() as u32 && check_valid_model {
> 
> Something looks odd -- we are checking whether the model is within
> `PMIC_MODELS`, but you already used it to index (which is safe, but
> because it will panic if wrong -- which we do not want!).
> 
>> +#[cfg(CONFIG_DEBUG_FS)]
>> +fn qcom_show_pmic_model_array(seq: &mut bindings::seq_file, p: &mut 
>> c_types::c_void) -> Result {
>> +    Ok(())
>> +}
>> +
>> +#[cfg(CONFIG_DEBUG_FS)]
>> +fn qcom_show_pmic_die_revision(seq: &mut bindings::seq_file, p: &mut 
>> c_types::c_void) -> Result {
>> +    Ok(())
>> +}
>> +
>> +#[cfg(CONFIG_DEBUG_FS)]
>> +fn qcom_show_chip_id(seq: &mut bindings::seq_file, p: &mut 
>> c_types::c_void) -> Result {
>> +    Ok(())
>> +}
> 
> Even if not commented out, this is still dead code -- it is best to
> remove it until you implement them.
> 
>> +    fmt: bindings::__le32,
> 
> Same thing about `bindings::` from v1 etc. -- if this were not a proof
> of concept, it should be properly abstracted.
> 
>> +const SOC_ID: [SocId; 105] = [
> 
> For lots of data, it is usually better to use `static` to have a
> single instance somewhere in memory (vs. e.g. inlining).
> 
>> +    SocId {
>> +        id: 87,
>> +        name: c_str!("MSM8960"),
>> +    },
> 
> For repetitive things like this (and especially if the automatic
> formatting makes it look bad!), a local "macro by example" is usually
> a good approach, e.g.:
> 
>     soc_ids!(3,
>          87 "MSM8960"
>         109 "APQ8064"
>         122 "MSM8660A etc etc"
>     );
> 
> or any other syntax you like for the particular case. It looks much
> better, and separates the details of the type from the data table.
> 
> The macro can be something like:
> 
>     macro_rules! soc_ids(
>         ($length:literal, $($id:literal $name:literal)*) => {
>             static SOC_ID: [SocID; $length] = [
>                 $(SocID { id: $id, name: $name },)*
>             ];
>         }
>     );
> 
> https://godbolt.org/z/dWofxYn19 to play with it.
> 
>> +struct SocId {
>> +    id: u32,
>> +    name: &'static str::CStr,
>> +}
> 
> Even if Rust allows otherwise, I would still try to put declarations
> before they are used.
> 
>> +fn socinfo_machine(id: &u32) -> Option<&'static str::CStr> {
> 
> It is simpler to pass an integer as-is, rather than a pointer to it.
> 
>> +    for idx in SOC_ID {
> 
> You probably want `&SOC_ID` here (see the `static` vs. `const`
> discussion above).
> 
>> +        if idx.id == *id {
>> +            return Some(idx.name);
>> +        }
>> +    }
>> +    None
> 
> Loops like this are good candidates for a functional style -- something 
> like:
> 
>     SOC_ID.iter().find(|x| x.id == id).map(|x| x.name)
> 
> Whether this looks better or not, of course, is up for debate ;)
> 
>> +        Ok(drv_data)
>> +    }
>> +    fn remove(device_id: i32, _drv_data: Self::DrvData) -> Result {
> 
> Newline between functions, please.
> 
> Normally we only prefix by underscore if unused.
> 
> Cheers,
> Miguel