[PATCH RFC 0/3] Initial work for Rust abstraction for HID device driver development

[PATCH RFC 0/3] Initial work for Rust abstraction for HID device driver development

[Rahul Rameshbabu]

Hello,

I am a hobbyist developer who has been working on a project to create a new Rust
HID device driver and the needed core abstractions for writing more HID device
drivers in Rust. My goal is to support the USB Monitor Control Class needed for
functionality such as backlight control for monitors like the Apple Studio
Display and Apple Pro Display XDR. A new backlight API will be required to
support multiple backlight instances and will be mapped per DRM connector. The
current backlight API is designed around the assumption of only a single
internal panel being present. I am currently working on making this new API for
DRM in parallel to my work on the HID side of the stack for supporting these
displays.

  https://binary-eater.github.io/tags/usb-monitor-control/

Julius Zint had attempted to do so a year ago with a C HID driver but was gated
by the lack of an appropriate backlight API for external displays. I asked him
for permission to do the work need in Rust and plan to accredit him for the HID
report handling for backlight in the USB Monitor Control Class standard.

  https://lore.kernel.org/lkml/f95da7ff-06dd-2c0e-d563-7e5ad61c3bcc@redhat.com/

I was hoping to get initial feedback on this work to make sure I am on the right
path for making a Rust HID abstraction that would be acceptable upstream. The
patches compile with WERROR being disabled. This is necessary since Rust treats
missing documentation comments as warnings (which is a good thing). I also need
to go in and add more SAFETY comments.

Thanks,
Rahul Rameshbabu

Rahul Rameshbabu (3):
  rust: core abstractions for HID drivers
  rust: hid: USB Monitor Control Class driver
  rust: hid: demo the core abstractions for probe and remove

 drivers/hid/Kconfig                |  16 ++
 drivers/hid/Makefile               |   1 +
 drivers/hid/hid_monitor_control.rs |  42 +++++
 rust/bindings/bindings_helper.h    |   1 +
 rust/kernel/hid.rs                 | 245 +++++++++++++++++++++++++++++
 rust/kernel/lib.rs                 |   2 +
 6 files changed, 307 insertions(+)
 create mode 100644 drivers/hid/hid_monitor_control.rs
 create mode 100644 rust/kernel/hid.rs

-- 
2.47.2

[PATCH RFC 1/3] rust: core abstractions for HID drivers

[Rahul Rameshbabu]

These abstractions enable the development of HID drivers in Rust by binding
with the HID core C API. They provide Rust types that map to the
equivalents in C. In this initial draft, only hid_device and hid_device_id
are provided direct Rust type equivalents. hid_driver is specially wrapped
with a custom Driver type. The module_hid_driver! macro provides analogous
functionality to its C equivalent.

Future work for these abstractions would include more bindings for common
HID-related types, such as hid_field, hid_report_enum, and hid_report.
Providing Rust equivalents to useful core HID functions will also be
necessary for HID driver development in Rust.

Some concerns with this initial draft
  - The need for a DeviceId and DeviceIdShallow type.
    + DeviceIdShallow is used to guarantee the safety requirement for the
      Sync trait.
  - The create_hid_driver call in the module_hid_driver! macro does not use
    Pin semantics for passing the ID_TABLE. I could not get Pin semantics
    to work in a const fn. I get a feeling this might be safe but need help
    reviewing this.

Signed-off-by: Rahul Rameshbabu <sergeantsagara@protonmail.com>
---
 drivers/hid/Kconfig             |   8 ++
 rust/bindings/bindings_helper.h |   1 +
 rust/kernel/hid.rs              | 245 ++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs              |   2 +
 4 files changed, 256 insertions(+)
 create mode 100644 rust/kernel/hid.rs

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index b53eb569bd49..e085964c7ffc 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -714,6 +714,14 @@ config HID_MEGAWORLD_FF
 	Say Y here if you have a Mega World based game controller and want
 	to have force feedback support for it.
 
+config RUST_HID_ABSTRACTIONS
+	bool "Rust HID abstractions support"
+	depends on RUST
+	depends on HID=y
+	help
+	Adds support needed for HID drivers written in Rust. It provides a
+	wrapper around the C hid core.
+
 config HID_REDRAGON
 	tristate "Redragon keyboards"
 	default !EXPERT
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 55354e4dec14..e2e95afe9f4a 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -16,6 +16,7 @@
 #include <linux/file.h>
 #include <linux/firmware.h>
 #include <linux/fs.h>
+#include <linux/hid.h>
 #include <linux/jiffies.h>
 #include <linux/jump_label.h>
 #include <linux/mdio.h>
diff --git a/rust/kernel/hid.rs b/rust/kernel/hid.rs
new file mode 100644
index 000000000000..f13476b49e7d
--- /dev/null
+++ b/rust/kernel/hid.rs
@@ -0,0 +1,245 @@
+// SPDX-License-Identifier: GPL-2.0
+
+// Copyright (C) 2025 Rahul Rameshbabu <sergeantsagara@protonmail.com>
+
+use crate::{error::*, prelude::*, types::Opaque};
+use core::marker::PhantomData;
+
+#[repr(transparent)]
+pub struct Device(Opaque<bindings::hid_device>);
+
+impl Device {
+    unsafe fn from_ptr<'a>(ptr: *mut bindings::hid_device) -> &'a mut Self {
+        let ptr = ptr.cast::<Self>();
+
+        unsafe { &mut *ptr }
+    }
+
+    pub fn vendor(&self) -> u32 {
+        let hdev = self.0.get();
+
+        unsafe { (*hdev).vendor }
+    }
+
+    pub fn product(&self) -> u32 {
+        let hdev = self.0.get();
+
+        unsafe { (*hdev).product }
+    }
+}
+
+#[repr(transparent)]
+pub struct DeviceIdShallow(Opaque<bindings::hid_device_id>);
+
+// SAFETY: `DeviceIdShallow` doesn't expose any &self method to access internal data, so it's safe to
+// share `&DriverVTable` across execution context boundaries.
+unsafe impl Sync for DeviceIdShallow {}
+
+impl DeviceIdShallow {
+    pub const fn new() -> Self {
+        DeviceIdShallow(Opaque::new(bindings::hid_device_id {
+            // SAFETY: The rest is zeroed out to initialize `struct hid_device_id`,
+            // sets `Option<&F>` to be `None`.
+            ..unsafe { ::core::mem::MaybeUninit::<bindings::hid_device_id>::zeroed().assume_init() }
+        }))
+    }
+
+    pub const fn new_usb(vendor: u32, product: u32) -> Self {
+        DeviceIdShallow(Opaque::new(bindings::hid_device_id {
+            bus: 0x3, /* BUS_USB */
+            vendor: vendor,
+            product: product,
+            // SAFETY: The rest is zeroed out to initialize `struct hid_device_id`,
+            // sets `Option<&F>` to be `None`.
+            ..unsafe { ::core::mem::MaybeUninit::<bindings::hid_device_id>::zeroed().assume_init() }
+        }))
+    }
+
+    const unsafe fn as_ptr(&self) -> *const bindings::hid_device_id {
+        self.0.get()
+    }
+}
+
+#[repr(transparent)]
+pub struct DeviceId(Opaque<bindings::hid_device_id>);
+
+impl DeviceId {
+    unsafe fn from_ptr<'a>(ptr: *mut bindings::hid_device_id) -> &'a mut Self {
+        let ptr = ptr.cast::<Self>();
+
+        unsafe { &mut *ptr }
+    }
+
+    unsafe fn from_const_ptr<'a>(ptr: *const bindings::hid_device_id) -> &'a Self {
+        let ptr = ptr.cast::<Self>();
+
+        unsafe { &(*ptr) }
+    }
+
+    pub fn vendor(&self) -> u32 {
+        let hdev_id = self.0.get();
+
+        unsafe { (*hdev_id).vendor }
+    }
+
+    pub fn product(&self) -> u32 {
+        let hdev_id = self.0.get();
+
+        unsafe { (*hdev_id).product }
+    }
+}
+
+/*
+#[repr(transparent)]
+pub struct Field(Opaque<bindings::hid_field>);
+
+#[repr(transparent)]
+pub struct ReportEnum(Opaque<bindings::hid_report_enum>);
+
+#[repr(transparent)]
+pub struct Report(Opaque<bindings::hid_report>);
+*/
+
+#[vtable]
+pub trait Driver {
+    fn probe(_dev: &mut Device, _id: &DeviceId) -> Result {
+        build_error!(VTABLE_DEFAULT_ERROR)
+    }
+
+    fn remove(_dev: &mut Device) {
+    }
+}
+
+struct Adapter<T: Driver> {
+    _p: PhantomData<T>,
+}
+
+impl<T: Driver> Adapter<T> {
+    unsafe extern "C" fn probe_callback(
+        hdev: *mut bindings::hid_device,
+        hdev_id: *const bindings::hid_device_id,
+    ) -> crate::ffi::c_int {
+        from_result(|| {
+            let dev = unsafe { Device::from_ptr(hdev) };
+            let dev_id = unsafe { DeviceId::from_const_ptr(hdev_id) };
+            T::probe(dev, dev_id)?;
+            Ok(0)
+        })
+    }
+
+    unsafe extern "C" fn remove_callback(hdev: *mut bindings::hid_device) {
+        let dev = unsafe { Device::from_ptr(hdev) };
+        T::remove(dev);
+    }
+}
+
+#[repr(transparent)]
+pub struct DriverVTable(Opaque<bindings::hid_driver>);
+
+// SAFETY: `DriverVTable` doesn't expose any &self method to access internal data, so it's safe to
+// share `&DriverVTable` across execution context boundaries.
+unsafe impl Sync for DriverVTable {}
+
+pub const fn create_hid_driver<T: Driver>(
+    name: &'static CStr,
+    id_table: &'static DeviceIdShallow,
+) -> DriverVTable {
+    DriverVTable(Opaque::new(bindings::hid_driver {
+        name: name.as_char_ptr().cast_mut(),
+        id_table: unsafe { id_table.as_ptr() },
+        probe: if T::HAS_PROBE {
+            Some(Adapter::<T>::probe_callback)
+        } else {
+            None
+        },
+        remove: if T::HAS_REMOVE {
+            Some(Adapter::<T>::remove_callback)
+        } else {
+            None
+        },
+        // SAFETY: The rest is zeroed out to initialize `struct hid_driver`,
+        // sets `Option<&F>` to be `None`.
+        ..unsafe { core::mem::MaybeUninit::<bindings::hid_driver>::zeroed().assume_init() }
+    }))
+}
+
+pub struct Registration {
+    driver: Pin<&'static mut DriverVTable>,
+}
+
+unsafe impl Send for Registration {}
+
+impl Registration {
+    pub fn register(
+        module: &'static crate::ThisModule,
+        driver: Pin<&'static mut DriverVTable>,
+        name: &'static CStr,
+    ) -> Result<Self> {
+        to_result(unsafe {
+            bindings::__hid_register_driver(driver.0.get(), module.0, name.as_char_ptr())
+        })?;
+
+        Ok(Registration { driver })
+    }
+}
+
+impl Drop for Registration {
+    fn drop(&mut self) {
+        unsafe {
+            bindings::hid_unregister_driver(self.driver.0.get())
+        };
+    }
+}
+
+#[macro_export]
+macro_rules! usb_device {
+    (vendor: $vendor:expr, product: $product:expr $(,)?) => {
+        $crate::hid::DeviceIdShallow::new_usb($vendor, $product)
+    }
+}
+
+#[macro_export]
+macro_rules! module_hid_driver {
+    (@replace_expr $_t:tt $sub:expr) => {$sub};
+
+    (@count_devices $($x:expr),*) => {
+        0usize $(+ $crate::module_hid_driver!(@replace_expr $x 1usize))*
+    };
+
+    (driver: $driver:ident, id_table: [$($dev_id:expr),+ $(,)?], name: $name:tt, $($f:tt)*) => {
+        struct Module {
+            _reg: $crate::hid::Registration,
+        }
+
+        $crate::prelude::module! {
+            type: Module,
+            name: $name,
+            $($f)*
+        }
+
+        const _: () = {
+            static NAME: &$crate::str::CStr = $crate::c_str!($name);
+
+            static ID_TABLE: [$crate::hid::DeviceIdShallow;
+                $crate::module_hid_driver!(@count_devices $($dev_id),+) + 1] = [
+                $($dev_id),+,
+                $crate::hid::DeviceIdShallow::new(),
+            ];
+
+            static mut DRIVER: $crate::hid::DriverVTable =
+                $crate::hid::create_hid_driver::<$driver>(NAME, unsafe { &ID_TABLE[0] });
+
+            impl $crate::Module for Module {
+                fn init(module: &'static $crate::ThisModule) -> Result<Self> {
+                    let driver = unsafe { &mut DRIVER };
+                    let mut reg = $crate::hid::Registration::register(
+                        module,
+                        ::core::pin::Pin::static_mut(driver),
+                        NAME,
+                    )?;
+                    Ok(Module { _reg: reg })
+                }
+            }
+        };
+    }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 496ed32b0911..51b8c2689264 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -49,6 +49,8 @@
 #[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)]
 pub mod firmware;
 pub mod fs;
+#[cfg(CONFIG_RUST_HID_ABSTRACTIONS)]
+pub mod hid;
 pub mod init;
 pub mod io;
 pub mod ioctl;
-- 
2.47.2

[PATCH RFC 2/3] rust: hid: USB Monitor Control Class driver

[Rahul Rameshbabu]

This code will eventually contain the logic needed to drive the backlight
of displays that implement the USB Monitor Control Class specification.
Examples include the Apple Studio Display and Apple Pro Display XDR
monitors. USB Monitor Control Class encompasses more than just backlight
control, so the driver could be further extended as monitors support more
functionality in the specification.

This code is a skeleton currently, where the focus right now is on the core
Rust API. The driver skeleton was written before approaching the Rust API
and C binding work. This was done to provide a guide for what the Rust API
should look like and avoid any rough C binding work from being exposed to
Rust HID device drivers.

To go forward with this driver for the purpose of external monitor
backlight control, a new DRM backlight API that is scoped per connector
will be required. I am currently in the process of developing this new API.
I document the details in my related blog posts. The issue with the current
backlight API is it was designed on the assumption that only internal
panels have controllable backlights. Using this assumption combined with
another that there can only ever be a single internal panel, having more
than one device register with the backlight interface would confuse
userspace applications.

Julius Zint originally tried to implement such a driver a bit more than a
year ago with a C driver but was blocked by the limitations of the
backlight API. I asked him for permission to continue the work in Rust
while accrediting him for the HID report parsing logic for the backlight
support in the USB Monitor Control Class specification.

Cc: Julius Zint <julius@zint.sh>
Link: https://lore.kernel.org/lkml/20230820094118.20521-1-julius@zint.sh/
Link: https://binary-eater.github.io/posts/linux_usb_monitor_control/
Link: https://www.usb.org/sites/default/files/usbmon11.pdf
Signed-off-by: Rahul Rameshbabu <sergeantsagara@protonmail.com>
---
 drivers/hid/Kconfig                |  8 +++++++
 drivers/hid/Makefile               |  1 +
 drivers/hid/hid_monitor_control.rs | 37 ++++++++++++++++++++++++++++++
 3 files changed, 46 insertions(+)
 create mode 100644 drivers/hid/hid_monitor_control.rs

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index e085964c7ffc..92be13acb956 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -722,6 +722,14 @@ config RUST_HID_ABSTRACTIONS
 	Adds support needed for HID drivers written in Rust. It provides a
 	wrapper around the C hid core.
 
+config HID_MONITOR_CONTROL
+	tristate "USB Monitor Control Class support"
+	depends on USB_HID
+	depends on RUST_HID_ABSTRACTIONS
+	help
+	Say Y here if you want to enable control over a monitor that uses USB
+	Monitor Control Class.
+
 config HID_REDRAGON
 	tristate "Redragon keyboards"
 	default !EXPERT
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index 482b096eea28..bf8b096bcf23 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -86,6 +86,7 @@ obj-$(CONFIG_HID_MCP2221)	+= hid-mcp2221.o
 obj-$(CONFIG_HID_MAYFLASH)	+= hid-mf.o
 obj-$(CONFIG_HID_MEGAWORLD_FF)	+= hid-megaworld.o
 obj-$(CONFIG_HID_MICROSOFT)	+= hid-microsoft.o
+obj-$(CONFIG_HID_MONITOR_CONTROL)	+= hid_monitor_control.o
 obj-$(CONFIG_HID_MONTEREY)	+= hid-monterey.o
 obj-$(CONFIG_HID_MULTITOUCH)	+= hid-multitouch.o
 obj-$(CONFIG_HID_NINTENDO)	+= hid-nintendo.o
diff --git a/drivers/hid/hid_monitor_control.rs b/drivers/hid/hid_monitor_control.rs
new file mode 100644
index 000000000000..18afd69a56d5
--- /dev/null
+++ b/drivers/hid/hid_monitor_control.rs
@@ -0,0 +1,37 @@
+// SPDX-License-Identifier: GPL-2.0
+
+// Copyright (C) 2025 Rahul Rameshbabu <sergeantsagara@protonmail.com>
+
+use kernel::prelude::*;
+use kernel::hid::{
+    self,
+    Driver,
+};
+
+struct HidMonitorControl;
+
+#[vtable]
+impl Driver for HidMonitorControl {
+    fn probe(dev: &mut hid::Device, id: &hid::DeviceId) -> Result<()> {
+        /* TODO implement */
+        Ok(())
+    }
+
+    fn remove(dev: &mut hid::Device) {
+        /* TODO implement */
+    }
+}
+
+kernel::module_hid_driver! {
+    driver: HidMonitorControl,
+    id_table: [
+        kernel::usb_device! {
+            vendor: /* TODO fill in */,
+            product: /* TODO fill in */,
+        },
+    ],
+    name: "monitor_control",
+    author: "Rahul Rameshbabu <sergeantsagara@protonmail.com>",
+    description: "Driver for the USB Monitor Control Class",
+    license: "GPL",
+}
-- 
2.47.2

[PATCH RFC 3/3] rust: hid: demo the core abstractions for probe and remove

[Rahul Rameshbabu]

This is a very basic "hello, world!" implementation to illustrate that the
probe and remove callbacks are working as expected. I chose an arbitrary
device I had on hand for populating in the HID device id table.

  [  +0.012968] monitor_control: Probing HID device vendor: 2389 product: 29204 using Rust!
  [  +0.000108] monitor_control: Removing HID device vendor: 2389 product: 29204 using Rust!

Signed-off-by: Rahul Rameshbabu <sergeantsagara@protonmail.com>
---
 drivers/hid/hid_monitor_control.rs | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid_monitor_control.rs b/drivers/hid/hid_monitor_control.rs
index 18afd69a56d5..aeb6e4058a6b 100644
--- a/drivers/hid/hid_monitor_control.rs
+++ b/drivers/hid/hid_monitor_control.rs
@@ -8,17 +8,22 @@
     Driver,
 };
 
+const USB_VENDOR_ID_NVIDIA: u32 = 0x0955;
+const USB_DEVICE_ID_NVIDIA_THUNDERSTRIKE_CONTROLLER: u32 = 0x7214;
+
 struct HidMonitorControl;
 
 #[vtable]
 impl Driver for HidMonitorControl {
     fn probe(dev: &mut hid::Device, id: &hid::DeviceId) -> Result<()> {
         /* TODO implement */
+        pr_info!("Probing HID device vendor: {} product: {} using Rust!\n", id.vendor(), id.product());
         Ok(())
     }
 
     fn remove(dev: &mut hid::Device) {
         /* TODO implement */
+        pr_info!("Removing HID device vendor: {} product: {} using Rust!\n", dev.vendor(), dev.product());
     }
 }
 
@@ -26,8 +31,8 @@ fn remove(dev: &mut hid::Device) {
     driver: HidMonitorControl,
     id_table: [
         kernel::usb_device! {
-            vendor: /* TODO fill in */,
-            product: /* TODO fill in */,
+            vendor: USB_VENDOR_ID_NVIDIA,
+            product: USB_DEVICE_ID_NVIDIA_THUNDERSTRIKE_CONTROLLER,
         },
     ],
     name: "monitor_control",
-- 
2.47.2

Re: [PATCH RFC 0/3] Initial work for Rust abstraction for HID device driver development

[Benjamin Tissoires]

Hi,

[quick reply because I am completely under the water for the next 2
weeks]

On Mar 13 2025, Rahul Rameshbabu wrote:
> Hello,
> 
> I am a hobbyist developer who has been working on a project to create a new Rust
> HID device driver and the needed core abstractions for writing more HID device
> drivers in Rust. My goal is to support the USB Monitor Control Class needed for
> functionality such as backlight control for monitors like the Apple Studio
> Display and Apple Pro Display XDR. A new backlight API will be required to
> support multiple backlight instances and will be mapped per DRM connector. The
> current backlight API is designed around the assumption of only a single
> internal panel being present. I am currently working on making this new API for
> DRM in parallel to my work on the HID side of the stack for supporting these
> displays.

Thanks a lot for this work, though I wonder if your goal is not too big,
too far from the HID point of view. HID is simple, and there is only a
few bindings that you would need to be able to make "simple" HID
drivers.

My assumption would be to introduce the binding with a functional but
small driver (like one that just changes the report descriptor, or does
a sime raw event processing). Then we can look at integrating with the
DRM interface.

Though it's up to you to decide how you want to play ;)

> 
>   https://binary-eater.github.io/tags/usb-monitor-control/
> 
> Julius Zint had attempted to do so a year ago with a C HID driver but was gated
> by the lack of an appropriate backlight API for external displays. I asked him
> for permission to do the work need in Rust and plan to accredit him for the HID
> report handling for backlight in the USB Monitor Control Class standard.
> 
>   https://lore.kernel.org/lkml/f95da7ff-06dd-2c0e-d563-7e5ad61c3bcc@redhat.com/
> 
> I was hoping to get initial feedback on this work to make sure I am on the right
> path for making a Rust HID abstraction that would be acceptable upstream. The
> patches compile with WERROR being disabled. This is necessary since Rust treats
> missing documentation comments as warnings (which is a good thing). I also need
> to go in and add more SAFETY comments.

K, I'll give you my opinion in the patches as the HID co-maintainer. I
do have a very little rust experience, but this is my first in kernel,
so I hope the more experience rust people here will chime in as well.

Cheers,
Benjamin

> 
> Thanks,
> Rahul Rameshbabu
> 
> Rahul Rameshbabu (3):
>   rust: core abstractions for HID drivers
>   rust: hid: USB Monitor Control Class driver
>   rust: hid: demo the core abstractions for probe and remove
> 
>  drivers/hid/Kconfig                |  16 ++
>  drivers/hid/Makefile               |   1 +
>  drivers/hid/hid_monitor_control.rs |  42 +++++
>  rust/bindings/bindings_helper.h    |   1 +
>  rust/kernel/hid.rs                 | 245 +++++++++++++++++++++++++++++
>  rust/kernel/lib.rs                 |   2 +
>  6 files changed, 307 insertions(+)
>  create mode 100644 drivers/hid/hid_monitor_control.rs
>  create mode 100644 rust/kernel/hid.rs
> 
> -- 
> 2.47.2
> 
> 

Re: [PATCH RFC 1/3] rust: core abstractions for HID drivers

[Benjamin Tissoires]

On Mar 13 2025, Rahul Rameshbabu wrote:
> These abstractions enable the development of HID drivers in Rust by binding
> with the HID core C API. They provide Rust types that map to the
> equivalents in C. In this initial draft, only hid_device and hid_device_id
> are provided direct Rust type equivalents. hid_driver is specially wrapped
> with a custom Driver type. The module_hid_driver! macro provides analogous
> functionality to its C equivalent.
> 
> Future work for these abstractions would include more bindings for common
> HID-related types, such as hid_field, hid_report_enum, and hid_report.

Yes, but you can also bypass this as a first step in the same way we do
for HID-BPF: we just consider everything to be a stream of bytes, and
we only care about .report_fixup() and .raw_event().

> Providing Rust equivalents to useful core HID functions will also be
> necessary for HID driver development in Rust.

Yeah, you'll need the back and forth communication with the HID device,
but this can come in later.

> 
> Some concerns with this initial draft
>   - The need for a DeviceId and DeviceIdShallow type.
>     + DeviceIdShallow is used to guarantee the safety requirement for the
>       Sync trait.
>   - The create_hid_driver call in the module_hid_driver! macro does not use
>     Pin semantics for passing the ID_TABLE. I could not get Pin semantics
>     to work in a const fn. I get a feeling this might be safe but need help
>     reviewing this.
> 
> Signed-off-by: Rahul Rameshbabu <sergeantsagara@protonmail.com>
> ---
>  drivers/hid/Kconfig             |   8 ++
>  rust/bindings/bindings_helper.h |   1 +
>  rust/kernel/hid.rs              | 245 ++++++++++++++++++++++++++++++++
>  rust/kernel/lib.rs              |   2 +
>  4 files changed, 256 insertions(+)
>  create mode 100644 rust/kernel/hid.rs
> 
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index b53eb569bd49..e085964c7ffc 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -714,6 +714,14 @@ config HID_MEGAWORLD_FF
>  	Say Y here if you have a Mega World based game controller and want
>  	to have force feedback support for it.
>  
> +config RUST_HID_ABSTRACTIONS
> +	bool "Rust HID abstractions support"
> +	depends on RUST
> +	depends on HID=y

naive question: does it has to be 'y', some distributions are using 'm'.

> +	help
> +	Adds support needed for HID drivers written in Rust. It provides a
> +	wrapper around the C hid core.
> +
>  config HID_REDRAGON
>  	tristate "Redragon keyboards"
>  	default !EXPERT
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 55354e4dec14..e2e95afe9f4a 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -16,6 +16,7 @@
>  #include <linux/file.h>
>  #include <linux/firmware.h>
>  #include <linux/fs.h>
> +#include <linux/hid.h>
>  #include <linux/jiffies.h>
>  #include <linux/jump_label.h>
>  #include <linux/mdio.h>
> diff --git a/rust/kernel/hid.rs b/rust/kernel/hid.rs
> new file mode 100644
> index 000000000000..f13476b49e7d
> --- /dev/null
> +++ b/rust/kernel/hid.rs
> @@ -0,0 +1,245 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +// Copyright (C) 2025 Rahul Rameshbabu <sergeantsagara@protonmail.com>
> +
> +use crate::{error::*, prelude::*, types::Opaque};
> +use core::marker::PhantomData;
> +
> +#[repr(transparent)]
> +pub struct Device(Opaque<bindings::hid_device>);
> +
> +impl Device {
> +    unsafe fn from_ptr<'a>(ptr: *mut bindings::hid_device) -> &'a mut Self {
> +        let ptr = ptr.cast::<Self>();
> +
> +        unsafe { &mut *ptr }
> +    }
> +
> +    pub fn vendor(&self) -> u32 {
> +        let hdev = self.0.get();
> +
> +        unsafe { (*hdev).vendor }
> +    }
> +
> +    pub fn product(&self) -> u32 {
> +        let hdev = self.0.get();
> +
> +        unsafe { (*hdev).product }
> +    }

I know this is a RFC, but there are a lot more of interesting fields
you'll want to export.

Can/Should this be automated somehow?

> +}
> +
> +#[repr(transparent)]
> +pub struct DeviceIdShallow(Opaque<bindings::hid_device_id>);
> +
> +// SAFETY: `DeviceIdShallow` doesn't expose any &self method to access internal data, so it's safe to
> +// share `&DriverVTable` across execution context boundaries.
> +unsafe impl Sync for DeviceIdShallow {}
> +
> +impl DeviceIdShallow {

I have a hard time understanding the "DeviceId" part.

In struct hid_device, we have a .id field which is incremented for every
new device. I assume this is different, but this still confuses me.

If that's the rust way of doing it that's fine of course.

[few minutes later] Oh, so you are mapping struct hid_device_id :)
Why dropping the 'HID' in front?

I guess the docs would explain that in the actual submission.


> +    pub const fn new() -> Self {
> +        DeviceIdShallow(Opaque::new(bindings::hid_device_id {
> +            // SAFETY: The rest is zeroed out to initialize `struct hid_device_id`,
> +            // sets `Option<&F>` to be `None`.
> +            ..unsafe { ::core::mem::MaybeUninit::<bindings::hid_device_id>::zeroed().assume_init() }
> +        }))
> +    }
> +
> +    pub const fn new_usb(vendor: u32, product: u32) -> Self {

We probably need the group here as well.

> +        DeviceIdShallow(Opaque::new(bindings::hid_device_id {
> +            bus: 0x3, /* BUS_USB */

group???

> +            vendor: vendor,
> +            product: product,
> +            // SAFETY: The rest is zeroed out to initialize `struct hid_device_id`,
> +            // sets `Option<&F>` to be `None`.
> +            ..unsafe { ::core::mem::MaybeUninit::<bindings::hid_device_id>::zeroed().assume_init() }
> +        }))
> +    }
> +
> +    const unsafe fn as_ptr(&self) -> *const bindings::hid_device_id {
> +        self.0.get()
> +    }
> +}
> +
> +#[repr(transparent)]
> +pub struct DeviceId(Opaque<bindings::hid_device_id>);
> +
> +impl DeviceId {
> +    unsafe fn from_ptr<'a>(ptr: *mut bindings::hid_device_id) -> &'a mut Self {
> +        let ptr = ptr.cast::<Self>();
> +
> +        unsafe { &mut *ptr }
> +    }
> +
> +    unsafe fn from_const_ptr<'a>(ptr: *const bindings::hid_device_id) -> &'a Self {
> +        let ptr = ptr.cast::<Self>();
> +
> +        unsafe { &(*ptr) }
> +    }
> +
> +    pub fn vendor(&self) -> u32 {
> +        let hdev_id = self.0.get();
> +
> +        unsafe { (*hdev_id).vendor }
> +    }
> +
> +    pub fn product(&self) -> u32 {
> +        let hdev_id = self.0.get();
> +
> +        unsafe { (*hdev_id).product }
> +    }

Again, you need the group and the bus at least.

> +}
> +
> +/*
> +#[repr(transparent)]
> +pub struct Field(Opaque<bindings::hid_field>);
> +
> +#[repr(transparent)]
> +pub struct ReportEnum(Opaque<bindings::hid_report_enum>);
> +
> +#[repr(transparent)]
> +pub struct Report(Opaque<bindings::hid_report>);
> +*/
> +
> +#[vtable]
> +pub trait Driver {
> +    fn probe(_dev: &mut Device, _id: &DeviceId) -> Result {
> +        build_error!(VTABLE_DEFAULT_ERROR)
> +    }
> +
> +    fn remove(_dev: &mut Device) {
> +    }
> +}
> +
> +struct Adapter<T: Driver> {
> +    _p: PhantomData<T>,
> +}
> +
> +impl<T: Driver> Adapter<T> {
> +    unsafe extern "C" fn probe_callback(
> +        hdev: *mut bindings::hid_device,
> +        hdev_id: *const bindings::hid_device_id,
> +    ) -> crate::ffi::c_int {
> +        from_result(|| {
> +            let dev = unsafe { Device::from_ptr(hdev) };
> +            let dev_id = unsafe { DeviceId::from_const_ptr(hdev_id) };
> +            T::probe(dev, dev_id)?;
> +            Ok(0)
> +        })
> +    }
> +
> +    unsafe extern "C" fn remove_callback(hdev: *mut bindings::hid_device) {
> +        let dev = unsafe { Device::from_ptr(hdev) };
> +        T::remove(dev);
> +    }
> +}
> +
> +#[repr(transparent)]
> +pub struct DriverVTable(Opaque<bindings::hid_driver>);
> +
> +// SAFETY: `DriverVTable` doesn't expose any &self method to access internal data, so it's safe to
> +// share `&DriverVTable` across execution context boundaries.
> +unsafe impl Sync for DriverVTable {}
> +
> +pub const fn create_hid_driver<T: Driver>(
> +    name: &'static CStr,
> +    id_table: &'static DeviceIdShallow,
> +) -> DriverVTable {
> +    DriverVTable(Opaque::new(bindings::hid_driver {
> +        name: name.as_char_ptr().cast_mut(),
> +        id_table: unsafe { id_table.as_ptr() },
> +        probe: if T::HAS_PROBE {
> +            Some(Adapter::<T>::probe_callback)
> +        } else {
> +            None
> +        },
> +        remove: if T::HAS_REMOVE {
> +            Some(Adapter::<T>::remove_callback)
> +        } else {
> +            None
> +        },
> +        // SAFETY: The rest is zeroed out to initialize `struct hid_driver`,
> +        // sets `Option<&F>` to be `None`.
> +        ..unsafe { core::mem::MaybeUninit::<bindings::hid_driver>::zeroed().assume_init() }
> +    }))
> +}
> +
> +pub struct Registration {
> +    driver: Pin<&'static mut DriverVTable>,
> +}
> +
> +unsafe impl Send for Registration {}
> +
> +impl Registration {
> +    pub fn register(
> +        module: &'static crate::ThisModule,
> +        driver: Pin<&'static mut DriverVTable>,
> +        name: &'static CStr,
> +    ) -> Result<Self> {
> +        to_result(unsafe {
> +            bindings::__hid_register_driver(driver.0.get(), module.0, name.as_char_ptr())
> +        })?;
> +
> +        Ok(Registration { driver })
> +    }
> +}
> +
> +impl Drop for Registration {
> +    fn drop(&mut self) {
> +        unsafe {
> +            bindings::hid_unregister_driver(self.driver.0.get())
> +        };
> +    }
> +}
> +
> +#[macro_export]
> +macro_rules! usb_device {
> +    (vendor: $vendor:expr, product: $product:expr $(,)?) => {
> +        $crate::hid::DeviceIdShallow::new_usb($vendor, $product)
> +    }
> +}
> +
> +#[macro_export]
> +macro_rules! module_hid_driver {
> +    (@replace_expr $_t:tt $sub:expr) => {$sub};
> +
> +    (@count_devices $($x:expr),*) => {
> +        0usize $(+ $crate::module_hid_driver!(@replace_expr $x 1usize))*
> +    };
> +
> +    (driver: $driver:ident, id_table: [$($dev_id:expr),+ $(,)?], name: $name:tt, $($f:tt)*) => {
> +        struct Module {
> +            _reg: $crate::hid::Registration,
> +        }
> +
> +        $crate::prelude::module! {
> +            type: Module,
> +            name: $name,
> +            $($f)*
> +        }
> +
> +        const _: () = {
> +            static NAME: &$crate::str::CStr = $crate::c_str!($name);
> +
> +            static ID_TABLE: [$crate::hid::DeviceIdShallow;
> +                $crate::module_hid_driver!(@count_devices $($dev_id),+) + 1] = [
> +                $($dev_id),+,
> +                $crate::hid::DeviceIdShallow::new(),
> +            ];
> +
> +            static mut DRIVER: $crate::hid::DriverVTable =
> +                $crate::hid::create_hid_driver::<$driver>(NAME, unsafe { &ID_TABLE[0] });
> +
> +            impl $crate::Module for Module {
> +                fn init(module: &'static $crate::ThisModule) -> Result<Self> {
> +                    let driver = unsafe { &mut DRIVER };
> +                    let mut reg = $crate::hid::Registration::register(
> +                        module,
> +                        ::core::pin::Pin::static_mut(driver),
> +                        NAME,
> +                    )?;
> +                    Ok(Module { _reg: reg })
> +                }
> +            }
> +        };
> +    }
> +}
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 496ed32b0911..51b8c2689264 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -49,6 +49,8 @@
>  #[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)]
>  pub mod firmware;
>  pub mod fs;
> +#[cfg(CONFIG_RUST_HID_ABSTRACTIONS)]
> +pub mod hid;
>  pub mod init;
>  pub mod io;
>  pub mod ioctl;
> -- 
> 2.47.2
> 
> 

Cheers,
Benjamin

Re: [PATCH RFC 2/3] rust: hid: USB Monitor Control Class driver

[Benjamin Tissoires]

On Mar 13 2025, Rahul Rameshbabu wrote:
> This code will eventually contain the logic needed to drive the backlight
> of displays that implement the USB Monitor Control Class specification.
> Examples include the Apple Studio Display and Apple Pro Display XDR
> monitors. USB Monitor Control Class encompasses more than just backlight
> control, so the driver could be further extended as monitors support more
> functionality in the specification.
> 
> This code is a skeleton currently, where the focus right now is on the core
> Rust API. The driver skeleton was written before approaching the Rust API
> and C binding work. This was done to provide a guide for what the Rust API
> should look like and avoid any rough C binding work from being exposed to
> Rust HID device drivers.

skeletons are good for documentation, but not really for code review as
they can not compile.

You should make this patch part of a documentation in
Documentation/hid/, and squash it with the next one (having a minimal
full driver instead of skeleton+fill in the voids).

Cheers,
Benjamin

> 
> To go forward with this driver for the purpose of external monitor
> backlight control, a new DRM backlight API that is scoped per connector
> will be required. I am currently in the process of developing this new API.
> I document the details in my related blog posts. The issue with the current
> backlight API is it was designed on the assumption that only internal
> panels have controllable backlights. Using this assumption combined with
> another that there can only ever be a single internal panel, having more
> than one device register with the backlight interface would confuse
> userspace applications.
> 
> Julius Zint originally tried to implement such a driver a bit more than a
> year ago with a C driver but was blocked by the limitations of the
> backlight API. I asked him for permission to continue the work in Rust
> while accrediting him for the HID report parsing logic for the backlight
> support in the USB Monitor Control Class specification.
> 
> Cc: Julius Zint <julius@zint.sh>
> Link: https://lore.kernel.org/lkml/20230820094118.20521-1-julius@zint.sh/
> Link: https://binary-eater.github.io/posts/linux_usb_monitor_control/
> Link: https://www.usb.org/sites/default/files/usbmon11.pdf
> Signed-off-by: Rahul Rameshbabu <sergeantsagara@protonmail.com>
> ---
>  drivers/hid/Kconfig                |  8 +++++++
>  drivers/hid/Makefile               |  1 +
>  drivers/hid/hid_monitor_control.rs | 37 ++++++++++++++++++++++++++++++
>  3 files changed, 46 insertions(+)
>  create mode 100644 drivers/hid/hid_monitor_control.rs
> 
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index e085964c7ffc..92be13acb956 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -722,6 +722,14 @@ config RUST_HID_ABSTRACTIONS
>  	Adds support needed for HID drivers written in Rust. It provides a
>  	wrapper around the C hid core.
>  
> +config HID_MONITOR_CONTROL
> +	tristate "USB Monitor Control Class support"
> +	depends on USB_HID
> +	depends on RUST_HID_ABSTRACTIONS
> +	help
> +	Say Y here if you want to enable control over a monitor that uses USB
> +	Monitor Control Class.
> +
>  config HID_REDRAGON
>  	tristate "Redragon keyboards"
>  	default !EXPERT
> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> index 482b096eea28..bf8b096bcf23 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -86,6 +86,7 @@ obj-$(CONFIG_HID_MCP2221)	+= hid-mcp2221.o
>  obj-$(CONFIG_HID_MAYFLASH)	+= hid-mf.o
>  obj-$(CONFIG_HID_MEGAWORLD_FF)	+= hid-megaworld.o
>  obj-$(CONFIG_HID_MICROSOFT)	+= hid-microsoft.o
> +obj-$(CONFIG_HID_MONITOR_CONTROL)	+= hid_monitor_control.o
>  obj-$(CONFIG_HID_MONTEREY)	+= hid-monterey.o
>  obj-$(CONFIG_HID_MULTITOUCH)	+= hid-multitouch.o
>  obj-$(CONFIG_HID_NINTENDO)	+= hid-nintendo.o
> diff --git a/drivers/hid/hid_monitor_control.rs b/drivers/hid/hid_monitor_control.rs
> new file mode 100644
> index 000000000000..18afd69a56d5
> --- /dev/null
> +++ b/drivers/hid/hid_monitor_control.rs
> @@ -0,0 +1,37 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +// Copyright (C) 2025 Rahul Rameshbabu <sergeantsagara@protonmail.com>
> +
> +use kernel::prelude::*;
> +use kernel::hid::{
> +    self,
> +    Driver,
> +};
> +
> +struct HidMonitorControl;
> +
> +#[vtable]
> +impl Driver for HidMonitorControl {
> +    fn probe(dev: &mut hid::Device, id: &hid::DeviceId) -> Result<()> {
> +        /* TODO implement */
> +        Ok(())
> +    }
> +
> +    fn remove(dev: &mut hid::Device) {
> +        /* TODO implement */
> +    }
> +}
> +
> +kernel::module_hid_driver! {
> +    driver: HidMonitorControl,
> +    id_table: [
> +        kernel::usb_device! {
> +            vendor: /* TODO fill in */,
> +            product: /* TODO fill in */,
> +        },
> +    ],
> +    name: "monitor_control",
> +    author: "Rahul Rameshbabu <sergeantsagara@protonmail.com>",
> +    description: "Driver for the USB Monitor Control Class",
> +    license: "GPL",
> +}
> -- 
> 2.47.2
> 
> 

Re: [PATCH RFC 3/3] rust: hid: demo the core abstractions for probe and remove

[Benjamin Tissoires]

On Mar 13 2025, Rahul Rameshbabu wrote:
> This is a very basic "hello, world!" implementation to illustrate that the
> probe and remove callbacks are working as expected. I chose an arbitrary
> device I had on hand for populating in the HID device id table.

Nice to see this live :)

Though as I mentioned in the previous patch, I'd prefer having one full
driver in a single patch and one separate patch with the skeleton in the
docs.

Do you have any meaningful processing that needs to be done in the
report fixup or in the .raw_event or .event callbacks?

It would be much more interesting to show how this works together on a
minimalistic driver.

FWIW, the driver in itself, though incomplete, looks familiar, which is
a good thing: we've got a probe and a remove. This is common with all
the other HID drivers, so it's not a brand new thing.

I wonder however how and if we could enforce the remove() to be
automated by the binding or rust, to not have to deal with resource
leaking. Because the removal part, especially on failure, is the hardest
part.

Cheers,
Benjamin

> 
>   [  +0.012968] monitor_control: Probing HID device vendor: 2389 product: 29204 using Rust!
>   [  +0.000108] monitor_control: Removing HID device vendor: 2389 product: 29204 using Rust!
> 
> Signed-off-by: Rahul Rameshbabu <sergeantsagara@protonmail.com>
> ---
>  drivers/hid/hid_monitor_control.rs | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hid/hid_monitor_control.rs b/drivers/hid/hid_monitor_control.rs
> index 18afd69a56d5..aeb6e4058a6b 100644
> --- a/drivers/hid/hid_monitor_control.rs
> +++ b/drivers/hid/hid_monitor_control.rs
> @@ -8,17 +8,22 @@
>      Driver,
>  };
>  
> +const USB_VENDOR_ID_NVIDIA: u32 = 0x0955;
> +const USB_DEVICE_ID_NVIDIA_THUNDERSTRIKE_CONTROLLER: u32 = 0x7214;
> +
>  struct HidMonitorControl;
>  
>  #[vtable]
>  impl Driver for HidMonitorControl {
>      fn probe(dev: &mut hid::Device, id: &hid::DeviceId) -> Result<()> {
>          /* TODO implement */
> +        pr_info!("Probing HID device vendor: {} product: {} using Rust!\n", id.vendor(), id.product());
>          Ok(())
>      }
>  
>      fn remove(dev: &mut hid::Device) {
>          /* TODO implement */
> +        pr_info!("Removing HID device vendor: {} product: {} using Rust!\n", dev.vendor(), dev.product());
>      }
>  }
>  
> @@ -26,8 +31,8 @@ fn remove(dev: &mut hid::Device) {
>      driver: HidMonitorControl,
>      id_table: [
>          kernel::usb_device! {
> -            vendor: /* TODO fill in */,
> -            product: /* TODO fill in */,
> +            vendor: USB_VENDOR_ID_NVIDIA,
> +            product: USB_DEVICE_ID_NVIDIA_THUNDERSTRIKE_CONTROLLER,
>          },
>      ],
>      name: "monitor_control",
> -- 
> 2.47.2
> 
> 

Re: [PATCH RFC 0/3] Initial work for Rust abstraction for HID device driver development

[Benno Lossin]

On Thu Mar 13, 2025 at 5:02 PM CET, Rahul Rameshbabu wrote:
> Hello,
>
> I am a hobbyist developer who has been working on a project to create a new Rust
> HID device driver and the needed core abstractions for writing more HID device
> drivers in Rust. My goal is to support the USB Monitor Control Class needed for
> functionality such as backlight control for monitors like the Apple Studio
> Display and Apple Pro Display XDR. A new backlight API will be required to
> support multiple backlight instances and will be mapped per DRM connector. The
> current backlight API is designed around the assumption of only a single
> internal panel being present. I am currently working on making this new API for
> DRM in parallel to my work on the HID side of the stack for supporting these
> displays.
>
>   https://binary-eater.github.io/tags/usb-monitor-control/
>
> Julius Zint had attempted to do so a year ago with a C HID driver but was gated
> by the lack of an appropriate backlight API for external displays. I asked him
> for permission to do the work need in Rust and plan to accredit him for the HID
> report handling for backlight in the USB Monitor Control Class standard.
>
>   https://lore.kernel.org/lkml/f95da7ff-06dd-2c0e-d563-7e5ad61c3bcc@redhat.com/
>
> I was hoping to get initial feedback on this work to make sure I am on the right
> path for making a Rust HID abstraction that would be acceptable upstream. The
> patches compile with WERROR being disabled. This is necessary since Rust treats
> missing documentation comments as warnings (which is a good thing). I also need
> to go in and add more SAFETY comments.
>
> Thanks,
> Rahul Rameshbabu
>
> Rahul Rameshbabu (3):
>   rust: core abstractions for HID drivers
>   rust: hid: USB Monitor Control Class driver
>   rust: hid: demo the core abstractions for probe and remove
>
>  drivers/hid/Kconfig                |  16 ++
>  drivers/hid/Makefile               |   1 +
>  drivers/hid/hid_monitor_control.rs |  42 +++++
>  rust/bindings/bindings_helper.h    |   1 +
>  rust/kernel/hid.rs                 | 245 +++++++++++++++++++++++++++++
>  rust/kernel/lib.rs                 |   2 +
>  6 files changed, 307 insertions(+)
>  create mode 100644 drivers/hid/hid_monitor_control.rs
>  create mode 100644 rust/kernel/hid.rs

I have taken a very quick look and haven't seen any big problems,
there are some minor things, but not worth mentioning for an RFC.

---
Cheers,
Benno

Re: [PATCH RFC 1/3] rust: core abstractions for HID drivers

[Danilo Krummrich]

On Thu, Mar 13, 2025 at 04:03:35PM +0000, Rahul Rameshbabu wrote:
> These abstractions enable the development of HID drivers in Rust by binding
> with the HID core C API. They provide Rust types that map to the
> equivalents in C. In this initial draft, only hid_device and hid_device_id
> are provided direct Rust type equivalents. hid_driver is specially wrapped
> with a custom Driver type. The module_hid_driver! macro provides analogous
> functionality to its C equivalent.
> 
> Future work for these abstractions would include more bindings for common
> HID-related types, such as hid_field, hid_report_enum, and hid_report.
> Providing Rust equivalents to useful core HID functions will also be
> necessary for HID driver development in Rust.
> 
> Some concerns with this initial draft
>   - The need for a DeviceId and DeviceIdShallow type.
>     + DeviceIdShallow is used to guarantee the safety requirement for the
>       Sync trait.
>   - The create_hid_driver call in the module_hid_driver! macro does not use
>     Pin semantics for passing the ID_TABLE. I could not get Pin semantics
>     to work in a const fn. I get a feeling this might be safe but need help
>     reviewing this.

For a lot of things in this patch we have common infrastructure, please see
rust/kernel/{device.rs, driver.rs, device_id.rs}. I think you should make use of
the common infrastructure that solves the corresponding problems already.

It provides generic infrastructure for handling device IDs, a generalized
Registration type, based on InPlaceModule with a common module_driver!
implementation for busses to implement their corresponding module macro, etc.

There are two busses upstream, which are based on this infrastructure:
rust/kernel/{pci.rs, platform.rs}.

There is a patch series that improves soundness of those two bus abstractions
[1], which should be taken into consideration too. Even though your
implementation isn't prone to the addressed issue, it would be good to align
things accordingly.

There is a third bus abstraction (auxiliary) on the mailing list [2], which
already implements the mentioned improvements, which you can use as canonical
example too.

[1] https://lore.kernel.org/rust-for-linux/20250313021550.133041-1-dakr@kernel.org/
[2] https://lore.kernel.org/rust-for-linux/20250313022454.147118-1-dakr@kernel.org/

> Signed-off-by: Rahul Rameshbabu <sergeantsagara@protonmail.com>
> ---
>  drivers/hid/Kconfig             |   8 ++
>  rust/bindings/bindings_helper.h |   1 +
>  rust/kernel/hid.rs              | 245 ++++++++++++++++++++++++++++++++
>  rust/kernel/lib.rs              |   2 +
>  4 files changed, 256 insertions(+)
>  create mode 100644 rust/kernel/hid.rs

Re: [PATCH RFC 2/3] rust: hid: USB Monitor Control Class driver

[Miguel Ojeda]

On Thu, Mar 13, 2025 at 5:58 PM Benjamin Tissoires <bentiss@kernel.org> wrote:
>
> skeletons are good for documentation, but not really for code review as
> they can not compile.
>
> You should make this patch part of a documentation in
> Documentation/hid/, and squash it with the next one (having a minimal
> full driver instead of skeleton+fill in the voids).

It could be part of the documentation of the `module_hid_driver!` for
instance, like we have done for other of those macros.

(In general, we try to use `Documentation/` for things that do not fit
as documentation for any of the code "items".)

Cheers,
Miguel

Re: [PATCH RFC 0/3] Initial work for Rust abstraction for HID device driver development

[Rahul Rameshbabu]

On Thu, 13 Mar, 2025 17:31:53 +0100 "Benjamin Tissoires" <bentiss@kernel.org> wrote:
> Hi,
>
> [quick reply because I am completely under the water for the next 2
> weeks]
>
> On Mar 13 2025, Rahul Rameshbabu wrote:
>> Hello,
>>
>> I am a hobbyist developer who has been working on a project to create a new Rust
>> HID device driver and the needed core abstractions for writing more HID device
>> drivers in Rust. My goal is to support the USB Monitor Control Class needed for
>> functionality such as backlight control for monitors like the Apple Studio
>> Display and Apple Pro Display XDR. A new backlight API will be required to
>> support multiple backlight instances and will be mapped per DRM connector. The
>> current backlight API is designed around the assumption of only a single
>> internal panel being present. I am currently working on making this new API for
>> DRM in parallel to my work on the HID side of the stack for supporting these
>> displays.
>
> Thanks a lot for this work, though I wonder if your goal is not too big,
> too far from the HID point of view. HID is simple, and there is only a
> few bindings that you would need to be able to make "simple" HID
> drivers.
>
> My assumption would be to introduce the binding with a functional but
> small driver (like one that just changes the report descriptor, or does
> a sime raw event processing). Then we can look at integrating with the
> DRM interface.
>
> Though it's up to you to decide how you want to play ;)
>

Thanks for the suggestion, Benjamin! I think its a great suggestion for
getting the Rust abstractions for HID cleaned up and integrated before
taking on this more herculian challenge. I think it would be ideal to
maybe do the following?

1. Focus on the core Rust HID abstractions first to get something merged
   for supporting "simple" HID drivers.
2. Build a reference driver to validate the abstrations support "simple"
   HID devices.
  - https://rust-for-linux.com/rust-reference-drivers
3. After getting the first two steps worked out, continue pursuing
   monitor control as a Rust driver and work on the needed changes for
   DRM connector backlight control API.

Luckily, it seems to me that parts of 3 can be done in parallel to 1 and
2. However, the advantage of this is that if the Rust HID abstration can
support "simple" HID drivers initially, it has a path to get merged and
iterated upon for supporting more complex drivers.

You mention this in the third patch in this series, but it would
probably be good to find a simple device that requires a report fixup or
some processing in .raw_event or .event callback.

Making a reference driver for the Glorious PC Gaming Race mice seems
like a good start. My only concern is the lack of complexity in the C
driver not needing a .remove callback implementation. I wanted to have
an example where architecting what device removal with Rust semantics
would look like. I'll get into more details where you bring this up in
third patch in this series.

Thanks for the feedback,
Rahul Rameshbabu

>>
>>   https://binary-eater.github.io/tags/usb-monitor-control/
>>
>> Julius Zint had attempted to do so a year ago with a C HID driver but was gated
>> by the lack of an appropriate backlight API for external displays. I asked him
>> for permission to do the work need in Rust and plan to accredit him for the HID
>> report handling for backlight in the USB Monitor Control Class standard.
>>
>>   https://lore.kernel.org/lkml/f95da7ff-06dd-2c0e-d563-7e5ad61c3bcc@redhat.com/
>>
>> I was hoping to get initial feedback on this work to make sure I am on the right
>> path for making a Rust HID abstraction that would be acceptable upstream. The
>> patches compile with WERROR being disabled. This is necessary since Rust treats
>> missing documentation comments as warnings (which is a good thing). I also need
>> to go in and add more SAFETY comments.
>
> K, I'll give you my opinion in the patches as the HID co-maintainer. I
> do have a very little rust experience, but this is my first in kernel,
> so I hope the more experience rust people here will chime in as well.
>
> Cheers,
> Benjamin
>
>>
>> Thanks,
>> Rahul Rameshbabu
>>
>> Rahul Rameshbabu (3):
>>   rust: core abstractions for HID drivers
>>   rust: hid: USB Monitor Control Class driver
>>   rust: hid: demo the core abstractions for probe and remove
>>
>>  drivers/hid/Kconfig                |  16 ++
>>  drivers/hid/Makefile               |   1 +
>>  drivers/hid/hid_monitor_control.rs |  42 +++++
>>  rust/bindings/bindings_helper.h    |   1 +
>>  rust/kernel/hid.rs                 | 245 +++++++++++++++++++++++++++++
>>  rust/kernel/lib.rs                 |   2 +
>>  6 files changed, 307 insertions(+)
>>  create mode 100644 drivers/hid/hid_monitor_control.rs
>>  create mode 100644 rust/kernel/hid.rs
>>
>> --
>> 2.47.2
>>
>>

Re: [PATCH RFC 1/3] rust: core abstractions for HID drivers

[Rahul Rameshbabu]

On Thu, 13 Mar, 2025 20:25:23 +0100 "Danilo Krummrich" <dakr@kernel.org> wrote:
> On Thu, Mar 13, 2025 at 04:03:35PM +0000, Rahul Rameshbabu wrote:
>> These abstractions enable the development of HID drivers in Rust by binding
>> with the HID core C API. They provide Rust types that map to the
>> equivalents in C. In this initial draft, only hid_device and hid_device_id
>> are provided direct Rust type equivalents. hid_driver is specially wrapped
>> with a custom Driver type. The module_hid_driver! macro provides analogous
>> functionality to its C equivalent.
>>
>> Future work for these abstractions would include more bindings for common
>> HID-related types, such as hid_field, hid_report_enum, and hid_report.
>> Providing Rust equivalents to useful core HID functions will also be
>> necessary for HID driver development in Rust.
>>
>> Some concerns with this initial draft
>>   - The need for a DeviceId and DeviceIdShallow type.
>>     + DeviceIdShallow is used to guarantee the safety requirement for the
>>       Sync trait.
>>   - The create_hid_driver call in the module_hid_driver! macro does not use
>>     Pin semantics for passing the ID_TABLE. I could not get Pin semantics
>>     to work in a const fn. I get a feeling this might be safe but need help
>>     reviewing this.
>
> For a lot of things in this patch we have common infrastructure, please see
> rust/kernel/{device.rs, driver.rs, device_id.rs}. I think you should make use of
> the common infrastructure that solves the corresponding problems already.

Absolutely! 9b90864bb42b ("rust: implement `IdArray`, `IdTable` and
`RawDeviceId`"). The types here look really useful, so I am sorry for
missing this. Will refactor my next revision to make use of this common
infrastructure.

>
> It provides generic infrastructure for handling device IDs, a generalized
> Registration type, based on InPlaceModule with a common module_driver!
> implementation for busses to implement their corresponding module macro, etc.

Wow, module_driver! is very nice. On one hand, out of all of Rust's
semantics, writing macro_rules! and proc macros were difficult for me
before I started working on this. I think I am a bit better with them
now after this. On the other hand looking at how module_pci_driver is
implemented, I love how much this common infrastructure simplifies the
effort for various buses. Will be moving to this in my next revision as
well.

>
> There are two busses upstream, which are based on this infrastructure:
> rust/kernel/{pci.rs, platform.rs}.

Thanks for the pointers.

>
> There is a patch series that improves soundness of those two bus abstractions
> [1], which should be taken into consideration too. Even though your
> implementation isn't prone to the addressed issue, it would be good to align
> things accordingly.
>
> There is a third bus abstraction (auxiliary) on the mailing list [2], which
> already implements the mentioned improvements, which you can use as canonical
> example too.
>
> [1] https://lore.kernel.org/rust-for-linux/20250313021550.133041-1-dakr@kernel.org/
> [2] https://lore.kernel.org/rust-for-linux/20250313022454.147118-1-dakr@kernel.org/

Will base my work on top of the patches in
https://web.git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/log/?h=rust/device.

Haven't had a chance yet to go through the patches in detail but the
main one of importance seems to be "rust: device: implement device
context marker" which adds the Normal and Core contexts for restricting
certain device functions from being called in certain bus callbacks
only. Will look at how the auxiliary bus abstraction makes use of this.

>

I did have a question with regards to a pattern of "getters" that I have
been implementing in my abstractions. Benjamin Tissoires brought up that
the pattern looked repetitive and was wondering if it can be generated
dynamically. I replied that I was of the opinion that it could not[1].

[1] https://lore.kernel.org/rust-for-linux/Z9MxI0u2yCfSzTvD@cassiopeiae/T/#m1b5b1ca96503a542c0da3089ef3f97e606032240

Thanks for the review,
Rahul Rameshbabu

>> Signed-off-by: Rahul Rameshbabu <sergeantsagara@protonmail.com>
>> ---
>>  drivers/hid/Kconfig             |   8 ++
>>  rust/bindings/bindings_helper.h |   1 +
>>  rust/kernel/hid.rs              | 245 ++++++++++++++++++++++++++++++++
>>  rust/kernel/lib.rs              |   2 +
>>  4 files changed, 256 insertions(+)
>>  create mode 100644 rust/kernel/hid.rs

Re: [PATCH RFC 2/3] rust: hid: USB Monitor Control Class driver

[Rahul Rameshbabu]

On Fri, 14 Mar, 2025 15:41:02 +0100 "Miguel Ojeda" <miguel.ojeda.sandonis@gmail.com> wrote:
> On Thu, Mar 13, 2025 at 5:58 PM Benjamin Tissoires <bentiss@kernel.org> wrote:
>>
>> skeletons are good for documentation, but not really for code review as
>> they can not compile.
>>
>> You should make this patch part of a documentation in
>> Documentation/hid/, and squash it with the next one (having a minimal
>> full driver instead of skeleton+fill in the voids).
>
> It could be part of the documentation of the `module_hid_driver!` for
> instance, like we have done for other of those macros.
>
> (In general, we try to use `Documentation/` for things that do not fit
> as documentation for any of the code "items".)
>
> Cheers,
> Miguel

In general, I will add a lot more documentation in my next revision. For
example, I have a bunch of unsafe usage right now without any SAFETY
comments.

Thanks for the review,
Rahul Rameshbabu

Re: [PATCH RFC 3/3] rust: hid: demo the core abstractions for probe and remove

[Rahul Rameshbabu]

On Thu, 13 Mar, 2025 18:05:36 +0100 "Benjamin Tissoires" <bentiss@kernel.org> wrote:
> On Mar 13 2025, Rahul Rameshbabu wrote:
>> This is a very basic "hello, world!" implementation to illustrate that the
>> probe and remove callbacks are working as expected. I chose an arbitrary
>> device I had on hand for populating in the HID device id table.
>
> Nice to see this live :)
>
> Though as I mentioned in the previous patch, I'd prefer having one full
> driver in a single patch and one separate patch with the skeleton in the
> docs.
>
> Do you have any meaningful processing that needs to be done in the
> report fixup or in the .raw_event or .event callbacks?
>
> It would be much more interesting to show how this works together on a
> minimalistic driver.

Agreed. Have a discussion about a device to potentially use for such a
reference driver in another thread[1].

>
> FWIW, the driver in itself, though incomplete, looks familiar, which is
> a good thing: we've got a probe and a remove. This is common with all
> the other HID drivers, so it's not a brand new thing.
>
> I wonder however how and if we could enforce the remove() to be
> automated by the binding or rust, to not have to deal with resource
> leaking. Because the removal part, especially on failure, is the hardest
> part.

One issue with the device I proposed in [1] is that it would not require
the implementation of remove() or automation through Rust since the
device is so "simple".

Rust has the Drop trait[2]. If my understanding is correct, contained
data that also implement the Drop trait cannot be enforced in terms of
the order they are dropped/provide any kind of dependency management
here. It's worth exploring. My concern is some very tricky ordering
requirements on removal. I extracted the below from
drivers/hid/hid-nvidia-shield.c.

  static void shield_remove(struct hid_device *hdev)
  {
  	struct shield_device *dev = hid_get_drvdata(hdev);
  	struct thunderstrike *ts;

  	ts = container_of(dev, struct thunderstrike, base);

  	hid_hw_close(hdev);
  	thunderstrike_destroy(ts);
  	del_timer_sync(&ts->psy_stats_timer);
  	cancel_work_sync(&ts->hostcmd_req_work);
  	hid_hw_stop(hdev);
  }

Here, we need to explicitly stop the timer before cancelling any work.

The problem here is Rust's Drop trait does not gaurantee ordering for
the teardown of members.

Lets pretend I have the following and its functional in Rust.

  // In hid_nvidia_shield.rs

  struct Thunderstrike {
      // Rest of my thunderstrike members from the C equivalent
      psyStatsTimer: TimerList, // TimerList implements Drop
      hostcmdReqWork: Work, // Work implements Drop
  }

  // hid.rs

  struct Device<T> {
      // Details omitted
      privateData: T,
  }

  impl<T> Drop for Device<T> {
      fn drop(&mut self) {
          // Implementation here
      }
  }

The problem here is when the Device instance is dropped, we cannot
guarantee the order of the Drop::drop calls for the psyStatsTimer or
hostcmdReqWork members as-is. There might be some clever trick to solve
this problem that I am not aware of.

[1] https://lore.kernel.org/rust-for-linux/Z9MxI0u2yCfSzTvD@cassiopeiae/T/#m87f121ec72ba159071b20dccb4071cd7d2398050
[2] https://doc.rust-lang.org/std/ops/trait.Drop.html

Thanks for the review,
Rahul Rameshbabu

>
> Cheers,
> Benjamin
>
>>
>>   [  +0.012968] monitor_control: Probing HID device vendor: 2389 product: 29204 using Rust!
>>   [  +0.000108] monitor_control: Removing HID device vendor: 2389 product: 29204 using Rust!
>>
>> Signed-off-by: Rahul Rameshbabu <sergeantsagara@protonmail.com>
>> ---
>>  drivers/hid/hid_monitor_control.rs | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/hid/hid_monitor_control.rs b/drivers/hid/hid_monitor_control.rs
>> index 18afd69a56d5..aeb6e4058a6b 100644
>> --- a/drivers/hid/hid_monitor_control.rs
>> +++ b/drivers/hid/hid_monitor_control.rs
>> @@ -8,17 +8,22 @@
>>      Driver,
>>  };
>>
>> +const USB_VENDOR_ID_NVIDIA: u32 = 0x0955;
>> +const USB_DEVICE_ID_NVIDIA_THUNDERSTRIKE_CONTROLLER: u32 = 0x7214;
>> +
>>  struct HidMonitorControl;
>>
>>  #[vtable]
>>  impl Driver for HidMonitorControl {
>>      fn probe(dev: &mut hid::Device, id: &hid::DeviceId) -> Result<()> {
>>          /* TODO implement */
>> +        pr_info!("Probing HID device vendor: {} product: {} using Rust!\n", id.vendor(), id.product());
>>          Ok(())
>>      }
>>
>>      fn remove(dev: &mut hid::Device) {
>>          /* TODO implement */
>> +        pr_info!("Removing HID device vendor: {} product: {} using Rust!\n", dev.vendor(), dev.product());
>>      }
>>  }
>>
>> @@ -26,8 +31,8 @@ fn remove(dev: &mut hid::Device) {
>>      driver: HidMonitorControl,
>>      id_table: [
>>          kernel::usb_device! {
>> -            vendor: /* TODO fill in */,
>> -            product: /* TODO fill in */,
>> +            vendor: USB_VENDOR_ID_NVIDIA,
>> +            product: USB_DEVICE_ID_NVIDIA_THUNDERSTRIKE_CONTROLLER,
>>          },
>>      ],
>>      name: "monitor_control",
>> --
>> 2.47.2
>>
>>

Re: [PATCH RFC 3/3] rust: hid: demo the core abstractions for probe and remove

[Daniel Brooks]

Rahul Rameshbabu <sergeantsagara@protonmail.com> writes:

> Rust has the Drop trait[2]. If my understanding is correct, contained
> data that also implement the Drop trait cannot be enforced in terms of
> the order they are dropped/provide any kind of dependency management
> here. It's worth exploring. My concern is some very tricky ordering
> requirements on removal.

A valid concern; I recommend a careful reading of chapter 10.8
Destructors from the Rust reference
<https://doc.rust-lang.org/reference/destructors.html>. Here are the
most important bits:

> The destructor of a type T consists of:
> 
>    1. If T: Drop, calling <T as std::ops::Drop>::drop
>    2. Recursively running the destructor of all of its fields.
>       • The fields of a struct are dropped in declaration order.
>       • The fields of the active enum variant are dropped in
>         declaration order.
>       • The fields of a tuple are dropped in order.
>       • The elements of an array or owned slice are dropped from the
>         first element to the last.
>       • The variables that a closure captures by move are dropped in
>         an unspecified order.
>       • Trait objects run the destructor of the underlying type.
>       • Other types don’t result in any further drops.

¶1 is a bit terse, but it just says that if the type has an
implementation of the Drop trait then the destructor is just a call to
the trait’s drop method. If there are tricky ordering requirements then
this is where they could be implemented.

¶2 tells you how the compiler builds a destructor for types that don’t
implement Drop. You can rely on it to drop all of the fields of a struct
in declaration order, so the tricky requirements from the code you
quoted could be satisfied merely by keeping the fields of the struct in
the right order if you wanted. I wouldn’t really recommend it
though. It is much better to write an explicit Drop impl that does it
correctly rather than reling on a comment next to the struct declaration
telling the reader that the field order is important somehow. In fact
the implementation guidelines for drivers should emphasize that if the
destruction order matters then the type must have a custom impl for the
Drop trait that does the destruction in the correct order.
    
> I extracted the below from
> drivers/hid/hid-nvidia-shield.c.
>
>   static void shield_remove(struct hid_device *hdev)
>   {
>   	struct shield_device *dev = hid_get_drvdata(hdev);
>   	struct thunderstrike *ts;
>
>   	ts = container_of(dev, struct thunderstrike, base);
>
>   	hid_hw_close(hdev);
>   	thunderstrike_destroy(ts);
>   	del_timer_sync(&ts->psy_stats_timer);
>   	cancel_work_sync(&ts->hostcmd_req_work);
>   	hid_hw_stop(hdev);
>   }
>
> Here, we need to explicitly stop the timer before cancelling any work.
>
> The problem here is Rust's Drop trait does not gaurantee ordering for
> the teardown of members.
>
> Lets pretend I have the following and its functional in Rust.
>
>   // In hid_nvidia_shield.rs
>
>   struct Thunderstrike {
>       // Rest of my thunderstrike members from the C equivalent
>       psyStatsTimer: TimerList, // TimerList implements Drop
>       hostcmdReqWork: Work, // Work implements Drop
>   }
>
>   // hid.rs
>
>   struct Device<T> {
>       // Details omitted
>       privateData: T,
>   }
>
>   impl<T> Drop for Device<T> {
>       fn drop(&mut self) {
>           // Implementation here
>       }
>   }
>
> The problem here is when the Device instance is dropped, we cannot
> guarantee the order of the Drop::drop calls for the psyStatsTimer or
> hostcmdReqWork members as-is. There might be some clever trick to solve
> this problem that I am not aware of.

Nah, it’s easy. Just drop the members in the right order:

impl Drop for Thunderstrike {
    fn drop(&mut self) {
        drop(self.psyStatsTimer);
        drop(self.hostedcmdReqWork);
    }
}

The compiler generates a destructor for the Device<T> struct and we know
from the reference that it will destroy the struct’s fields. Thus we can
write Thunderstrike’s drop method so that it destroys the fields in the
correct order. No clever tricks required.

On Thu, 13 Mar, 2025 18:05:36 +0100 "Benjamin Tissoires" <bentiss@kernel.org> wrote

> I wonder however how and if we could enforce the remove() to be
> automated by the binding or rust, to not have to deal with resource
> leaking. Because the removal part, especially on failure, is the hardest
> part.

Many conditions can be handled automatically but probably not all.

A good example might be conditionally destructing data that might not be
initilized yet. If that data is stored in an Optional field, then
dropping it will automatically do the right thing. If the field is None
then the destructor won’t do anything. Otherwise the field is Some(T)
and it will automatically call the destructor for the type T. No manual
work need be done to handle that condition at all.

db48x