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

[Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>]

Hi Antonio,

Thanks a lot for giving Rust in the kernel a try!

Cc'ing the socinfo maintainers.

A few comments inline. In general, you should seek to avoid `unsafe`
in the Rust module as much as possible by providing safe abstractions
inside the `kernel` crate instead, which are the ones dealing with the
C-Rust interface / bindings.

On Tue, Aug 17, 2021 at 3:24 AM Antonio Martorana
<amartora@codeaurora.org> wrote:
>
> +       depends on HAS_RUST && QCOM_SMEM

This should be `RUST`, not `HAS_RUST` (the former is whether it is
actually enabled, the latter whether the toolchain was found).

> @@ -0,0 +1,464 @@

The SPDX identifier is missing at the top.

> +#![feature(allocator_api,global_asm)]

Please format the code (you can use `make rustfmt` or manually call
the `rustfmt` tool yourself).

Also, please pass the linter too if you did not do it (`CLIPPY=1`).

> +module! {
> +    type: SocInfoDriver,
> +    name: b"socinfo_rust",
> +    author: b"Antonio Martorana",
> +    description: b"QCOM socinfo rust implementation",
> +    license: b"GPL v2",
> +}

This is a proof of concept, so it is OK, but I am not sure if we
should say "rust" in the description for actual non-proof-of-concept
modules. My guess is that most maintainers will only want to
maintainer a single module, whether in C or Rust.

> +/*
> + * SMEM item id, used to acquire handles to respective
> + * SMEM region.
> + */
> +const SMEM_HW_SW_BUILD_ID: u32 = 137;

In general, we do not use `/*`-style comments. In addition, this
should be a documentation comment, i.e. `///`.

Same for other places.

> +/* C code has #ifdef */
> +const SMEM_IMAGE_VERSION_BLOCKS_COUNT: usize = 32;
> +const SMEM_IMAGE_VERSION_SIZE: usize = 4096;
> +const SMEM_IMAGE_VERSION_NAME_SIZE: usize = 75;
> +const SMEM_IMAGE_VERSION_VARIANT_SIZE: usize = 20;
> +const SMEM_IMAGE_VERSION_OEM_SIZE: usize = 32;

We have support for conditional compilation based on the kernel
configuration via e.g. an attribute like `#[cfg(CONFIG_X)]`.

Ideally you can put this inside a module, which would allow you to
have a single condition compilation avoid having to repeat the
attribute, as well as avoiding repeated prefixes like
`SMEM_IMAGE_VERSION`.

> +/*
> + * SMEM Image table indices
> + */
> +const SMEM_IMAGE_TABLE_BOOT_INDEX: u32 = 0;
> +const SMEM_IMAGE_TABLE_TZ_INDEX: u32 = 1;
> +const SMEM_IMAGE_TABLE_RPM_INDEX: u32 = 3;
> +const SMEM_IMAGE_TABLE_APPS_INDEX: u32 = 10;
> +const SMEM_IMAGE_TABLE_MPSS_INDEX: u32 = 11;
> +const SMEM_IMAGE_TABLE_ADSP_INDEX: u32 = 12;
> +const SMEM_IMAGE_TABLE_CNSS_INDEX: u32 = 13;
> +const SMEM_IMAGE_TABLE_VIDEO_INDEX: u32 = 14;
> +const SMEM_IMAGE_VERSION_TABLE: u32 = 496;

Same here -- this could be a `mod` and a doc comment on it.

> +struct SocInfo{
> +    fmt: bindings::__le32,

Rust modules should not access `bindings` in general -- abstractions
should be provided in `rust/`.

Also, `__le32` is for `sparse` -- to do something similar, we could
use a wrapper type that encodes the endianness.

> +    unsafe {let ref_mut_seq_priv = seq_private.as_mut().unwrap(); }

Unsafe blocks must have a `SAFETY: ` annotation, justifying all the
requirements of `as_mut()` (alignment, no aliasing, etc.).

They should also be as small as possible (to clearly indicate the part
that is unsafe), without using a single block for several unsafe
operations.

> +    if model < 0{
> +        return -22; //EINVAL
> +    }

We have the constants available. Also, the function should return a
`Result`, not a naked integer.

> +        unsafe{ bindings::seq_printf(seq as *mut bindings::seq_file,format,PMIC_MODELS[model as usize])};

`seq_printf` should likely be a macro like `pr_info!` etc. that we have.

> +/*
> +fn qcom_show_pmic_model_array(seq: &mut bindings::seq_file, p: &mut c_types::c_void) -> u32{
> +
> +}
> +
> +fn qcom_show_pmic_die_revision(seq: &mut bindings::seq_file, p: &mut c_types::c_void) -> u32{
> +
> +}
> +
> +fn qcom_show_chip_id(seq: &mut bindings::seq_file, p: &mut c_types::c_void) -> u32{
> +
> +}
> +
> +*/

Commented out code (also in other places).

Cheers,
Miguel