| Message ID | 20250313160220.6410-2-sergeantsagara@protonmail.com |
|---|---|
| Headers | show |
| Series | Initial work for Rust abstraction for HID device driver development | expand |
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
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
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 >> >>
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