[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <20231011.231607.1747074555988728415.fujita.tomonori@gmail.com>
Date: Wed, 11 Oct 2023 23:16:07 +0900 (JST)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: benno.lossin@...ton.me
Cc: fujita.tomonori@...il.com, netdev@...r.kernel.org,
rust-for-linux@...r.kernel.org, andrew@...n.ch,
miguel.ojeda.sandonis@...il.com, greg@...ah.com, tmgross@...ch.edu
Subject: Re: [PATCH net-next v3 1/3] rust: core abstractions for network
PHY drivers
On Mon, 09 Oct 2023 12:19:54 +0000
Benno Lossin <benno.lossin@...ton.me> wrote:
I skipped the topics that you've already discussed with Andrew.
> On 09.10.23 03:39, FUJITA Tomonori wrote:
>> This patch adds abstractions to implement network PHY drivers; the
>> driver registration and bindings for some of callback functions in
>> struct phy_driver and many genphy_ functions.
>>
>> This feature is enabled with CONFIG_RUST_PHYLIB_BINDINGS.
>>
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@...il.com>
>> ---
>> init/Kconfig | 8 +
>> rust/Makefile | 1 +
>> rust/bindings/bindings_helper.h | 3 +
>> rust/kernel/lib.rs | 3 +
>> rust/kernel/net.rs | 6 +
>> rust/kernel/net/phy.rs | 733 ++++++++++++++++++++++++++++++++
>> 6 files changed, 754 insertions(+)
>> create mode 100644 rust/kernel/net.rs
>> create mode 100644 rust/kernel/net/phy.rs
(snip)
>> +impl Device {
>> + /// Creates a new [`Device`] instance from a raw pointer.
>> + ///
>> + /// # Safety
>> + ///
>> + /// For the duration of the lifetime 'a, the pointer must be valid for writing and nobody else
>> + /// may read or write to the `phy_device` object.
>> + pub unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
>> + unsafe { &mut *ptr.cast() }
>
> Missing `SAFETY` comment.
Added:
// SAFETY: The safety requirements guarantee the validity of the dereference, while the
// `Device` type being transparent makes the cast ok.
>> + /// Gets the id of the PHY.
>> + pub fn phy_id(&mut self) -> u32 {
>> + let phydev = self.0.get();
>> + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
>> + unsafe { (*phydev).phy_id }
>> + }
>> +
>> + /// Gets the state of the PHY.
>> + pub fn state(&mut self) -> DeviceState {
>> + let phydev = self.0.get();
>> + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
>> + let state = unsafe { (*phydev).state };
>> + // FIXME: enum-cast
>> + match state {
>> + bindings::phy_state::PHY_DOWN => DeviceState::Down,
>> + bindings::phy_state::PHY_READY => DeviceState::Ready,
>> + bindings::phy_state::PHY_HALTED => DeviceState::Halted,
>> + bindings::phy_state::PHY_ERROR => DeviceState::Error,
>> + bindings::phy_state::PHY_UP => DeviceState::Up,
>> + bindings::phy_state::PHY_RUNNING => DeviceState::Running,
>> + bindings::phy_state::PHY_NOLINK => DeviceState::NoLink,
>> + bindings::phy_state::PHY_CABLETEST => DeviceState::CableTest,
>> + }
>> + }
>> +
>> + /// Returns true if the link is up.
>> + pub fn get_link(&mut self) -> bool {
>
> I would call this function `is_link_up`.
>
>> + const LINK_IS_UP: u32 = 1;
>> + let phydev = self.0.get();
>> + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
>> + unsafe { (*phydev).link() == LINK_IS_UP }
>
> Can you move the call to `link` and the `==` operation out
> of the `unsafe` block? They are safe operations. (also do
> that below where possible)
Sure, fixed.
>> +/// Creates the kernel's `phy_driver` instance.
>> +///
>> +/// This is used by [`module_phy_driver`] macro to create a static array of phy_driver`.
>
> Missing '`'.
Fixed.
>> +/// Registration structure for a PHY driver.
>> +///
>> +/// # Invariants
>> +///
>> +/// The `drivers` points to an array of `struct phy_driver`, which is
>> +/// registered to the kernel via `phy_drivers_register`.
>
> Since it is a reference you do not need to explicitly state
> that it points to an array of `struct phy_driver`. Instead I would
> suggest the following invariant:
>
> All elements of the `drivers` slice are valid and currently registered
> to the kernel via `phy_drivers_register`.
Surely, makes sense.
>> +pub struct Registration {
>> + drivers: Option<&'static [Opaque<bindings::phy_driver>]>,
>
> Why is this an `Option`?
Oops, removed; leftover of older version.
>> +}
>> +
>> +impl Registration {
>> + /// Registers a PHY driver.
>> + #[must_use]
>> + pub fn register(
>> + module: &'static crate::ThisModule,
>> + drivers: &'static [Opaque<bindings::phy_driver>],
>> + ) -> Result<Self> {
>> + if drivers.len() == 0 {
>> + return Err(code::EINVAL);
>> + }
>> + // SAFETY: `drivers` has static lifetime and used only in the C side.
>> + to_result(unsafe {
>> + bindings::phy_drivers_register(drivers[0].get(), drivers.len() as i32, module.0)
>> + })?;
>
> This `register` function seems to assume that the values of the
> `drivers` array are initialized and otherwise also considered valid.
> So please change that or make this function `unsafe`.
Understood.
>> + Ok(Registration {
>
> Please add an `INVARIANT` comment similar to a `SAFETY` comment
> that explains why the invariant is upheld.
Added.
>> +#[macro_export]
>> +macro_rules! module_phy_driver {
>> + (@replace_expr $_t:tt $sub:expr) => {$sub};
>> +
>> + (@count_devices $($x:expr),*) => {
>> + 0usize $(+ $crate::module_phy_driver!(@replace_expr $x 1usize))*
>> + };
>> +
>> + (@device_table [$($dev:expr),+]) => {
>> + #[no_mangle]
>> + static __mod_mdio__phydev_device_table: [
>
> Shouldn't this have a unique name? If we define two different
> phy drivers with this macro we would have a symbol collision?
>
>> + kernel::bindings::mdio_device_id;
>
> Please use absolute paths in macros:
> `::kernel::bindings::mdio_device_id` (also below).
Updated.
>> + $crate::module_phy_driver!(@count_devices $($dev),+) + 1
>> + ] = [
>> + $(kernel::bindings::mdio_device_id {
>> + phy_id: $dev.id,
>> + phy_id_mask: $dev.mask_as_int()
>> + }),+,
>> + kernel::bindings::mdio_device_id {
>> + phy_id: 0,
>> + phy_id_mask: 0
>> + }
>> + ];
>> + };
>> +
>> + (drivers: [$($driver:ident),+], device_table: [$($dev:expr),+], $($f:tt)*) => {
>> + struct Module {
>> + _reg: kernel::net::phy::Registration,
>> + }
>> +
>> + $crate::prelude::module! {
>> + type: Module,
>> + $($f)*
>> + }
>> +
>> + static mut DRIVERS: [
>> + kernel::types::Opaque<kernel::bindings::phy_driver>;
>> + $crate::module_phy_driver!(@count_devices $($driver),+)
>> + ] = [
>> + $(kernel::net::phy::create_phy_driver::<$driver>()),+
>> + ];
>> +
>> + impl kernel::Module for Module {
>> + fn init(module: &'static ThisModule) -> Result<Self> {
>> + // SAFETY: static `DRIVERS` array is used only in the C side.
>
> In order for this SAFETY comment to be correct, you need to ensure
> that nobody else can access the `DRIVERS` static. You can do that by
> placing both the `static mut DRIVERS` and the `impl ::kernel::Module
> for Module` items inside of a `const _: () = {}`, so like this:
>
> const _: () = {
> static mut DRIVERS: [...] = ...;
> impl ::kernel::Module for Module { ... }
> };
>
> You can also mention this in the SAFETY comment.
Great, that's exactly what to be needed here. Thanks a lot!
Powered by blists - more mailing lists