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

[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

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 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 1/3] rust: core abstractions for HID drivers

[Rahul Rameshbabu]

On Thu, 13 Mar, 2025 17:54:57 +0100 "Benjamin Tissoires" <bentiss@kernel.org> wrote:
> 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().
>

Thanks, I took a look at struct hid_bpf_ops and how it is used in
drivers/hid/bpf/hid_bpf_dispatch.c. Would you then suggest doing this
for the C-Rust layer and just having pure Rust code for handling the
stream of bytes, instead of doing more C-Rust bindings? This is
something that can be discussed after implementing a Rust abstraction
with a "simple" reference HID driver.

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

My distribution of choice uses 'm' as well. I probably should double
check how the #[cfg(CONFIG_RUST_HID_ABSTRACTIONS)] Rust attribute works.
I think the problem here is that all the Rust abstrations today get
compiled into the kernel image, so if the HID Rust abstrations depend on
a module, that sort of breaks the abstration that components compiled
into the kernel as built-in do not depend on symbols from a module.

I believe there are a number of other Rust abstractions that fall into
this problem. CONFIG_RUST_FW_LOADER_ABSTRACTIONS is one such example. I
would definitely be interested in discussion on how to solve this
problem in general for the Rust abstractions.

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

That would be interesting. I am not sure there is a path for automation
of this right now. This is a similar problem to C++ with
setters/getters. I would be interested if someone from the Rust for
Linux side has any commentary on this topic.

>> +}
>> +
>> +#[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?

This has to do with module scoping in Rust (similar to namespaces in C++
as an analogy). The way I would reference this in HID drivers would be
hid::DeviceId vs hid::HidDeviceId. I think, because of this, its more
common among the Rust abstraction to drop the name of the subsystem in
the type name prefixes.

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

I'll work an adding proper documentation in my next RFC revision. I
wanted to get this initial RFC out just to make sure I am on the right
path.

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

  #define HID_USB_DEVICE(ven, prod)				\
    .bus = BUS_USB, .vendor = (ven), .product = (prod)

When we use HID_USB_DEVICE to instantiate a hid_device_id element in the
.id_table, the C macro does not populate the .group field. Is the ideal
for anything new in the HID subsystem to explicitly specify groups?
Through git blaming, I found 070748ed0b52 ("HID: Create a generic device
group"). The commit description was very useful.

  Devices that do not have a special driver are handled by the generic
  driver. This patch does the same thing using device groups; Instead of
  forcing a particular driver, the appropriate driver is picked up by
  udev. As a consequence, one can now move a device from generic to
  specific handling by a simple rebind.

I assume we want to explicitly specify group going forward for matching
with the generic driver or not and for rebinding purposes with udev?

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

Will add throw those and more in my next revision.

>> +}
>> +
>> +/*
>> +#[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

Thanks so much for the review,
Rahul Rameshbabu

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