On Thu, 13 Mar, 2025 17:54:57 +0100 "Benjamin Tissoires" <bentiss@xxxxxxxxxx> 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@xxxxxxxxxxxxxx> >> --- >> 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@xxxxxxxxxxxxxx> >> + >> +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