Re: [PATCH 6/7] rust: miscdevice: expose the parent device as &Device<Bound>

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

On Sat May 31, 2025 at 12:46 PM CEST, Danilo Krummrich wrote:
> On Sat, May 31, 2025 at 10:27:44AM +0200, Benno Lossin wrote:
>> On Fri May 30, 2025 at 4:24 PM CEST, Danilo Krummrich wrote:
>> > @@ -227,11 +229,21 @@ fn drop(self: Pin<&mut Self>) {
>> >      }
>> >  }
>> >  
>> > +/// The arguments passed to the file operation callbacks of a [`MiscDeviceRegistration`].
>> > +pub struct MiscArgs<'a, T: MiscDevice> {
>> > +    /// The [`Device`] representation of the `struct miscdevice`.
>> > +    pub device: &'a Device,
>> > +    /// The parent [`Device`] of [`Self::device`].
>> > +    pub parent: Option<&'a Device<Bound>>,
>> > +    /// The `RegistrationData` passed to [`MiscDeviceRegistration::register`].
>> > +    pub data: &'a T::RegistrationData,
>> 
>> Here I would also just use `T`, remove the `MiscDevice` bound and then
>> use `MiscArgs<'_, Self::RegistrationData>` below.
>
> It has the disadvantage that the documentation of the `data` field above needs
> to be much more vague, since we can't claim that it's the `RegistrationData`
> passed to `MiscDeviceRegistration::register` anymore -- given that, I'm not sure
> it's worth changing.

Yeah that's not ideal... Then keep it this way.

>> > +}
>> > +
>> >  /// Trait implemented by the private data of an open misc device.
>> >  #[vtable]
>> >  pub trait MiscDevice: Sized {
>> >      /// What kind of pointer should `Self` be wrapped in.
>> > -    type Ptr: ForeignOwnable + Send + Sync;
>> > +    type Ptr: Send + Sync;
>> 
>> There is no info about this change in the commit message. Why are we
>> changing this? This seems a bit orthogonal to the other change, maybe do
>> it in a separate patch?
>
> It's a consequence of the implementation:
>
> A `Ptr` instance is created in the misc device's file operations open() callback
> and dropped in the fops release() callback.
>
> Previously, this was stored in the private data pointer of the struct file that
> is passed for every file operation in open().
>
> Also note that when open is called the private data pointer in a struct file
> points to the corresponding struct miscdevice.
>
> With this patch, we keep the pointer to the struct miscdevice in the private
> data pointer of struct file, but instead store the `Ptr` instance in
> `RawDeviceRegistration::private`.
>
> Subsequently, ForeignOwnable is not a required trait anymore.

That's true, but it also wouldn't hurt to keep it for this patch and do
the change in a separate one. Or mention it in the commit message :)

> We need this in order to keep access to the `RawDeviceRegistration` throughout
> file operations to be able to pass the misc device's parent as &Device<Bound>
> through the `MiscArgs` above.
>
>> Also `Ptr` doesn't make much sense for the name, since now that the
>> `ForeignOwnable` bound is gone, I could set this to `Self` and then have
>> access to `&Self` in the callbacks.
>
> We can't make it `Self`, it might still be some pointer type, require pin-init,
> etc. So, it has to be a generic type.

`MiscDevice::open` could return an `impl PinInit<Self, Error>` :)

> But, I agree that we should not name it `Ptr`, probably should never have been
> named `Ptr`, but `Data`, `Private` or similar.
>
>> Would that also make sense to use as a general change? So don't store
>> `Self::Ptr`, but `Self` directly?
>
> I think it can't be `Self`, see above.

The rust_misc_device example would still work if we changed this to
`Self`. Now it's not a complicated user of the API and someone might
want to store `Self` in an `Arc` and then store that as the private
data, as the MiscDevice is also referenced from somewhere else. But I
don't know if that is common or an intended use-case :)

For simple use-cases however, I think that `Self` definitely is the
right choice (as opposed to `Pin<KBox<Self>>` for example, as that has
an extra allocation :)

---
Cheers,
Benno