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

[Greg KH <gregkh@linuxfoundation.org>]

On Wed, May 29, 2024 at 09:28:21AM +0900, FUJITA Tomonori wrote:
> Hi,
> 
> On Tue, 28 May 2024 14:45:02 +0200
> Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > On Tue, May 28, 2024 at 02:19:24PM +0200, Danilo Krummrich wrote:
> >> However, if you have a driver that needs the firmware abstractions, I would be
> >> surprised if there were any hesitations to already merge the minimum device
> >> abstractions [1] and this one (once reviewed) without the rest. At least there
> >> aren't any from my side.
> >> 
> >> [1] https://lore.kernel.org/rust-for-linux/20240520172554.182094-2-dakr@redhat.com/
> > 
> > No, the device abstractions are NOT ready for merging just yet, sorry,
> > if for no other reason than a non-RFC has never been posted :)
> 
> Indeed, I understand that you aren't convinced.
> 
> We can start with much simpler device abstractions than the above
> minimum for the firmware abstractions.
> 
> All the firmware abstractions need is wrapping C's pointer to a device
> object with Rust struct only during a caller knows the pointer is
> valid. No play with the reference count of a struct device.

Makes sense, but note that almost no one should be dealing with "raw"
struct device pointers :)

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

thanks,

greg k-h