On Mon, 09 Oct 2023 12:19:54 +0000
Benno Lossin [off-list ref] wrote:

I skipped the topics that you've already discussed with Andrew.

On 09.10.23 03:39, FUJITA Tomonori wrote:

quoted

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@gmail.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)

quoted

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

quoted

+    /// 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`.

quoted

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

quoted

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

quoted

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

quoted

+pub struct Registration {
+    drivers: Option<&'static [Opaque<bindings::phy_driver>]>,

Why is this an `Option`?

Oops, removed; leftover of older version.

quoted

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

quoted

+        Ok(Registration {

Please add an `INVARIANT` comment similar to a `SAFETY` comment
that explains why the invariant is upheld.

Added.

quoted

+#[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?

quoted

+            kernel::bindings::mdio_device_id;

Please use absolute paths in macros:
`::kernel::bindings::mdio_device_id` (also below).

Updated.

quoted

+            $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!