| Message ID | 20250313160220.6410-6-sergeantsagara@protonmail.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | Initial work for Rust abstraction for HID device driver development | expand |
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. 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. 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@protonmail.com> > --- > 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 > >
On Thu, 13 Mar, 2025 18:05:36 +0100 "Benjamin Tissoires" <bentiss@kernel.org> 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@protonmail.com> >> --- >> 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 >> >>
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
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",
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. [ +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@protonmail.com> --- drivers/hid/hid_monitor_control.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)