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

[Danilo Krummrich <dakr@redhat.com>]

On Thu, May 30, 2024 at 01:24:33PM +0900, FUJITA Tomonori wrote:
> Hi,
> 
> On Thu, 30 May 2024 04:01:39 +0200
> Danilo Krummrich <dakr@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@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.

I don't see how this is different than what we do in C. If you (for whatever
reason) want to keep a pointer to a struct device you should make sure to
increase the reference count of this device, such that it can't get freed for
the time being.

This is a 1:1 representation of that and conceptually identical.

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

No, saying that the caller must ensure that the device "has a non-zero reference
count" is a perfectly valid requirement.

It doensn't mean that the calling code has to peek the actual reference count,
but it means that it must be ensured that while we try to increase the reference
count it can't drop to zero unexpectedly.

Your patch, as a subset of this one, has the same requirements as listed below.

> 
> +    /// 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() }
> +    }
>