Re: [PATCH v2 1/2] rust: add abstraction for struct device

[Danilo Krummrich <dakr@redhat.com>]

On Mon, Jun 10, 2024 at 11:38:07AM -0700, Boqun Feng wrote:
> On Mon, Jun 10, 2024 at 08:02:27PM +0200, Danilo Krummrich wrote:
> [...]
> > +/// A reference-counted device.
> > +///
> > +/// This structure represents the Rust abstraction for a C `struct device`. This implementation
> > +/// abstracts the usage of an already existing C `struct device` within Rust code that we get
> > +/// passed from the C side.
> > +///
> > +/// An instance of this abstraction can be obtained temporarily or permanent.
> > +///
> > +/// A temporary one is bound to the lifetime of the C `struct device` pointer used for creation.
> > +/// A permanent instance is always reference-counted and hence not restricted by any lifetime
> > +/// boundaries.
> > +///
> > +/// For subsystems it is recommended to create a permanent instance to wrap into a subsystem
> > +/// specifc device structure (e.g. `pci::Device`). This is useful for passing it to drivers in
> > +/// `T::probe()`, such that a driver can store the `ARef<Device>` (equivalent to storing a
> > +/// `struct device` pointer in a C driver) for arbitrary purposes, e.g. allocating DMA coherent
> > +/// memory.
> > +///
> > +/// # Invariants
> > +///
> > +/// The pointer stored in `Self` is non-null and valid for the lifetime of the `ARef` instance. In
> > +/// particular, the `ARef` instance owns an increment on the underlying object’s reference count.
> > +#[repr(transparent)]
> > +pub struct Device(Opaque<bindings::device>);
> > +
> [...]
> > +
> > +// SAFETY: `Device` only holds a pointer to a C `struct device`, which is safe to be used from any
> > +// thread.
> > +unsafe impl Send for Device {}
> > +
> > +// SAFETY: `Device` only holds a pointer to a C `struct device`, references to which are safe to be
> > +// used from any thread.
> > +unsafe impl Sync for Device {}
> 
> These comments need some rework, `Device` is not a pointer to `struct
> device` anymore. For the `Sync` one, how about:

Indeed, I forgot to update them.

> 
> // SAFETY: `Device` can be shared among threads because all immutable
> // methods are protected by the synchronization in `struct device`.
> unsafe impl Sync for Device {}

Sounds good.

> 
> and for `Send`, I actually don't think we can easily say the generic
> `Device` is `Send`: you can create a `struct device` where `->release`
> requires to be run on the same thread that creates the `device`, and
> nothing is wrong about it, I think (e.g. making a thread be the sole

Hm, I guess in this case we actually can't argue that it's the owners fault to
pass a pointer of this device somewhere else. Since it's C the owner can't
enforce that someone else is not taking a reference and prevent sharing
ownership...

> owner of some special devices). Unless, in the #Invariants of `Device`,
> and the #safety of `from_ptr`, you mention that `Device` assume its
> `->release` can be called on any thread.

...hence, I agree we should indeed add to the #Invariants and #Safety section
that `->release` must be callable  from any thread.

However, this is just theory, do we actually have cases where `device::release`
is not allowed to be called from any thread? If so, this would be very confusing
for a reference counted type from a design point of view...

- Danilo

> 
> Regards,
> Boqun
> 
> > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> > index fbd91a48ff8b..dd1207f1a873 100644
> > --- a/rust/kernel/lib.rs
> > +++ b/rust/kernel/lib.rs
> > @@ -28,6 +28,7 @@
> >  
> >  pub mod alloc;
> >  mod build_assert;
> > +pub mod device;
> >  pub mod error;
> >  pub mod init;
> >  pub mod ioctl;
> > -- 
> > 2.45.1
> > 
>