Re: [PATCH v1 2/3] rust: core abstractions for HID drivers

[Danilo Krummrich <dakr@kernel.org>]

(Cc: +Jiri)

On Sun, Jun 29, 2025 at 04:51:15AM +0000, Rahul Rameshbabu wrote:
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c3f7fbd0d67a..487750d9fd1e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10686,6 +10686,13 @@ F:	include/uapi/linux/hid*
>  F:	samples/hid/
>  F:	tools/testing/selftests/hid/
>  
> +HID CORE LAYER [RUST]
> +M:	Rahul Rameshbabu <sergeantsagara@protonmail.com>
> +L:	linux-input@vger.kernel.org
> +S:	Maintained
> +T:	git git://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git
> +F:	rust/kernel/hid.rs

I assume this is agreed with the HID CORE LAYER maintainers?

There are multiple possible options, for instance:

  1) Maintain the Rust abstractions as part of the existing MAINTAINERS entry.
     Optionally, the author can be added as another maintainer or reviewer.

  2) Add a separate MAINTAINERS entry; patches still go through the same
     subsystem tree.

  3) Add a separate MAINTAINERS entry; patches don't go through the subsystem
     tree (e.g. because the subsystem maintainers don't want to deal with it).

I usually recommend (1), but that's of course up to you and the HID maintainers.

Here it seems you want to go with option (2). Given that I recommend to add the
HID maintainers as reviewers (if they like) to the new MAINTAINERS entry, so
they can easily follow on what's going on.

> +/// The HID driver trait.
> +///
> +/// # Example
> +///
> +///```
> +/// use kernel::hid;
> +///
> +/// const USB_VENDOR_ID_VALVE: u32 = 0x28de;
> +/// const USB_DEVICE_ID_STEAM_DECK: u32 = 0x1205;

I think we should make the existing constants from drivers/hid/hid-ids.h
available, rather than defining them again.

<snip>

> +pub trait Driver: Send {
> +    /// The type holding information about each device id supported by the driver.
> +    // TODO: Use `associated_type_defaults` once stabilized:
> +    //
> +    // ```
> +    // type IdInfo: 'static = ();
> +    // ```
> +    type IdInfo: 'static;
> +
> +    /// The table of device ids supported by the driver.
> +    const ID_TABLE: IdTable<Self::IdInfo>;
> +
> +    /// Called before report descriptor parsing. Can be used to mutate the
> +    /// report descriptor before the core HID logic processes the descriptor.
> +    /// Useful for problematic report descriptors that prevent HID devices from
> +    /// functioning correctly.
> +    ///
> +    /// Optional to implement.
> +    fn report_fixup<'a, 'b: 'a>(_hdev: &Device, _rdesc: &'b mut [u8]) -> &'a [u8] {

report_fixup() is a bus device callback and seems to be synchronized against
other bus device callbacks, hence the device argument should be
&Device<device::Core>.

> +        build_error!(VTABLE_DEFAULT_ERROR)
> +    }
> +}
> +
> +/// An adapter for the registration of HID drivers.
> +pub struct Adapter<T: Driver>(T);
> +
> +// SAFETY: A call to `unregister` for a given instance of `RegType` is guaranteed to be valid if
> +// a preceding call to `register` has been successful.
> +unsafe impl<T: Driver + 'static> driver::RegistrationOps for Adapter<T> {
> +    type RegType = bindings::hid_driver;
> +
> +    unsafe fn register(
> +        hdrv: &Opaque<Self::RegType>,
> +        name: &'static CStr,
> +        module: &'static ThisModule,
> +    ) -> Result {
> +        // SAFETY: It's safe to set the fields of `struct hid_driver` on initialization.
> +        let raw_hdrv = unsafe { &mut *hdrv.get() };

I don't think you can guarantee that all fields of `hdrv` are properly
initialized to create this mutable reference. I think you should do what PCI and
platform does instead.

> +impl<T: Driver + 'static> Adapter<T> {
> +    extern "C" fn report_fixup_callback(
> +        hdev: *mut bindings::hid_device,
> +        buf: *mut u8,
> +        size: *mut kernel::ffi::c_uint,
> +    ) -> *const u8 {
> +        // SAFETY: The HID subsystem only ever calls the report_fixup callback
> +        // with a valid pointer to a `struct hid_device`.
> +        //
> +        // INVARIANT: `hdev` is valid for the duration of
> +        // `report_fixup_callback()`.
> +        let hdev = unsafe { &*hdev.cast::<Device>() };

::<Device<device::Core>

> +
> +        // SAFETY: The HID subsystem only ever calls the report_fixup callback
> +        // with a valid pointer to a `kernel::ffi::c_uint`.
> +        //
> +        // INVARIANT: `size` is valid for the duration of
> +        // `report_fixup_callback()`.
> +        let buf_len: usize = match unsafe { *size }.try_into() {
> +            Ok(len) => len,
> +            Err(e) => {
> +                pr_err!(
> +                    "Cannot fix report description due to length conversion failure: {}!\n",
> +                    e
> +                );

You have a valid device at this point, please use it to print with dev_err!().

> +
> +                return buf;
> +            }
> +        };
> +
> +        // Build a mutable Rust slice from buf and size
> +        //
> +        // SAFETY: The HID subsystem only ever calls the report_fixup callback
> +        // with a valid pointer to a `u8` buffer.
> +        //
> +        // INVARIANT: `buf` is valid for the duration of
> +        // `report_fixup_callback()`.
> +        let rdesc_slice = unsafe { core::slice::from_raw_parts_mut(buf, buf_len) };
> +        let rdesc_slice = T::report_fixup(hdev, rdesc_slice);
> +
> +        match rdesc_slice.len().try_into() {
> +            // SAFETY: The HID subsystem only ever calls the report_fixup
> +            // callback with a valid pointer to a `kernel::ffi::c_uint`.
> +            //
> +            // INVARIANT: `size` is valid for the duration of
> +            // `report_fixup_callback()`.
> +            Ok(len) => unsafe { *size = len },
> +            Err(e) => {
> +                pr_err!("Fixed report description will not be used due to {}!\n", e);

Same here.

> +
> +                return buf;
> +            }
> +        }
> +
> +        rdesc_slice.as_ptr()
> +    }
> +}

You also need to call the macros that generate the device context Deref
hierarchy and ARef<Device> conversion for you, i.e.

	// SAFETY: `Device` is a transparent wrapper of a type that doesn't depend on `Device`'s generic
	// argument.
	kernel::impl_device_context_deref!(unsafe { Device });
	kernel::impl_device_context_into_aref!(Device);

You probably don't need that latter, given that the only bus callback you
implement is report_fixup() and you don't implement AlwaysRefCounted for Device
yet.