Re: [RFC PATCH 7/8] rust: add firmware abstractions

[Danilo Krummrich <dakr@redhat.com>]

On Fri, Jun 07, 2024 at 09:11:32PM +0900, FUJITA Tomonori wrote:
> Hi,
> 
> On Fri, 31 May 2024 11:59:47 +0200
> Danilo Krummrich <dakr@redhat.com> wrote:
> 
> > Once we get to a conclusion I can send a series with only the device and firmare
> > abstractions such that we can get them in outside of the scope of the reset of
> > both series to get your driver going.
> 
> Since your discussion with Greg seems to continue for a while, let me
> include the following patch that Greg approved with the next version
> of the PHY driver patchset.

This all doesn't make a lot of sense. If that's the case, Greg approved
something the he keeps arguing about in the discussion of the original patch.
Please see the discussion about the NULL pointer check [1].

Besides that, I really don't think it's the correct approach to just (partially)
pick up a patch from the mailing list that is actively discussed and submit
versions that are slightly altered in the points that are actively discussed.

This really just complicates the situation and does not help getting to an
agreement.

> 
> You have the new version of the firmware patch? The unused functions
> will not be merged so only request_firmware() and release_firmware()
> can be included. If not, I can include my firmware patch that I wrote
> before.
> 

Please find the updated firmware patch in [2].

However, I propose to just send a new series with just the device abstraction
and this firmware patch and proceed from there.

[1] https://lore.kernel.org/rust-for-linux/Zl8_bXqK-T24y1kp@cassiopeiae/
[2] https://gitlab.freedesktop.org/drm/nova/-/commit/0683e186929c4922d565e5315525925aa2d8d686

NAK for the patch below, for the reasons above.

Please also see comments inline.

> =
> From: Danilo Krummrich <dakr@redhat.com>
> Date: Fri, 7 Jun 2024 20:14:59 +0900
> Subject: [PATCH] add abstraction for struct device
> 
> Add abstraction for struct device. This implements only the minimum
> necessary functions required for the abstractions of firmware API;
> that is, wrapping C's pointer to a device object with Rust struct only
> during a caller knows the pointer is valid (e.g., the probe callback).
> 
> 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>
> Co-developed-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  rust/kernel/device.rs | 31 +++++++++++++++++++++++++++++++
>  rust/kernel/lib.rs    |  1 +
>  2 files changed, 32 insertions(+)
>  create mode 100644 rust/kernel/device.rs
> 
> diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> new file mode 100644
> index 000000000000..55ec4f364628
> --- /dev/null
> +++ b/rust/kernel/device.rs
> @@ -0,0 +1,31 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Generic devices that are part of the kernel's driver model.
> +//!
> +//! C header: [`include/linux/device.h`](srctree/include/linux/device.h)
> +
> +use crate::types::Opaque;
> +
> +/// Wraps the kernel's `struct task_struct`.
> +#[repr(transparent)]
> +pub struct Device(Opaque<bindings::device>);
> +
> +impl Device {
> +    /// Creates a new [`Device`] instance from a raw pointer.
> +    ///
> +    /// # Safety
> +    ///
> +    /// For the duration of 'a, the pointer must point at a valid `device`.

The original patch says: "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."

This is way more precise than just saying "For the duration of 'a, the pointer
must point at a valid `device`.", hence we should keep the original comment.

> +    pub unsafe fn from_raw<'a>(ptr: *mut bindings::device) -> &'a Self {
> +        // CAST: `Self` is a `repr(transparent)` wrapper around `bindings::device`.
> +        let ptr = ptr.cast::<Self>();
> +        // SAFETY: by the function requirements the pointer is valid and we have unique access for
> +        // the duration of `'a`.
> +        unsafe { &mut *ptr }

Why not just

+        // SAFETY: Guaranteed by the safety requirements of the function.
+        unsafe { &*ptr.cast() }

as in the original commit?

> +    }
> +
> +    /// Returns the raw pointer to the device.
> +    pub(crate) fn as_raw(&self) -> *mut bindings::device {
> +        self.0.get()
> +    }
> +}
> 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.34.1
>