* [RFC] soc: qcom: socinfo.rs: Add Rust Socinfo Driver implementation
@ 2021-08-17 1:23 Antonio Martorana
2021-08-17 16:43 ` Miguel Ojeda
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Antonio Martorana @ 2021-08-17 1:23 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho
Cc: Antonio Martorana, rust-for-linux, Trilok Soni, Elliot Berman
Add initial work for Rust port of qcom/socinfo.c driver. Socinfo driver
reads QCOM SMEM to learn current device information (e.g. SM8250).
- Added several C libraries to bindings_helper.h for FFI support from bindgen.
- Implemented PlatformDriver and KernelModule traits for SocInfoDriver
- Register with platform device framework
- Add devicetree compatibles
- Did not port functions referenced from SMEM, wanted to show a
working proof of concept for Rust leaf driver with FFI code
- Register the discovered SMEM data with socinfo core.
---
This patch relies on Mateusz Jabłoński's try_format! macro patch from GitHub:
https://github.com/Rust-for-Linux/linux/pull/478
Signed-off-by: Antonio Martorana <amartora@codeaurora.org>
---
drivers/soc/qcom/Kconfig | 9 +
drivers/soc/qcom/Makefile | 1 +
drivers/soc/qcom/socinfo_rust.rs | 464 +++++++++++++++++++++++++++++++++++++++
rust/kernel/bindings_helper.h | 7 +
4 files changed, 481 insertions(+)
create mode 100644 drivers/soc/qcom/socinfo_rust.rs
diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index 79b568f..3154b1a 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -190,6 +190,15 @@ config QCOM_SOCINFO
Say yes here to support the Qualcomm socinfo driver, providing
information about the SoC to user space.
+config QCOM_SOCINFO_RUST
+ tristate "Rust implementation of Qualcomm socinfo driver"
+ depends on HAS_RUST && QCOM_SMEM
+ help
+ This driver provides alternative Rust-based kernel-side support
+ for the socinfo driver.
+
+ If unsure, say N.
+
config QCOM_WCNSS_CTRL
tristate "Qualcomm WCNSS control driver"
depends on ARCH_QCOM || COMPILE_TEST
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index ad675a6..340ac65 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -19,6 +19,7 @@ obj-$(CONFIG_QCOM_SMEM) += smem.o
obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
obj-$(CONFIG_QCOM_SMP2P) += smp2p.o
obj-$(CONFIG_QCOM_SMSM) += smsm.o
+obj-$(CONFIG_QCOM_SOCINFO_RUST) += socinfo_rust.o
obj-$(CONFIG_QCOM_SOCINFO) += socinfo.o
obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
obj-$(CONFIG_QCOM_APR) += apr.o
diff --git a/drivers/soc/qcom/socinfo_rust.rs b/drivers/soc/qcom/socinfo_rust.rs
new file mode 100644
index 0000000..8af697a
--- /dev/null
+++ b/drivers/soc/qcom/socinfo_rust.rs
@@ -0,0 +1,464 @@
+//! Qualcomm support for socinfo.c
+
+#![no_std]
+#![feature(allocator_api,global_asm)]
+
+use kernel::{
+ {c_str,},
+ bindings,
+ bindings::*,
+ c_types,
+ linked_list::Wrapper,
+ of::ConstOfMatchTable,
+ platdev,
+ platdev::PlatformDriver,
+ prelude::*,
+ str,
+ str::CStr
+};
+
+use alloc::{boxed::Box,try_format};
+use core::{pin::Pin};
+
+module! {
+ type: SocInfoDriver,
+ name: b"socinfo_rust",
+ author: b"Antonio Martorana",
+ description: b"QCOM socinfo rust implementation",
+ license: b"GPL v2",
+}
+
+/*
+ * 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{
+ let major: u32 = (ver >> 16) & 0xFFFF;
+ return major;
+}
+
+const fn socinfo_minor(ver: u32) -> u32{
+ let minor: u32 = ver & 0xFFFF;
+ return minor;
+}
+
+const fn socinfo_version(maj: u32, min: u32) -> u32{
+ let version: u32 = ((maj & 0xFFFF) << 16) | (min & 0xFFFF);
+ return version;
+}
+
+const SMEM_SOCINFO_BUILD_ID_LENGTH: usize = 32;
+const SMEM_SOCINFO_CHIP_ID_LENGTH: usize = 32;
+
+/*
+ * SMEM item id, used to acquire handles to respective
+ * SMEM region.
+ */
+const SMEM_HW_SW_BUILD_ID: u32 = 137;
+
+/* 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;
+
+/*
+ * 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;
+
+const PMIC_MODELS: [&str;37] = [
+ "Unknown PMIC model",
+ "PM8941",
+ "PM8841",
+ "PM8019",
+ "PM8226",
+ "PM8110",
+ "PMA8084",
+ "PMI8962",
+ "PMD9635",
+ "PM8994",
+ "PMI8994",
+ "PM8916",
+ "PM8004",
+ "PM8909/PM8058",
+ "PM8028",
+ "PM8901",
+ "PM8950/PM8027",
+ "PMI8950/ISL9519",
+ "PM8921",
+ "PM8018",
+ "PM8998/PM8015",
+ "PMI8998/PM8014",
+ "PM8821",
+ "PM8038",
+ "PM8005/PM8922",
+ "PM8917",
+ "PM660L",
+ "PM660",
+ "",
+ "",
+ "PM8150",
+ "PM8150L",
+ "PM8150B",
+ "PMK8002",
+ "",
+ "",
+ "PM8009",
+];
+
+
+struct SocInfo{
+ fmt: bindings::__le32,
+ id: bindings::__le32,
+ ver: bindings::__le32,
+ build_id: [c_types::c_char; SMEM_SOCINFO_BUILD_ID_LENGTH],
+ raw_id: bindings::__le32,
+ raw_ver: bindings::__le32,
+ hw_plat: bindings::__le32,
+ plat_ver: bindings::__le32,
+ accessory_chip: bindings::__le32,
+ hw_plat_subtype: bindings::__le32,
+ pmic_model: bindings::__le32,
+ pmic_die_rev: bindings::__le32,
+ pmic_model_1: bindings::__le32,
+ pmic_die_rev_1: bindings::__le32,
+ pmic_model_2: bindings::__le32,
+ pmic_die_rev_2: bindings::__le32,
+ foundry_id: bindings::__le32,
+ serial_num: bindings::__le32,
+ num_pmics: bindings::__le32,
+ pmic_array_offset: bindings::__le32,
+ chip_family: bindings::__le32,
+ raw_device_family: bindings::__le32,
+ raw_device_num: bindings::__le32,
+ nproduct_id: bindings::__le32,
+ chip_id: [c_types::c_char; SMEM_SOCINFO_CHIP_ID_LENGTH],
+ num_cluster: bindings::__le32,
+ ncluster_array_offset: bindings::__le32,
+ num_defective_parts: bindings::__le32,
+ nmodem_supported: bindings::__le32,
+}
+
+#[derive(Default)]
+struct SocInfoParams{
+ raw_device_family: u32,
+ hw_plat_subtype: u32,
+ accessory_chip: u32,
+ raw_device_num: u32,
+ chip_family: u32,
+ foundry_id: u32,
+ plat_ver: u32,
+ raw_ver: u32,
+ hw_plat: u32,
+ fmt: u32,
+ nproduct_id: u32,
+ num_clusters: u32,
+ ncluster_array_offset: u32,
+ num_defective_parts: u32,
+ ndefective_parts_array_offset: u32,
+ nmodem_supported: u32,
+}
+
+struct SmemImageVersion{
+ name: [c_types::c_char; SMEM_IMAGE_VERSION_NAME_SIZE],
+ variant: [c_types::c_char; SMEM_IMAGE_VERSION_VARIANT_SIZE],
+ pad: c_types::c_char,
+ oem: [c_types::c_char; SMEM_IMAGE_VERSION_OEM_SIZE],
+}
+
+const SOC_ID: [SocId;105] = [
+ SocId { id:87, name:c_str!("MSM8960")},
+ SocId{ id:109, name:c_str!("APQ8064") },
+ SocId{ id:122, name:c_str!("MSM8660A") },
+ SocId{ id:123, name:c_str!("MSM8260A") },
+ SocId{ id:124, name:c_str!("APQ8060A") },
+ SocId{ id:126, name:c_str!("MSM8974") },
+ SocId{ id:130, name:c_str!("MPQ8064") },
+ SocId{ id:138, name:c_str!("MSM8960AB") },
+ SocId{ id:139, name:c_str!("APQ8060AB") },
+ SocId{ id:140, name:c_str!("MSM8260AB") },
+ SocId{ id:141, name:c_str!("MSM8660AB") },
+ SocId{ id:145, name:c_str!("MSM8626") },
+ SocId{ id:147, name:c_str!("MSM8610") },
+ SocId{ id:153, name:c_str!("APQ8064AB") },
+ SocId{ id:158, name:c_str!("MSM8226") },
+ SocId{ id:159, name:c_str!("MSM8526") },
+ SocId{ id:161, name:c_str!("MSM8110") },
+ SocId{ id:162, name:c_str!("MSM8210") },
+ SocId{ id:163, name:c_str!("MSM8810") },
+ SocId{ id:164, name:c_str!("MSM8212") },
+ SocId{ id:165, name:c_str!("MSM8612") },
+ SocId{ id:166, name:c_str!("MSM8112") },
+ SocId{ id:168, name:c_str!("MSM8225Q") },
+ SocId{ id:169, name:c_str!("MSM8625Q") },
+ SocId{ id:170, name:c_str!("MSM8125Q") },
+ SocId{ id:172, name:c_str!("APQ8064AA") },
+ SocId{ id:178, name:c_str!("APQ8084") },
+ SocId{ id:184, name:c_str!("APQ8074") },
+ SocId{ id:185, name:c_str!("MSM8274") },
+ SocId{ id:186, name:c_str!("MSM8674") },
+ SocId{ id:194, name:c_str!("MSM8974PRO") },
+ SocId{ id:198, name:c_str!("MSM8126") },
+ SocId{ id:199, name:c_str!("APQ8026") },
+ SocId{ id:200, name:c_str!("MSM8926") },
+ SocId{ id:205, name:c_str!("MSM8326") },
+ SocId{ id:206, name:c_str!("MSM8916") },
+ SocId{ id:207, name:c_str!("MSM8994") },
+ SocId{ id:208, name:c_str!("APQ8074-AA") },
+ SocId{ id:209, name:c_str!("APQ8074-AB") },
+ SocId{ id:210, name:c_str!("APQ8074PRO") },
+ SocId{ id:211, name:c_str!("MSM8274-AA") },
+ SocId{ id:212, name:c_str!("MSM8274-AB") },
+ SocId{ id:213, name:c_str!("MSM8274PRO") },
+ SocId{ id:214, name:c_str!("MSM8674-AA") },
+ SocId{ id:215, name:c_str!("MSM8674-AB") },
+ SocId{ id:216, name:c_str!("MSM8674PRO") },
+ SocId{ id:217, name:c_str!("MSM8974-AA") },
+ SocId{ id:218, name:c_str!("MSM8974-AB") },
+ SocId{ id:219, name:c_str!("APQ8028") },
+ SocId{ id:220, name:c_str!("MSM8128") },
+ SocId{ id:221, name:c_str!("MSM8228") },
+ SocId{ id:222, name:c_str!("MSM8528") },
+ SocId{ id:223, name:c_str!("MSM8628") },
+ SocId{ id:224, name:c_str!("MSM8928") },
+ SocId{ id:225, name:c_str!("MSM8510") },
+ SocId{ id:226, name:c_str!("MSM8512") },
+ SocId{ id:233, name:c_str!("MSM8936") },
+ SocId{ id:239, name:c_str!("MSM8939") },
+ SocId{ id:240, name:c_str!("APQ8036") },
+ SocId{ id:241, name:c_str!("APQ8039") },
+ SocId{ id:246, name:c_str!("MSM8996") },
+ SocId{ id:247, name:c_str!("APQ8016") },
+ SocId{ id:248, name:c_str!("MSM8216") },
+ SocId{ id:249, name:c_str!("MSM8116") },
+ SocId{ id:250, name:c_str!("MSM8616") },
+ SocId{ id:251, name:c_str!("MSM8992") },
+ SocId{ id:253, name:c_str!("APQ8094") },
+ SocId{ id:290, name:c_str!("MDM9607") },
+ SocId{ id:291, name:c_str!("APQ8096") },
+ SocId{ id:292, name:c_str!("MSM8998") },
+ SocId{ id:293, name:c_str!("MSM8953") },
+ SocId{ id:296, name:c_str!("MDM8207") },
+ SocId{ id:297, name:c_str!("MDM9207") },
+ SocId{ id:298, name:c_str!("MDM9307") },
+ SocId{ id:299, name:c_str!("MDM9628") },
+ SocId{ id:304, name:c_str!("APQ8053") },
+ SocId{ id:305, name:c_str!("MSM8996SG") },
+ SocId{ id:310, name:c_str!("MSM8996AU") },
+ SocId{ id:311, name:c_str!("APQ8096AU") },
+ SocId{ id:312, name:c_str!("APQ8096SG") },
+ SocId{ id:317, name:c_str!("SDM660") },
+ SocId{ id:318, name:c_str!("SDM630") },
+ SocId{ id:319, name:c_str!("APQ8098") },
+ SocId{ id:321, name:c_str!("SDM845") },
+ SocId{ id:322, name:c_str!("MDM9206") },
+ SocId{ id:324, name:c_str!("SDA660") },
+ SocId{ id:325, name:c_str!("SDM658") },
+ SocId{ id:326, name:c_str!("SDA658") },
+ SocId{ id:327, name:c_str!("SDA630") },
+ SocId{ id:338, name:c_str!("SDM450") },
+ SocId{ id:341, name:c_str!("SDA845") },
+ SocId{ id:345, name:c_str!("SDM636") },
+ SocId{ id:346, name:c_str!("SDA636") },
+ SocId{ id:349, name:c_str!("SDM632") },
+ SocId{ id:350, name:c_str!("SDA632") },
+ SocId{ id:351, name:c_str!("SDA450") },
+ SocId{ id:356, name:c_str!("SM8250") },
+ SocId{ id:394, name:c_str!("SM6125") },
+ SocId{ id:402, name:c_str!("IPQ6018") },
+ SocId{ id:403, name:c_str!("IPQ6028") },
+ SocId{ id:421, name:c_str!("IPQ6000") },
+ SocId{ id:422, name:c_str!("IPQ6010") },
+ SocId{ id:425, name:c_str!("SC7180") },
+ SocId{ id:453, name:c_str!("IPQ6005") },
+ SocId{ id:455, name:c_str!("QRB5165") },
+];
+
+
+struct SocId{
+ id: u32,
+ name: &'static str::CStr
+}
+
+fn socinfo_machine(id: &u32) -> Option<&'static str::CStr>{
+ for idx in SOC_ID {
+ if idx.id == *id{
+ return Some(idx.name);
+ }
+ }
+ return None;
+}
+
+
+fn qcom_show_build_id(seq: &mut bindings::seq_file, p: &mut c_types::c_void) -> u32{
+
+ let seq_private= seq.private as *mut SocInfo;
+ unsafe {let ref_mut_seq_priv = seq_private.as_mut().unwrap(); }
+
+ let mut format = c_str!("%s\n");
+ let format = format.as_char_ptr();
+
+ unsafe{ bindings::seq_printf(seq as *mut bindings::seq_file,format,(*seq_private).build_id)};
+
+ return 0;
+}
+
+fn qcom_show_pmic_model(seq: &mut bindings::seq_file, p: &mut c_types::c_void) -> i32{
+ let socinfo= seq.private as *mut SocInfo;
+ let mut model;
+
+ let mut format = c_str!("%s\n");
+ let format = format.as_char_ptr();
+
+ let mut secondary_format = c_str!("unknown (%d)\n");
+ let secondary_format = secondary_format.as_char_ptr();
+
+ unsafe {
+ let mut_socinfo = socinfo.as_mut().unwrap();
+ let test_model = socinfo_minor((*socinfo).pmic_model);
+ model = test_model;
+ }
+
+ if model < 0{
+ return -22; //EINVAL
+ }
+
+ let mut check_valid_model: bool = false;
+ if PMIC_MODELS[model as usize] != ""{
+ check_valid_model = true;
+ }
+
+ if model < PMIC_MODELS.len() as u32 && check_valid_model {
+ unsafe{ bindings::seq_printf(seq as *mut bindings::seq_file,format,PMIC_MODELS[model as usize])};
+ }
+ else{
+ unsafe{ bindings::seq_printf(seq as *mut bindings::seq_file,secondary_format,model)};
+ }
+
+ return 0;
+}
+/*
+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{
+
+}
+
+*/
+
+struct QcomSocInfo{
+ soc_dev: *mut bindings::soc_device,
+ attr: bindings::soc_device_attribute,
+ dbg_root: *mut bindings::dentry,
+ info: SocInfoParams
+}
+
+impl QcomSocInfo{
+ fn socinfo_debugfs_init(&self, info: &mut SocInfo, info_size: usize) -> Option<c_types::c_void>{
+ None
+ }
+
+ fn socinfo_debugfs_exit(&self) -> Option<c_types::c_void>{
+ None
+ }
+
+ fn new() -> Self {
+ QcomSocInfo{
+ soc_dev: core::ptr::null_mut(),
+ attr: bindings::soc_device_attribute::default(),
+ dbg_root: core::ptr::null_mut(),
+ info: <SocInfoParams as Default>::default(),
+ }
+ }
+}
+
+struct SocInfoDriver {
+ _pdev: Pin<Box<platdev::Registration>>,
+}
+
+impl PlatformDriver for SocInfoDriver {
+ type DrvData = Pin<Box<QcomSocInfo>>;
+ fn probe(device_id: i32) -> Result<Self::DrvData>{
+ pr_info!("Just checking if probe works, with device id {}\n", device_id);
+
+ let mut drv_data= Pin::from(Box::try_new(QcomSocInfo::new())?);
+
+ let item_size: *mut usize = core::ptr::null_mut();
+
+ let mut info = unsafe {
+ bindings::qcom_smem_get(
+ QCOM_SMEM_HOST_ANY,
+ SMEM_HW_SW_BUILD_ID,
+ item_size,
+ ) as *mut SocInfo
+ };
+
+ let info_mut: &mut SocInfo;
+ unsafe { info_mut = info.as_mut().unwrap(); }
+
+ drv_data.as_mut().attr.family = c_str!("Snapdragon").as_char_ptr();
+ drv_data.as_mut().attr.machine = socinfo_machine(&info_mut.id).unwrap().as_char_ptr();
+
+ drv_data.as_mut().attr.soc_id = CStr::from_bytes_with_nul(
+ try_format!(
+ "{}\0",
+ info_mut.id
+ )?.as_bytes()
+ ).unwrap().as_char_ptr();
+
+ drv_data.as_mut().attr.revision = CStr::from_bytes_with_nul(
+ try_format!(
+ "{}.{}\0",
+ socinfo_major(info_mut.ver),
+ socinfo_minor(info_mut.ver)
+ )?.as_bytes()
+ ).unwrap().as_char_ptr();
+
+
+ unsafe{
+ drv_data.as_mut().soc_dev = bindings::soc_device_register(&mut drv_data.as_mut().attr );
+ drv_data.as_mut().socinfo_debugfs_init(info.as_mut().unwrap(),*item_size);
+ //socinfo_debugfs_init(/*&mut drv_data.as_mut(),*/ info.as_mut().unwrap(),*item_size) ;
+ bindings::add_device_randomness(info as *mut c_types::c_void,*item_size as u32)
+ };
+
+ Ok(drv_data)
+ }
+ fn remove(device_id: i32, _drv_data: Self::DrvData) -> Result {
+ pr_info!("removing socinfo_rust now with device id {}\n", device_id);
+
+ unsafe {bindings::soc_device_unregister(_drv_data.soc_dev) };
+ _drv_data.as_ref().socinfo_debugfs_exit();
+ //socinfo_debugfs_exit(&_drv_data.as_ref());
+ Ok(())
+ }
+}
+
+impl KernelModule for SocInfoDriver {
+ fn init() -> Result<Self> {
+ const OF_MATCH_TBL: ConstOfMatchTable<1> = ConstOfMatchTable::new_const([c_str!("qcom-socinfo")]);
+
+ let pdev = platdev::Registration::new_pinned::<SocInfoDriver>(
+ c_str!("qcom-socinfo"),
+ Some(&OF_MATCH_TBL),
+ &THIS_MODULE,
+ )?;
+ Ok(SocInfoDriver {_pdev: pdev} )
+ }
+}
\ No newline at end of file
diff --git a/rust/kernel/bindings_helper.h b/rust/kernel/bindings_helper.h
index c64a630..a8c73450 100644
--- a/rust/kernel/bindings_helper.h
+++ b/rust/kernel/bindings_helper.h
@@ -18,6 +18,13 @@
#include <linux/platform_device.h>
#include <linux/of_platform.h>
#include <linux/security.h>
+#include <linux/sys_soc.h>
+#include <linux/err.h>
+#include <linux/debugfs.h>
+#include <linux/soc/qcom/smem.h>
+#include <linux/types.h>
+#include <linux/device.h>
+#include <linux/random.h>
// `bindgen` gets confused at certain things
const gfp_t BINDINGS_GFP_KERNEL = GFP_KERNEL;
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [RFC] soc: qcom: socinfo.rs: Add Rust Socinfo Driver implementation 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 2 siblings, 0 replies; 6+ 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] 6+ messages in thread
* Re: [RFC] soc: qcom: socinfo.rs: Add Rust Socinfo Driver implementation 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 2 siblings, 0 replies; 6+ messages in thread From: Wei Liu @ 2021-08-18 9:36 UTC (permalink / raw) To: Antonio Martorana Cc: Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho, rust-for-linux, Trilok Soni, Elliot Berman, Wei Liu On Mon, Aug 16, 2021 at 06:23:32PM -0700, Antonio Martorana wrote: [...] > new file mode 100644 > index 0000000..8af697a > --- /dev/null > +++ b/drivers/soc/qcom/socinfo_rust.rs > @@ -0,0 +1,464 @@ > +//! Qualcomm support for socinfo.c > + > +#![no_std] > +#![feature(allocator_api,global_asm)] Not really related to this patch, but I wish there was a more ergonomic way to specify these crate level features. I think these two lines are c&p in a lot of places. [...] > +/* > + * 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{ > + let major: u32 = (ver >> 16) & 0xFFFF; > + return major; This can be simplified as const fn socinfo_major(ver: u32) -> u32 { (ver >> 16) & 0xFFFF } > +} > + > +const fn socinfo_minor(ver: u32) -> u32{ > + let minor: u32 = ver & 0xFFFF; > + return minor; > +} > + > +const fn socinfo_version(maj: u32, min: u32) -> u32{ > + let version: u32 = ((maj & 0xFFFF) << 16) | (min & 0xFFFF); > + return version; > +} Same for these two functions. They can be simplified as well. I have one further question: why aren't these helpers implemented as methods of SocInfo? Like: impl SocInfo { fn major(&self) -> u32 { (self.ver >> 16) & 0xFFFF } } Thanks, Wei. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [RFC v2] soc: qcom: socinfo.rs: Add Rust Socinfo Driver implementation 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 ` Antonio Martorana 2021-08-20 22:18 ` Miguel Ojeda 2 siblings, 1 reply; 6+ messages in thread From: Antonio Martorana @ 2021-08-20 19:38 UTC (permalink / raw) Cc: Antonio Martorana, rust-for-linux To: Miguel Ojeda <ojeda@kernel.org>, Alex Gaynor <alex.gaynor@gmail.com>, Wedson Almeida Filho <wedsonaf@google.com> Cc: rust-for-linux@vger.kernel.org, Trilok Soni <tsoni@codeaurora.org>, Elliot Berman <quic_eberman@quicinc.com>, linux-arm-msm <linux-arm-msm@vger.kernel.org>, Alex Gaynor <alex.gaynor@gmail.com>, Wei Liu <wei.liu@kernel.org> Add initial work for Rust port of qcom/socinfo.c driver. Socinfo driver reads QCOM SMEM to learn current device information (e.g. SM8250). - Added several C libraries to bindings_helper.h for FFI support from bindgen. - Implemented PlatformDriver and KernelModule traits for SocInfoDriver - Register with platform device framework - Add devicetree compatibles - Did not port functions referenced from SMEM, wanted to show a working proof of concept for Rust leaf driver with FFI code - Register the discovered SMEM data with socinfo core. --- This patch relies on Mateusz Jabłoński's try_format! macro patch from GitHub: https://github.com/Rust-for-Linux/linux/pull/478 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/ . Signed-off-by: Antonio Martorana <amartora@codeaurora.org> --- drivers/soc/qcom/Kconfig | 9 + drivers/soc/qcom/Makefile | 1 + drivers/soc/qcom/socinfo_rust.rs | 794 +++++++++++++++++++++++++++++++++++++++ rust/kernel/bindings_helper.h | 7 + 4 files changed, 811 insertions(+) create mode 100644 drivers/soc/qcom/socinfo_rust.rs diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig index 79b568f..d1b23c1 100644 --- a/drivers/soc/qcom/Kconfig +++ b/drivers/soc/qcom/Kconfig @@ -190,6 +190,15 @@ config QCOM_SOCINFO Say yes here to support the Qualcomm socinfo driver, providing information about the SoC to user space. +config QCOM_SOCINFO_RUST + tristate "Rust implementation of Qualcomm socinfo driver" + depends on RUST && QCOM_SMEM + help + This driver provides alternative Rust-based kernel-side support + for the socinfo driver. + + If unsure, say N. + config QCOM_WCNSS_CTRL tristate "Qualcomm WCNSS control driver" depends on ARCH_QCOM || COMPILE_TEST diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile index ad675a6..340ac65 100644 --- a/drivers/soc/qcom/Makefile +++ b/drivers/soc/qcom/Makefile @@ -19,6 +19,7 @@ obj-$(CONFIG_QCOM_SMEM) += smem.o obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o obj-$(CONFIG_QCOM_SMP2P) += smp2p.o obj-$(CONFIG_QCOM_SMSM) += smsm.o +obj-$(CONFIG_QCOM_SOCINFO_RUST) += socinfo_rust.o obj-$(CONFIG_QCOM_SOCINFO) += socinfo.o obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o obj-$(CONFIG_QCOM_APR) += apr.o diff --git a/drivers/soc/qcom/socinfo_rust.rs b/drivers/soc/qcom/socinfo_rust.rs new file mode 100644 index 0000000..3c4fb4c --- /dev/null +++ b/drivers/soc/qcom/socinfo_rust.rs @@ -0,0 +1,794 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! Qualcomm support for socinfo.c + +#![no_std] +#![feature(allocator_api, global_asm)] + +use kernel::{ + bindings, bindings::*, c_str, c_types, linked_list::Wrapper, of::ConstOfMatchTable, platdev, + platdev::PlatformDriver, prelude::*, str, str::CStr, sys_soc, Error, +}; + +use alloc::{boxed::Box, try_format}; +use core::{convert::TryInto, pin::Pin}; + +module! { + type: SocInfoDriver, + name: b"socinfo_rust", + author: b"Antonio Martorana", + description: b"QCOM socinfo rust implementation", + license: b"GPL v2", +} + +/// 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 { + (ver >> 16) & 0xFFFF +} + +const fn socinfo_minor(ver: u32) -> u32 { + ver & 0xFFFF +} + +const fn socinfo_version(maj: u32, min: u32) -> u32 { + ((maj & 0xFFFF) << 16) | (min & 0xFFFF) +} + +const SMEM_SOCINFO_BUILD_ID_LENGTH: usize = 32; +const SMEM_SOCINFO_CHIP_ID_LENGTH: usize = 32; + +/// SMEM item id, used to acquire handles to respective +/// SMEM region. +const SMEM_HW_SW_BUILD_ID: u32 = 137; + +#[cfg(CONFIG_DEBUG_FS)] +mod config_debug_fs { + pub const SMEM_IMAGE_VERSION_BLOCKS_COUNT: usize = 32; + pub const SMEM_IMAGE_VERSION_SIZE: usize = 4096; + pub const SMEM_IMAGE_VERSION_NAME_SIZE: usize = 75; + pub const SMEM_IMAGE_VERSION_VARIANT_SIZE: usize = 20; + pub const SMEM_IMAGE_VERSION_OEM_SIZE: usize = 32; + + /// + /// SMEM Image table indices + /// + pub const SMEM_IMAGE_TABLE_BOOT_INDEX: u32 = 0; + pub const SMEM_IMAGE_TABLE_TZ_INDEX: u32 = 1; + pub const SMEM_IMAGE_TABLE_RPM_INDEX: u32 = 3; + pub const SMEM_IMAGE_TABLE_APPS_INDEX: u32 = 10; + pub const SMEM_IMAGE_TABLE_MPSS_INDEX: u32 = 11; + pub const SMEM_IMAGE_TABLE_ADSP_INDEX: u32 = 12; + pub const SMEM_IMAGE_TABLE_CNSS_INDEX: u32 = 13; + pub const SMEM_IMAGE_TABLE_VIDEO_INDEX: u32 = 14; + pub const SMEM_IMAGE_VERSION_TABLE: u32 = 496; +} + +#[cfg(CONFIG_DEBUG_FS)] +const PMIC_MODELS: [&str; 37] = [ + "Unknown PMIC model", + "PM8941", + "PM8841", + "PM8019", + "PM8226", + "PM8110", + "PMA8084", + "PMI8962", + "PMD9635", + "PM8994", + "PMI8994", + "PM8916", + "PM8004", + "PM8909/PM8058", + "PM8028", + "PM8901", + "PM8950/PM8027", + "PMI8950/ISL9519", + "PM8921", + "PM8018", + "PM8998/PM8015", + "PMI8998/PM8014", + "PM8821", + "PM8038", + "PM8005/PM8922", + "PM8917", + "PM660L", + "PM660", + "", + "", + "PM8150", + "PM8150L", + "PM8150B", + "PMK8002", + "", + "", + "PM8009", +]; + +#[cfg(CONFIG_DEBUG_FS)] +#[derive(Default)] +struct SocInfoParams { + raw_device_family: u32, + hw_plat_subtype: u32, + accessory_chip: u32, + raw_device_num: u32, + chip_family: u32, + foundry_id: u32, + plat_ver: u32, + raw_ver: u32, + hw_plat: u32, + fmt: u32, + nproduct_id: u32, + num_clusters: u32, + ncluster_array_offset: u32, + num_defective_parts: u32, + ndefective_parts_array_offset: u32, + nmodem_supported: u32, +} + +#[cfg(CONFIG_DEBUG_FS)] +struct SmemImageVersion { + name: [c_types::c_char; config_debug_fs::SMEM_IMAGE_VERSION_NAME_SIZE], + variant: [c_types::c_char; config_debug_fs::SMEM_IMAGE_VERSION_VARIANT_SIZE], + pad: c_types::c_char, + oem: [c_types::c_char; config_debug_fs::SMEM_IMAGE_VERSION_OEM_SIZE], +} + +#[cfg(CONFIG_DEBUG_FS)] +fn qcom_show_build_id(seq: &mut bindings::seq_file, p: &mut c_types::c_void) -> Result { + let seq_private = seq.private as *mut SocInfo; + unsafe { + let ref_mut_seq_priv = seq_private.as_mut().unwrap(); + } + + let mut format = c_str!("%s\n"); + let format = format.as_char_ptr(); + + unsafe { + bindings::seq_printf( + seq as *mut bindings::seq_file, + format, + (*seq_private).build_id, + ) + }; + + Ok(()) +} + +#[cfg(CONFIG_DEBUG_FS)] +fn qcom_show_pmic_model(seq: &mut bindings::seq_file, p: &mut c_types::c_void) -> Result { + let socinfo = seq.private as *mut SocInfo; + let mut model; + + let mut format = c_str!("%s\n"); + let format = format.as_char_ptr(); + + let mut secondary_format = c_str!("unknown (%d)\n"); + let secondary_format = secondary_format.as_char_ptr(); + + // SAFETY: `socinfo` is guaranteed to be: + // - A valid (and instalized), non-null pointer + // - Is properly aligned + // - Adheres to aliasing rules + unsafe { + let mut_socinfo = socinfo.as_mut().unwrap(); + let test_model = socinfo_minor((*socinfo).pmic_model); + model = test_model; + } + + let mut check_valid_model: bool = false; + if !PMIC_MODELS[model as usize].is_empty() { + check_valid_model = true; + } + + if model < PMIC_MODELS.len() as u32 && check_valid_model { + unsafe { + bindings::seq_printf( + seq as *mut bindings::seq_file, + format, + PMIC_MODELS[model as usize], + ) + }; + } else { + unsafe { bindings::seq_printf(seq as *mut bindings::seq_file, secondary_format, model) }; + } + + Ok(()) +} + +#[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(()) +} + +struct SocInfo { + fmt: bindings::__le32, + id: bindings::__le32, + ver: bindings::__le32, + build_id: [c_types::c_char; SMEM_SOCINFO_BUILD_ID_LENGTH], + raw_id: bindings::__le32, + raw_ver: bindings::__le32, + hw_plat: bindings::__le32, + plat_ver: bindings::__le32, + accessory_chip: bindings::__le32, + hw_plat_subtype: bindings::__le32, + pmic_model: bindings::__le32, + pmic_die_rev: bindings::__le32, + pmic_model_1: bindings::__le32, + pmic_die_rev_1: bindings::__le32, + pmic_model_2: bindings::__le32, + pmic_die_rev_2: bindings::__le32, + foundry_id: bindings::__le32, + serial_num: bindings::__le32, + num_pmics: bindings::__le32, + pmic_array_offset: bindings::__le32, + chip_family: bindings::__le32, + raw_device_family: bindings::__le32, + raw_device_num: bindings::__le32, + nproduct_id: bindings::__le32, + chip_id: [c_types::c_char; SMEM_SOCINFO_CHIP_ID_LENGTH], + num_cluster: bindings::__le32, + ncluster_array_offset: bindings::__le32, + num_defective_parts: bindings::__le32, + nmodem_supported: bindings::__le32, +} + +const SOC_ID: [SocId; 105] = [ + SocId { + id: 87, + name: c_str!("MSM8960"), + }, + SocId { + id: 109, + name: c_str!("APQ8064"), + }, + SocId { + id: 122, + name: c_str!("MSM8660A"), + }, + SocId { + id: 123, + name: c_str!("MSM8260A"), + }, + SocId { + id: 124, + name: c_str!("APQ8060A"), + }, + SocId { + id: 126, + name: c_str!("MSM8974"), + }, + SocId { + id: 130, + name: c_str!("MPQ8064"), + }, + SocId { + id: 138, + name: c_str!("MSM8960AB"), + }, + SocId { + id: 139, + name: c_str!("APQ8060AB"), + }, + SocId { + id: 140, + name: c_str!("MSM8260AB"), + }, + SocId { + id: 141, + name: c_str!("MSM8660AB"), + }, + SocId { + id: 145, + name: c_str!("MSM8626"), + }, + SocId { + id: 147, + name: c_str!("MSM8610"), + }, + SocId { + id: 153, + name: c_str!("APQ8064AB"), + }, + SocId { + id: 158, + name: c_str!("MSM8226"), + }, + SocId { + id: 159, + name: c_str!("MSM8526"), + }, + SocId { + id: 161, + name: c_str!("MSM8110"), + }, + SocId { + id: 162, + name: c_str!("MSM8210"), + }, + SocId { + id: 163, + name: c_str!("MSM8810"), + }, + SocId { + id: 164, + name: c_str!("MSM8212"), + }, + SocId { + id: 165, + name: c_str!("MSM8612"), + }, + SocId { + id: 166, + name: c_str!("MSM8112"), + }, + SocId { + id: 168, + name: c_str!("MSM8225Q"), + }, + SocId { + id: 169, + name: c_str!("MSM8625Q"), + }, + SocId { + id: 170, + name: c_str!("MSM8125Q"), + }, + SocId { + id: 172, + name: c_str!("APQ8064AA"), + }, + SocId { + id: 178, + name: c_str!("APQ8084"), + }, + SocId { + id: 184, + name: c_str!("APQ8074"), + }, + SocId { + id: 185, + name: c_str!("MSM8274"), + }, + SocId { + id: 186, + name: c_str!("MSM8674"), + }, + SocId { + id: 194, + name: c_str!("MSM8974PRO"), + }, + SocId { + id: 198, + name: c_str!("MSM8126"), + }, + SocId { + id: 199, + name: c_str!("APQ8026"), + }, + SocId { + id: 200, + name: c_str!("MSM8926"), + }, + SocId { + id: 205, + name: c_str!("MSM8326"), + }, + SocId { + id: 206, + name: c_str!("MSM8916"), + }, + SocId { + id: 207, + name: c_str!("MSM8994"), + }, + SocId { + id: 208, + name: c_str!("APQ8074-AA"), + }, + SocId { + id: 209, + name: c_str!("APQ8074-AB"), + }, + SocId { + id: 210, + name: c_str!("APQ8074PRO"), + }, + SocId { + id: 211, + name: c_str!("MSM8274-AA"), + }, + SocId { + id: 212, + name: c_str!("MSM8274-AB"), + }, + SocId { + id: 213, + name: c_str!("MSM8274PRO"), + }, + SocId { + id: 214, + name: c_str!("MSM8674-AA"), + }, + SocId { + id: 215, + name: c_str!("MSM8674-AB"), + }, + SocId { + id: 216, + name: c_str!("MSM8674PRO"), + }, + SocId { + id: 217, + name: c_str!("MSM8974-AA"), + }, + SocId { + id: 218, + name: c_str!("MSM8974-AB"), + }, + SocId { + id: 219, + name: c_str!("APQ8028"), + }, + SocId { + id: 220, + name: c_str!("MSM8128"), + }, + SocId { + id: 221, + name: c_str!("MSM8228"), + }, + SocId { + id: 222, + name: c_str!("MSM8528"), + }, + SocId { + id: 223, + name: c_str!("MSM8628"), + }, + SocId { + id: 224, + name: c_str!("MSM8928"), + }, + SocId { + id: 225, + name: c_str!("MSM8510"), + }, + SocId { + id: 226, + name: c_str!("MSM8512"), + }, + SocId { + id: 233, + name: c_str!("MSM8936"), + }, + SocId { + id: 239, + name: c_str!("MSM8939"), + }, + SocId { + id: 240, + name: c_str!("APQ8036"), + }, + SocId { + id: 241, + name: c_str!("APQ8039"), + }, + SocId { + id: 246, + name: c_str!("MSM8996"), + }, + SocId { + id: 247, + name: c_str!("APQ8016"), + }, + SocId { + id: 248, + name: c_str!("MSM8216"), + }, + SocId { + id: 249, + name: c_str!("MSM8116"), + }, + SocId { + id: 250, + name: c_str!("MSM8616"), + }, + SocId { + id: 251, + name: c_str!("MSM8992"), + }, + SocId { + id: 253, + name: c_str!("APQ8094"), + }, + SocId { + id: 290, + name: c_str!("MDM9607"), + }, + SocId { + id: 291, + name: c_str!("APQ8096"), + }, + SocId { + id: 292, + name: c_str!("MSM8998"), + }, + SocId { + id: 293, + name: c_str!("MSM8953"), + }, + SocId { + id: 296, + name: c_str!("MDM8207"), + }, + SocId { + id: 297, + name: c_str!("MDM9207"), + }, + SocId { + id: 298, + name: c_str!("MDM9307"), + }, + SocId { + id: 299, + name: c_str!("MDM9628"), + }, + SocId { + id: 304, + name: c_str!("APQ8053"), + }, + SocId { + id: 305, + name: c_str!("MSM8996SG"), + }, + SocId { + id: 310, + name: c_str!("MSM8996AU"), + }, + SocId { + id: 311, + name: c_str!("APQ8096AU"), + }, + SocId { + id: 312, + name: c_str!("APQ8096SG"), + }, + SocId { + id: 317, + name: c_str!("SDM660"), + }, + SocId { + id: 318, + name: c_str!("SDM630"), + }, + SocId { + id: 319, + name: c_str!("APQ8098"), + }, + SocId { + id: 321, + name: c_str!("SDM845"), + }, + SocId { + id: 322, + name: c_str!("MDM9206"), + }, + SocId { + id: 324, + name: c_str!("SDA660"), + }, + SocId { + id: 325, + name: c_str!("SDM658"), + }, + SocId { + id: 326, + name: c_str!("SDA658"), + }, + SocId { + id: 327, + name: c_str!("SDA630"), + }, + SocId { + id: 338, + name: c_str!("SDM450"), + }, + SocId { + id: 341, + name: c_str!("SDA845"), + }, + SocId { + id: 345, + name: c_str!("SDM636"), + }, + SocId { + id: 346, + name: c_str!("SDA636"), + }, + SocId { + id: 349, + name: c_str!("SDM632"), + }, + SocId { + id: 350, + name: c_str!("SDA632"), + }, + SocId { + id: 351, + name: c_str!("SDA450"), + }, + SocId { + id: 356, + name: c_str!("SM8250"), + }, + SocId { + id: 394, + name: c_str!("SM6125"), + }, + SocId { + id: 402, + name: c_str!("IPQ6018"), + }, + SocId { + id: 403, + name: c_str!("IPQ6028"), + }, + SocId { + id: 421, + name: c_str!("IPQ6000"), + }, + SocId { + id: 422, + name: c_str!("IPQ6010"), + }, + SocId { + id: 425, + name: c_str!("SC7180"), + }, + SocId { + id: 453, + name: c_str!("IPQ6005"), + }, + SocId { + id: 455, + name: c_str!("QRB5165"), + }, +]; + +struct SocId { + id: u32, + name: &'static str::CStr, +} + +fn socinfo_machine(id: &u32) -> Option<&'static str::CStr> { + for idx in SOC_ID { + if idx.id == *id { + return Some(idx.name); + } + } + None +} + +struct QcomSocInfo { + soc_dev: *mut bindings::soc_device, + attr: bindings::soc_device_attribute, + dbg_root: *mut bindings::dentry, + info: SocInfoParams, +} + +impl QcomSocInfo { + fn socinfo_debugfs_init( + &self, + info: &mut SocInfo, + info_size: usize, + ) -> Option<c_types::c_void> { + None + } + + fn socinfo_debugfs_exit(&self) -> Option<c_types::c_void> { + None + } + + fn new() -> Self { + QcomSocInfo { + soc_dev: core::ptr::null_mut(), + attr: bindings::soc_device_attribute::default(), + dbg_root: core::ptr::null_mut(), + info: <SocInfoParams as Default>::default(), + } + } +} + +struct SocInfoDriver { + _pdev: Pin<Box<platdev::Registration>>, +} + +impl PlatformDriver for SocInfoDriver { + type DrvData = Pin<Box<QcomSocInfo>>; + + fn probe(device_id: i32) -> Result<Self::DrvData> { + pr_info!( + "Just checking if probe works, with device id {}\n", + device_id + ); + + let mut drv_data = Pin::from(Box::try_new(QcomSocInfo::new())?); + + let item_size: *mut usize = core::ptr::null_mut(); + + let mut info = unsafe { + bindings::qcom_smem_get( + QCOM_SMEM_HOST_ANY.try_into().unwrap(), + SMEM_HW_SW_BUILD_ID, + item_size, + ) as *mut SocInfo + }; + + let info_mut: &mut SocInfo; + unsafe { + info_mut = info.as_mut().unwrap(); + } + + drv_data.as_mut().attr.family = c_str!("Snapdragon").as_char_ptr(); + drv_data.as_mut().attr.machine = socinfo_machine(&info_mut.id).unwrap().as_char_ptr(); + + drv_data.as_mut().attr.soc_id = + CStr::from_bytes_with_nul(try_format!("{}\0", info_mut.id)?.as_bytes()) + .unwrap() + .as_char_ptr(); + + drv_data.as_mut().attr.revision = CStr::from_bytes_with_nul( + try_format!( + "{}.{}\0", + socinfo_major(info_mut.ver), + socinfo_minor(info_mut.ver) + )? + .as_bytes(), + ) + .unwrap() + .as_char_ptr(); + + unsafe { + drv_data.as_mut().soc_dev = bindings::soc_device_register(&mut drv_data.as_mut().attr); + drv_data + .as_mut() + .socinfo_debugfs_init(info.as_mut().unwrap(), *item_size); + bindings::add_device_randomness(info as *mut c_types::c_void, *item_size as u32) + }; + + Ok(drv_data) + } + fn remove(device_id: i32, _drv_data: Self::DrvData) -> Result { + pr_info!("removing socinfo_rust now with device id {}\n", device_id); + + unsafe { bindings::soc_device_unregister(_drv_data.soc_dev) }; + _drv_data.as_ref().socinfo_debugfs_exit(); + Ok(()) + } +} + +impl KernelModule for SocInfoDriver { + fn init() -> Result<Self> { + const OF_MATCH_TBL: ConstOfMatchTable<1> = + ConstOfMatchTable::new_const([c_str!("qcom-socinfo")]); + + let pdev = platdev::Registration::new_pinned::<SocInfoDriver>( + c_str!("qcom-socinfo"), + Some(&OF_MATCH_TBL), + &THIS_MODULE, + )?; + Ok(SocInfoDriver { _pdev: pdev }) + } +} diff --git a/rust/kernel/bindings_helper.h b/rust/kernel/bindings_helper.h index c64a630..a8c73450 100644 --- a/rust/kernel/bindings_helper.h +++ b/rust/kernel/bindings_helper.h @@ -18,6 +18,13 @@ #include <linux/platform_device.h> #include <linux/of_platform.h> #include <linux/security.h> +#include <linux/sys_soc.h> +#include <linux/err.h> +#include <linux/debugfs.h> +#include <linux/soc/qcom/smem.h> +#include <linux/types.h> +#include <linux/device.h> +#include <linux/random.h> // `bindgen` gets confused at certain things const gfp_t BINDINGS_GFP_KERNEL = GFP_KERNEL; -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC v2] soc: qcom: socinfo.rs: Add Rust Socinfo Driver implementation 2021-08-20 19:38 ` [RFC v2] " Antonio Martorana @ 2021-08-20 22:18 ` Miguel Ojeda 2021-08-20 23:05 ` amartora 0 siblings, 1 reply; 6+ messages in thread From: Miguel Ojeda @ 2021-08-20 22:18 UTC (permalink / raw) To: Antonio Martorana; +Cc: rust-for-linux 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] 6+ messages in thread
* Re: [RFC v2] soc: qcom: socinfo.rs: Add Rust Socinfo Driver implementation 2021-08-20 22:18 ` Miguel Ojeda @ 2021-08-20 23:05 ` amartora 0 siblings, 0 replies; 6+ 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] 6+ messages in thread
end of thread, other threads:[~2021-08-20 23:05 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 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 2021-08-20 23:05 ` 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).