Received-SPF: pass (google.com: domain of linux-kernel+bounces-208761-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Feedback-ID: iad51458e:Fastmail
Date: Mon, 10 Jun 2024 11:38:07 -0700
From: Boqun Feng <boqun.feng@gmail.com>
To: Danilo Krummrich <dakr@redhat.com>
Cc: gregkh@linuxfoundation.org, rafael@kernel.org, mcgrof@kernel.org,
	russell.h.weight@intel.com, ojeda@kernel.org, alex.gaynor@gmail.com,
	wedsonaf@gmail.com, gary@garyguo.net, bjorn3_gh@protonmail.com,
	benno.lossin@proton.me, a.hindborg@samsung.com,
	aliceryhl@google.com, airlied@gmail.com, fujita.tomonori@gmail.com,
	pstanner@redhat.com, ajanulgu@redhat.com, lyude@redhat.com,
	rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/2] rust: add abstraction for struct device
Message-ID: <ZmdID8AlXtoxUfC1@boqun-archlinux>
References: <20240610180318.72152-1-dakr@redhat.com>
 <20240610180318.72152-2-dakr@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20240610180318.72152-2-dakr@redhat.com>

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:

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

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
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.

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
>