Re: [PATCH v3 15/16] rust: platform: add basic platform device / driver abstractions

[Danilo Krummrich <dakr@kernel.org>]

On Thu, Oct 31, 2024 at 09:23:58AM +0100, Alice Ryhl wrote:
> On Wed, Oct 30, 2024 at 7:07 PM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > On Wed, Oct 30, 2024 at 04:50:43PM +0100, Alice Ryhl wrote:
> > > On Tue, Oct 22, 2024 at 11:33 PM Danilo Krummrich <dakr@kernel.org> wrote:
> > > > +/// Drivers must implement this trait in order to get a platform driver registered. Please refer to
> > > > +/// the `Adapter` documentation for an example.
> > > > +pub trait Driver {
> > > > +    /// The type holding information about each device id supported by the driver.
> > > > +    ///
> > > > +    /// TODO: Use associated_type_defaults once stabilized:
> > > > +    ///
> > > > +    /// type IdInfo: 'static = ();
> > > > +    type IdInfo: 'static;
> > > > +
> > > > +    /// The table of device ids supported by the driver.
> > > > +    const ID_TABLE: IdTable<Self::IdInfo>;
> > > > +
> > > > +    /// Platform driver probe.
> > > > +    ///
> > > > +    /// Called when a new platform device is added or discovered.
> > > > +    /// Implementers should attempt to initialize the device here.
> > > > +    fn probe(dev: &mut Device, id_info: Option<&Self::IdInfo>) -> Result<Pin<KBox<Self>>>;
> > >
> > > This forces the user to put their driver data in a KBox, but they
> > > might want to use an Arc instead. You don't actually *need* a KBox -
> > > any ForeignOwnable seems to fit your purposes.
> >
> > This is intentional, I do need a `KBox` here.
> >
> > The reason is that I want to enforce that the returned `Pin<KBox<Self>>` has
> > exactly the lifetime of the binding of the device and driver, i.e. from probe()
> > until remove(). This is the lifetime the structure should actually represent.
> >
> > This way we can attach things like `Registration` objects to this structure, or
> > anything else that should only exist from probe() until remove().
> >
> > If a driver needs some private driver data that needs to be reference counted,
> > it is usually attached to the class representation of the driver.
> >
> > For instance, in Nova the reference counted stuff is attached to the DRM device
> > and then I just have the DRM device (which itself is reference counted) embedded
> > in the `Driver` structure.
> >
> > In any case, drivers can always embed a separate `Arc` in their `Driver`
> > structure if they really have a need for that.
> 
> Is this needed for soundness of those registrations?

Yes, we must ensure that the class is unregistered before the driver is removed.

However, I just noticed that by letting class abstractions return the
`Registration` without being wrapped as `Devres<Registration>` the driver can
theoretically still stuff them in whatever other structure and keep the
registration alive.

So, maybe there is no way around `Devres` even for class registrations.

> 
> Also, I've often seen drivers use devm_kzalloc or similar to allocate
> exactly this object. KBox doesn't allow for that.
> 
> 
> Alice
>