Re: [PATCH net-next v1 3/4] rust: net::phy support Firmware API

[Danilo Krummrich <dakr@redhat.com>]

On 4/15/24 12:47, FUJITA Tomonori wrote:
> This patch adds support to the following basic Firmware API:
> 
> - request_firmware
> - release_firmware
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> CC: Luis Chamberlain <mcgrof@kernel.org>
> CC: Russ Weight <russ.weight@linux.dev>
> ---
>   drivers/net/phy/Kconfig         |  1 +
>   rust/bindings/bindings_helper.h |  1 +
>   rust/kernel/net/phy.rs          | 45 +++++++++++++++++++++++++++++++++
>   3 files changed, 47 insertions(+)

As Greg already mentioned, this shouldn't be implemented specifically for struct
phy_device, but rather for a generic struct device.

I already got some generic firmware abstractions [1][2] sitting on top of a patch
series adding some basic generic device / driver abstractions [3].

I won't send out an isolated version of the device / driver series, but the full
patch series for the Nova stub driver [4] once I got everything in place. This was
requested by Greg to be able to see the full picture.

The series will then also include the firmware abstractions.

In order to use them from your PHY driver, I think all you need to do is to implement
AsRef<> for your phy::Device:

impl AsRef<device::Device> for Device {
     fn as_ref(&self) -> &device::Device {
         // SAFETY: By the type invariants, we know that `self.ptr` is non-null and valid.
         unsafe { device::Device::from_raw(&mut (*self.ptr).mdio.dev) }
     }
}

- Danilo

[1] https://gitlab.freedesktop.org/drm/nova/-/commit/e9bb608206f3c30a0f8d71fe472719778a113b28
[2] https://gitlab.freedesktop.org/drm/nova/-/tree/topic/firmware
[3] https://github.com/Rust-for-Linux/linux/tree/staging/rust-device
[4] https://lore.kernel.org/dri-devel/Zfsj0_tb-0-tNrJy@cassiopeiae/T/#u

> 
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index 7fddc8306d82..3ad04170aa4e 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -64,6 +64,7 @@ config RUST_PHYLIB_ABSTRACTIONS
>           bool "Rust PHYLIB abstractions support"
>           depends on RUST
>           depends on PHYLIB=y
> +        depends on FW_LOADER=y
>           help
>             Adds support needed for PHY drivers written in Rust. It provides
>             a wrapper around the C phylib core.
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 65b98831b975..556f95c55b7b 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -9,6 +9,7 @@
>   #include <kunit/test.h>
>   #include <linux/errname.h>
>   #include <linux/ethtool.h>
> +#include <linux/firmware.h>
>   #include <linux/jiffies.h>
>   #include <linux/mdio.h>
>   #include <linux/phy.h>
> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
> index 421a231421f5..095dc3ccc553 100644
> --- a/rust/kernel/net/phy.rs
> +++ b/rust/kernel/net/phy.rs
> @@ -9,6 +9,51 @@
>   use crate::{bindings, error::*, prelude::*, str::CStr, types::Opaque};
>   
>   use core::marker::PhantomData;
> +use core::ptr::{self, NonNull};
> +
> +/// A pointer to the kernel's `struct firmware`.
> +///
> +/// # Invariants
> +///
> +/// The pointer points at a `struct firmware`, and has ownership over the object.
> +pub struct Firmware(NonNull<bindings::firmware>);
> +
> +impl Firmware {
> +    /// Loads a firmware.
> +    pub fn new(name: &CStr, dev: &Device) -> Result<Firmware> {
> +        let phydev = dev.0.get();
> +        let mut ptr: *mut bindings::firmware = ptr::null_mut();
> +        let p_ptr: *mut *mut bindings::firmware = &mut ptr;
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Device`.
> +        // So it's just an FFI call.
> +        let ret = unsafe {
> +            bindings::request_firmware(
> +                p_ptr as *mut *const bindings::firmware,
> +                name.as_char_ptr().cast(),
> +                &mut (*phydev).mdio.dev,
> +            )
> +        };
> +        let fw = NonNull::new(ptr).ok_or_else(|| Error::from_errno(ret))?;
> +        // INVARIANT: We checked that the firmware was successfully loaded.
> +        Ok(Firmware(fw))
> +    }
> +
> +    /// Accesses the firmware contents.
> +    pub fn data(&self) -> &[u8] {
> +        // SAFETY: The type invariants guarantee that `self.0.as_ptr()` is valid.
> +        // They also guarantee that `self.0.as_ptr().data` pointers to
> +        // a valid memory region of size `self.0.as_ptr().size`.
> +        unsafe { core::slice::from_raw_parts((*self.0.as_ptr()).data, (*self.0.as_ptr()).size) }
> +    }
> +}
> +
> +impl Drop for Firmware {
> +    fn drop(&mut self) {
> +        // SAFETY: By the type invariants, `self.0.as_ptr()` is valid and
> +        // we have ownership of the object so can free it.
> +        unsafe { bindings::release_firmware(self.0.as_ptr()) }
> +    }
> +}
>   
>   /// PHY state machine states.
>   ///