Re: [PATCH v4 2/3] rust: miscdevice: add additional data to MiscDeviceRegistration

["Benno Lossin" <lossin@kernel.org>]

On Thu Jun 5, 2025 at 6:52 PM CEST, Christian Schrefl wrote:
> On 05.06.25 6:05 PM, Benno Lossin wrote:
>> On Thu Jun 5, 2025 at 4:57 PM CEST, Christian Schrefl wrote:
>>> On 04.06.25 1:29 AM, Benno Lossin wrote:
>>>> On Mon Jun 2, 2025 at 11:16 PM CEST, Christian Schrefl wrote:
>>>>> On 31.05.25 2:23 PM, Benno Lossin wrote:
>>>>>> On Fri May 30, 2025 at 10:46 PM CEST, Christian Schrefl wrote:
>>>>>>>  #[pinned_drop]
>>>>>>> -impl<T> PinnedDrop for MiscDeviceRegistration<T> {
>>>>>>> +impl<T: MiscDevice> PinnedDrop for MiscDeviceRegistration<T> {
>>>>>>>      fn drop(self: Pin<&mut Self>) {
>>>>>>>          // SAFETY: We know that the device is registered by the type invariants.
>>>>>>>          unsafe { bindings::misc_deregister(self.inner.get()) };
>>>>>>> +
>>>>>>> +        // SAFETY: `self.data` is valid for dropping and nothing uses it anymore.
>>>>>>
>>>>>> Ditto.
>>>>>
>>>>> I'm not quite sure how to formulate these, what do you think of:
>>>>>
>>>>> /// - `inner` is a registered misc device.
>>>>
>>>> This doesn't really mean something to me, maybe it's better to reference
>>>> the registering function?
>>>
>>> That is from previous code so this should probably not be changed
>>> in this series.
>> 
>> I personally wouldn't mind a commit that fixes this up, but if you don't
>> want to do it, let me know then we can make this a good-first-issue.
>
> I can do it, but I think it would make a good-first-issue so lets go
> with that for now.

Feel free to open the issue :)

>>>>> /// - `data` contains a valid `T::RegistrationData` for the whole lifetime of [`MiscDeviceRegistration`]
>>>>
>>>> This sounds good. But help me understand, why do we need `Opaque` /
>>>> `UnsafePinned` again? If we're only using shared references, then we
>>>> could also just store the object by value?
>>>
>>> Since the Module owns the `MiscDeviceRegistration` it may create `&mut MiscDeviceRegistration`,
>>> so from what I understand having a `& RegistrationData` reference into that is UB without
>>> `UnsafePinned` (or `Opaque` since that includes `UnsafePinned` semantics).
>> 
>> And the stored `T::RegistrationData` is shared as read-only with the C
>> side? Yes in that case we want `UnsafePinned<UnsafeCell<>>` (or for the
>> moment `Opaque`).
>
> Not really shared with the C side, but with the `open` implementation in
> `MiscDevice` that is (indirectly) called by C. (`UnsafeCell` will probably not be
> needed, as `UnsafePinned` will almost certainly have `UnsafeCell` semantics in upstream).

Ah yes, I meant "shared with other Rust code through the C side" ie the
pointer round-trips through C (that isn't actually relevant, but that's
why I mentioned C).

> Thinking about this has made me realize that the current code already is a bit
> iffy, since `MiscDevice::open` gets `&MiscDeviceRegistration<Self>` as an argument. (It
> should be fine since `UnsafeCell` and `UnsafePinned` semantics also apply to "parrent" types
> i.e. `&MiscDeviceRegistration` also has the semantics of `Opaque`).

It's fine, since all non-ZST fields are `Opaque`. Otherwise we'd need to
wrap all fields with that.

>>>>> /// - no mutable references to `data` may be created.
>>>>
>>>>>>> +        unsafe { core::ptr::drop_in_place(self.data.get()) };
>>>>>>>      }
>>>>>>>  }
>>>>>>>  
>>>>>>> @@ -109,6 +135,13 @@ pub trait MiscDevice: Sized {
>>>>>>>      /// What kind of pointer should `Self` be wrapped in.
>>>>>>>      type Ptr: ForeignOwnable + Send + Sync;
>>>>>>>  
>>>>>>> +    /// The additional data carried by the [`MiscDeviceRegistration`] for this [`MiscDevice`].
>>>>>>> +    /// If no additional data is required than the unit type `()` should be used.
>>>>>>> +    ///
>>>>>>> +    /// This data can be accessed in [`MiscDevice::open()`] using
>>>>>>> +    /// [`MiscDeviceRegistration::data()`].
>>>>>>> +    type RegistrationData: Sync;
>>>>>>
>>>>>> Why do we require `Sync` here?
>>>>>
>>>>> Needed for `MiscDeviceRegistration` to be `Send`, see response above.
>>>>
>>>> You could also just ask the type there to be `Sync`, then users will get
>>>> an error when they try to use `MiscDevice` in a way where
>>>> `RegistrationData` is required to be `Sync`.
>>>
>>> I don't think there is any point to allow defining a `MiscDevice` implementation
>>> that cant actually be used/registered.
>> 
>> Sure, but the bound asserting that it is `Sync` doesn't need to be here,
>> having it just on the `impl Sync for MiscDeviceRegistration` is good
>> enough. (though one could argue that people would get an earlier error
>> if it is already asserted here. I think we should have some general
>> guidelines here :)
>
> That would require a `Send` bound in the `register` function,
> since a `MiscDevice` with `!Sync` `Data` would be valid now
> (meaning that `MiscDeviceRegistration` may also be `!Sync`).
>
> If you want I can go with that. I'm not really sure if its
> really better (tough I don't feel that strongly either
> way).

We don't lose anything by doing this, so I think we should do it.
If in the future someone invents a way `MiscDevice` that's only in the
current thread and it can be registered (so like a "thread-local"
`MiscDevice` :), then this will be less painful to change.

---
Cheers,
Benno