Message-ID: <4qgnlilx2vyi6yf73d47z2o342lotq7vsvycigsv5fb3rcwsjv@t4ank62skqny>
Date: Sat, 13 Dec 2025 05:54:55 +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 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.

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

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

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

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