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@xxxxxxxxxxxxxx> 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
