[RFC PATCH] rust: hwmon: add basic hwmon abstractions

[RFC PATCH] rust: hwmon: add basic hwmon abstractions

[Donjuanplatinum]

Add a minimal Rust abstraction for the Hardware Monitoring (hwmon)
subsystem.

Currently, this abstraction supports a Minimum Viable Product (MVP) scope:
registering a single temperature sensor with the read-only `temp1_input`
attribute. Support for multi-channel, additional sensor types, and
writable attributes is deferred to future patches to keep this initial
foundation reviewable.

The primary architectural challenge in wrapping hwmon is initializing
`struct hwmon_chip_info`, which requires stable, self-referential
pointers to nested NULL-terminated arrays. Instead of relying on complex
macro generation for static allocation, this implementation leverages
a single heap allocation (`KBox`). It safely wires the self-referential
raw pointers using unaliased `&raw mut` during the allocation's
exclusive ownership phase, and then securely seals it by pinning
(`Pin::new_unchecked`).

For resource management, this implementation uses standard Rust RAII
(calling `hwmon_device_unregister` in `Drop`) rather than `devm_`
variants, ensuring natural drop ordering and avoiding complex C-side
trampolines for generic types.

Signed-off-by: Donjuanplatinum <donplat@barrensea.org>
---
 rust/bindings/bindings_helper.h   |   1 +
 rust/kernel/hwmon.rs              | 249 ++++++++++++++++++++++++++++++
 rust/kernel/lib.rs                |   2 +
 samples/rust/Kconfig              |  11 ++
 samples/rust/Makefile             |   1 +
 samples/rust/rust_driver_hwmon.rs |  50 ++++++
 6 files changed, 314 insertions(+)
 create mode 100644 rust/kernel/hwmon.rs
 create mode 100644 samples/rust/rust_driver_hwmon.rs

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 446dbeaf0..e08e9181d 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -60,6 +60,7 @@
 #include <linux/file.h>
 #include <linux/firmware.h>
 #include <linux/fs.h>
+#include <linux/hwmon.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/io-pgtable.h>
diff --git a/rust/kernel/hwmon.rs b/rust/kernel/hwmon.rs
new file mode 100644
index 000000000..5697a1a0d
--- /dev/null
+++ b/rust/kernel/hwmon.rs
@@ -0,0 +1,249 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Hardware Monitoring (Hwmon) abstractions.
+//!
+//! C header: [`include/linux/hwmon.h`](srctree/include/linux/hwmon.h)
+//!
+//! Currently, this abstraction supports registering a single temperature sensor with the
+//! `temp1_input` attribute (read-only). Multi-channel support, additional sensor types (fan,
+//! voltage, etc.), and writable attributes will be added in follow-up patches.
+
+use crate::{
+    bindings,
+    device::Device,
+    error::{from_err_ptr, from_result, Result},
+    prelude::*,
+    str::CStr,
+};
+
+use core::marker::{PhantomData, PhantomPinned};
+
+/// Sensor type.
+#[derive(Copy, Clone, Debug, PartialEq, Eq)]
+pub enum SensorType {
+    /// Temperature sensor.
+    Temp,
+}
+
+impl TryFrom<u32> for SensorType {
+    type Error = Error;
+
+    fn try_from(value: u32) -> Result<Self> {
+        match value {
+            bindings::hwmon_sensor_types_hwmon_temp => Ok(Self::Temp),
+            _ => Err(EINVAL),
+        }
+    }
+}
+
+/// Temperature attribute.
+#[derive(Copy, Clone, Debug, PartialEq, Eq)]
+pub enum TempAttr {
+    /// Temperature input value, in millidegrees Celsius.
+    Input,
+}
+
+impl TryFrom<u32> for TempAttr {
+    type Error = Error;
+
+    fn try_from(value: u32) -> Result<Self> {
+        match value {
+            bindings::hwmon_temp_attributes_hwmon_temp_input => Ok(Self::Input),
+            _ => Err(ENOTSUPP),
+        }
+    }
+}
+
+const HWMON_T_INPUT: u32 = 1u32 << bindings::hwmon_temp_attributes_hwmon_temp_input;
+
+/// The hwmon driver trait.
+#[vtable]
+pub trait Driver: Send + Sync {
+    /// Reads a sensor value.
+    fn read(&self, sensor: SensorType, attr: u32, channel: u32) -> Result<crate::ffi::c_long>;
+
+    /// Returns the sysfs file permission bits for a sensor attribute.
+    fn is_visible(&self, sensor: SensorType, attr: u32, channel: u32) -> u16;
+}
+
+/// Adapter translating C hwmon callbacks to [`Driver`] trait method calls.
+struct Adapter<T: Driver> {
+    _p: PhantomData<T>,
+}
+
+impl<T: Driver> Adapter<T> {
+    /// # Safety
+    ///
+    /// Called by the hwmon core during and after registration with the `drvdata`
+    /// pointer set in `hwmon_device_register_with_info`. The pointer remains valid
+    /// until `hwmon_device_unregister` returns in [`Registration`]'s `Drop`.
+    unsafe extern "C" fn is_visible_callback(
+        drvdata: *const core::ffi::c_void,
+        type_: u32,
+        attr: u32,
+        channel: crate::ffi::c_int,
+    ) -> u16 {
+        if drvdata.is_null() {
+            return 0;
+        }
+        // SAFETY: `drvdata` is `inner_ptr` set in `Registration::new`. The
+        // hwmon core's barrier in `hwmon_device_unregister` ensures this
+        // pointer is valid for the lifetime of any callback invocation.
+        let inner = unsafe { &*drvdata.cast::<HwmonInner<T>>() };
+
+        let sensor = match SensorType::try_from(type_) {
+            Ok(s) => s,
+            Err(_) => return 0,
+        };
+
+        // C core guarantees `channel >= 0`.
+        T::is_visible(&inner.driver, sensor, attr, channel as u32)
+    }
+
+    /// # Safety
+    ///
+    /// Called by the hwmon core. `dev` is the device created during registration, and `val`
+    /// points to writable memory for the result.
+    unsafe extern "C" fn read_callback(
+        dev: *mut bindings::device,
+        type_: u32,
+        attr: u32,
+        channel: crate::ffi::c_int,
+        val: *mut crate::ffi::c_long,
+    ) -> crate::ffi::c_int {
+        from_result(|| {
+            // SAFETY: `dev_get_drvdata` returns the pointer set during registration, valid
+            // until `hwmon_device_unregister` completes.
+            let drvdata = unsafe { bindings::dev_get_drvdata(dev) };
+            if drvdata.is_null() {
+                return Err(EINVAL);
+            }
+            let inner = unsafe { &*drvdata.cast::<HwmonInner<T>>() };
+
+            let sensor = SensorType::try_from(type_)?;
+            // C core guarantees `channel >= 0`.
+            let result = T::read(&inner.driver, sensor, attr, channel as u32)?;
+
+            // SAFETY: `val` is provided by the hwmon core and points to a valid `long`.
+            unsafe { *val = result };
+            Ok(0)
+        })
+    }
+}
+
+/// Container holding the driver and all C structures for hwmon registration.
+struct HwmonInner<T: Driver> {
+    driver: T,
+    ops: bindings::hwmon_ops,
+    temp_config: [u32; 2],
+    temp_channel: bindings::hwmon_channel_info,
+    channel_info_array: [*const bindings::hwmon_channel_info; 2],
+    chip_info: bindings::hwmon_chip_info,
+    _pin: PhantomPinned,
+}
+
+impl<T: Driver> HwmonInner<T> {
+    /// Returns a placeholder with all pointer fields set to null. The returned structure is
+    /// safe to drop — no dynamic resources are held before registration completes.
+    fn new_placeholder(driver: T) -> Self {
+        Self {
+            driver,
+            ops: bindings::hwmon_ops {
+                is_visible: Some(Adapter::<T>::is_visible_callback),
+                visible: 0,
+                read: Some(Adapter::<T>::read_callback),
+                read_string: None,
+                write: None,
+            },
+            temp_config: [0; 2],
+            temp_channel: bindings::hwmon_channel_info {
+                type_: 0,
+                config: core::ptr::null(),
+            },
+            channel_info_array: [core::ptr::null(), core::ptr::null()],
+            chip_info: bindings::hwmon_chip_info {
+                ops: core::ptr::null(),
+                info: core::ptr::null(),
+            },
+            _pin: PhantomPinned,
+        }
+    }
+}
+
+/// A registered hwmon device.
+pub struct Registration<T: Driver> {
+    hwmon_dev: *mut bindings::device,
+    // Held exclusively for drop ordering: keeps `HwmonInner<T>` alive until
+    // `hwmon_device_unregister` returns. The value is never read.
+    #[expect(dead_code)]
+    inner: Pin<KBox<HwmonInner<T>>>,
+}
+
+impl<T: Driver> Registration<T> {
+    /// Registers a new hwmon device.
+    ///
+    /// The device is registered as a child of `parent`. `name` must not contain characters
+    /// rejected by `hwmon_is_bad_char` (hyphens, spaces, asterisks).
+    pub fn new(parent: &Device, name: &CStr, data: T) -> Result<Self> {
+        let mut boxed = KBox::new(HwmonInner::new_placeholder(data), GFP_KERNEL)?;
+
+        // Wire self-referential pointers before pinning. We obtain a mutable raw pointer
+        // from the uniquely-owned `KBox` — no intermediate `&mut` reference is created,
+        // so no aliasing rules are violated.
+        //
+        // SAFETY: `boxed` is exclusively owned. We write only to C-struct fields meant
+        // for one-time initialization, never to `driver`.
+        let inner_ptr: *mut HwmonInner<T> = &raw mut *boxed;
+        unsafe {
+            (*inner_ptr).temp_config = [HWMON_T_INPUT, 0];
+            (*inner_ptr).temp_channel = bindings::hwmon_channel_info {
+                type_: bindings::hwmon_sensor_types_hwmon_temp,
+                config: (*inner_ptr).temp_config.as_ptr(),
+            };
+            (*inner_ptr).channel_info_array =
+                [&raw const (*inner_ptr).temp_channel, core::ptr::null()];
+            (*inner_ptr).chip_info = bindings::hwmon_chip_info {
+                ops: &raw const (*inner_ptr).ops,
+                info: (*inner_ptr).channel_info_array.as_ptr(),
+            };
+        }
+
+        // SAFETY: `HwmonInner<T>` is `!Unpin` (via `PhantomPinned`). All self-referential
+        // pointers are now set to their final values. The struct will never be moved.
+        let inner = unsafe { Pin::new_unchecked(boxed) };
+
+        let drvdata: *mut core::ffi::c_void = inner_ptr.cast();
+
+        // SAFETY: `chip_info` and all nested pointers target memory within the same
+        // allocation, which remains valid until `hwmon_device_unregister` in `Drop`.
+        // `parent.as_raw()` and `name.as_char_ptr()` are valid. The name is copied by
+        // the kernel (via `dev_set_name` → `kvasprintf_const`), so no lifetime issue.
+        let hwmon_dev = from_err_ptr(unsafe {
+            bindings::hwmon_device_register_with_info(
+                parent.as_raw(),
+                name.as_char_ptr(),
+                drvdata,
+                &raw const (*inner_ptr).chip_info,
+                core::ptr::null_mut(),
+            )
+        })?;
+
+        Ok(Self { hwmon_dev, inner })
+    }
+}
+
+impl<T: Driver> Drop for Registration<T> {
+    fn drop(&mut self) {
+        // SAFETY: `hwmon_dev` was returned by a successful registration. This call waits for
+        // all in-flight callbacks before returning.
+        unsafe { bindings::hwmon_device_unregister(self.hwmon_dev) };
+    }
+}
+
+// SAFETY: `T: Driver` requires `T: Send`. `Pin<KBox<HwmonInner<T>>>` is `Send` when `T: Send`.
+unsafe impl<T: Driver> Send for Registration<T> {}
+
+// SAFETY: `T: Driver` requires `T: Sync`, which makes `HwmonInner<T>: Sync`, and therefore
+// `Pin<KBox<HwmonInner<T>>>: Sync`. `Registration` has no public methods that could cause
+// data races through shared references.
+unsafe impl<T: Driver> Sync for Registration<T> {}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index b72b2fbe0..0ff9d6e6e 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -74,6 +74,8 @@
 pub mod fs;
 #[cfg(CONFIG_GPU_BUDDY = "y")]
 pub mod gpu;
+#[cfg(CONFIG_HWMON = "y")]
+pub mod hwmon;
 #[cfg(CONFIG_I2C = "y")]
 pub mod i2c;
 pub mod id_pool;
diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
index c49ab9106..d306b681c 100644
--- a/samples/rust/Kconfig
+++ b/samples/rust/Kconfig
@@ -161,6 +161,17 @@ config SAMPLE_RUST_DRIVER_AUXILIARY
 
 	  If unsure, say N.
 
+config SAMPLE_RUST_HWMON
+	tristate "Hwmon Driver"
+	depends on HWMON=y
+	help
+	  This option builds the Rust hwmon driver sample.
+
+	  To compile this as a module, choose M here:
+	  the module will be called rust_driver_hwmon.
+
+	  If unsure, say N.
+
 config SAMPLE_RUST_SOC
 	tristate "SoC Driver"
 	select SOC_BUS
diff --git a/samples/rust/Makefile b/samples/rust/Makefile
index 6c0aaa58c..0109d7a8d 100644
--- a/samples/rust/Makefile
+++ b/samples/rust/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_SAMPLE_RUST_DRIVER_USB)		+= rust_driver_usb.o
 obj-$(CONFIG_SAMPLE_RUST_DRIVER_FAUX)		+= rust_driver_faux.o
 obj-$(CONFIG_SAMPLE_RUST_DRIVER_AUXILIARY)	+= rust_driver_auxiliary.o
 obj-$(CONFIG_SAMPLE_RUST_CONFIGFS)		+= rust_configfs.o
+obj-$(CONFIG_SAMPLE_RUST_HWMON)			+= rust_driver_hwmon.o
 obj-$(CONFIG_SAMPLE_RUST_SOC)			+= rust_soc.o
 
 rust_print-y := rust_print_main.o rust_print_events.o
diff --git a/samples/rust/rust_driver_hwmon.rs b/samples/rust/rust_driver_hwmon.rs
new file mode 100644
index 000000000..3362de924
--- /dev/null
+++ b/samples/rust/rust_driver_hwmon.rs
@@ -0,0 +1,50 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+//! Rust hwmon device sample.
+
+use kernel::{faux, hwmon, prelude::*};
+
+module! {
+    type: SampleModule,
+    name: "rust_hwmon_driver",
+    authors: ["DonjuanPlatium"],
+    description: "Rust hwmon device sample",
+    license: "GPL",
+}
+
+struct SampleHwmon;
+
+#[vtable]
+impl hwmon::Driver for SampleHwmon {
+    fn read(&self, _sensor: hwmon::SensorType, _attr: u32, _channel: u32) -> Result<c_long> {
+        // Always return 25°C.
+        Ok(25000)
+    }
+
+    fn is_visible(&self, _sensor: hwmon::SensorType, _attr: u32, _channel: u32) -> u16 {
+        // All declared attributes are world-readable.
+        0o444
+    }
+}
+
+struct SampleModule {
+    _hwmon: hwmon::Registration<SampleHwmon>,
+    _faux: faux::Registration,
+}
+
+impl kernel::Module for SampleModule {
+    fn init(_module: &'static ThisModule) -> Result<Self> {
+        pr_info!("Initialising Rust Hwmon Sample\n");
+
+        let faux = faux::Registration::new(c"rust-hwmon-sample-device", None)?;
+
+        let hwmon = hwmon::Registration::new(faux.as_ref(), c"sample", SampleHwmon)?;
+
+        pr_info!("Registered hwmon device\n");
+
+        Ok(Self {
+            _hwmon: hwmon,
+            _faux: faux,
+        })
+    }
+}
-- 
2.53.0

Re: [RFC PATCH] rust: hwmon: add basic hwmon abstractions

[Guenter Roeck]

On 5/12/26 08:57, Donjuanplatinum wrote:
> Add a minimal Rust abstraction for the Hardware Monitoring (hwmon)
> subsystem.
> 
> Currently, this abstraction supports a Minimum Viable Product (MVP) scope:
> registering a single temperature sensor with the read-only `temp1_input`
> attribute. Support for multi-channel, additional sensor types, and
> writable attributes is deferred to future patches to keep this initial
> foundation reviewable.
> 
> The primary architectural challenge in wrapping hwmon is initializing
> `struct hwmon_chip_info`, which requires stable, self-referential
> pointers to nested NULL-terminated arrays. Instead of relying on complex
> macro generation for static allocation, this implementation leverages
> a single heap allocation (`KBox`). It safely wires the self-referential
> raw pointers using unaliased `&raw mut` during the allocation's
> exclusive ownership phase, and then securely seals it by pinning
> (`Pin::new_unchecked`).
> 
> For resource management, this implementation uses standard Rust RAII
> (calling `hwmon_device_unregister` in `Drop`) rather than `devm_`
> variants, ensuring natural drop ordering and avoiding complex C-side
> trampolines for generic types.
> 
> Signed-off-by: Donjuanplatinum <donplat@barrensea.org>

Sorry, rust support in the hardware monitoring subsystem will have to wait
for someone else to pick up maintainership.

Guenter

Re: [RFC PATCH] rust: hwmon: add basic hwmon abstractions

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Use-After-Free via `name` sysfs attribute.
- [Medium] Bypassing the mandatory `pin_init!` safety framework for pinned initialization.
- [Low] Missing `#[inline]` annotations on small abstraction helpers.
- [Low] Misspelled author name in module metadata.
--

commit c36781b9829b25d8c4099a3b3ad9cfa1c367592f
Author: Donjuanplatinum <donplat@barrensea.org>

rust: hwmon: add basic hwmon abstractions

Add a minimal Rust abstraction for the Hardware Monitoring subsystem.
This supports registering a single temperature sensor with the temp1_input
attribute.

> diff --git a/rust/kernel/hwmon.rs b/rust/kernel/hwmon.rs
> new file mode 100644
> index 0000000000000..5697a1a0d6c29
> --- /dev/null
> +++ b/rust/kernel/hwmon.rs

[ ... ]

> +impl TryFrom<u32> for SensorType {
> +    type Error = Error;
> +
> +    fn try_from(value: u32) -> Result<Self> {

This isn't a bug, but should this small abstraction helper be annotated with
#[inline] to match the Rust subsystem guidelines?

[ ... ]

> +impl TryFrom<u32> for TempAttr {
> +    type Error = Error;
> +
> +    fn try_from(value: u32) -> Result<Self> {

This isn't a bug, but should this helper also be annotated with #[inline]?

[ ... ]

> +impl<T: Driver> HwmonInner<T> {
> +    /// Returns a placeholder with all pointer fields set to null. The returned structure is
> +    /// safe to drop — no dynamic resources are held before registration completes.
> +    fn new_placeholder(driver: T) -> Self {

This isn't a bug, but could this abstraction function also be annotated
with #[inline]?

[ ... ]

> +impl<T: Driver> Registration<T> {
> +    /// Registers a new hwmon device.
> +    ///
> +    /// The device is registered as a child of parent. name must not contain characters
> +    /// rejected by hwmon_is_bad_char (hyphens, spaces, asterisks).
> +    pub fn new(parent: &Device, name: &CStr, data: T) -> Result<Self> {
> +        let mut boxed = KBox::new(HwmonInner::new_placeholder(data), GFP_KERNEL)?;

Is it possible to use the try_pin_init! framework here instead of manually
allocating a KBox and later sealing it with Pin::new_unchecked? 

The subsystem documentation states that try_pin_init! should be used to
initialize structs that require pinning.

[ ... ]

> +        // SAFETY: chip_info and all nested pointers target memory within the same
> +        // allocation, which remains valid until hwmon_device_unregister in Drop.
> +        // parent.as_raw() and name.as_char_ptr() are valid. The name is copied by
> +        // the kernel (via dev_set_name -> kvasprintf_const), so no lifetime issue.
> +        let hwmon_dev = from_err_ptr(unsafe {
> +            bindings::hwmon_device_register_with_info(
> +                parent.as_raw(),
> +                name.as_char_ptr(),

The safety comment states the name is copied by the kernel via dev_set_name,
but does the hwmon core use this pointer directly for the name sysfs
attribute rather than just the kobject directory name?

If hwmon_device_register_with_info() stores the raw pointer directly in
hwdev->name without copying it, and this function accepts a temporary &CStr
that can be dropped immediately after registration, will reading
/sys/class/hwmon/hwmonX/name from userspace dereference a freed pointer?

Could this API require a &'static CStr, or securely allocate and copy the
string to prevent a use-after-free?

> +                drvdata,
> +                &raw const (*inner_ptr).chip_info,
> +                core::ptr::null_mut(),
> +            )
> +        })?;

[ ... ]

> diff --git a/samples/rust/rust_driver_hwmon.rs b/samples/rust/rust_driver_hwmon.rs
> new file mode 100644
> index 0000000000000..3362de9246e09
> --- /dev/null
> +++ b/samples/rust/rust_driver_hwmon.rs

[ ... ]

> +module! {
> +    type: SampleModule,
> +    name: "rust_hwmon_driver",
> +    authors: ["DonjuanPlatium"],

This isn't a bug, but there is a typo in the author name (DonjuanPlatium
instead of DonjuanPlatinum).

> +    description: "Rust hwmon device sample",
> +    license: "GPL",
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512155747.10136-1-donplat@barrensea.org?part=1

Re: [RFC PATCH] rust: hwmon: add basic hwmon abstractions

[kernel test robot]

Hi Donjuanplatinum,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build warnings:

[auto build test WARNING on rust/rust-next]
[also build test WARNING on groeck-staging/hwmon groeck-staging/hwmon-next groeck-staging/watchdog groeck-staging/watchdog-next linus/master v7.1-rc3 next-20260508]
[cannot apply to ah-configfs/configfs-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Donjuanplatinum/rust-hwmon-add-basic-hwmon-abstractions/20260514-050322
base:   https://github.com/Rust-for-Linux/linux rust-next
patch link:    https://lore.kernel.org/r/20260512155747.10136-1-donplat%40barrensea.org
patch subject: [RFC PATCH] rust: hwmon: add basic hwmon abstractions
config: x86_64-rhel-9.4-rust (https://download.01.org/0day-ci/archive/20260514/202605140550.6k70Rrdm-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
rustc: rustc 1.88.0 (6b00bc388 2025-06-23)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260514/202605140550.6k70Rrdm-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202605140550.6k70Rrdm-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> warning: statement has unnecessary safety comment
   --> rust/kernel/hwmon.rs:196:9
   |
   196 |         let inner_ptr: *mut HwmonInner<T> = &raw mut *boxed;
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   help: consider removing the safety comment
   --> rust/kernel/hwmon.rs:194:9
   |
   194 |         // SAFETY: `boxed` is exclusively owned. We write only to C-struct fields meant
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_safety_comment
   = note: requested on the command line with `-W clippy::unnecessary-safety-comment`

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH 0/2] rust: hwmon: rework hwmon abstractions, add TMP108 driver

[skxby]

Hi Guenter,

[1] Positioning

This series follows up on Donjuanplatinum's RFC [1], which proposed basic
Rust abstractions for the hwmon subsystem. Your reply [2] noted that Rust
support in hwmon would have to wait for someone else to pick up
maintainership, and the thread has been quiet since. I am not re-litigating
that decision here. I have taken the proposed abstractions, reworked them
against the current rust tree, and written a real driver on top of them
(TMP108, mirroring drivers/hwmon/tmp108.c). I am posting the result so the
work itself - the shape of the abstraction and the driver built on it - can
be looked at on its own merits, whenever and by whoever takes the
maintainership question up.

[2] What this series contains

  Patch 1 - rust: hwmon: add hwmon abstractions (rework of the RFC layer)
    rust/kernel/hwmon.rs: generic hwmon registration with pinning and a
    self-referential Registration<T> type; attribute callback dispatch for
    two sensor types (chip and temp); a Driver trait with three required
    temp methods (read/write/is_visible) and three chip methods marked
    #[optional] (chip_read/chip_write/chip_is_visible), following the
    existing #[vtable] idiom in rust/kernel/miscdevice.rs. Attribute
    channel masks are derived from the modeled attribute enums rather than
    a hardcoded table. No driver-specific logic lives in this file.

  Patch 2 - hwmon: (tmp108) add Rust TMP108 driver
    A Rust driver for the TI TMP108, exposing 8 sysfs attributes:
    temp1_input, temp1_min, temp1_max, temp1_min_hyst, temp1_max_hyst,
    temp1_min_alarm, temp1_max_alarm, and update_interval (chip channel,
    four conversion-rate buckets, via a Rust-side port of
    find_closest_descending). Numeric and error semantics follow the C
    driver (millidegree truncation, write clamping, absolute-value
    hysteresis ABI, EAGAIN for one-shot conversion not ready).

[3] Scope statement

The abstraction deliberately covers a subset of hwmon, and the cut is made
by a single rule: an attribute family is included if its shape can be
reused unchanged by a second, different sensor type; it is left out if it
would have to be re-cut per driver. Under that rule the temp attribute set
is the 7 entries shared by TMP108 and the hwmon temp templates
(input/min/max/*_hyst/*_alarm); the crit family is left out of the enum
entirely (TMP108 has no crit registers, so keeping it would force a
per-driver re-cut), and update_interval lives in a separate ChipAttr
because it cannot be reused on the temp type at all. Everything else
(label strings, in/curr/power/fan channels, multi-channel config, the
remaining 16 chip attributes) is left for follow-up work.

On the driver side, update_interval was originally a known gap relative to
the C driver; it is implemented in this series, so patch 2 now matches the
C driver's attribute surface item for item.

[4] Testing

Evidence is stated per level, and the wording matches the level:

  - Both patches apply and compile against the 7.3.0-rc5 source tree
    (kernel tarball, rustc 1.85.0, bindgen 0.73.2). A forced rebuild of
    the driver reports zero warnings and zero errors; CLIPPY=1 reports
    zero warnings, and a build with RUSTDOCFLAGS="-D warnings" reports
    none either. checkpatch reports 0 errors / 0 warnings on both files
    of the series (491 and 573 lines checked, "ready for submission").

  - The driver is boot-tested and exercised end-to-end on i2c-stub in a
    VM. A 22-case regression suite over update_interval (four rate
    buckets, write-then-read-back, out-of-range clamping including
    midpoints, cross-chip table isolation, bit-exact register checks)
    passes 22/22 on the frozen build.

  - The full 8-attribute sysfs surface was diffed attribute by attribute
    against the C driver under i2c-stub; read and write values match on
    every attribute. One environment-dependent difference is documented
    in [5]: update_interval's power-on default reads 4000 ms on i2c-stub
    vs 1000 ms for the C driver, because the C driver sets the rate in
    probe and this driver does not yet replicate that probe-time
    initialization.

  - Lifecycle stress: 500 register/unregister cycles completed with no
    failures and no residue (fails=0, duplicate-attribute=0, kmemleak
    clean). Concurrent access against module removal was exercised too:
    8 concurrent readers across 200 removal cycles, and 4 readers plus 4
    writers across 50 removal cycles, both completed with no faults
    observed in dmesg, no sysfs residue, and kmemleak clean.

    Those concurrent runs were made on a kernel built without KASAN or
    KCSAN. "no faults observed" means exactly that - no observable fault
    under this configuration - and not that a use-after-free or a
    time-of-check/time-of-use window is absent. The structural argument
    for the unregister path is separate, and is stated in [8]: when
    hwmon_device_unregister returns, in-flight sysfs callbacks have
    drained (kernfs_drain in fs/kernfs/dir.c), which is what makes
    dropping the self-referential state safe.

  - The lifecycle stress also surfaced a real defect in this series, and
    this revision fixes it. On the hwmon registration error path, module
    init returned early and left the i2c client it had created
    unregistered, so a failed registration could keep that client
    attached and hold its address until reboot. The error path now
    unregisters the client it created before returning. Before the fix
    the stress loop failed 924/1000 operations; after the fix the frozen
    build completed 500/500 cycles clean. The defect was in the driver's
    module-init error path (samples/rust/tmp108.rs); the abstraction's own
    Registration/Drop path was not implicated.

[5] Known gaps

  - HWMON_C_REGISTER_TZ: The Rust version does not set HWMON_C_REGISTER_TZ,
    so no thermal zone device is registered (the C driver registers one
    per visible temp channel when CONFIG_THERMAL_OF is enabled). Thermal
    integration is left for a follow-up.
  - One-shot mode interaction with system suspend/resume is not modeled
    yet; the EAGAIN path exists but its PM interaction is deferred.
  - The p3t1035 compatible chip (hysteresis bits differ, max/min
    visibility differs) is not handled; the driver is TMP108-only for now.
  - update_interval's power-on default differs from the C driver (4000 ms
    on i2c-stub vs 1000 ms); see [4]. This is a probe-time initialization
    gap in the driver, not an ABI difference.
  - This series is built and exercised against a 7.3.0-rc5 source tarball
    (no git tree). That shows the patches are self-consistent with that
    tree; it does not by itself show they apply unchanged to the current
    rust-next or hwmon-next tip, whose rust/kernel/ APIs may have drifted.
  - All end-to-end evidence comes from i2c-stub in a VM. No physical
    TMP108 hardware was exercised; real-device behavior (in particular
    alarm timing and conversion latency) is unverified.

[6] Open questions

  1. Does the chip-channel shape (ChipAttr + #[optional]
     chip_read/chip_write/chip_is_visible, no default bodies, macro-injected
     build_error!) match what a hwmon Rust abstraction should look like, or
     would you prefer a different split (e.g. a single read/write with an
     enum over both types)?
  2. Is deriving the channel config masks from the attribute enums
     (instead of hardcoding a table) acceptable for the abstraction, or is
     an explicit per-driver config table preferred?
  3. Should the crit family be re-added to the temp attribute enum for
     completeness (drivers that have crit registers would then not need to
     extend the enum), at the cost of a wider enum that most drivers partially
     use?
  4. Is a TMP108 driver useful as the first in-tree consumer of the
     abstraction, or would a different driver (or a sample under samples/)
     serve better?

[7] Baseline

Both patches apply and compile against the 7.3.0-rc5 mainline source
tarball (no git tree; commit identity = the 7.3.0-rc5 release), with the
rust toolchain versions listed in [4].

[8] Differences from the RFC v0.1 abstraction

Relative to Donjuanplatinum's original RFC layer, this series changes:

  - Registration: the RFC used devm-based registration; here a
    self-referential Registration<T> owns its inner state, pins it, and
    unregisters in Drop. hwmon_device_unregister returning guarantees all
    in-flight sysfs callbacks have drained (kernfs_drain waits for active
    references in fs/kernfs/dir.c), which is what makes dropping the
    self-referential state safe.
  - Channel info: the channel info array now carries a chip channel in
    addition to two temp channels (3 entries + NULL terminator), and the
    chip config mask is included.
  - Optional methods: chip callbacks use #[optional] with no default
    bodies (per rust/macros/vtable.rs, which rejects #[optional] methods
    with bodies); the adapter layer registers hwmon_ops callbacks only
    when the driver implements them.
  - Error-path cleanup: the RFC layer relied on devm; here the driver's
    module-init error path releases what it created explicitly. If hwmon
    registration fails, the i2c client that init created is unregistered
    before init returns, so a failed registration does not leave the
    client attached. SAFETY: that client pointer is created in this
    function (non-null, not ERR_PTR) and, on the failure branch, has not
    been handed to hwmon and no in-flight callback can reach it, so the
    explicit i2c_unregister_device is sound.

[9] Closing

I am not expecting a merge decision with this posting. My aim is to put a
concrete, working series in front of the subsystem so the abstraction
design can be reviewed on its own merits. I am awaiting guidance on the
maintainership question and on the open questions above, and will revise
the series accordingly.

Thanks,
<SENDER NAME>

Re: [PATCH 0/2] rust: hwmon: rework hwmon abstractions, add TMP108 driver

[Donjuanplatinum]

1. Please clarify the extent of AI involvement with the guideliens:
https://docs.kernel.org/process/coding-assistants.html

2. Please Cc the Rust-for-Linux mailinglist.

> Thanks,
> <SENDER NAME>
3. ?  

4. Please use your legal name in the sign-off filed.

5. Given the outstanding maintainership question, it may be worth
establishing whether there is sufficient interest in maintaining and
extending this abstraction before proceeding further.

Re: [PATCH 0/2] rust: hwmon: rework hwmon abstractions, add TMP108 driver

[skxby]

Hi Donjuanplatinum,

Thank you for taking the time to look at this - and for replying at all.
Let me answer each point.

1. AI involvement

Fair question, and I should have stated it up front. The work was
developed with AI assistance: I run a multi-agent setup (a scheduler
plus several model-backed agents) that produced code drafts, ran the
build/regression/stress-test cycles and drafted the commit messages and
cover letter. Every patch was reviewed by me, and I built and tested
each change myself; the two bugs we found during this round (an i2c
client leak on the registration error path, and a missing null
terminator in chip_config) were both found by the lifecycle stress tests
I ran and reproduced before fixing. I signed off on the series and I
take responsibility for it.

For v2 I will add an "Assisted-by: LLM" tag as described in
Documentation/process/coding-assistants.html. Please tell me if you want
the tool names listed there as well.

2. Cc Rust-for-Linux

Agreed - I did not Cc rust-for-linux on v1, and I should have. v2 will
Cc rust-for-linux@vger.kernel.org.

3. "?"

I am not sure what this referred to - could you say a little more about
what you were questioning there? I would rather ask than guess.

4. Legal name in sign-off

You are right that "skxby <2374604267@qq.com>" is not a legal name.
The series will be re-signed with my real name and address in v2.

5. Maintainership and interest

I am not asking anyone to take this over, and I am not claiming
maintainership. The point of posting was to have the shape of the
abstraction and one real driver on top of it looked at, and to find out
whether that shape is the right one at all - which your reply and the
review have already started to do.

On that note: Sashiko's review of patch 1 found issues that are real,
and I agree with the ones I have checked so far. In particular it
flagged the missing null terminator in chip_config - which we had
already found and fixed ourselves through stress testing, and which is a
good sign the review is looking at the right things - and it flagged
that chip_read/chip_write are called unconditionally in the read/write
callbacks, which would break any driver that does not implement those
optional methods. That second one is a genuine design flaw in the
abstraction, not in the driver, and I am fixing it in v2 along with the
name lifetime issue and the locking documentation.

So: is there enough interest to keep going? That is exactly what I am
trying to establish too, and your answer - either way - is the most
useful input available. If the answer is "not yet", I would rather hear
it now than keep posting revisions.

Thanks again for looking.

<SENDER REAL NAME>