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

[Greg KH <gregkh@linuxfoundation.org>]

On Mon, Apr 15, 2024 at 07:47:00PM +0900, 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 +++++++++++++++++++++++++++++++++

Please do not bury this in the phy.rs file, put it in drivers/base/ next
to the firmware functions it is calling.

>  3 files changed, 47 insertions(+)
> 
> 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();

That's risky, how do you "know" this device really is a phydev?  That's
not how the firmware api works, use a real 'struct device' please.


> +        let mut ptr: *mut bindings::firmware = ptr::null_mut();
> +        let p_ptr: *mut *mut bindings::firmware = &mut ptr;

I'm sorry, but I don't understand the two step thing here, you need a
pointer for where the C code can put something, is this really how you
do that in rust?  Shouldn't it point to data somehow down below?

> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Device`.

Again, phydev is not part of the firmware api.

> +        // So it's just an FFI call.
> +        let ret = unsafe {
> +            bindings::request_firmware(
> +                p_ptr as *mut *const bindings::firmware,

Where is this coming from?

> +                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] {

But firmware is not a u8, it's a structure.  Can't the rust bindings
handle that?  Oh wait, data is a u8, but the bindings should show that,
right?


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

If new fails, does accessing this also fail?

And how are you using this?  I guess I'll dig in the next patch...

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

So drop will never be called if new fails?

Again, please don't put this in the phy driver, put it where it belongs
so we can add the other firmware functions when needed.

thanks,

greg k-h