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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux