Re: [PATCH v3 1/2] rust: miscdevice: add additional data to MiscDeviceRegistration

[Boqun Feng <boqun.feng@gmail.com>]

On Sat, May 17, 2025 at 03:42:29PM +0200, Danilo Krummrich wrote:
> On Sat, May 17, 2025 at 01:33:49PM +0200, Christian Schrefl wrote:
> > +pub struct MiscDeviceRegistration<T: MiscDevice> {
> >      #[pin]
> >      inner: Opaque<bindings::miscdevice>,
> > +    #[pin]
> > +    data: UnsafePinned<T::RegistrationData>,
> >      _t: PhantomData<T>,
> >  }
> 
> I recommend not to store data within a Registration type itself.
> 
> I know that this is designed with the focus on using misc device directly from
> the module scope; and in this context it works great.
> 
> However, it becomes quite suboptimal when used from a driver scope. For
> instance, if the misc device is registered within a platform driver's probe()
> function.
> 
> I know this probably isn't supported yet. At least, I assume it isn't supported
> "officially", given that the abstraction does not provide an option to set a
> parent device. Yet I think we should consider it.
> 
> The reason this is suboptimal is that, from the callbacks of a misc device we
> may want to access device resources from the platform device.
> 
> Since device resources have to be protected with Devres, we'd need to access
> them with Revocable::try_access_with() for instance.
> 
> However, it would be much better if we had proof that the parent device of the
> misc device (i.e. the platform device) is bound (i.e. provide a &Device<Bound>)
> and hence are able to access device resources directly.
> 
> The only way to prove this, is to prove that the misc device registration is
> guaranteed to be removed when the parent device (i.e. the platform driver) is
> unbound.
> 
> And this we can only prove if we wrap MiscDeviceRegistration itself in a
> Devres; we don't want MiscDeviceRegistration to out-live the driver it was
> registered by anyways, so that's a free optimization.
> 
> If the data above is stored directly in the MiscDeviceRegistration however it
> means that we can only access it through a Devres<MiscDeviceRegistration>, which
> would be annoying.
> 
> To be fair, storing data in MiscDeviceRegistration is not the main issue of why
> this is suboptimal in driver, but it adds to the problem.
> 
> In general, the design of MiscDeviceRegistration is a bit suboptimal to be used
> within drivers. For drivers it works much better when the Registration type
> really *only* represents the state of a thing being registered, such that we can
> guard it with Devres *without* any downsides or additional complexity. One
> example for that would be the drm::driver::Registration [1].
> 
> If we want misc device to work optimally with drivers as well, we need to split
> things in two types: `misc::Device`:
> 
> 	struct Device<T: MiscDevice> {
> 	   #[pin]
> 	   misc: Opaque<bindings::miscdevice>,
> 	   #[pin]
> 	   data: UnsafePinned<T::RegistrationData>,
> 	   _t: PhantomData<T>,
> 	}
> 
> and `misc::Registration`:
> 
> 	struct Registration(ARef<misc::Device>);
> 
> 
> and make the `misc::Device` own the data, not the `misc::Registration`.
> 
> This way we can wrap misc::Registration into a Devres, with all guarantees it
> gives us and an no downsides.
> 
> I'm not saying that I want to block this patch, especially given that using the
> misc device abstraction doesn't seem to be supported to be used from drivers,
> but please understand that the design of the misc device abstraction, while it
> works fine for the module scope, really is sub-optimal for the use within
> drivers and hence should be re-worked.
> 
> Can we please either do the re-work right away or add a proper TODO?
> 

Well, I'd say we do the re-work right away, because I don't see any
other work depends on this right now. Let's do the right thing.

Regards,
Boqun

> [1] https://gitlab.freedesktop.org/drm/nova/-/blob/nova-next/rust/kernel/drm/driver.rs?ref_type=heads#L121