On Thu, 13 Mar, 2025 18:05:36 +0100 "Benjamin Tissoires" <bentiss@xxxxxxxxxx> wrote: > On Mar 13 2025, Rahul Rameshbabu wrote: >> This is a very basic "hello, world!" implementation to illustrate that the >> probe and remove callbacks are working as expected. I chose an arbitrary >> device I had on hand for populating in the HID device id table. > > Nice to see this live :) > > Though as I mentioned in the previous patch, I'd prefer having one full > driver in a single patch and one separate patch with the skeleton in the > docs. > > Do you have any meaningful processing that needs to be done in the > report fixup or in the .raw_event or .event callbacks? > > It would be much more interesting to show how this works together on a > minimalistic driver. Agreed. Have a discussion about a device to potentially use for such a reference driver in another thread[1]. > > FWIW, the driver in itself, though incomplete, looks familiar, which is > a good thing: we've got a probe and a remove. This is common with all > the other HID drivers, so it's not a brand new thing. > > 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. One issue with the device I proposed in [1] is that it would not require the implementation of remove() or automation through Rust since the device is so "simple". 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. 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. [1] https://lore.kernel.org/rust-for-linux/Z9MxI0u2yCfSzTvD@cassiopeiae/T/#m87f121ec72ba159071b20dccb4071cd7d2398050 [2] https://doc.rust-lang.org/std/ops/trait.Drop.html Thanks for the review, Rahul Rameshbabu > > Cheers, > Benjamin > >> >> [ +0.012968] monitor_control: Probing HID device vendor: 2389 product: 29204 using Rust! >> [ +0.000108] monitor_control: Removing HID device vendor: 2389 product: 29204 using Rust! >> >> Signed-off-by: Rahul Rameshbabu <sergeantsagara@xxxxxxxxxxxxxx> >> --- >> drivers/hid/hid_monitor_control.rs | 9 +++++++-- >> 1 file changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/hid/hid_monitor_control.rs b/drivers/hid/hid_monitor_control.rs >> index 18afd69a56d5..aeb6e4058a6b 100644 >> --- a/drivers/hid/hid_monitor_control.rs >> +++ b/drivers/hid/hid_monitor_control.rs >> @@ -8,17 +8,22 @@ >> Driver, >> }; >> >> +const USB_VENDOR_ID_NVIDIA: u32 = 0x0955; >> +const USB_DEVICE_ID_NVIDIA_THUNDERSTRIKE_CONTROLLER: u32 = 0x7214; >> + >> struct HidMonitorControl; >> >> #[vtable] >> impl Driver for HidMonitorControl { >> fn probe(dev: &mut hid::Device, id: &hid::DeviceId) -> Result<()> { >> /* TODO implement */ >> + pr_info!("Probing HID device vendor: {} product: {} using Rust!\n", id.vendor(), id.product()); >> Ok(()) >> } >> >> fn remove(dev: &mut hid::Device) { >> /* TODO implement */ >> + pr_info!("Removing HID device vendor: {} product: {} using Rust!\n", dev.vendor(), dev.product()); >> } >> } >> >> @@ -26,8 +31,8 @@ fn remove(dev: &mut hid::Device) { >> driver: HidMonitorControl, >> id_table: [ >> kernel::usb_device! { >> - vendor: /* TODO fill in */, >> - product: /* TODO fill in */, >> + vendor: USB_VENDOR_ID_NVIDIA, >> + product: USB_DEVICE_ID_NVIDIA_THUNDERSTRIKE_CONTROLLER, >> }, >> ], >> name: "monitor_control", >> -- >> 2.47.2 >> >>