Message-ID: <87frjdhaii.fsf@protonmail.com>
Date: Sun, 16 Mar 2025 04:20:35 +0000
From: Rahul Rameshbabu <sergeantsagara@...tonmail.com>
To: Benjamin Tissoires <bentiss@...nel.org>
Cc: linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org, linux-input@...r.kernel.org, dri-devel@...ts.freedesktop.org, Jiri Kosina <jikos@...nel.org>, Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, Björn Roy Baron <bjorn3_gh@...tonmail.com>, Benno Lossin <benno.lossin@...ton.me>, Andreas Hindborg <a.hindborg@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>, Danilo Krummrich <dakr@...nel.org>
Subject: Re: [PATCH RFC 3/3] rust: hid: demo the core abstractions for probe and remove

On Thu, 13 Mar, 2025 18:05:36 +0100 "Benjamin Tissoires" <bentiss@...nel.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@...tonmail.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
>>
>>

Powered by blists - more mailing lists