Re: [PATCH RFC 3/3] rust: hid: demo the core abstractions for probe and remove

[Daniel Brooks <db48x@db48x.net>]

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