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
>>
>>
