Received-SPF: pass (google.com: domain of linux-kernel+bounces-184042-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Mon, 20 May 2024 20:00:23 +0200
From: Greg KH <gregkh@linuxfoundation.org>
To: Danilo Krummrich <dakr@redhat.com>
Cc: rafael@kernel.org, bhelgaas@google.com, ojeda@kernel.org,
	alex.gaynor@gmail.com, wedsonaf@gmail.com, boqun.feng@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, lina@asahilina.net, pstanner@redhat.com,
	ajanulgu@redhat.com, lyude@redhat.com,
	rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-pci@vger.kernel.org
Subject: Re: [RFC PATCH 01/11] rust: add abstraction for struct device
Message-ID: <2024052038-deviancy-criteria-e4fe@gregkh>
References: <20240520172554.182094-1-dakr@redhat.com>
 <20240520172554.182094-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: <20240520172554.182094-2-dakr@redhat.com>

On Mon, May 20, 2024 at 07:25:38PM +0200, Danilo Krummrich wrote:
> Add an (always) reference counted abstraction for a generic struct
> device. This abstraction encapsulates existing struct device instances
> and manages its reference count.
> 
> Subsystems may use this abstraction as a base to abstract subsystem
> specific device instances based on a generic struct device.
> 
> Co-developed-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> ---
>  rust/helpers.c        |  1 +
>  rust/kernel/device.rs | 76 +++++++++++++++++++++++++++++++++++++++++++

What's the status of moving .rs files next to their respective .c files
in the build system?  Keeping them separate like this just isn't going
to work, sorry.

> --- /dev/null
> +++ b/rust/kernel/device.rs
> @@ -0,0 +1,76 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Generic devices that are part of the kernel's driver model.
> +//!
> +//! C header: [`include/linux/device.h`](../../../../include/linux/device.h)

relative paths for a common "include <linux/device.h" type of thing?
Rust can't handle include paths from directories?

> +
> +use crate::{
> +    bindings,
> +    types::{ARef, Opaque},
> +};
> +use core::ptr;
> +
> +/// A ref-counted device.
> +///
> +/// # 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 underlying object’s reference count.
> +#[repr(transparent)]
> +pub struct Device(Opaque<bindings::device>);
> +
> +impl Device {
> +    /// Creates a new ref-counted instance of an existing device pointer.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers must ensure that `ptr` is valid, non-null, and has a non-zero reference count.

Callers NEVER care about the reference count of a struct device, anyone
poking in that is asking for trouble.

And why non-NULL?  Can't you check for that here?  Shouldn't you check
for that here?  Many driver core functions can handle a NULL pointer
just fine (i.e. get/put_device() can), why should Rust code assume that
a pointer passed to it from the C layer is going to have stricter rules
than the C layer can provide?

> +    pub unsafe fn from_raw(ptr: *mut bindings::device) -> ARef<Self> {
> +        // SAFETY: By the safety requirements, ptr is valid.
> +        // Initially increase the reference count by one to compensate for the final decrement once
> +        // this newly created `ARef<Device>` instance is dropped.
> +        unsafe { bindings::get_device(ptr) };
> +
> +        // CAST: `Self` is a `repr(transparent)` wrapper around `bindings::device`.
> +        let ptr = ptr.cast::<Self>();
> +
> +        // SAFETY: By the safety requirements, ptr is valid.
> +        unsafe { ARef::from_raw(ptr::NonNull::new_unchecked(ptr)) }
> +    }
> +
> +    /// Obtain the raw `struct device *`.
> +    pub(crate) fn as_raw(&self) -> *mut bindings::device {
> +        self.0.get()
> +    }
> +
> +    /// Convert a raw `struct device` pointer to a `&Device`.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers must ensure that `ptr` is valid, non-null, and has a non-zero reference count for
> +    /// the entire duration when the returned reference exists.

Again, non-NULL might not be true, and reference counts are never
tracked by any user EXCEPT to increment/decrement it, you never know if
it is 0 or not, all you know is that if a pointer is given to you by the
driver core to a 'struct device' for a function that it is a valid
reference at that point in time, or maybe NULL, until your function
returns.  Anything after that can not be counted on.

thanks,

greg k-h