* Re: [RFC] soc: qcom: socinfo.rs: Add Rust Socinfo Driver implementation
[not found] <1629163412-157074-1-git-send-email-amartora@codeaurora.org>
@ 2021-08-17 16:43 ` Miguel Ojeda
[not found] ` <1629488333-305361-1-git-send-email-amartora@codeaurora.org>
1 sibling, 0 replies; 2+ messages in thread
From: Miguel Ojeda @ 2021-08-17 16:43 UTC (permalink / raw)
To: Antonio Martorana
Cc: Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho, rust-for-linux,
Trilok Soni, Elliot Berman, agross, bjorn.andersson,
linux-arm-msm
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
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [RFC v2] soc: qcom: socinfo.rs: Add Rust Socinfo Driver implementation
[not found] ` <CANiq72=QzrN=Mo5M377ijeTy0ZV-7r33UCRXB1JyQLx0Z9KdWQ@mail.gmail.com>
@ 2021-08-20 23:05 ` amartora
0 siblings, 0 replies; 2+ messages in thread
From: amartora @ 2021-08-20 23:05 UTC (permalink / raw)
To: Miguel Ojeda
Cc: rust-for-linux, Alex Gaynor, Wedson Almeida Filho, Trilok Soni,
Elliot Berman, linux-arm-msm, Wei Liu
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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-08-20 23:05 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <1629163412-157074-1-git-send-email-amartora@codeaurora.org>
2021-08-17 16:43 ` [RFC] soc: qcom: socinfo.rs: Add Rust Socinfo Driver implementation Miguel Ojeda
[not found] ` <1629488333-305361-1-git-send-email-amartora@codeaurora.org>
[not found] ` <CANiq72=QzrN=Mo5M377ijeTy0ZV-7r33UCRXB1JyQLx0Z9KdWQ@mail.gmail.com>
2021-08-20 23:05 ` [RFC v2] " amartora
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).