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

[Danilo Krummrich <dakr@kernel.org>]

On Sat, May 31, 2025 at 02:10:08PM +0200, Benno Lossin wrote:
> 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 :)

The data returned by open() can be anything. It can also be some arbitrary
Arc<T> that already exists and is looked up in open(). It can also be something
new that is created within open() and requires in-place initialization.

So, if we want to change this, we could return an `impl PinInit<Self, Error>` as
you suggest above and initialize it in-place in
`RawDeviceRegistration::private`.

I agree that this is the correct thing to do, but that really sounds like a
subsequent patch.