* [PATCH v2 0/4] Initial work for Rust abstraction for HID device driver development
@ 2025-07-13 21:11 Rahul Rameshbabu
2025-07-13 21:11 ` [PATCH v2 1/4] HID: core: Change hid_driver to use a const char* for name Rahul Rameshbabu
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Rahul Rameshbabu @ 2025-07-13 21:11 UTC (permalink / raw)
To: rust-for-linux, linux-input
Cc: Jiri Kosina, a.hindborg, alex.gaynor, aliceryhl, benno.lossin,
Benjamin Tissoires, bjorn3_gh, boqun.feng, dakr, db48x, gary,
ojeda, tmgross, peter.hutterer, Rahul Rameshbabu
Hello again,
I incorporated Danilo's and Peter's feedback from my v1. Additionally, I
implemented the AlwaysRefCounted trait for kernel::hid::Device and needed some
bindings from kref.
Link: https://lore.kernel.org/rust-for-linux/20250629045031.92358-2-sergeantsagara@protonmail.com/
Link: https://lore.kernel.org/rust-for-linux/20250313160220.6410-2-sergeantsagara@protonmail.com/
Link: https://binary-eater.github.io/tags/usb-monitor-control/
Rahul Rameshbabu (4):
HID: core: Change hid_driver to use a const char* for name
rust: add kref bindings
rust: core abstractions for HID drivers
rust: hid: Glorious Gaming PC Race Model O and O- mice reference
driver
MAINTAINERS | 15 +
drivers/hid/Kconfig | 16 ++
drivers/hid/Makefile | 1 +
drivers/hid/hid-glorious.c | 2 +
drivers/hid/hid_glorious_rust.rs | 59 ++++
include/linux/hid.h | 2 +-
rust/bindings/bindings_helper.h | 3 +
rust/helpers/helpers.c | 1 +
rust/helpers/kref.c | 13 +
rust/kernel/hid.rs | 458 +++++++++++++++++++++++++++++++
rust/kernel/lib.rs | 2 +
11 files changed, 571 insertions(+), 1 deletion(-)
create mode 100644 drivers/hid/hid_glorious_rust.rs
create mode 100644 rust/helpers/kref.c
create mode 100644 rust/kernel/hid.rs
base-commit: 2009a2d5696944d85c34d75e691a6f3884e787c0
--
2.47.2
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 1/4] HID: core: Change hid_driver to use a const char* for name
2025-07-13 21:11 [PATCH v2 0/4] Initial work for Rust abstraction for HID device driver development Rahul Rameshbabu
@ 2025-07-13 21:11 ` Rahul Rameshbabu
2025-07-13 21:12 ` [PATCH v2 2/4] rust: add kref bindings Rahul Rameshbabu
` (2 subsequent siblings)
3 siblings, 0 replies; 12+ messages in thread
From: Rahul Rameshbabu @ 2025-07-13 21:11 UTC (permalink / raw)
To: rust-for-linux, linux-input
Cc: Jiri Kosina, a.hindborg, alex.gaynor, aliceryhl, benno.lossin,
Benjamin Tissoires, bjorn3_gh, boqun.feng, dakr, db48x, gary,
ojeda, tmgross, peter.hutterer, Rahul Rameshbabu
name is never mutated by the core HID stack. Making name a const char*
simplifies passing the string from Rust to C. Otherwise, it becomes difficult to
pass a 'static lifetime CStr from Rust to a char*, rather than a const char*,
due to lack of guarantee that the underlying data of the CStr will not be
mutated by the C code.
Signed-off-by: Rahul Rameshbabu <sergeantsagara@protonmail.com>
---
include/linux/hid.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 568a9d8c749b..d65c202783da 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -816,7 +816,7 @@ struct hid_usage_id {
* zero from them.
*/
struct hid_driver {
- char *name;
+ const char *name;
const struct hid_device_id *id_table;
struct list_head dyn_list;
--
2.47.2
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 2/4] rust: add kref bindings
2025-07-13 21:11 [PATCH v2 0/4] Initial work for Rust abstraction for HID device driver development Rahul Rameshbabu
2025-07-13 21:11 ` [PATCH v2 1/4] HID: core: Change hid_driver to use a const char* for name Rahul Rameshbabu
@ 2025-07-13 21:12 ` Rahul Rameshbabu
2025-07-20 16:02 ` Rahul Rameshbabu
2025-07-13 21:12 ` [PATCH v2 3/4] rust: core abstractions for HID drivers Rahul Rameshbabu
2025-07-13 21:12 ` [PATCH v2 4/4] rust: hid: Glorious Gaming PC Race Model O and O- mice reference driver Rahul Rameshbabu
3 siblings, 1 reply; 12+ messages in thread
From: Rahul Rameshbabu @ 2025-07-13 21:12 UTC (permalink / raw)
To: rust-for-linux, linux-input
Cc: Jiri Kosina, a.hindborg, alex.gaynor, aliceryhl, benno.lossin,
Benjamin Tissoires, bjorn3_gh, boqun.feng, dakr, db48x, gary,
ojeda, tmgross, peter.hutterer, Rahul Rameshbabu
Introduce kref bindings for use in the Rust HID abstractions. Needed for
implementing the AlwaysRefCounted trait for kernel::hid::Device. Add rust
helpers for binding the kref_get and kref_put static inline C functions.
Signed-off-by: Rahul Rameshbabu <sergeantsagara@protonmail.com>
---
rust/bindings/bindings_helper.h | 1 +
rust/helpers/helpers.c | 1 +
rust/helpers/kref.c | 13 +++++++++++++
3 files changed, 15 insertions(+)
create mode 100644 rust/helpers/kref.c
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 8cbb660e2ec2..41a98d5b6521 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -54,6 +54,7 @@
#include <linux/fs.h>
#include <linux/jiffies.h>
#include <linux/jump_label.h>
+#include <linux/kref.h>
#include <linux/mdio.h>
#include <linux/miscdevice.h>
#include <linux/of_device.h>
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index b15b3cddad4e..7f5403d6c51c 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -25,6 +25,7 @@
#include "fs.c"
#include "io.c"
#include "jump_label.c"
+#include "kref.c"
#include "kunit.c"
#include "mm.c"
#include "mutex.c"
diff --git a/rust/helpers/kref.c b/rust/helpers/kref.c
new file mode 100644
index 000000000000..25eeb0a724ac
--- /dev/null
+++ b/rust/helpers/kref.c
@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/kref.h>
+
+void rust_helper_kref_get(struct kref *kref)
+{
+ kref_get(kref);
+}
+
+void rust_helper_kref_put(struct kref *kref, void (*release)(struct kref *kref))
+{
+ kref_put(kref, release);
+}
--
2.47.2
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 3/4] rust: core abstractions for HID drivers
2025-07-13 21:11 [PATCH v2 0/4] Initial work for Rust abstraction for HID device driver development Rahul Rameshbabu
2025-07-13 21:11 ` [PATCH v2 1/4] HID: core: Change hid_driver to use a const char* for name Rahul Rameshbabu
2025-07-13 21:12 ` [PATCH v2 2/4] rust: add kref bindings Rahul Rameshbabu
@ 2025-07-13 21:12 ` Rahul Rameshbabu
2025-07-13 21:57 ` Miguel Ojeda
2025-07-15 15:04 ` Danilo Krummrich
2025-07-13 21:12 ` [PATCH v2 4/4] rust: hid: Glorious Gaming PC Race Model O and O- mice reference driver Rahul Rameshbabu
3 siblings, 2 replies; 12+ messages in thread
From: Rahul Rameshbabu @ 2025-07-13 21:12 UTC (permalink / raw)
To: rust-for-linux, linux-input
Cc: Jiri Kosina, a.hindborg, alex.gaynor, aliceryhl, benno.lossin,
Benjamin Tissoires, bjorn3_gh, boqun.feng, dakr, db48x, gary,
ojeda, tmgross, peter.hutterer, 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.
Only the .report_fixup callback is binded to Rust so far.
Future work for these abstractions would include more bindings for common
HID-related types, such as hid_field, hid_report_enum, and hid_report as well as
more bus callbacks. Providing Rust equivalents to useful core HID functions will
also be necessary for HID driver development in Rust.
Signed-off-by: Rahul Rameshbabu <sergeantsagara@protonmail.com>
---
Notes:
Notes:
Some points I did not address from the last review cycle:
* Enabling CONFIG_HID=m with the Rust bindings
- This is a limitation due to the Makefile rules that currently exist,
compiling all the Rust bindings into kernel.o, which gets linked into the
core kernel image.
From rust/Makefile
$(obj)/kernel.o: private rustc_target_flags = --extern ffi --extern pin_init \
--extern build_error --extern macros --extern bindings --extern uapi
$(obj)/kernel.o: $(src)/kernel/lib.rs $(obj)/build_error.o $(obj)/pin_init.o \
$(obj)/$(libmacros_name) $(obj)/bindings.o $(obj)/uapi.o FORCE
+$(call if_changed_rule,rustc_library)
ifdef CONFIG_JUMP_LABEL
$(obj)/kernel.o: $(obj)/kernel/generated_arch_static_branch_asm.rs
endif
- I would be happy to work on support for module-level bindings, but this is
not a problem unique only to the Rust HID binding work.
* I did not look into autogenerating all the getter functions for various
fields exported from the binded C structures.
- I would be interested in hearing opinions from folks actively involved
with Rust for Linux on this topic.
Changelog:
RFC->v1:
* Use Danilo's core infrastructure
* Account for HID device groups
* Remove probe and remove callbacks
* Implement report_fixup support
* Properly comment code including SAFETY comments
v1->v2:
* Binded drivers/hid/hid-ids.h for use in Rust drivers
* Remove pre-emptive referencing of a C HID driver instance before
it is fully initialized in the driver registration path
* Moved static getters to generic Device trait implementation, so
they can be used by all device::DeviceContext
* Use core macros for supporting DeviceContext transitions
* Implemented the AlwaysRefCounted and AsRef traits
* Make use for dev_err! as appropriate
MAINTAINERS | 8 +
drivers/hid/Kconfig | 8 +
rust/bindings/bindings_helper.h | 2 +
rust/kernel/hid.rs | 458 ++++++++++++++++++++++++++++++++
rust/kernel/lib.rs | 2 +
5 files changed, 478 insertions(+)
create mode 100644 rust/kernel/hid.rs
diff --git a/MAINTAINERS b/MAINTAINERS
index c3f7fbd0d67a..eebc810432c5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10686,6 +10686,14 @@ F: include/uapi/linux/hid*
F: samples/hid/
F: tools/testing/selftests/hid/
+HID CORE LAYER [RUST]
+M: Rahul Rameshbabu <sergeantsagara@protonmail.com>
+R: Benjamin Tissoires <bentiss@kernel.org>
+L: rust-for-linux@vger.kernel.org
+S: Maintained
+T: git https://github.com/Rust-for-Linux/linux.git hid-next
+F: rust/kernel/hid.rs
+
HID LOGITECH DRIVERS
R: Filipe Laíns <lains@riseup.net>
L: linux-input@vger.kernel.org
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 43859fc75747..5c8839ddc999 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -744,6 +744,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 41a98d5b6521..0beb334fa64f 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -52,6 +52,8 @@
#include <linux/file.h>
#include <linux/firmware.h>
#include <linux/fs.h>
+#include <linux/hid.h>
+#include "../../drivers/hid/hid-ids.h"
#include <linux/jiffies.h>
#include <linux/jump_label.h>
#include <linux/kref.h>
diff --git a/rust/kernel/hid.rs b/rust/kernel/hid.rs
new file mode 100644
index 000000000000..dabf7dce615f
--- /dev/null
+++ b/rust/kernel/hid.rs
@@ -0,0 +1,458 @@
+// SPDX-License-Identifier: GPL-2.0
+
+// Copyright (C) 2025 Rahul Rameshbabu <sergeantsagara@protonmail.com>
+
+//! Abstractions for the HID interface.
+//!
+//! C header: [`include/linux/hid.h`](srctree/include/linux/hid.h)
+
+use crate::{device, device_id::RawDeviceId, driver, error::*, prelude::*, types::Opaque};
+use core::{
+ marker::PhantomData,
+ ptr::{addr_of_mut, NonNull},
+};
+
+/// Indicates the item is static read-only.
+///
+/// Refer to [Device Class Definition for HID 1.11]
+/// Section 6.2.2.5 Input, Output, and Feature Items.
+///
+/// [Device Class Definition for HID 1.11]: https://www.usb.org/sites/default/files/hid1_11.pdf
+pub const MAIN_ITEM_CONSTANT: u8 = bindings::HID_MAIN_ITEM_CONSTANT as u8;
+/// Indicates the item represents data from a physical control.
+///
+/// Refer to [Device Class Definition for HID 1.11]
+/// Section 6.2.2.5 Input, Output, and Feature Items.
+///
+/// [Device Class Definition for HID 1.11]: https://www.usb.org/sites/default/files/hid1_11.pdf
+pub const MAIN_ITEM_VARIABLE: u8 = bindings::HID_MAIN_ITEM_VARIABLE as u8;
+/// Indicates the item should be treated as a relative change from the previous
+/// report.
+///
+/// Refer to [Device Class Definition for HID 1.11]
+/// Section 6.2.2.5 Input, Output, and Feature Items.
+///
+/// [Device Class Definition for HID 1.11]: https://www.usb.org/sites/default/files/hid1_11.pdf
+pub const MAIN_ITEM_RELATIVE: u8 = bindings::HID_MAIN_ITEM_RELATIVE as u8;
+/// Indicates the item should wrap around when reaching the extreme high or
+/// extreme low values.
+///
+/// Refer to [Device Class Definition for HID 1.11]
+/// Section 6.2.2.5 Input, Output, and Feature Items.
+///
+/// [Device Class Definition for HID 1.11]: https://www.usb.org/sites/default/files/hid1_11.pdf
+pub const MAIN_ITEM_WRAP: u8 = bindings::HID_MAIN_ITEM_WRAP as u8;
+/// Indicates the item should wrap around when reaching the extreme high or
+/// extreme low values.
+///
+/// Refer to [Device Class Definition for HID 1.11]
+/// Section 6.2.2.5 Input, Output, and Feature Items.
+///
+/// [Device Class Definition for HID 1.11]: https://www.usb.org/sites/default/files/hid1_11.pdf
+pub const MAIN_ITEM_NONLINEAR: u8 = bindings::HID_MAIN_ITEM_NONLINEAR as u8;
+/// Indicates whether the control has a preferred state it will physically
+/// return to without user intervention.
+///
+/// Refer to [Device Class Definition for HID 1.11]
+/// Section 6.2.2.5 Input, Output, and Feature Items.
+///
+/// [Device Class Definition for HID 1.11]: https://www.usb.org/sites/default/files/hid1_11.pdf
+pub const MAIN_ITEM_NO_PREFERRED: u8 = bindings::HID_MAIN_ITEM_NO_PREFERRED as u8;
+/// Indicates whether the control has a physical state where it will not send
+/// any reports.
+///
+/// Refer to [Device Class Definition for HID 1.11]
+/// Section 6.2.2.5 Input, Output, and Feature Items.
+///
+/// [Device Class Definition for HID 1.11]: https://www.usb.org/sites/default/files/hid1_11.pdf
+pub const MAIN_ITEM_NULL_STATE: u8 = bindings::HID_MAIN_ITEM_NULL_STATE as u8;
+/// Indicates whether the control requires host system logic to change state.
+///
+/// Refer to [Device Class Definition for HID 1.11]
+/// Section 6.2.2.5 Input, Output, and Feature Items.
+///
+/// [Device Class Definition for HID 1.11]: https://www.usb.org/sites/default/files/hid1_11.pdf
+pub const MAIN_ITEM_VOLATILE: u8 = bindings::HID_MAIN_ITEM_VOLATILE as u8;
+/// Indicates whether the item is fixed size or a variable buffer of bytes.
+///
+/// Refer to [Device Class Definition for HID 1.11]
+/// Section 6.2.2.5 Input, Output, and Feature Items.
+///
+/// [Device Class Definition for HID 1.11]: https://www.usb.org/sites/default/files/hid1_11.pdf
+pub const MAIN_ITEM_BUFFERED_BYTE: u8 = bindings::HID_MAIN_ITEM_BUFFERED_BYTE as u8;
+
+/// HID device groups are intended to help categories HID devices based on a set
+/// of common quirks and logic that they will require to function correctly.
+pub enum Group {
+ /// Indicates a generic device that should need no custom logic from the
+ /// core HID stack.
+ Generic = bindings::HID_GROUP_GENERIC as isize,
+ /// Maps multitouch devices to hid-multitouch instead of hid-generic.
+ Multitouch = bindings::HID_GROUP_MULTITOUCH as isize,
+ /// Used for autodetecing and mapping of HID sensor hubs to
+ /// hid-sensor-hub.
+ SensorHub = bindings::HID_GROUP_SENSOR_HUB as isize,
+ /// Used for autodetecing and mapping Win 8 multitouch devices to set the
+ /// needed quirks.
+ MultitouchWin8 = bindings::HID_GROUP_MULTITOUCH_WIN_8 as isize,
+
+ /// Used to distinguish Synpatics touchscreens from other products. The
+ /// touchscreens will be handled by hid-multitouch instead, while everything
+ /// else will be managed by hid-rmi.
+ RMI = bindings::HID_GROUP_RMI as isize,
+ /// Used for hid-core handling to automatically identify Wacom devices and
+ /// have them probed by hid-wacom.
+ Wacom = bindings::HID_GROUP_WACOM as isize,
+ /// Used by logitech-djreceiver and logitech-djdevice to autodetect if
+ /// devices paied to the DJ receivers are DJ devices and handle them with
+ /// the device driver.
+ LogitechDJDevice = bindings::HID_GROUP_LOGITECH_DJ_DEVICE as isize,
+ /// Since the Valve Steam Controller only has vendor-specific usages,
+ /// prevent hid-generic from parsing its reports since there would be
+ /// nothing hid-generic could do for the device.
+ Steam = bindings::HID_GROUP_STEAM as isize,
+ /// Used to differentiate 27 Mhz frequency Logitech DJ devices from other
+ /// Logitech DJ devices.
+ Logitech27MHzDevice = bindings::HID_GROUP_LOGITECH_27MHZ_DEVICE as isize,
+ /// Used for autodetecting and mapping Vivaldi devices to hid-vivaldi.
+ Vivaldi = bindings::HID_GROUP_VIVALDI as isize,
+}
+
+impl Group {
+ /// Internal function used to convert Group variants into u16
+ const fn into(self) -> u16 {
+ // variants assigned constants that can be represented as u16
+ self as u16
+ }
+}
+
+/// The HID device representation.
+///
+/// This structure represents the Rust abstraction for a C `struct hid_device`. The implementation
+/// abstracts the usage of an already existing C `struct hid_device` within Rust code that we get
+/// passed from the C side.
+///
+/// # Invariants
+///
+/// A [`Device`] instance represents a valid `struct hid_device` created by the C portion of the kernel.
+#[repr(transparent)]
+pub struct Device<Ctx: device::DeviceContext = device::Normal>(
+ Opaque<bindings::hid_device>,
+ PhantomData<Ctx>,
+);
+
+impl<Ctx: device::DeviceContext> Device<Ctx> {
+ fn as_raw(&self) -> *mut bindings::hid_device {
+ self.0.get()
+ }
+
+ /// Returns the HID transport bus ID.
+ pub fn bus(&self) -> u16 {
+ // SAFETY: `self.as_raw` is a valid pointer to a `struct hid_device`
+ unsafe { *self.as_raw() }.bus
+ }
+
+ /// Returns the HID report group.
+ pub fn group(&self) -> u16 {
+ // SAFETY: `self.as_raw` is a valid pointer to a `struct hid_device`
+ unsafe { *self.as_raw() }.group
+ }
+
+ /// Returns the HID vendor ID.
+ pub fn vendor(&self) -> u32 {
+ // SAFETY: `self.as_raw` is a valid pointer to a `struct hid_device`
+ unsafe { *self.as_raw() }.vendor
+ }
+
+ /// Returns the HID product ID.
+ pub fn product(&self) -> u32 {
+ // SAFETY: `self.as_raw` is a valid pointer to a `struct hid_device`
+ unsafe { *self.as_raw() }.product
+ }
+}
+
+// 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);
+
+// SAFETY: Instances of `Device` are always reference-counted.
+unsafe impl crate::types::AlwaysRefCounted for Device {
+ fn inc_ref(&self) {
+ // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero.
+ unsafe { bindings::kref_get(&mut ((*self.as_raw()).ref_)) }
+ }
+
+ unsafe fn dec_ref(obj: NonNull<Self>) {
+ // SAFETY: The safety requirements guarantee that the refcount is non-zero.
+ unsafe {
+ bindings::kref_put(
+ &mut ((*obj.cast::<bindings::hid_device>().as_ptr()).ref_),
+ Some(bindings::hiddev_free),
+ )
+ }
+ }
+}
+
+impl<Ctx: device::DeviceContext> AsRef<device::Device<Ctx>> for Device<Ctx> {
+ fn as_ref(&self) -> &device::Device<Ctx> {
+ // SAFETY: By the type invariant of `Self`, `self.as_raw()` is a pointer to a valid
+ // `struct hid_device`.
+ let dev = unsafe { addr_of_mut!((*self.as_raw()).dev) };
+
+ // SAFETY: `dev` points to a valid `struct device`.
+ unsafe { device::Device::as_ref(dev) }
+ }
+}
+
+/// Abstraction for the HID device ID structure ([`struct hid_device_id`]).
+#[repr(transparent)]
+#[derive(Clone, Copy)]
+pub struct DeviceId(bindings::hid_device_id);
+
+impl DeviceId {
+ /// Equivalent to C's `HID_USB_DEVICE` macro.
+ ///
+ /// Create a new `hid::DeviceId` from a group, vendor ID, and device ID
+ /// number.
+ pub const fn new_usb(group: Group, vendor: u32, product: u32) -> Self {
+ Self(bindings::hid_device_id {
+ bus: 0x3, /* BUS_USB */
+ group: group.into(),
+ vendor,
+ product,
+ driver_data: 0,
+ })
+ }
+
+ /// Returns the HID transport bus ID.
+ pub fn bus(&self) -> u16 {
+ self.0.bus
+ }
+
+ /// Returns the HID report group.
+ pub fn group(&self) -> u16 {
+ self.0.group
+ }
+
+ /// Returns the HID vendor ID.
+ pub fn vendor(&self) -> u32 {
+ self.0.vendor
+ }
+
+ /// Returns the HID product ID.
+ pub fn product(&self) -> u32 {
+ self.0.product
+ }
+}
+
+// SAFETY:
+// * `DeviceId` is a `#[repr(transparent)` wrapper of `hid_device_id` and does not add
+// additional invariants, so it's safe to transmute to `RawType`.
+// * `DRIVER_DATA_OFFSET` is the offset to the `driver_data` field.
+unsafe impl RawDeviceId for DeviceId {
+ type RawType = bindings::hid_device_id;
+
+ const DRIVER_DATA_OFFSET: usize = core::mem::offset_of!(bindings::hid_device_id, driver_data);
+
+ fn index(&self) -> usize {
+ self.0.driver_data
+ }
+}
+
+/// IdTable type for HID
+pub type IdTable<T> = &'static dyn kernel::device_id::IdTable<DeviceId, T>;
+
+/// Create a HID `IdTable` with its alias for modpost.
+#[macro_export]
+macro_rules! hid_device_table {
+ ($table_name:ident, $module_table_name:ident, $id_info_type: ty, $table_data: expr) => {
+ const $table_name: $crate::device_id::IdArray<
+ $crate::hid::DeviceId,
+ $id_info_type,
+ { $table_data.len() },
+ > = $crate::device_id::IdArray::new($table_data);
+
+ $crate::module_device_table!("hid", $module_table_name, $table_name);
+ };
+}
+
+/// The HID driver trait.
+///
+/// # Example
+///
+///```
+/// use kernel::{bindings, hid};
+///
+/// struct MyDriver;
+///
+/// kernel::hid_device_table!(
+/// HID_TABLE,
+/// MODULE_HID_TABLE,
+/// <MyDriver as hid::Driver>::IdInfo,
+/// [(
+/// hid::DeviceId::new_usb(
+/// hid::Group::Steam,
+/// bindings::USB_VENDOR_ID_VALVE,
+/// bindings::USB_DEVICE_ID_STEAM_DECK,
+/// ),
+/// (),
+/// )]
+/// );
+///
+/// #[vtable]
+/// impl hid::Driver for MyDriver {
+/// type IdInfo = ();
+/// const ID_TABLE: hid::IdTable<Self::IdInfo> = &HID_TABLE;
+///
+/// /// This function is optional to implement.
+/// fn report_fixup<'a, 'b: 'a>(_hdev: &hid::Device<device::Core>, rdesc: &'b mut [u8]) -> &'a [u8] {
+/// // Perform some report descriptor fixup
+/// rdesc
+/// }
+/// }
+///```
+/// Drivers must implement this trait in order to get a HID driver registered.
+/// Please refer to the `Adapter` documentation for an example.
+#[vtable]
+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<device::Core>, _rdesc: &'b mut [u8]) -> &'a [u8] {
+ 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.
+ unsafe {
+ (*hdrv.get()).name = name.as_char_ptr();
+ (*hdrv.get()).id_table = T::ID_TABLE.as_ptr();
+ (*hdrv.get()).report_fixup = if T::HAS_REPORT_FIXUP {
+ Some(Self::report_fixup_callback)
+ } else {
+ None
+ };
+ }
+
+ // SAFETY: `hdrv` is guaranteed to be a valid `RegType`
+ to_result(unsafe {
+ bindings::__hid_register_driver(hdrv.get(), module.0, name.as_char_ptr())
+ })
+ }
+
+ unsafe fn unregister(hdrv: &Opaque<Self::RegType>) {
+ // SAFETY: `hdrv` is guaranteed to be a valid `RegType`
+ unsafe { bindings::hid_unregister_driver(hdrv.get()) }
+ }
+}
+
+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::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) => {
+ dev_err!(
+ hdev.as_ref(),
+ "Cannot fix report description due to length conversion failure: {}!\n",
+ e
+ );
+
+ 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) => {
+ dev_err!(
+ hdev.as_ref(),
+ "Fixed report description will not be used due to {}!\n",
+ e
+ );
+
+ return buf;
+ }
+ }
+
+ rdesc_slice.as_ptr()
+ }
+}
+
+/// Declares a kernel module that exposes a single HID driver.
+///
+/// # Example
+///
+///```ignore
+/// kernel::module_hid_driver! {
+/// type: MyDriver,
+/// name: "Module name",
+/// authors: ["Author name"],
+/// description: "Description",
+/// license: "GPL",
+/// }
+///```
+#[macro_export]
+macro_rules! module_hid_driver {
+ ($($f:tt)*) => {
+ $crate::module_driver!(<T>, $crate::hid::Adapter<T>, { $($f)* });
+ };
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 6b4774b2b1c3..a50f7d099e8a 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -78,6 +78,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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 4/4] rust: hid: Glorious Gaming PC Race Model O and O- mice reference driver
2025-07-13 21:11 [PATCH v2 0/4] Initial work for Rust abstraction for HID device driver development Rahul Rameshbabu
` (2 preceding siblings ...)
2025-07-13 21:12 ` [PATCH v2 3/4] rust: core abstractions for HID drivers Rahul Rameshbabu
@ 2025-07-13 21:12 ` Rahul Rameshbabu
2025-07-13 21:46 ` Miguel Ojeda
3 siblings, 1 reply; 12+ messages in thread
From: Rahul Rameshbabu @ 2025-07-13 21:12 UTC (permalink / raw)
To: rust-for-linux, linux-input
Cc: Jiri Kosina, a.hindborg, alex.gaynor, aliceryhl, benno.lossin,
Benjamin Tissoires, bjorn3_gh, boqun.feng, dakr, db48x, gary,
ojeda, tmgross, peter.hutterer, Rahul Rameshbabu
Demonstrate how to perform a report fixup from a Rust HID driver. The mice
specify the const flag incorrectly in the consumer input report descriptor,
which leads to inputs being ignored. Correctly patch the report descriptor for
the Model O and O- mice.
Portions of the HID report post-fixup:
device 0:0
...
0x81, 0x06, // Input (Data,Var,Rel) 84
...
0x81, 0x06, // Input (Data,Var,Rel) 112
...
0x81, 0x06, // Input (Data,Var,Rel) 140
Signed-off-by: Rahul Rameshbabu <sergeantsagara@protonmail.com>
---
Notes:
Changelog:
v1->v2:
* Use vendor id and device id from drivers/hid/hid-ids.h bindings
* Make use for dev_err! as appropriate
MAINTAINERS | 7 ++++
drivers/hid/Kconfig | 8 +++++
drivers/hid/Makefile | 1 +
drivers/hid/hid-glorious.c | 2 ++
drivers/hid/hid_glorious_rust.rs | 59 ++++++++++++++++++++++++++++++++
5 files changed, 77 insertions(+)
create mode 100644 drivers/hid/hid_glorious_rust.rs
diff --git a/MAINTAINERS b/MAINTAINERS
index eebc810432c5..9a77078eaae8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10200,6 +10200,13 @@ L: platform-driver-x86@vger.kernel.org
S: Maintained
F: drivers/platform/x86/gigabyte-wmi.c
+GLORIOUS RUST DRIVER [RUST]
+M: Rahul Rameshbabu <sergeantsagara@protonmail.com>
+L: rust-for-linux@vger.kernel.org
+S: Maintained
+T: git https://github.com/Rust-for-Linux/linux.git hid-next
+F: drivers/hid/hid_glorious_rust.rs
+
GNSS SUBSYSTEM
M: Johan Hovold <johan@kernel.org>
S: Maintained
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 5c8839ddc999..3592a4de113f 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -406,6 +406,14 @@ config HID_GLORIOUS
Support for Glorious PC Gaming Race mice such as
the Glorious Model O, O- and D.
+config HID_GLORIOUS_RUST
+ tristate "Glorious O and O- mice Rust reference driver"
+ depends on USB_HID
+ depends on RUST_HID_ABSTRACTIONS
+ help
+ Support for Glorious PC Gaming Race O and O- mice
+ in Rust
+
config HID_HOLTEK
tristate "Holtek HID devices"
depends on USB_HID
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index 10ae5dedbd84..bd86b3db5d88 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -55,6 +55,7 @@ obj-$(CONFIG_HID_FT260) += hid-ft260.o
obj-$(CONFIG_HID_GEMBIRD) += hid-gembird.o
obj-$(CONFIG_HID_GFRM) += hid-gfrm.o
obj-$(CONFIG_HID_GLORIOUS) += hid-glorious.o
+obj-$(CONFIG_HID_GLORIOUS_RUST) += hid_glorious_rust.o
obj-$(CONFIG_HID_VIVALDI_COMMON) += hid-vivaldi-common.o
obj-$(CONFIG_HID_GOODIX_SPI) += hid-goodix-spi.o
obj-$(CONFIG_HID_GOOGLE_HAMMER) += hid-google-hammer.o
diff --git a/drivers/hid/hid-glorious.c b/drivers/hid/hid-glorious.c
index 5bbd81248053..d7362852c20f 100644
--- a/drivers/hid/hid-glorious.c
+++ b/drivers/hid/hid-glorious.c
@@ -76,8 +76,10 @@ static int glorious_probe(struct hid_device *hdev,
}
static const struct hid_device_id glorious_devices[] = {
+#if !IS_ENABLED(CONFIG_HID_GLORIOUS_RUST)
{ HID_USB_DEVICE(USB_VENDOR_ID_SINOWEALTH,
USB_DEVICE_ID_GLORIOUS_MODEL_O) },
+#endif
{ HID_USB_DEVICE(USB_VENDOR_ID_SINOWEALTH,
USB_DEVICE_ID_GLORIOUS_MODEL_D) },
{ HID_USB_DEVICE(USB_VENDOR_ID_LAVIEW,
diff --git a/drivers/hid/hid_glorious_rust.rs b/drivers/hid/hid_glorious_rust.rs
new file mode 100644
index 000000000000..833e069ff446
--- /dev/null
+++ b/drivers/hid/hid_glorious_rust.rs
@@ -0,0 +1,59 @@
+// SPDX-License-Identifier: GPL-2.0
+
+// Copyright (C) 2025 Rahul Rameshbabu <sergeantsagara@protonmail.com>
+
+//! Rust reference HID driver for Glorious Model O and O- mice
+
+use kernel::{self, bindings, device, hid, prelude::*};
+
+struct GloriousRust;
+
+kernel::hid_device_table!(
+ HID_TABLE,
+ MODULE_HID_TABLE,
+ <GloriousRust as hid::Driver>::IdInfo,
+ [(
+ hid::DeviceId::new_usb(
+ hid::Group::Generic,
+ bindings::USB_VENDOR_ID_SINOWEALTH,
+ bindings::USB_DEVICE_ID_GLORIOUS_MODEL_O,
+ ),
+ (),
+ )]
+);
+
+#[vtable]
+impl hid::Driver for GloriousRust {
+ type IdInfo = ();
+ const ID_TABLE: hid::IdTable<Self::IdInfo> = &HID_TABLE;
+
+ /// Glorious Model O and O- specify the const flag in the consumer input
+ /// report descriptor, which leads to inputs being ignored. Fix this by
+ /// patching the descriptor.
+ fn report_fixup<'a, 'b: 'a>(hdev: &hid::Device<device::Core>, rdesc: &'b mut [u8]) -> &'a [u8] {
+ if rdesc.len() == 213
+ && (rdesc[84] == 129 && rdesc[85] == 3)
+ && (rdesc[112] == 129 && rdesc[113] == 3)
+ && (rdesc[140] == 129 && rdesc[141] == 3)
+ {
+ dev_info!(
+ hdev.as_ref(),
+ "patching Glorious Model O consumer control report descriptor\n"
+ );
+
+ rdesc[85] = hid::MAIN_ITEM_VARIABLE | hid::MAIN_ITEM_RELATIVE;
+ rdesc[113] = hid::MAIN_ITEM_VARIABLE | hid::MAIN_ITEM_RELATIVE;
+ rdesc[141] = hid::MAIN_ITEM_VARIABLE | hid::MAIN_ITEM_RELATIVE;
+ }
+
+ rdesc
+ }
+}
+
+kernel::module_hid_driver! {
+ type: GloriousRust,
+ name: "GloriousRust",
+ authors: ["Rahul Rameshbabu <sergeantsagara@protonmail.com>"],
+ description: "Rust reference HID driver for Glorious Model O and O- mice",
+ license: "GPL",
+}
--
2.47.2
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 4/4] rust: hid: Glorious Gaming PC Race Model O and O- mice reference driver
2025-07-13 21:12 ` [PATCH v2 4/4] rust: hid: Glorious Gaming PC Race Model O and O- mice reference driver Rahul Rameshbabu
@ 2025-07-13 21:46 ` Miguel Ojeda
0 siblings, 0 replies; 12+ messages in thread
From: Miguel Ojeda @ 2025-07-13 21:46 UTC (permalink / raw)
To: Rahul Rameshbabu
Cc: rust-for-linux, linux-input, Jiri Kosina, a.hindborg, alex.gaynor,
aliceryhl, benno.lossin, Benjamin Tissoires, bjorn3_gh,
boqun.feng, dakr, db48x, gary, ojeda, tmgross, peter.hutterer
On Sun, Jul 13, 2025 at 11:13 PM Rahul Rameshbabu
<sergeantsagara@protonmail.com> wrote:
>
> +L: rust-for-linux@vger.kernel.org
> +T: git https://github.com/Rust-for-Linux/linux.git hid-next
Hmm... I think we discussed going through HID, no? Or did something change?
> + Support for Glorious PC Gaming Race O and O- mice
> + in Rust
Period at the end.
> +//! Rust reference HID driver for Glorious Model O and O- mice
Period at the end.
> + /// Glorious Model O and O- specify the const flag in the consumer input
> + /// report descriptor, which leads to inputs being ignored. Fix this by
> + /// patching the descriptor.
If these are the function docs (i.e. `///`), then please write them as
usually we would do, e.g. "Fix ... (new paragraph) ...".
Thanks!
Cheers,
Miguel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 3/4] rust: core abstractions for HID drivers
2025-07-13 21:12 ` [PATCH v2 3/4] rust: core abstractions for HID drivers Rahul Rameshbabu
@ 2025-07-13 21:57 ` Miguel Ojeda
2025-07-20 16:44 ` Rahul Rameshbabu
2025-07-15 15:04 ` Danilo Krummrich
1 sibling, 1 reply; 12+ messages in thread
From: Miguel Ojeda @ 2025-07-13 21:57 UTC (permalink / raw)
To: Rahul Rameshbabu
Cc: rust-for-linux, linux-input, Jiri Kosina, a.hindborg, alex.gaynor,
aliceryhl, benno.lossin, Benjamin Tissoires, bjorn3_gh,
boqun.feng, dakr, db48x, gary, ojeda, tmgross, peter.hutterer
Hi Rahul,
Some mostly doc-related comments below...
On Sun, Jul 13, 2025 at 11:12 PM Rahul Rameshbabu
<sergeantsagara@protonmail.com> wrote:
>
> Some points I did not address from the last review cycle:
>
> * Enabling CONFIG_HID=m with the Rust bindings
> - This is a limitation due to the Makefile rules that currently exist,
> compiling all the Rust bindings into kernel.o, which gets linked into the
> core kernel image.
> - I would be happy to work on support for module-level bindings, but this is
> not a problem unique only to the Rust HID binding work.
I am working on supporting that, so for the moment your approach is fine.
> +L: rust-for-linux@vger.kernel.org
> +T: git https://github.com/Rust-for-Linux/linux.git hid-next
Hmm... I think we discussed going through HID, no? Or did something change?
> +/// [Device Class Definition for HID 1.11]: https://www.usb.org/sites/default/files/hid1_11.pdf
> +pub const MAIN_ITEM_CONSTANT: u8 = bindings::HID_MAIN_ITEM_CONSTANT as u8;
> +/// Indicates the item represents data from a physical control.
Please add new-lines between items.
> + Generic = bindings::HID_GROUP_GENERIC as isize,
> + /// Maps multitouch devices to hid-multitouch instead of hid-generic.
Same between `enum` variants.
> + /// Internal function used to convert Group variants into u16
[`Group`]
[`u16`]
> + const fn into(self) -> u16 {
Could the `enum` be `repr(u16)` directly?
Are those `as isize` for a reason?
> + // variants assigned constants that can be represented as u16
Please format consistently comments as well.
> + /// Returns the HID report group.
> + pub fn group(&self) -> u16 {
> + // SAFETY: `self.as_raw` is a valid pointer to a `struct hid_device`
> + unsafe { *self.as_raw() }.group
> + }
Do you need to expose the group as a raw integer?
Also, are these used currently?
> +/// Abstraction for the HID device ID structure ([`struct hid_device_id`]).
This intra-doc pointing is probably broken -- we don't have yet
automatic support to link to C ones yet (please check `make .....
rustdoc`).
> + bus: 0x3, /* BUS_USB */
Please use `//` for comments unless there is a reason that wouldn't
work (e.g. to comment out within code).
> +/// IdTable type for HID
[`IdTable`]
Period at the end.
(I see others elsewhere are like this, so that is probably you had it
like this -- they should be fixed too)
> +/// # Example
Please use the plural even if there is one example.
> +/// // Perform some report descriptor fixup
Period at the end.
> +///```
Missing space (several).
> + "Cannot fix report description due to length conversion failure: {}!\n",
This could probably be a bit shorter using {e}
> + // Build a mutable Rust slice from buf and size
Markdown.
> + // SAFETY: The HID subsystem only ever calls the report_fixup callback
Markdown (several).
> +/// fn report_fixup<'a, 'b: 'a>(_hdev: &hid::Device<device::Core>, rdesc: &'b mut [u8]) -> &'a [u8] {
The example does not build -- please see how to test them here:
https://docs.kernel.org/rust/testing.html#kunit-tests-are-documentation-tests
Thanks!
Cheers,
Miguel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 3/4] rust: core abstractions for HID drivers
2025-07-13 21:12 ` [PATCH v2 3/4] rust: core abstractions for HID drivers Rahul Rameshbabu
2025-07-13 21:57 ` Miguel Ojeda
@ 2025-07-15 15:04 ` Danilo Krummrich
2025-07-19 22:46 ` Rahul Rameshbabu
1 sibling, 1 reply; 12+ messages in thread
From: Danilo Krummrich @ 2025-07-15 15:04 UTC (permalink / raw)
To: Rahul Rameshbabu
Cc: rust-for-linux, linux-input, Jiri Kosina, a.hindborg, alex.gaynor,
aliceryhl, benno.lossin, Benjamin Tissoires, bjorn3_gh,
boqun.feng, db48x, gary, ojeda, tmgross, peter.hutterer
On Sun Jul 13, 2025 at 11:12 PM CEST, Rahul Rameshbabu wrote:
> +// SAFETY: Instances of `Device` are always reference-counted.
> +unsafe impl crate::types::AlwaysRefCounted for Device {
> + fn inc_ref(&self) {
> + // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero.
> + unsafe { bindings::kref_get(&mut ((*self.as_raw()).ref_)) }
I'm confused, what's the lifecycle of a struct hid_device? It embedds a struct
device, so it also inherits its reference count. Additionally it also has a
struct kref. Can you elaborate please?
I don't know what the struct kref is for, but I'm pretty sure you want to manage
the reference count of the embedded struct device here.
> + }
> +
> + unsafe fn dec_ref(obj: NonNull<Self>) {
> + // SAFETY: The safety requirements guarantee that the refcount is non-zero.
> + unsafe {
> + bindings::kref_put(
> + &mut ((*obj.cast::<bindings::hid_device>().as_ptr()).ref_),
I think you want &raw mut instead.
> + Some(bindings::hiddev_free),
> + )
> + }
> + }
> +}
> +
> +impl<Ctx: device::DeviceContext> AsRef<device::Device<Ctx>> for Device<Ctx> {
> + fn as_ref(&self) -> &device::Device<Ctx> {
> + // SAFETY: By the type invariant of `Self`, `self.as_raw()` is a pointer to a valid
> + // `struct hid_device`.
> + let dev = unsafe { addr_of_mut!((*self.as_raw()).dev) };
You also use &raw mut meanwhile.
> +
> + // SAFETY: `dev` points to a valid `struct device`.
> + unsafe { device::Device::as_ref(dev) }
> + }
> +}
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 3/4] rust: core abstractions for HID drivers
2025-07-15 15:04 ` Danilo Krummrich
@ 2025-07-19 22:46 ` Rahul Rameshbabu
0 siblings, 0 replies; 12+ messages in thread
From: Rahul Rameshbabu @ 2025-07-19 22:46 UTC (permalink / raw)
To: Danilo Krummrich
Cc: rust-for-linux, linux-input, Jiri Kosina, a.hindborg, alex.gaynor,
aliceryhl, benno.lossin, Benjamin Tissoires, bjorn3_gh,
boqun.feng, db48x, gary, ojeda, tmgross, peter.hutterer
On Tue, 15 Jul, 2025 17:04:07 +0200 "Danilo Krummrich" <dakr@kernel.org> wrote:
> On Sun Jul 13, 2025 at 11:12 PM CEST, Rahul Rameshbabu wrote:
>> +// SAFETY: Instances of `Device` are always reference-counted.
>> +unsafe impl crate::types::AlwaysRefCounted for Device {
>> + fn inc_ref(&self) {
>> + // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero.
>> + unsafe { bindings::kref_get(&mut ((*self.as_raw()).ref_)) }
>
> I'm confused, what's the lifecycle of a struct hid_device? It embedds a struct
> device, so it also inherits its reference count. Additionally it also has a
> struct kref. Can you elaborate please?
>
> I don't know what the struct kref is for, but I'm pretty sure you want to manage
> the reference count of the embedded struct device here.
>
Hi Danilo,
The hid subsystem uses a separate ref counter from the one maintained by
the embedded struct device. hid_device_release is assigned to the
embedded struct device's release method and makes use of this separate
reference (in drivers/hid/hid-core.c).
drivers/hid/hid-debug.c makes use of the same reference in a similar
manner to what I do here in hid_debug_events_release and
hid_debug_events_open.
Basically, the hid subsystem has a separate reference counter on top of
the one embedded in the kobject of struct device;
I decided to follow the same pattern used in the core C HID subsystem.
This pattern is found in hid-core + hid_debug.
However, thank you for bringing this up since I think that you are right
that it would be better to increment the reference count for the
underlying device rather than use this separate external reference
count.
It might even make sense to refactor core C HID to remove this separate
kref and have hid_debug increase the reference count of the underlying
struct device's kobject.
Right now, we can have the following situation in the hid subsystem.
1. struct device's kobject decrements to 0
2. struct device's release callback is called
3. This invokes hid_device_ref and decrements struct hid_device's ref
4. hid_debug is in use, so the ref has not gone to 0 yet
5. When hid_debug is ready to tear down the instance, hiddev_free will
get called
The above creates a weird situation where the HID device can still be
in-use even though the underlying device instance has reached a
reference count of 0.
I will first start with fixing this in my Rust code, but I will take a
look at cleaning this up in the core HID subsystem as well.
>> + }
>> +
>> + unsafe fn dec_ref(obj: NonNull<Self>) {
>> + // SAFETY: The safety requirements guarantee that the refcount is non-zero.
>> + unsafe {
>> + bindings::kref_put(
>> + &mut ((*obj.cast::<bindings::hid_device>().as_ptr()).ref_),
>
> I think you want &raw mut instead.
>
>> + Some(bindings::hiddev_free),
>> + )
>> + }
>> + }
>> +}
>> +
>> +impl<Ctx: device::DeviceContext> AsRef<device::Device<Ctx>> for Device<Ctx> {
>> + fn as_ref(&self) -> &device::Device<Ctx> {
>> + // SAFETY: By the type invariant of `Self`, `self.as_raw()` is a pointer to a valid
>> + // `struct hid_device`.
>> + let dev = unsafe { addr_of_mut!((*self.as_raw()).dev) };
>
> You also use &raw mut meanwhile.
>
>> +
>> + // SAFETY: `dev` points to a valid `struct device`.
>> + unsafe { device::Device::as_ref(dev) }
>> + }
>> +}
https://doc.rust-lang.org/std/primitive.pointer.html#3-create-it-using-raw
Yes, I want &raw mut in the places you mentioned. Thanks :)
Thanks,
Rahul Rameshbabu
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/4] rust: add kref bindings
2025-07-13 21:12 ` [PATCH v2 2/4] rust: add kref bindings Rahul Rameshbabu
@ 2025-07-20 16:02 ` Rahul Rameshbabu
2025-07-20 16:07 ` Boqun Feng
0 siblings, 1 reply; 12+ messages in thread
From: Rahul Rameshbabu @ 2025-07-20 16:02 UTC (permalink / raw)
To: rust-for-linux
Cc: linux-input, Jiri Kosina, a.hindborg, alex.gaynor, aliceryhl,
benno.lossin, Benjamin Tissoires, bjorn3_gh, boqun.feng, dakr,
db48x, gary, ojeda, tmgross, peter.hutterer
On Sun, 13 Jul, 2025 21:12:06 +0000 "Rahul Rameshbabu" <sergeantsagara@protonmail.com> wrote:
> Introduce kref bindings for use in the Rust HID abstractions. Needed for
> implementing the AlwaysRefCounted trait for kernel::hid::Device. Add rust
> helpers for binding the kref_get and kref_put static inline C functions.
>
> Signed-off-by: Rahul Rameshbabu <sergeantsagara@protonmail.com>
> ---
> rust/bindings/bindings_helper.h | 1 +
> rust/helpers/helpers.c | 1 +
> rust/helpers/kref.c | 13 +++++++++++++
> 3 files changed, 15 insertions(+)
> create mode 100644 rust/helpers/kref.c
>
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 8cbb660e2ec2..41a98d5b6521 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -54,6 +54,7 @@
> #include <linux/fs.h>
> #include <linux/jiffies.h>
> #include <linux/jump_label.h>
> +#include <linux/kref.h>
> #include <linux/mdio.h>
> #include <linux/miscdevice.h>
> #include <linux/of_device.h>
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index b15b3cddad4e..7f5403d6c51c 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -25,6 +25,7 @@
> #include "fs.c"
> #include "io.c"
> #include "jump_label.c"
> +#include "kref.c"
> #include "kunit.c"
> #include "mm.c"
> #include "mutex.c"
> diff --git a/rust/helpers/kref.c b/rust/helpers/kref.c
> new file mode 100644
> index 000000000000..25eeb0a724ac
> --- /dev/null
> +++ b/rust/helpers/kref.c
> @@ -0,0 +1,13 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/kref.h>
> +
> +void rust_helper_kref_get(struct kref *kref)
> +{
> + kref_get(kref);
> +}
> +
> +void rust_helper_kref_put(struct kref *kref, void (*release)(struct kref *kref))
> +{
> + kref_put(kref, release);
> +}
I am about to send out my v3, and this patch will no longer be needed.
Would it make sense to send this out separately? I can also just keep it
stashed away till someone needs it.
Thanks,
Rahul Rameshbabu
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/4] rust: add kref bindings
2025-07-20 16:02 ` Rahul Rameshbabu
@ 2025-07-20 16:07 ` Boqun Feng
0 siblings, 0 replies; 12+ messages in thread
From: Boqun Feng @ 2025-07-20 16:07 UTC (permalink / raw)
To: Rahul Rameshbabu
Cc: rust-for-linux, linux-input, Jiri Kosina, a.hindborg, alex.gaynor,
aliceryhl, benno.lossin, Benjamin Tissoires, bjorn3_gh, dakr,
db48x, gary, ojeda, tmgross, peter.hutterer
On Sun, Jul 20, 2025 at 04:02:09PM +0000, Rahul Rameshbabu wrote:
[...]
> > diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> > index b15b3cddad4e..7f5403d6c51c 100644
> > --- a/rust/helpers/helpers.c
> > +++ b/rust/helpers/helpers.c
> > @@ -25,6 +25,7 @@
> > #include "fs.c"
> > #include "io.c"
> > #include "jump_label.c"
> > +#include "kref.c"
> > #include "kunit.c"
> > #include "mm.c"
> > #include "mutex.c"
> > diff --git a/rust/helpers/kref.c b/rust/helpers/kref.c
> > new file mode 100644
> > index 000000000000..25eeb0a724ac
> > --- /dev/null
> > +++ b/rust/helpers/kref.c
> > @@ -0,0 +1,13 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <linux/kref.h>
> > +
> > +void rust_helper_kref_get(struct kref *kref)
> > +{
> > + kref_get(kref);
> > +}
> > +
> > +void rust_helper_kref_put(struct kref *kref, void (*release)(struct kref *kref))
> > +{
> > + kref_put(kref, release);
> > +}
>
> I am about to send out my v3, and this patch will no longer be needed.
> Would it make sense to send this out separately? I can also just keep it
> stashed away till someone needs it.
>
I think you just drop this, we will have the Refcount abstraction:
https://lore.kernel.org/rust-for-linux/20250622125802.3224264-1-gary@kernel.org/
, and we can build an kref API on top of that.
Thanks!
Regards,
Boqun
> Thanks,
> Rahul Rameshbabu
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 3/4] rust: core abstractions for HID drivers
2025-07-13 21:57 ` Miguel Ojeda
@ 2025-07-20 16:44 ` Rahul Rameshbabu
0 siblings, 0 replies; 12+ messages in thread
From: Rahul Rameshbabu @ 2025-07-20 16:44 UTC (permalink / raw)
To: Miguel Ojeda
Cc: rust-for-linux, linux-input, Jiri Kosina, a.hindborg, alex.gaynor,
aliceryhl, benno.lossin, Benjamin Tissoires, bjorn3_gh,
boqun.feng, dakr, db48x, gary, ojeda, tmgross, peter.hutterer
On Sun, 13 Jul, 2025 23:57:38 +0200 "Miguel Ojeda" <miguel.ojeda.sandonis@gmail.com> wrote:
>> + /// Returns the HID report group.
>> + pub fn group(&self) -> u16 {
>> + // SAFETY: `self.as_raw` is a valid pointer to a `struct hid_device`
>> + unsafe { *self.as_raw() }.group
>> + }
>
> Do you need to expose the group as a raw integer?
>
> Also, are these used currently?
Hi Miguel,
I originally did not expose the report group in the Rust bindings since
I did not make use of them. Benjamin has a preference access to them be
exposed in the initial bindings. I exposed this before implementing the
enum in a later revision. I agree that we should wrap this using the
enum now.
Thanks,
Rahul Rameshbabu
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2025-07-20 16:44 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-13 21:11 [PATCH v2 0/4] Initial work for Rust abstraction for HID device driver development Rahul Rameshbabu
2025-07-13 21:11 ` [PATCH v2 1/4] HID: core: Change hid_driver to use a const char* for name Rahul Rameshbabu
2025-07-13 21:12 ` [PATCH v2 2/4] rust: add kref bindings Rahul Rameshbabu
2025-07-20 16:02 ` Rahul Rameshbabu
2025-07-20 16:07 ` Boqun Feng
2025-07-13 21:12 ` [PATCH v2 3/4] rust: core abstractions for HID drivers Rahul Rameshbabu
2025-07-13 21:57 ` Miguel Ojeda
2025-07-20 16:44 ` Rahul Rameshbabu
2025-07-15 15:04 ` Danilo Krummrich
2025-07-19 22:46 ` Rahul Rameshbabu
2025-07-13 21:12 ` [PATCH v2 4/4] rust: hid: Glorious Gaming PC Race Model O and O- mice reference driver Rahul Rameshbabu
2025-07-13 21:46 ` Miguel Ojeda
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).