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

[FUJITA Tomonori]

Hi,

On Thu, 30 May 2024 04:01:39 +0200
Danilo Krummrich <dakr at redhat.com> wrote:

> On Thu, May 30, 2024 at 08:28:24AM +0900, FUJITA Tomonori wrote:
>> Hi,
>> 
>> On Wed, 29 May 2024 21:57:03 +0200
>> Greg KH <gregkh at linuxfoundation.org> wrote:
>> 
>> >> For a Rust PHY driver, you know that you have a valid pointer to C's
>> >> device object of C's PHY device during the probe callback. The driver
>> >> creates a Rust device object to wrap the C pointer to the C's device
>> >> object and passes it to the firmware abstractions. The firmware
>> >> abstractions gets the C's pointer from the Rust object and calls C's
>> >> function to load firmware, returns the result.
>> >> 
>> >> You have concerns about the simple code like the following?
>> >> 
>> >> 
>> >> diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
>> >> new file mode 100644
>> >> index 000000000000..6144437984a9
>> >> --- /dev/null
>> >> +++ b/rust/kernel/device.rs
>> >> @@ -0,0 +1,30 @@
>> >> +// 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;
>> >> +
>> >> +#[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`.
>> > 
>> > If the following rust code does what this comment says, then sure, I'm
>> > ok with it for now if it helps you all out with stuff like the firmware
>> > interface for the phy rust code.
>> 
>> Great, thanks a lot!
>> 
>> Danilo and Wedson, are there any concerns about pushing this patch [1]
>> for the firmware abstractions?
> 
> Well, if everyone is fine with this one I don't see why we can't we go with [1]
> directly? AFAICS, we'd only need the following fix:
> 
> -//! C header: [`include/linux/device.h`](../../../../include/linux/device.h)
> +//! C header: [`include/linux/device.h`](srctree/include/linux/device.h)
> 
> [1] https://lore.kernel.org/rust-for-linux/20240520172554.182094-2-dakr@redhat.com/

The difference is that your patch touches the reference count of a
struct device. My patch doesn't.

The following part in your patch allows the rust code to freely play
with the reference count of a struct device. Your Rust drm driver
interact with the reference count in a different way than C's drm
drivers if I understand correctly. I'm not sure that Greg will be
convinenced with that approach.

+// SAFETY: Instances of `Device` are always ref-counted.
+unsafe impl crate::types::AlwaysRefCounted for Device {
+    fn inc_ref(&self) {
+        // SAFETY: The existence of a shared reference guarantees that the refcount is nonzero.
+        unsafe { bindings::get_device(self.as_raw()) };
+    }
+
+    unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
+        // SAFETY: The safety requirements guarantee that the refcount is nonzero.
+        unsafe { bindings::put_device(obj.cast().as_ptr()) }
+    }
+}

The following comments give the impression that Rust abstractions
wrongly interact with the reference count; callers check out the
reference counter. Nobody should do that.

+    /// Callers must ensure that `ptr` is valid, non-null, and has a non-zero reference count.
+    pub unsafe fn from_raw(ptr: *mut bindings::device) -> ARef<Self> {

+    /// 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.
+    pub unsafe fn as_ref<'a>(ptr: *mut bindings::device) -> &'a Self {
+        // SAFETY: Guaranteed by the safety requirements of the function.
+        unsafe { &*ptr.cast() }
+    }