Message-ID: <zgzucan5ysxexmmjantx6sz7upbtouazj4hagjzlhaoo5hvzo5@limvqfce2lrw>
Date: Mon, 15 Dec 2025 21:44:26 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
To: Matthew Maurer <mmaurer@...gle.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 Sat, Dec 13, 2025 at 08:58:56AM -0800, Matthew Maurer wrote:
> 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.

It should be a reference to the _bound_ device. I don't think we have
a way to do it. I suggest just fixing the comment instead.

> 
> >
> > > +    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.

Sorry, I should be more explicit: sort by the name, not by the value. It
would make it easier to extend and reduce possible conflicts.

> 
> >
> > > +    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.

I'd say, the easiet way would be to define PMIC_ENTRIES and then search
in it. It's the most idiomatic from my point of view. Other options
require extra knowledge or extra checks.

> >
> > > +    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.

Just drop it here, it will go to the commit message.

> 
> >
> > > +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

-- 
With best wishes
Dmitry

Powered by blists - more mailing lists