Message-ID: <CAGSQo03uOmC2G-MnkY-8_R8Bo_s7Q97xEh-re4WPqSuWkotOuA@mail.gmail.com>
Date: Sat, 13 Dec 2025 08:58:56 -0800
From: Matthew Maurer <mmaurer@...gle.com>
To: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
Cc: Bjorn Andersson <andersson@...nel.org>, Konrad Dybcio <konradybcio@...nel.org>, 
	Satya Durga Srinivasu Prabhala <satyap@...cinc.com>, Miguel Ojeda <ojeda@...nel.org>, Boqun Feng <boqun.feng@...il.com>, 
	Gary Guo <gary@...yguo.net>, Björn Roy Baron <bjorn3_gh@...tonmail.com>, 
	Benno Lossin <lossin@...nel.org>, Andreas Hindborg <a.hindborg@...nel.org>, 
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>, 
	Danilo Krummrich <dakr@...nel.org>, Trilok Soni <tsoni@...cinc.com>, linux-kernel@...r.kernel.org, 
	linux-arm-msm@...r.kernel.org, rust-for-linux@...r.kernel.org
Subject: Re: [PATCH RFC] soc: qcom: socinfo: Re-implement in Rust

On Fri, Dec 12, 2025 at 7:55 PM Dmitry Baryshkov
<dmitry.baryshkov@....qualcomm.com> wrote:
>
> On Sat, Dec 13, 2025 at 12:36:00AM +0000, Matthew Maurer wrote:
> > Re-implements qcom-socinfo driver in Rust, using `Scoped`-based DebugFS
> > bindings.
> >
> > Signed-off-by: Matthew Maurer <mmaurer@...gle.com>
> > ---
> > This patch converts the QC socinfo driver to Rust, intended to be the
> > first Rust driver in Android that is owned by a vendor rather than the
> > platform.
> >
> > This driver is currently quirk-compatible in DebugFS-exported values. If
> > the maintainers do not believe that maintaining the exact formats is a
> > benefit, we could simplify the code further by dropping some of the
> > custom formatting functions used to match the output.
> >
> > I sent an earlier draft of this privately a while back that was much
> > rougher, but didn't get much feedback.
> >
> > Now that we've got all the interfaces we need for this driver reasonably
> > available (DebugFS was the one that took a bit here), I wanted to bring
> > it up again, this time as an RFC. With the new features, the only place
> > it needs `unsafe` is to define an abstraction over `qcom_get_smem`.
> >
> > I have tested this on a SM8650-HDK by diffing the contents of the
> > exported DebugFS and examining the files under /sys/bus/soc/devices/soc0
> >
> > While I believe I have everything correctly exported via DebugFS, I have
> > not checked equivalence across a large swath of devices, only the one.
> > ---
>
> > +
> > +pub(crate) fn qcom_smem_get(host: i32, item: u32) -> Result<&'static [u8]> {
> > +    let mut size = 0;
> > +    // SAFETY: qcom_smem_get only requires that the size pointer be a writable size_t,
> > +    // host and item are error checked in the qcom_smem module.
> > +    let err_ptr = unsafe { kernel::bindings::qcom_smem_get(host as u32, item, &mut size) };
> > +    let ptr = from_err_ptr(err_ptr)?;
> > +    // SAFETY: If qcom_smem_get does not return an error, the returned pointer points to a readable
> > +    // byte buffer with its size written into size. Because these buffers are derived from the
> > +    // static ranges in the DT, this buffer remains accessible even if the qcom_smem module is
> > +    // unloaded, so 'static is appropriate. The underlying buffer cannot mutate, so upgrading it
> > +    // to a reference is allowed.
>
> This is definitely not true. The smem data is ioremap'ped during
> qcom_smem_probe() and will be unmapped if qcom_smem is unloaded.
> Accessing such a pointer after unloading smem would result in the
> exception. Current socinfo ignores that and keeps on accessing the data
> at runtime (which also allows it to read DSP versions which are updated
> at runtime), but the comment needs to be corrected.

Thanks for the info. In v2 I'll look into seeing if I can make those
slices hold a reference on the smem module while they're alive.

>
> > +    Ok(unsafe { core::slice::from_raw_parts(ptr as *const u8, size) })
> > +}
> > +
>
> > +
> > +pub(crate) static SOC_IDS: &[SocId] = &[
> > +    id_entry!(MSM8260),
> > +    id_entry!(MSM8660),
>
> As we are performing a linear search over the array, would it be sensible
> to sort it?

I directly copied the ordering used in the C code. We can't easily see
that it's sorted at a source level, because the key is the soc ID,
which comes from the device tree headers. For the first four elements
at least, it *is* in sorted order already, we're just not currently
relying on it. I could potentially add a debug assert to check that
things are in sorted order and switch to assuming that if you'd like.

>
> > +    id_entry!(APQ8060),
> > +    id_entry!(MSM8960),
>
> > +
> > +pub(crate) const PMIC_MODELS: [Option<&str>; 92] = {
> > +    let mut models = [None; 92];
>
> I don't like the magic 92 appearing twice here just because the last
> element of the array has number 91. Is there a sensible but idiomatic
> way to express that (note in C we just use flex array without the size
> and don't specify the size at all, so we don't have the duplication).

The usual way to do this in Rust is to use a slice (`&[Option<&str>]`)
rather than an array (which looks like `[T; N]`, as I did above). The
reason I didn't do it here was to avoid explicitly listing any punched
holes (i.e. with slice, I'd have to write something like
`&[Some("Foo"), None, Some("Bar")]`). Rust doesn't directly have a
partial array initialization syntax because it requires that
everything be fully initialized, which is why I was doing it through
mutation. If we're going to leave everything default `None`, and
express which elements should be `Some`, we need a bound on how many
elements "everything" should be.

Some options:
1. Make holes explicit
```
pub(crate) const PMIC_MODELS: &[Option<&'str>] = &[
  Some("foo"),
  Some("bar"),
  None,
  Some("baz"),
  // ...
};
```
This is the one I'd suggest if we want to get rid of the 92. It has
the downside of some extra explicit `None` entries, but the array
isn't *that* sparse.

2. Factor out 92 into a constant.
3. Define the constant in terms of index/value pairs instead. I could
use `const`-time code to produce the array we want:
```
const PMIC_ENTRIES: &[(usize, &str)] = &[(1, "foo"), (9, "bar"), (42, "baz")];

const PMIC_MODELS_LEN: usize = {
    let mut max = 0;
    let mut i = 0;
    while i < PMIC_ENTRIES.len() {
        if PMIC_ENTRIES[i].0 > max {
            max = PMIC_ENTRIES[i].0;
        }
        i += 1;
    }
    max + 1
};

pub const PMIC_MODELS: [Option<&'static str>; PMIC_MODELS_LEN] = {
    let mut models = [None; PMIC_MODELS_LEN];
    let mut i = 0;
    while i < PMIC_ENTRIES.len() {
        let (idx, val) = PMIC_ENTRIES[i];
        models[idx] = Some(val);
        i += 1;
    }
    models
};
```
(The slightly icky looking loops are because not all features are
available in const mode.)
This seems a bit overkill for what's going on.
>
> > +    models[0] = Some("Unknown PMIC model");
> > +    models[1] = Some("PM8941");
> > +    models[2] = Some("PM8841");
> > +    models[3] = Some("PM8019");
> > +    models[4] = Some("PM8226");
> > +    models[5] = Some("PM8110");
> > +    models[6] = Some("PMA8084");
> > +    models[7] = Some("PMI8962");
> > +    models[8] = Some("PMD9635");
> > +    models[9] = Some("PM8994");
> > +    models[10] = Some("PMI8994");
> > +    models[11] = Some("PM8916");
> > +    models[12] = Some("PM8004");
> > +    models[13] = Some("PM8909/PM8058");
> > +    models[14] = Some("PM8028");
> > +    models[15] = Some("PM8901");
> > +    models[16] = Some("PM8950/PM8027");
> > +    models[17] = Some("PMI8950/ISL9519");
> > +    models[18] = Some("PMK8001/PM8921");
> > +    models[19] = Some("PMI8996/PM8018");
> > +    models[20] = Some("PM8998/PM8015");
> > +    models[21] = Some("PMI8998/PM8014");
> > +    models[22] = Some("PM8821");
> > +    models[23] = Some("PM8038");
> > +    models[24] = Some("PM8005/PM8922");
> > +    models[25] = Some("PM8917/PM8937");
> > +    models[26] = Some("PM660L");
> > +    models[27] = Some("PM660");
> > +    models[30] = Some("PM8150");
> > +    models[31] = Some("PM8150L");
> > +    models[32] = Some("PM8150B");
> > +    models[33] = Some("PMK8002");
> > +    models[36] = Some("PM8009");
> > +    models[37] = Some("PMI632");
> > +    models[38] = Some("PM8150C");
> > +    models[40] = Some("PM6150");
> > +    models[41] = Some("SMB2351");
> > +    models[44] = Some("PM8008");
> > +    models[45] = Some("PM6125");
> > +    models[46] = Some("PM7250B");
> > +    models[47] = Some("PMK8350");
> > +    models[48] = Some("PM8350");
> > +    models[49] = Some("PM8350C");
> > +    models[50] = Some("PM8350B");
> > +    models[51] = Some("PMR735A");
> > +    models[52] = Some("PMR735B");
> > +    models[54] = Some("PM6350");
> > +    models[55] = Some("PM4125");
> > +    models[58] = Some("PM8450");
> > +    models[65] = Some("PM8010");
> > +    models[69] = Some("PM8550VS");
> > +    models[70] = Some("PM8550VE");
> > +    models[71] = Some("PM8550B");
> > +    models[72] = Some("PMR735D");
> > +    models[73] = Some("PM8550");
> > +    models[74] = Some("PMK8550");
> > +    models[78] = Some("PMM8650AU");
> > +    models[79] = Some("PMM8650AU_PSAIL");
> > +    models[80] = Some("PM7550");
> > +    models[82] = Some("PMC8380");
> > +    models[83] = Some("SMB2360");
> > +    models[91] = Some("PMIV0108");
> > +    models
> > +};
> > +
> > diff --git a/drivers/soc/qcom/socinfo/socinfo.rs b/drivers/soc/qcom/socinfo/socinfo.rs
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..bff1bf8fd672e3c461352075aa85ef8fdedff953
> > --- /dev/null
> > +++ b/drivers/soc/qcom/socinfo/socinfo.rs
> > @@ -0,0 +1,362 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +// Copyright (C) 2025 Google LLC.
> > +
> > +//! Re-implementation of Qualcomm's Socinfo driver in Rust
>
> I think this comment can be dropped. If it gets merged, there is no old
> Socinfo driver.

Agreed. We do need a crate level doc comment, but it can just say
"Qualcomm SocInfo Driver" or something.

>
> > +use core::fmt;
> > +use core::fmt::Formatter;
> > +use kernel::debugfs::{Scope, ScopedDir};
> > +use kernel::device::Core;
> > +use kernel::module_platform_driver;
> > +use kernel::platform::{self, Device};
> > +use kernel::prelude::{fmt, pin_data, Error, PinInit, Result};
> > +use kernel::soc;
> > +use kernel::str::{CStr, CStrExt, CString};
> > +use kernel::transmute::FromBytes;
> > +use kernel::{c_str, pr_warn, try_pin_init};
> > +use pin_init::pin_init_scope;
> > +
>
> > +        let versions = self.versions.unwrap_or(&[]);
> > +        let versions2 = self.versions2.unwrap_or(&[]);
> > +        for (version, image_name) in versions
> > +            .iter()
> > +            .take(32)
> > +            .chain(versions2.iter())
> > +            .zip(IMAGE_NAMES.iter())
> > +        {
> > +            version.build_debugfs(dir, image_name);
> > +        }
>
> I like this idiomatic code, we even get what original driver misses:
> size checks for those memory regions.
>
> > +    }
> > +}
> > +
>
> --
> With best wishes
> Dmitry

Powered by blists - more mailing lists