| Message ID | 20240415104701.4772-4-fujita.tomonori@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: add Applied Micro QT2025 PHY driver | expand |
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
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 I fully agree with GregKH here. You should be writing a generic Rust abstraction around firmware loading any Linux driver can use. There should not be anything PHY specific in it. Andrew
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. > ///
Hi, On Mon, 15 Apr 2024 13:10:59 +0200 Greg KH <gregkh@linuxfoundation.org> wrote: > 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. Sure. I had a version of creating rust/kernel/firmware.rs but I wanted to know if a temporary solution could be accepted. With the build system for Rust, we can't put it in drivers/base/ yet. >> 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 guaranteed. The above `Device` is phy::Device. > That's not how the firmware api works, use a real 'struct device' please. Right, I'll do in v2. > >> + 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? Oops, can be simpler: let mut ptr: *const bindings::firmware = ptr::null_mut(); let ret = unsafe { bindings::request_firmware(&mut ptr, name.as_char_ptr().cast(), &mut (*phydev).mdio.dev) }; >> + // 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? In the C side: struct firmware { size_t size; const u8 *data; /* firmware loader private fields */ void *priv; }; data() function allows a driver in Rust to access to the above data member in Rust. A driver could define its own structure over &[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) } > > If new fails, does accessing this also fail? If a new() fails, a Firmware object isn't created. So data() function cannot be called. > 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? Yes, like data() function.
On Thu, Apr 18, 2024 at 09:51:08PM +0900, FUJITA Tomonori wrote: > Hi, > > On Mon, 15 Apr 2024 13:10:59 +0200 > Greg KH <gregkh@linuxfoundation.org> wrote: > > > 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. > > Sure. I had a version of creating rust/kernel/firmware.rs but I wanted > to know if a temporary solution could be accepted. > > With the build system for Rust, we can't put it in drivers/base/ yet. What is the status of fixing that?
On Thu, Apr 18, 2024 at 09:51:08PM +0900, FUJITA Tomonori wrote: > >> 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 guaranteed. The above `Device` is phy::Device. How are we supposed to know that from reading this diff? I think you all need to work on naming things better, as that's going to get _VERY_ confusing very quickly. thanks, greg k-h
Hi, On Mon, 15 Apr 2024 17:45:46 +0200 Danilo Krummrich <dakr@redhat.com> wrote: > 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. Yeah, I have a version of creating rust/kernel/firmware.rs locally but I wanted to know if a temporary solution could be accepted. > 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) } > } > } My implementation uses RawDevice trait in old rust branch (Wedson implemented, I suppose): https://github.com/Rust-for-Linux/linux/blob/18b7491480025420896e0c8b73c98475c3806c6f/rust/kernel/device.rs#L37 pub unsafe trait RawDevice { /// Returns the raw `struct device` related to `self`. fn raw_device(&self) -> *mut bindings::device; Which is better?
Hi, On Thu, 18 Apr 2024 15:07:19 +0200 Greg KH <gregkh@linuxfoundation.org> wrote: > On Thu, Apr 18, 2024 at 09:51:08PM +0900, FUJITA Tomonori wrote: >> >> 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 guaranteed. The above `Device` is phy::Device. > > How are we supposed to know that from reading this diff? I think you > all need to work on naming things better, as that's going to get _VERY_ > confusing very quickly. I guess that usually a path to a module prevent confusion. For example, a MAC driver could look like: fn probe(dev: &mut pci::Device, _: pci::DeviceId) -> Result { let netdev = net::Device::new()?; let phydev = phy::Device::new()?; ... Ok(()) }
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. ///
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(+)