From: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
To: Antonio Martorana <amartora@codeaurora.org>
Cc: rust-for-linux <rust-for-linux@vger.kernel.org>
Subject: Re: [RFC v2] soc: qcom: socinfo.rs: Add Rust Socinfo Driver implementation
Date: Sat, 21 Aug 2021 00:18:30 +0200 [thread overview]
Message-ID: <CANiq72=QzrN=Mo5M377ijeTy0ZV-7r33UCRXB1JyQLx0Z9KdWQ@mail.gmail.com> (raw)
In-Reply-To: <1629488333-305361-1-git-send-email-amartora@codeaurora.org>
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
next prev parent reply other threads:[~2021-08-20 22:18 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-17 1:23 [RFC] soc: qcom: socinfo.rs: Add Rust Socinfo Driver implementation Antonio Martorana
2021-08-17 16:43 ` Miguel Ojeda
2021-08-18 9:36 ` Wei Liu
2021-08-20 19:38 ` [RFC v2] " Antonio Martorana
2021-08-20 22:18 ` Miguel Ojeda [this message]
2021-08-20 23:05 ` amartora
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CANiq72=QzrN=Mo5M377ijeTy0ZV-7r33UCRXB1JyQLx0Z9KdWQ@mail.gmail.com' \
--to=miguel.ojeda.sandonis@gmail.com \
--cc=amartora@codeaurora.org \
--cc=rust-for-linux@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).