[net-next,v5,1/5] rust: core abstractions for network PHY drivers

Commit Message

FUJITA Tomonori Oct. 17, 2023, 11:30 a.m. UTC

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_ABSTRACTIONS=y.

This patch enables unstable const_maybe_uninit_zeroed feature for
kernel crate to enable unsafe code to handle a constant value with
uninitialized data. With the feature, the abstractions can initialize
a phy_driver structure with zero easily; instead of initializing all
the members by hand. It's supposed to be stable in the not so distant
future.

Link: https://github.com/rust-lang/rust/pull/116218

Signed-off-by: FUJITA Tomonori <[e-mail]>
---
 drivers/net/phy/Kconfig         |   8 +
 rust/bindings/bindings_helper.h |   3 +
 rust/kernel/lib.rs              |   3 +
 rust/kernel/net.rs              |   6 +
 rust/kernel/net/phy.rs          | 701 ++++++++++++++++++++++++++++++++
 5 files changed, 721 insertions(+)
 create mode 100644 rust/kernel/net.rs
 create mode 100644 rust/kernel/net/phy.rs

Comments

Benno Lossin Oct. 18, 2023, 3:07 p.m. UTC | #1

On 17.10.23 13:30, 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_ABSTRACTIONS=y.
> 
> This patch enables unstable const_maybe_uninit_zeroed feature for
> kernel crate to enable unsafe code to handle a constant value with
> uninitialized data. With the feature, the abstractions can initialize
> a phy_driver structure with zero easily; instead of initializing all
> the members by hand. It's supposed to be stable in the not so distant
> future.
> 
> Link: https://github.com/rust-lang/rust/pull/116218
> 
> Signed-off-by: FUJITA Tomonori <[e-mail]>
> ---
>   drivers/net/phy/Kconfig         |   8 +
>   rust/bindings/bindings_helper.h |   3 +
>   rust/kernel/lib.rs              |   3 +
>   rust/kernel/net.rs              |   6 +
>   rust/kernel/net/phy.rs          | 701 ++++++++++++++++++++++++++++++++
>   5 files changed, 721 insertions(+)
>   create mode 100644 rust/kernel/net.rs
>   create mode 100644 rust/kernel/net/phy.rs
> 
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index 421d2b62918f..0faebdb184ca 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -60,6 +60,14 @@ config FIXED_PHY
> 
>   	  Currently tested with mpc866ads and mpc8349e-mitx.
> 
> +config RUST_PHYLIB_ABSTRACTIONS
> +        bool "PHYLIB abstractions support"
> +        depends on RUST
> +        depends on PHYLIB=y
> +        help
> +          Adds support needed for PHY drivers written in Rust. It provides
> +          a wrapper around the C phylib core.
> +
>   config SFP
>   	tristate "SFP cage support"
>   	depends on I2C && PHYLINK
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index c91a3c24f607..ec4ee09a34ad 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -8,6 +8,9 @@
> 
>   #include <kunit/test.h>
>   #include <linux/errname.h>
> +#include <linux/ethtool.h>
> +#include <linux/mdio.h>
> +#include <linux/phy.h>
>   #include <linux/slab.h>
>   #include <linux/refcount.h>
>   #include <linux/wait.h>
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index e8811700239a..0588422e273c 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -14,6 +14,7 @@
>   #![no_std]
>   #![feature(allocator_api)]
>   #![feature(coerce_unsized)]
> +#![feature(const_maybe_uninit_zeroed)]
>   #![feature(dispatch_from_dyn)]
>   #![feature(new_uninit)]
>   #![feature(receiver_trait)]
> @@ -36,6 +37,8 @@
>   pub mod ioctl;
>   #[cfg(CONFIG_KUNIT)]
>   pub mod kunit;
> +#[cfg(CONFIG_NET)]
> +pub mod net;
>   pub mod prelude;
>   pub mod print;
>   mod static_assert;
> diff --git a/rust/kernel/net.rs b/rust/kernel/net.rs
> new file mode 100644
> index 000000000000..fe415cb369d3
> --- /dev/null
> +++ b/rust/kernel/net.rs
> @@ -0,0 +1,6 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Networking.
> +
> +#[cfg(CONFIG_RUST_PHYLIB_ABSTRACTIONS)]
> +pub mod phy;
> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
> new file mode 100644
> index 000000000000..7d4927ece32f
> --- /dev/null
> +++ b/rust/kernel/net/phy.rs
> @@ -0,0 +1,701 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +// Copyright (C) 2023 FUJITA Tomonori <[e-mail]>
> +
> +//! Network PHY device.
> +//!
> +//! C headers: [`include/linux/phy.h`](../../../../include/linux/phy.h).
> +//!

Add a new section "# Abstraction Overview" or similar.

> +//! The C side (PHYLIB) guarantees that there is only one thread accessing to one `phy_device` instance
> +//! during a callback in `phy_driver`. The callback safely accesses to the instance exclusively.
> +//! Except for `resume()` and `suspend()`, PHYLIB holds `phy_device`'s lock during a callback.

I would start with "During the calls to most functions in [`Driver`],
the C side (`PHYLIB`) holds a lock that is unique for every instance of
[`Device`]". Then you can note the exception for `resume` and `suspend`
(and also link them e.g. [`Driver::resume`]).

> +//! PHYLIB doesn't hold `phy_device`'s lock in both `resume()` and `suspend()`. Instead, PHYLIB
> +//! updates `phy_device`'s state with `phy_device`'s lock hold, to guarantee that resume() accesses
> +//! to the instance exclusively. Note that `resume()` and `suspend()` also are called where only
> +//! one thread can access to the instance.

I would just write "`PHYLIB` uses a different serialization technique for
[`Driver::resume`] and [`Driver::suspend`]: <use the above explanation>".

> +//! This abstractions creates [`Device`] instance only during a callback so it's guaranteed that
> +//! only one reference exists. All its methods can accesses to the instance exclusively.

Not really sure if this is needed, what are you trying to explain here?

> +//! All the PHYLIB helper functions for `phy_device` modify some members in `phy_device`. Except for
> +//! getter functions, [`Device`] methods take `&mut self`. This also applied to `read()`, which reads
> +//! a hardware register and updates the stats.

I would use [`Device`] instead of `phy_device`, since the Rust reader
might not be aware what wraps `phy_device`.

> +use crate::{bindings, error::*, prelude::vtable, str::CStr, types::Opaque};
> +use core::marker::PhantomData;
> +
> +/// Corresponds to the kernel's `enum phy_state`.
> +#[derive(PartialEq)]
> +pub enum DeviceState {
> +    /// PHY device and driver are not ready for anything.
> +    Down,
> +    /// PHY is ready to send and receive packets.
> +    Ready,
> +    /// PHY is up, but no polling or interrupts are done.
> +    Halted,
> +    /// PHY is up, but is in an error state.
> +    Error,
> +    /// PHY and attached device are ready to do work.
> +    Up,
> +    /// PHY is currently running.
> +    Running,
> +    /// PHY is up, but not currently plugged in.
> +    NoLink,
> +    /// PHY is performing a cable test.
> +    CableTest,
> +}
> +
> +/// Represents duplex mode.
> +pub enum DuplexMode {
> +    /// PHY is in full-duplex mode.
> +    Full,
> +    /// PHY is in half-duplex mode.
> +    Half,
> +    /// PHY is in unknown duplex mode.
> +    Unknown,
> +}
> +
> +/// An instance of a PHY device.
> +///
> +/// Wraps the kernel's `struct phy_device`.
> +///
> +/// # Invariants
> +///
> +/// `self.0` is always in a valid state.
> +#[repr(transparent)]
> +pub struct Device(Opaque<bindings::phy_device>);
> +
> +impl Device {
> +    /// Creates a new [`Device`] instance from a raw pointer.
> +    ///
> +    /// # Safety
> +    ///
> +    /// This function must only be called from the callbacks in `phy_driver`. PHYLIB guarantees
> +    /// the exclusive access for the duration of the lifetime `'a`.

I would not put the second sentence in the `# Safety` section. Just move it
above. The reason behind this is the following: the second sentence is not
a precondition needed to call the function.

> +    unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
> +        // CAST: `Self` is a `repr(transparent)` wrapper around `bindings::phy_device`.
> +        let ptr = ptr.cast::<Self>();
> +        // SAFETY: by the function requirements the pointer is valid and we have unique access for
> +        // the duration of `'a`.
> +        unsafe { &mut *ptr }
> +    }
> +
> +    /// Gets the id of the PHY.
> +    pub fn phy_id(&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(&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 };
> +        // TODO: this conversion code will be replaced with automatically generated code by bindgen
> +        // when it becomes possible.
> +        // better to call WARN_ONCE() when the state is out-of-range (needs to add WARN_ONCE support).
> +        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,
> +            _ => DeviceState::Error,
> +        }
> +    }
> +
> +    /// Returns true if the link is up.
> +    pub fn get_link(&self) -> bool {

I still think this name should be changed. My response at [1] has not yet
been replied to. This has already been discussed before:
- https://lore.kernel.org/rust-for-linux/2023100237-satirical-prance-bd57@gregkh/
- https://lore.kernel.org/rust-for-linux/[e-mail]/
- https://lore.kernel.org/rust-for-linux/[e-mail]/

And I want to suggest to change it to `is_link_up`.

Reasons why I do not like the name:
- `get_`/`put_` are used for ref counting on the C side, I would like to
   avoid confusion.
- `get` in Rust is often used to fetch a value from e.g. a datastructure
   such as a hashmap, so I expect the call to do some computation.
- getters in Rust usually are not prefixed with `get_`, but rather are
   just the name of the field.
- in this case I like the name `is_link_up` much better, since code becomes
   a lot more readable with that.
- I do not want this pattern as an example for other drivers.

[1]: https://lore.kernel.org/rust-for-linux/[e-mail]/

> +        const LINK_IS_UP: u32 = 1;
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        let phydev = unsafe { *self.0.get() };
> +        phydev.link() == LINK_IS_UP
> +    }

[...]

> +/// An instance of a PHY driver.
> +///
> +/// Wraps the kernel's `struct phy_driver`.
> +///
> +/// # Invariants
> +///
> +/// `self.0` is always in a valid state.
> +#[repr(transparent)]
> +pub struct DriverType(Opaque<bindings::phy_driver>);

I think `DriverVTable` is a better name.

> +/// Creates the kernel's `phy_driver` instance.

This function returns a `DriverType`, not a `phy_driver`.

> +///
> +/// This is used by [`module_phy_driver`] macro to create a static array of `phy_driver`.
> +///
> +/// [`module_phy_driver`]: crate::module_phy_driver
> +pub const fn create_phy_driver<T: Driver>() -> DriverType {
> +    // All the fields of `struct phy_driver` are initialized properly.
> +    // It ensures the invariants.

Use `// INVARIANT: `.

> +    DriverType(Opaque::new(bindings::phy_driver {

[...]

> +
> +/// Registration structure for a PHY driver.
> +///
> +/// # Invariants
> +///
> +/// The `drivers` slice are currently registered to the kernel via `phy_drivers_register`.
> +pub struct Registration {
> +    drivers: &'static [DriverType],
> +}

You did not reply to my suggestion [2] to remove this type,
what do you think?

[2]: https://lore.kernel.org/rust-for-linux/[e-mail]/

Andrew Lunn Oct. 18, 2023, 8:27 p.m. UTC | #2

> +    /// Reads a given C22 PHY register.
> +    pub fn read(&mut self, regnum: u16) -> Result<u16> {
> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        // So an FFI call with a valid pointer.
> +        let ret = unsafe {
> +            bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into())

If i've understood the discussion about &mut, it is not needed here,
and for write. Performing a read/write does not change anything in
phydev. There was mention of statistics, but they are in the mii_bus
structure, which is pointed to by this structure, but is not part of
this structure.

> +        };
> +        if ret < 0 {
> +            Err(Error::from_errno(ret))
> +        } else {
> +            Ok(ret as u16)
> +        }
> +    }
> +
> +    /// Writes a given C22 PHY register.
> +    pub fn write(&mut self, regnum: u16, val: u16) -> Result {
> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        // So an FFI call with a valid pointer.
> +        to_result(unsafe {
> +            bindings::mdiobus_write((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into(), val)
> +        })
> +    }
> +
> +    /// Reads a paged register.
> +    pub fn read_paged(&mut self, page: u16, regnum: u16) -> Result<u16> {

From my reading of the code, read_paged also does not modify phydev.

     Andrew

FUJITA Tomonori Oct. 19, 2023, 12:24 a.m. UTC | #3

On Wed, 18 Oct 2023 15:07:52 +0000
Benno Lossin <[e-mail]> wrote:

>> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
>> new file mode 100644
>> index 000000000000..7d4927ece32f
>> --- /dev/null
>> +++ b/rust/kernel/net/phy.rs
>> @@ -0,0 +1,701 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +// Copyright (C) 2023 FUJITA Tomonori <[e-mail]>
>> +
>> +//! Network PHY device.
>> +//!
>> +//! C headers: [`include/linux/phy.h`](../../../../include/linux/phy.h).
>> +//!
> 
> Add a new section "# Abstraction Overview" or similar.

With the rest of comments on this secsion addressed, how about the following?

//! Network PHY device.
//!
//! C headers: [`include/linux/phy.h`](../../../../include/linux/phy.h).
//!
//! # Abstraction Overview
//!
//! "During the calls to most functions in [`Driver`], the C side (`PHYLIB`) holds a lock that is unique
//! for every instance of [`Device`]". `PHYLIB` uses a different serialization technique for
//! [`Driver::resume`] and [`Driver::suspend`]: `PHYLIB` updates `phy_device`'s state with the lock hold,
//! to guarantee that [`Driver::resume`] accesses to the instance exclusively. [`Driver::resume`] and
//! [`Driver::suspend`] also are called where only one thread can access to the instance.
//!
//! All the PHYLIB helper functions for [`Device`] modify some members in [`Device`]. Except for
//! getter functions, [`Device`] methods take `&mut self`. This also applied to `[Device::read]`,
//! which reads a hardware register and updates the stats.


>> +//! The C side (PHYLIB) guarantees that there is only one thread accessing to one `phy_device` instance
>> +//! during a callback in `phy_driver`. The callback safely accesses to the instance exclusively.
>> +//! Except for `resume()` and `suspend()`, PHYLIB holds `phy_device`'s lock during a callback.
> 
> I would start with "During the calls to most functions in [`Driver`],
> the C side (`PHYLIB`) holds a lock that is unique for every instance of
> [`Device`]". Then you can note the exception for `resume` and `suspend`
> (and also link them e.g. [`Driver::resume`]).
> 
>> +//! PHYLIB doesn't hold `phy_device`'s lock in both `resume()` and `suspend()`. Instead, PHYLIB
>> +//! updates `phy_device`'s state with `phy_device`'s lock hold, to guarantee that resume() accesses
>> +//! to the instance exclusively. Note that `resume()` and `suspend()` also are called where only
>> +//! one thread can access to the instance.
> 
> I would just write "`PHYLIB` uses a different serialization technique for
> [`Driver::resume`] and [`Driver::suspend`]: <use the above explanation>".
> 
>> +//! This abstractions creates [`Device`] instance only during a callback so it's guaranteed that
>> +//! only one reference exists. All its methods can accesses to the instance exclusively.
> 
> Not really sure if this is needed, what are you trying to explain here?

I tried to add an explanation that Device::from_raw() return &mut. But
I guess that the description of Device is enough.


>> +//! All the PHYLIB helper functions for `phy_device` modify some members in `phy_device`. Except for
>> +//! getter functions, [`Device`] methods take `&mut self`. This also applied to `read()`, which reads
>> +//! a hardware register and updates the stats.
> 
> I would use [`Device`] instead of `phy_device`, since the Rust reader
> might not be aware what wraps `phy_device`.
> 
>> +use crate::{bindings, error::*, prelude::vtable, str::CStr, types::Opaque};
>> +use core::marker::PhantomData;

(snip)


>> +/// An instance of a PHY device.
>> +///
>> +/// Wraps the kernel's `struct phy_device`.
>> +///
>> +/// # Invariants
>> +///
>> +/// `self.0` is always in a valid state.
>> +#[repr(transparent)]
>> +pub struct Device(Opaque<bindings::phy_device>);
>> +
>> +impl Device {
>> +    /// Creates a new [`Device`] instance from a raw pointer.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// This function must only be called from the callbacks in `phy_driver`. PHYLIB guarantees
>> +    /// the exclusive access for the duration of the lifetime `'a`.
> 
> I would not put the second sentence in the `# Safety` section. Just move it
> above. The reason behind this is the following: the second sentence is not
> a precondition needed to call the function.

Where is the `above`? You meant the following?

impl Device {
    /// Creates a new [`Device`] instance from a raw pointer.
    ///
    /// `PHYLIB` guarantees the exclusive access for the duration of the lifetime `'a`.
    ///
    /// # Safety
    ///
    /// This function must only be called from the callbacks in `phy_driver`.
    unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a  mut Self {


>> +    unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
>> +        // CAST: `Self` is a `repr(transparent)` wrapper around `bindings::phy_device`.
>> +        let ptr = ptr.cast::<Self>();
>> +        // SAFETY: by the function requirements the pointer is valid and we have unique access for
>> +        // the duration of `'a`.
>> +        unsafe { &mut *ptr }
>> +    }
>> +
>> +    /// Gets the id of the PHY.
>> +    pub fn phy_id(&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(&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 };
>> +        // TODO: this conversion code will be replaced with automatically generated code by bindgen
>> +        // when it becomes possible.
>> +        // better to call WARN_ONCE() when the state is out-of-range (needs to add WARN_ONCE support).
>> +        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,
>> +            _ => DeviceState::Error,
>> +        }
>> +    }
>> +
>> +    /// Returns true if the link is up.
>> +    pub fn get_link(&self) -> bool {
> 
> I still think this name should be changed. My response at [1] has not yet
> been replied to. This has already been discussed before:
> - https://lore.kernel.org/rust-for-linux/2023100237-satirical-prance-bd57@gregkh/
> - https://lore.kernel.org/rust-for-linux/[e-mail]/
> - https://lore.kernel.org/rust-for-linux/[e-mail]/
> 
> And I want to suggest to change it to `is_link_up`.
> 
> Reasons why I do not like the name:
> - `get_`/`put_` are used for ref counting on the C side, I would like to
>    avoid confusion.
> - `get` in Rust is often used to fetch a value from e.g. a datastructure
>    such as a hashmap, so I expect the call to do some computation.
> - getters in Rust usually are not prefixed with `get_`, but rather are
>    just the name of the field.
> - in this case I like the name `is_link_up` much better, since code becomes
>    a lot more readable with that.
> - I do not want this pattern as an example for other drivers.
> 
> [1]: https://lore.kernel.org/rust-for-linux/[e-mail]/

IIRC, Andrew suggested the current name. If the change is fine by him,
I'll rename.


>> +/// An instance of a PHY driver.
>> +///
>> +/// Wraps the kernel's `struct phy_driver`.
>> +///
>> +/// # Invariants
>> +///
>> +/// `self.0` is always in a valid state.
>> +#[repr(transparent)]
>> +pub struct DriverType(Opaque<bindings::phy_driver>);
> 
> I think `DriverVTable` is a better name.

Renamed.

>> +/// Creates the kernel's `phy_driver` instance.
> 
> This function returns a `DriverType`, not a `phy_driver`.

How about?

/// Creates the [`DriverVTable`] instance from [`Driver`] trait.

>> +///
>> +/// This is used by [`module_phy_driver`] macro to create a static array of `phy_driver`.
>> +///
>> +/// [`module_phy_driver`]: crate::module_phy_driver
>> +pub const fn create_phy_driver<T: Driver>() -> DriverType {
>> +    // All the fields of `struct phy_driver` are initialized properly.
>> +    // It ensures the invariants.
> 
> Use `// INVARIANT: `.

Oops,

// INVARIANT: All the fields of `struct phy_driver` are initialized properly.
DriverVTable(Opaque::new(bindings::phy_driver {
    name: T::NAME.as_char_ptr().cast_mut(),


>> +/// Registration structure for a PHY driver.
>> +///
>> +/// # Invariants
>> +///
>> +/// The `drivers` slice are currently registered to the kernel via `phy_drivers_register`.
>> +pub struct Registration {
>> +    drivers: &'static [DriverType],
>> +}
> 
> You did not reply to my suggestion [2] to remove this type,
> what do you think?
> 
> [2]: https://lore.kernel.org/rust-for-linux/[e-mail]/

I tried before but I'm not sure it simplifies the implementation.

Firstly, instead of Reservation, we need a public function like

pub fn phy_drivers_register(module: &'static crate::ThisModule, drivers: &[DriverVTable]) -> Result {
    to_result(unsafe {
        bindings::phy_drivers_register(drivers[0].0.get(), drivers.len().try_into()?, module.0)
    })
}

This is because module.0 is private.

Also if we keep DriverVtable.0 private, we need another public function.

pub unsafe fn phy_drivers_unregister(drivers: &'static [DriverVTable])
{
    unsafe {
        bindings::phy_drivers_unregister(drivers[0].0.get(), drivers.len() as i32)
    };
}

DriverVTable isn't guaranteed to be registered to the kernel so needs
to be unsafe, I guesss.

Also Module trait support exit()?

FUJITA Tomonori Oct. 19, 2023, 12:41 a.m. UTC | #4

On Wed, 18 Oct 2023 22:27:55 +0200
Andrew Lunn <[e-mail]> wrote:

>> +    /// Reads a given C22 PHY register.
>> +    pub fn read(&mut self, regnum: u16) -> Result<u16> {
>> +        let phydev = self.0.get();
>> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
>> +        // So an FFI call with a valid pointer.
>> +        let ret = unsafe {
>> +            bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into())
> 
> If i've understood the discussion about &mut, it is not needed here,
> and for write. Performing a read/write does not change anything in
> phydev. There was mention of statistics, but they are in the mii_bus
> structure, which is pointed to by this structure, but is not part of
> this structure.

If I understand correctly, he said that either (&self or &mut self) is
fine for read().

https://lore.kernel.org/netdev/[e-mail]/

Since `&mut self` is unique, only one thread per instance of `Self`
can call that function. So use this when the C side would use a lock.
(or requires that only one thread calls that code)

Since multiple `&self` references are allowed to coexist, you should
use this for functions which perform their own serialization/do not
require serialization.


I applied the first case here.


>> +        };
>> +        if ret < 0 {
>> +            Err(Error::from_errno(ret))
>> +        } else {
>> +            Ok(ret as u16)
>> +        }
>> +    }
>> +
>> +    /// Writes a given C22 PHY register.
>> +    pub fn write(&mut self, regnum: u16, val: u16) -> Result {
>> +        let phydev = self.0.get();
>> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
>> +        // So an FFI call with a valid pointer.
>> +        to_result(unsafe {
>> +            bindings::mdiobus_write((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into(), val)
>> +        })
>> +    }
>> +
>> +    /// Reads a paged register.
>> +    pub fn read_paged(&mut self, page: u16, regnum: u16) -> Result<u16> {
> 
> From my reading of the code, read_paged also does not modify phydev.

__phy_read is called so I use &mut self like read().

Benno Lossin Oct. 19, 2023, 1:45 p.m. UTC | #5

On 19.10.23 02:24, FUJITA Tomonori wrote:
> On Wed, 18 Oct 2023 15:07:52 +0000
> Benno Lossin <[e-mail]> wrote:
> 
>>> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
>>> new file mode 100644
>>> index 000000000000..7d4927ece32f
>>> --- /dev/null
>>> +++ b/rust/kernel/net/phy.rs
>>> @@ -0,0 +1,701 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +
>>> +// Copyright (C) 2023 FUJITA Tomonori <[e-mail]>
>>> +
>>> +//! Network PHY device.
>>> +//!
>>> +//! C headers: [`include/linux/phy.h`](../../../../include/linux/phy.h).
>>> +//!
>>
>> Add a new section "# Abstraction Overview" or similar.
> 
> With the rest of comments on this secsion addressed, how about the following?
> 
> //! Network PHY device.
> //!
> //! C headers: [`include/linux/phy.h`](../../../../include/linux/phy.h).
> //!
> //! # Abstraction Overview
> //!
> //! "During the calls to most functions in [`Driver`], the C side (`PHYLIB`) holds a lock that is unique

Please remove the quotes ", they were intended to separate my comments
from my suggestion.

> //! for every instance of [`Device`]". `PHYLIB` uses a different serialization technique for
> //! [`Driver::resume`] and [`Driver::suspend`]: `PHYLIB` updates `phy_device`'s state with the lock hold,

hold -> held

> //! to guarantee that [`Driver::resume`] accesses to the instance exclusively. [`Driver::resume`] and

to guarantee -> thus guaranteeing
accesses to the instance exclusively. -> has exclusive access to the instance.

> //! [`Driver::suspend`] also are called where only one thread can access to the instance.
> //!
> //! All the PHYLIB helper functions for [`Device`] modify some members in [`Device`]. Except for

PHYLIB -> `PHYLIB`

> //! getter functions, [`Device`] methods take `&mut self`. This also applied to `[Device::read]`,

`[Device::read]` -> [`Device::read`]

> //! which reads a hardware register and updates the stats.

Otherwise this looks good.

[...]

>>> +impl Device {
>>> +    /// Creates a new [`Device`] instance from a raw pointer.
>>> +    ///
>>> +    /// # Safety
>>> +    ///
>>> +    /// This function must only be called from the callbacks in `phy_driver`. PHYLIB guarantees
>>> +    /// the exclusive access for the duration of the lifetime `'a`.
>>
>> I would not put the second sentence in the `# Safety` section. Just move it
>> above. The reason behind this is the following: the second sentence is not
>> a precondition needed to call the function.
> 
> Where is the `above`? You meant the following?
> 
> impl Device {
>      /// Creates a new [`Device`] instance from a raw pointer.
>      ///
>      /// `PHYLIB` guarantees the exclusive access for the duration of the lifetime `'a`.
>      ///
>      /// # Safety
>      ///
>      /// This function must only be called from the callbacks in `phy_driver`.
>      unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a  mut Self {

Yes this is what I had in mind. Although now that I see it in code,
I am not so sure that this comment is needed. If you feel the same
way, just remove it.

That being said, I am not too happy with the safety requirement of this
function. It does not really match with the safety comment in the function
body. Since I have not yet finished my safety standardization, I think we
can defer that problem until it is finished. Unless some other reviewer
wants to change this, you can keep it as is.

>>> +    unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
>>> +        // CAST: `Self` is a `repr(transparent)` wrapper around `bindings::phy_device`.
>>> +        let ptr = ptr.cast::<Self>();
>>> +        // SAFETY: by the function requirements the pointer is valid and we have unique access for
>>> +        // the duration of `'a`.
>>> +        unsafe { &mut *ptr }
>>> +    }
>>> +
>>> +    /// Gets the id of the PHY.
>>> +    pub fn phy_id(&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(&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 };
>>> +        // TODO: this conversion code will be replaced with automatically generated code by bindgen
>>> +        // when it becomes possible.
>>> +        // better to call WARN_ONCE() when the state is out-of-range (needs to add WARN_ONCE support).
>>> +        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,
>>> +            _ => DeviceState::Error,
>>> +        }
>>> +    }
>>> +
>>> +    /// Returns true if the link is up.
>>> +    pub fn get_link(&self) -> bool {
>>
>> I still think this name should be changed. My response at [1] has not yet
>> been replied to. This has already been discussed before:
>> - https://lore.kernel.org/rust-for-linux/2023100237-satirical-prance-bd57@gregkh/
>> - https://lore.kernel.org/rust-for-linux/[e-mail]/
>> - https://lore.kernel.org/rust-for-linux/[e-mail]/
>>
>> And I want to suggest to change it to `is_link_up`.
>>
>> Reasons why I do not like the name:
>> - `get_`/`put_` are used for ref counting on the C side, I would like to
>>     avoid confusion.
>> - `get` in Rust is often used to fetch a value from e.g. a datastructure
>>     such as a hashmap, so I expect the call to do some computation.
>> - getters in Rust usually are not prefixed with `get_`, but rather are
>>     just the name of the field.
>> - in this case I like the name `is_link_up` much better, since code becomes
>>     a lot more readable with that.
>> - I do not want this pattern as an example for other drivers.
>>
>> [1]: https://lore.kernel.org/rust-for-linux/[e-mail]/
> 
> IIRC, Andrew suggested the current name. If the change is fine by him,
> I'll rename.
> 
> 
>>> +/// An instance of a PHY driver.
>>> +///
>>> +/// Wraps the kernel's `struct phy_driver`.
>>> +///
>>> +/// # Invariants
>>> +///
>>> +/// `self.0` is always in a valid state.
>>> +#[repr(transparent)]
>>> +pub struct DriverType(Opaque<bindings::phy_driver>);
>>
>> I think `DriverVTable` is a better name.
> 
> Renamed.
> 
>>> +/// Creates the kernel's `phy_driver` instance.
>>
>> This function returns a `DriverType`, not a `phy_driver`.
> 
> How about?
> 
> /// Creates the [`DriverVTable`] instance from [`Driver`] trait.

Sounds good, but to this sounds a bit more natural:

     /// Creates a [`DriverVTable`] instance from a [`Driver`].

>>> +///
>>> +/// This is used by [`module_phy_driver`] macro to create a static array of `phy_driver`.
>>> +///
>>> +/// [`module_phy_driver`]: crate::module_phy_driver
>>> +pub const fn create_phy_driver<T: Driver>() -> DriverType {
>>> +    // All the fields of `struct phy_driver` are initialized properly.
>>> +    // It ensures the invariants.
>>
>> Use `// INVARIANT: `.
> 
> Oops,
> 
> // INVARIANT: All the fields of `struct phy_driver` are initialized properly.
> DriverVTable(Opaque::new(bindings::phy_driver {
>      name: T::NAME.as_char_ptr().cast_mut(),

Seems good.

> 
> 
>>> +/// Registration structure for a PHY driver.
>>> +///
>>> +/// # Invariants
>>> +///
>>> +/// The `drivers` slice are currently registered to the kernel via `phy_drivers_register`.
>>> +pub struct Registration {
>>> +    drivers: &'static [DriverType],
>>> +}
>>
>> You did not reply to my suggestion [2] to remove this type,
>> what do you think?
>>
>> [2]: https://lore.kernel.org/rust-for-linux/[e-mail]/
> 
> I tried before but I'm not sure it simplifies the implementation.
> 
> Firstly, instead of Reservation, we need a public function like
> 
> pub fn phy_drivers_register(module: &'static crate::ThisModule, drivers: &[DriverVTable]) -> Result {
>      to_result(unsafe {
>          bindings::phy_drivers_register(drivers[0].0.get(), drivers.len().try_into()?, module.0)
>      })
> }
> 
> This is because module.0 is private.

Why can't this be part of the macro?

> Also if we keep DriverVtable.0 private, we need another public function.
> 
> pub unsafe fn phy_drivers_unregister(drivers: &'static [DriverVTable])
> {
>      unsafe {
>          bindings::phy_drivers_unregister(drivers[0].0.get(), drivers.len() as i32)
>      };
> }
> 
> DriverVTable isn't guaranteed to be registered to the kernel so needs
> to be unsafe, I guesss.

In one of the options I suggest to make that an invariant of `DriverVTable`.

> 
> Also Module trait support exit()?

Yes, just implement `Drop` and do the cleanup there.

In the two options that I suggested there is a trade off. I do not know
which option is better, I hoped that you or Andrew would know more:
Option 1:
* advantages:
   - manual creation of a phy driver module becomes possible.
   - less complex `module_phy_driver` macro.
   - no static variable needed.
* disadvantages:
   - calls `phy_drivers_register` for every driver on module
     initialization.
   - calls `phy_drivers_unregister` for every driver on module
     exit.

Option 2:
* advantages:
   - less complex `module_phy_driver` macro.
   - no static variable needed.
   - only a single call to
     `phy_drivers_register`/`phy_drivers_unregister`.
* disadvantages:
   - no safe manual creation of phy drivers possible, the only safe
     way is to use the `module_phy_driver` macro.

I suppose that it would be ok to call the register function multiple
times, since it only is on module startup/shutdown and it is not
performance critical.

Benno Lossin Oct. 19, 2023, 1:57 p.m. UTC | #6

On 19.10.23 02:41, FUJITA Tomonori wrote:
> On Wed, 18 Oct 2023 22:27:55 +0200
> Andrew Lunn <[e-mail]> wrote:
> 
>>> +    /// Reads a given C22 PHY register.
>>> +    pub fn read(&mut self, regnum: u16) -> Result<u16> {
>>> +        let phydev = self.0.get();
>>> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
>>> +        // So an FFI call with a valid pointer.
>>> +        let ret = unsafe {
>>> +            bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into())
>>
>> If i've understood the discussion about &mut, it is not needed here,
>> and for write. Performing a read/write does not change anything in
>> phydev. There was mention of statistics, but they are in the mii_bus
>> structure, which is pointed to by this structure, but is not part of
>> this structure.
> 
> If I understand correctly, he said that either (&self or &mut self) is
> fine for read().
> 
> https://lore.kernel.org/netdev/[e-mail]/
> 
> Since `&mut self` is unique, only one thread per instance of `Self`
> can call that function. So use this when the C side would use a lock.
> (or requires that only one thread calls that code)
> 
> Since multiple `&self` references are allowed to coexist, you should
> use this for functions which perform their own serialization/do not
> require serialization.
> 
> 
> I applied the first case here.

I will try to explain things a bit more.

So this case is a bit difficult to figure out, because what is
going on is not really a pattern that is used in Rust.
We already have exclusive access to the `phy_device`, so in Rust
you would not need to lock anything to also have exclusive access to the
embedded `mii_bus`. In this sense, mutable references (`&mut T`) are
infectious.

Since C always locks the `mdio_lock` when we call the read & write
functions, we however could also just use a shared reference (`&T`)
for the function receiver, since the C side guarantees serialization.

Another reason for choosing `&mut self` here is the following: it is
easier to later change to `&self` compared to going with `&self` now
and changing to `&mut self` later. This is because if you have a `&mut T`
you can also call all of its `&T` functions, but not the other way around.
`&mut self` is as a receiver also more conservative, since it is more
strict as to where it can be called. So let's just go with that.

FUJITA Tomonori Oct. 19, 2023, 2:42 p.m. UTC | #7

On Thu, 19 Oct 2023 13:45:27 +0000
Benno Lossin <[e-mail]> wrote:

> On 19.10.23 02:24, FUJITA Tomonori wrote:
>> On Wed, 18 Oct 2023 15:07:52 +0000
>> Benno Lossin <[e-mail]> wrote:
>> 
>>>> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
>>>> new file mode 100644
>>>> index 000000000000..7d4927ece32f
>>>> --- /dev/null
>>>> +++ b/rust/kernel/net/phy.rs
>>>> @@ -0,0 +1,701 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +
>>>> +// Copyright (C) 2023 FUJITA Tomonori <[e-mail]>
>>>> +
>>>> +//! Network PHY device.
>>>> +//!
>>>> +//! C headers: [`include/linux/phy.h`](../../../../include/linux/phy.h).
>>>> +//!
>>>
>>> Add a new section "# Abstraction Overview" or similar.
>> 
>> With the rest of comments on this secsion addressed, how about the following?
>> 
>> //! Network PHY device.
>> //!
>> //! C headers: [`include/linux/phy.h`](../../../../include/linux/phy.h).
>> //!
>> //! # Abstraction Overview
>> //!
>> //! "During the calls to most functions in [`Driver`], the C side (`PHYLIB`) holds a lock that is unique
> 
> Please remove the quotes ", they were intended to separate my comments
> from my suggestion.
> 
>> //! for every instance of [`Device`]". `PHYLIB` uses a different serialization technique for
>> //! [`Driver::resume`] and [`Driver::suspend`]: `PHYLIB` updates `phy_device`'s state with the lock hold,
> 
> hold -> held
> 
>> //! to guarantee that [`Driver::resume`] accesses to the instance exclusively. [`Driver::resume`] and
> 
> to guarantee -> thus guaranteeing
> accesses to the instance exclusively. -> has exclusive access to the instance.
> 
>> //! [`Driver::suspend`] also are called where only one thread can access to the instance.
>> //!
>> //! All the PHYLIB helper functions for [`Device`] modify some members in [`Device`]. Except for
> 
> PHYLIB -> `PHYLIB`
> 
>> //! getter functions, [`Device`] methods take `&mut self`. This also applied to `[Device::read]`,
> 
> `[Device::read]` -> [`Device::read`]
> 
>> //! which reads a hardware register and updates the stats.
> 
> Otherwise this looks good.

Thanks, I fixed the comment accordingly.


> [...]
> 
>>>> +impl Device {
>>>> +    /// Creates a new [`Device`] instance from a raw pointer.
>>>> +    ///
>>>> +    /// # Safety
>>>> +    ///
>>>> +    /// This function must only be called from the callbacks in `phy_driver`. PHYLIB guarantees
>>>> +    /// the exclusive access for the duration of the lifetime `'a`.
>>>
>>> I would not put the second sentence in the `# Safety` section. Just move it
>>> above. The reason behind this is the following: the second sentence is not
>>> a precondition needed to call the function.
>> 
>> Where is the `above`? You meant the following?
>> 
>> impl Device {
>>      /// Creates a new [`Device`] instance from a raw pointer.
>>      ///
>>      /// `PHYLIB` guarantees the exclusive access for the duration of the lifetime `'a`.
>>      ///
>>      /// # Safety
>>      ///
>>      /// This function must only be called from the callbacks in `phy_driver`.
>>      unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a  mut Self {
> 
> Yes this is what I had in mind. Although now that I see it in code,
> I am not so sure that this comment is needed. If you feel the same
> way, just remove it.

Then let me drop it.

> That being said, I am not too happy with the safety requirement of this
> function. It does not really match with the safety comment in the function
> body. Since I have not yet finished my safety standardization, I think we
> can defer that problem until it is finished. Unless some other reviewer
> wants to change this, you can keep it as is.

Understood. 


>> /// Creates the [`DriverVTable`] instance from [`Driver`] trait.
> 
> Sounds good, but to this sounds a bit more natural:
> 
>      /// Creates a [`DriverVTable`] instance from a [`Driver`].

Oops, fixed.


>>>> +/// Registration structure for a PHY driver.
>>>> +///
>>>> +/// # Invariants
>>>> +///
>>>> +/// The `drivers` slice are currently registered to the kernel via `phy_drivers_register`.
>>>> +pub struct Registration {
>>>> +    drivers: &'static [DriverType],
>>>> +}
>>>
>>> You did not reply to my suggestion [2] to remove this type,
>>> what do you think?
>>>
>>> [2]: https://lore.kernel.org/rust-for-linux/[e-mail]/
>> 
>> I tried before but I'm not sure it simplifies the implementation.
>> 
>> Firstly, instead of Reservation, we need a public function like
>> 
>> pub fn phy_drivers_register(module: &'static crate::ThisModule, drivers: &[DriverVTable]) -> Result {
>>      to_result(unsafe {
>>          bindings::phy_drivers_register(drivers[0].0.get(), drivers.len().try_into()?, module.0)
>>      })
>> }
>> 
>> This is because module.0 is private.
> 
> Why can't this be part of the macro?

I'm not sure I correctly understand what you suggest so you meant the following?

    (drivers: [$($driver:ident),+], device_table: [$($dev:expr),+], $($f:tt)*) => {
        struct Module {
             _drv:  [
                ::kernel::net::phy::DriverVTable;
                $crate::module_phy_driver!(@count_devices $($driver),+)
            ],
        }
        unsafe impl Sync for Module {}

        $crate::prelude::module! {
            type: Module,
            $($f)*
        }

        impl ::kernel::Module for Module {
            fn init(module: &'static ThisModule) -> Result<Self> {
                let drv = [
                    $(::kernel::net::phy::create_phy_driver::<$driver>()),+
                ];
                ::kernel::error::to_result(unsafe {
                    ::kernel::bindings::phy_drivers_register(drv[0].0.get(), drv.len().try_into()?, module.0)
                })?;

                Ok(Module {
                    _drv: drv,
                })
            }
        }

Then we got the following error:

error[E0616]: field `0` of struct `DriverVTable` is private
  --> drivers/net/phy/ax88796b_rust.rs:12:1
     |
     12 | / kernel::module_phy_driver! {
     13 | |     drivers: [PhyAX88772A, PhyAX88772C, PhyAX88796B],
     14 | |     device_table: [
     15 | |         DeviceId::new_with_driver::<PhyAX88772A>(),
     ...  |
     22 | |     license: "GPL",
     23 | | }
        | |_^ private field
	   |
	      = note: this error originates in the macro
	      `kernel::module_phy_driver` (in Nightly builds, run with
	      -Z macro-backtrace for more info)

error[E0616]: field `0` of struct `kernel::ThisModule` is private
  --> drivers/net/phy/ax88796b_rust.rs:12:1
     |
     12 | / kernel::module_phy_driver! {
     13 | |     drivers: [PhyAX88772A, PhyAX88772C, PhyAX88796B],
     14 | |     device_table: [
     15 | |         DeviceId::new_with_driver::<PhyAX88772A>(),
     ...  |
     22 | |     license: "GPL",
     23 | | }
        | |_^ private field


>> Also if we keep DriverVtable.0 private, we need another public function.
>> 
>> pub unsafe fn phy_drivers_unregister(drivers: &'static [DriverVTable])
>> {
>>      unsafe {
>>          bindings::phy_drivers_unregister(drivers[0].0.get(), drivers.len() as i32)
>>      };
>> }
>> 
>> DriverVTable isn't guaranteed to be registered to the kernel so needs
>> to be unsafe, I guesss.
> 
> In one of the options I suggest to make that an invariant of `DriverVTable`.
> 
>> 
>> Also Module trait support exit()?
> 
> Yes, just implement `Drop` and do the cleanup there.
> 
> In the two options that I suggested there is a trade off. I do not know
> which option is better, I hoped that you or Andrew would know more:
> Option 1:
> * advantages:
>    - manual creation of a phy driver module becomes possible.
>    - less complex `module_phy_driver` macro.
>    - no static variable needed.
> * disadvantages:
>    - calls `phy_drivers_register` for every driver on module
>      initialization.
>    - calls `phy_drivers_unregister` for every driver on module
>      exit.
> 
> Option 2:
> * advantages:
>    - less complex `module_phy_driver` macro.
>    - no static variable needed.
>    - only a single call to
>      `phy_drivers_register`/`phy_drivers_unregister`.
> * disadvantages:
>    - no safe manual creation of phy drivers possible, the only safe
>      way is to use the `module_phy_driver` macro.
> 
> I suppose that it would be ok to call the register function multiple
> times, since it only is on module startup/shutdown and it is not
> performance critical.

I think that we can use the current implantation using Reservation
struct until someone requests manual creation. I doubt that we will
need to support such.

Benno Lossin Oct. 19, 2023, 3:20 p.m. UTC | #8

On 19.10.23 16:42, FUJITA Tomonori wrote:
>>>>> +/// Registration structure for a PHY driver.
>>>>> +///
>>>>> +/// # Invariants
>>>>> +///
>>>>> +/// The `drivers` slice are currently registered to the kernel via `phy_drivers_register`.
>>>>> +pub struct Registration {
>>>>> +    drivers: &'static [DriverType],
>>>>> +}
>>>>
>>>> You did not reply to my suggestion [2] to remove this type,
>>>> what do you think?
>>>>
>>>> [2]: https://lore.kernel.org/rust-for-linux/[e-mail]/
>>>
>>> I tried before but I'm not sure it simplifies the implementation.
>>>
>>> Firstly, instead of Reservation, we need a public function like
>>>
>>> pub fn phy_drivers_register(module: &'static crate::ThisModule, drivers: &[DriverVTable]) -> Result {
>>>       to_result(unsafe {
>>>           bindings::phy_drivers_register(drivers[0].0.get(), drivers.len().try_into()?, module.0)
>>>       })
>>> }
>>>
>>> This is because module.0 is private.
>>
>> Why can't this be part of the macro?
> 
> I'm not sure I correctly understand what you suggest so you meant the following?
> 
>      (drivers: [$($driver:ident),+], device_table: [$($dev:expr),+], $($f:tt)*) => {
>          struct Module {
>               _drv:  [
>                  ::kernel::net::phy::DriverVTable;
>                  $crate::module_phy_driver!(@count_devices $($driver),+)
>              ],
>          }
>          unsafe impl Sync for Module {}
> 
>          $crate::prelude::module! {
>              type: Module,
>              $($f)*
>          }
> 
>          impl ::kernel::Module for Module {
>              fn init(module: &'static ThisModule) -> Result<Self> {
>                  let drv = [
>                      $(::kernel::net::phy::create_phy_driver::<$driver>()),+
>                  ];
>                  ::kernel::error::to_result(unsafe {
>                      ::kernel::bindings::phy_drivers_register(drv[0].0.get(), drv.len().try_into()?, module.0)

You can just do this (I omitted the `::kernel::` prefix for
readability, if you add this in the macro, please include it):

     // CAST: `DriverVTable` is `repr(transparent)` and wrapping `bindings::phy_driver`.
     let ptr = drv.as_mut_ptr().cast::<bindings::phy_driver>();
     let len = drv.len().try_into()?;
     // SAFETY: ...
     to_result(unsafe { bindings::phy_drivers_register(ptr, len, module.0) })?;

>                  })?;
> 
>                  Ok(Module {
>                      _drv: drv,
>                  })
>              }
>          }
> 
> Then we got the following error:
> 
> error[E0616]: field `0` of struct `DriverVTable` is private
>    --> drivers/net/phy/ax88796b_rust.rs:12:1
>       |
>       12 | / kernel::module_phy_driver! {
>       13 | |     drivers: [PhyAX88772A, PhyAX88772C, PhyAX88796B],
>       14 | |     device_table: [
>       15 | |         DeviceId::new_with_driver::<PhyAX88772A>(),
>       ...  |
>       22 | |     license: "GPL",
>       23 | | }
>          | |_^ private field
> 	   |
> 	      = note: this error originates in the macro
> 	      `kernel::module_phy_driver` (in Nightly builds, run with
> 	      -Z macro-backtrace for more info)
> 
> error[E0616]: field `0` of struct `kernel::ThisModule` is private
>    --> drivers/net/phy/ax88796b_rust.rs:12:1
>       |
>       12 | / kernel::module_phy_driver! {
>       13 | |     drivers: [PhyAX88772A, PhyAX88772C, PhyAX88796B],
>       14 | |     device_table: [
>       15 | |         DeviceId::new_with_driver::<PhyAX88772A>(),
>       ...  |
>       22 | |     license: "GPL",
>       23 | | }
>          | |_^ private field
> 
> 
>>> Also if we keep DriverVtable.0 private, we need another public function.
>>>
>>> pub unsafe fn phy_drivers_unregister(drivers: &'static [DriverVTable])
>>> {
>>>       unsafe {
>>>           bindings::phy_drivers_unregister(drivers[0].0.get(), drivers.len() as i32)
>>>       };
>>> }
>>>
>>> DriverVTable isn't guaranteed to be registered to the kernel so needs
>>> to be unsafe, I guesss.
>>
>> In one of the options I suggest to make that an invariant of `DriverVTable`.
>>
>>>
>>> Also Module trait support exit()?
>>
>> Yes, just implement `Drop` and do the cleanup there.
>>
>> In the two options that I suggested there is a trade off. I do not know
>> which option is better, I hoped that you or Andrew would know more:
>> Option 1:
>> * advantages:
>>     - manual creation of a phy driver module becomes possible.
>>     - less complex `module_phy_driver` macro.
>>     - no static variable needed.
>> * disadvantages:
>>     - calls `phy_drivers_register` for every driver on module
>>       initialization.
>>     - calls `phy_drivers_unregister` for every driver on module
>>       exit.
>>
>> Option 2:
>> * advantages:
>>     - less complex `module_phy_driver` macro.
>>     - no static variable needed.
>>     - only a single call to
>>       `phy_drivers_register`/`phy_drivers_unregister`.
>> * disadvantages:
>>     - no safe manual creation of phy drivers possible, the only safe
>>       way is to use the `module_phy_driver` macro.
>>
>> I suppose that it would be ok to call the register function multiple
>> times, since it only is on module startup/shutdown and it is not
>> performance critical.
> 
> I think that we can use the current implantation using Reservation
> struct until someone requests manual creation. I doubt that we will
> need to support such.

I would like to remove the mutable static variable and simplify
the macro.

FUJITA Tomonori Oct. 19, 2023, 3:32 p.m. UTC | #9

On Thu, 19 Oct 2023 15:20:51 +0000
Benno Lossin <[e-mail]> wrote:

> On 19.10.23 16:42, FUJITA Tomonori wrote:
>>>>>> +/// Registration structure for a PHY driver.
>>>>>> +///
>>>>>> +/// # Invariants
>>>>>> +///
>>>>>> +/// The `drivers` slice are currently registered to the kernel via `phy_drivers_register`.
>>>>>> +pub struct Registration {
>>>>>> +    drivers: &'static [DriverType],
>>>>>> +}
>>>>>
>>>>> You did not reply to my suggestion [2] to remove this type,
>>>>> what do you think?
>>>>>
>>>>> [2]: https://lore.kernel.org/rust-for-linux/[e-mail]/
>>>>
>>>> I tried before but I'm not sure it simplifies the implementation.
>>>>
>>>> Firstly, instead of Reservation, we need a public function like
>>>>
>>>> pub fn phy_drivers_register(module: &'static crate::ThisModule, drivers: &[DriverVTable]) -> Result {
>>>>       to_result(unsafe {
>>>>           bindings::phy_drivers_register(drivers[0].0.get(), drivers.len().try_into()?, module.0)
>>>>       })
>>>> }
>>>>
>>>> This is because module.0 is private.
>>>
>>> Why can't this be part of the macro?
>> 
>> I'm not sure I correctly understand what you suggest so you meant the following?
>> 
>>      (drivers: [$($driver:ident),+], device_table: [$($dev:expr),+], $($f:tt)*) => {
>>          struct Module {
>>               _drv:  [
>>                  ::kernel::net::phy::DriverVTable;
>>                  $crate::module_phy_driver!(@count_devices $($driver),+)
>>              ],
>>          }
>>          unsafe impl Sync for Module {}
>> 
>>          $crate::prelude::module! {
>>              type: Module,
>>              $($f)*
>>          }
>> 
>>          impl ::kernel::Module for Module {
>>              fn init(module: &'static ThisModule) -> Result<Self> {
>>                  let drv = [
>>                      $(::kernel::net::phy::create_phy_driver::<$driver>()),+
>>                  ];
>>                  ::kernel::error::to_result(unsafe {
>>                      ::kernel::bindings::phy_drivers_register(drv[0].0.get(), drv.len().try_into()?, module.0)
> 
> You can just do this (I omitted the `::kernel::` prefix for
> readability, if you add this in the macro, please include it):
> 
>      // CAST: `DriverVTable` is `repr(transparent)` and wrapping `bindings::phy_driver`.
>      let ptr = drv.as_mut_ptr().cast::<bindings::phy_driver>();
>      let len = drv.len().try_into()?;
>      // SAFETY: ...
>      to_result(unsafe { bindings::phy_drivers_register(ptr, len, module.0) })?;
> 
>>                  })?;

The above solves DriverVTable.0 but still the macro can't access to
kernel::ThisModule.0. I got the following error:

error[E0616]: field `0` of struct `kernel::ThisModule` is private
  --> drivers/net/phy/ax88796b_rust.rs:12:1
     |
     12 | / kernel::module_phy_driver! {
     13 | |     drivers: [PhyAX88772A, PhyAX88772C, PhyAX88796B],
     14 | |     device_table: [
     15 | |         DeviceId::new_with_driver::<PhyAX88772A>(),
     ...  |
     22 | |     license: "GPL",
     23 | | }
        | |_^ private field
	   |
	      = note: this error originates in the macro
	      `kernel::module_phy_driver` (in Nightly builds, run with
	      -Z macro-backtrace for more info)


>>                  Ok(Module {
>>                      _drv: drv,
>>                  })
>>              }
>>          }
>> 
>> Then we got the following error:
>> 
>> error[E0616]: field `0` of struct `DriverVTable` is private
>>    --> drivers/net/phy/ax88796b_rust.rs:12:1
>>       |
>>       12 | / kernel::module_phy_driver! {
>>       13 | |     drivers: [PhyAX88772A, PhyAX88772C, PhyAX88796B],
>>       14 | |     device_table: [
>>       15 | |         DeviceId::new_with_driver::<PhyAX88772A>(),
>>       ...  |
>>       22 | |     license: "GPL",
>>       23 | | }
>>          | |_^ private field
>> 	   |
>> 	      = note: this error originates in the macro
>> 	      `kernel::module_phy_driver` (in Nightly builds, run with
>> 	      -Z macro-backtrace for more info)
>> 
>> error[E0616]: field `0` of struct `kernel::ThisModule` is private
>>    --> drivers/net/phy/ax88796b_rust.rs:12:1
>>       |
>>       12 | / kernel::module_phy_driver! {
>>       13 | |     drivers: [PhyAX88772A, PhyAX88772C, PhyAX88796B],
>>       14 | |     device_table: [
>>       15 | |         DeviceId::new_with_driver::<PhyAX88772A>(),
>>       ...  |
>>       22 | |     license: "GPL",
>>       23 | | }
>>          | |_^ private field
>> 
>> 
>>>> Also if we keep DriverVtable.0 private, we need another public function.
>>>>
>>>> pub unsafe fn phy_drivers_unregister(drivers: &'static [DriverVTable])
>>>> {
>>>>       unsafe {
>>>>           bindings::phy_drivers_unregister(drivers[0].0.get(), drivers.len() as i32)
>>>>       };
>>>> }
>>>>
>>>> DriverVTable isn't guaranteed to be registered to the kernel so needs
>>>> to be unsafe, I guesss.
>>>
>>> In one of the options I suggest to make that an invariant of `DriverVTable`.
>>>
>>>>
>>>> Also Module trait support exit()?
>>>
>>> Yes, just implement `Drop` and do the cleanup there.
>>>
>>> In the two options that I suggested there is a trade off. I do not know
>>> which option is better, I hoped that you or Andrew would know more:
>>> Option 1:
>>> * advantages:
>>>     - manual creation of a phy driver module becomes possible.
>>>     - less complex `module_phy_driver` macro.
>>>     - no static variable needed.
>>> * disadvantages:
>>>     - calls `phy_drivers_register` for every driver on module
>>>       initialization.
>>>     - calls `phy_drivers_unregister` for every driver on module
>>>       exit.
>>>
>>> Option 2:
>>> * advantages:
>>>     - less complex `module_phy_driver` macro.
>>>     - no static variable needed.
>>>     - only a single call to
>>>       `phy_drivers_register`/`phy_drivers_unregister`.
>>> * disadvantages:
>>>     - no safe manual creation of phy drivers possible, the only safe
>>>       way is to use the `module_phy_driver` macro.
>>>
>>> I suppose that it would be ok to call the register function multiple
>>> times, since it only is on module startup/shutdown and it is not
>>> performance critical.
>> 
>> I think that we can use the current implantation using Reservation
>> struct until someone requests manual creation. I doubt that we will
>> need to support such.
> 
> I would like to remove the mutable static variable and simplify
> the macro.

It's worse than having public unsafe function (phy_drivers_unregister)?

Benno Lossin Oct. 19, 2023, 4:37 p.m. UTC | #10

On 19.10.23 17:32, FUJITA Tomonori wrote:
>> You can just do this (I omitted the `::kernel::` prefix for
>> readability, if you add this in the macro, please include it):
>>
>>       // CAST: `DriverVTable` is `repr(transparent)` and wrapping `bindings::phy_driver`.
>>       let ptr = drv.as_mut_ptr().cast::<bindings::phy_driver>();
>>       let len = drv.len().try_into()?;
>>       // SAFETY: ...
>>       to_result(unsafe { bindings::phy_drivers_register(ptr, len, module.0) })?;
>>
>>>                   })?;
> 
> The above solves DriverVTable.0 but still the macro can't access to
> kernel::ThisModule.0. I got the following error:

I think we could just provide an `as_ptr` getter function
for `ThisModule`. But need to check with the others.

[...]

>>>> I suppose that it would be ok to call the register function multiple
>>>> times, since it only is on module startup/shutdown and it is not
>>>> performance critical.
>>>
>>> I think that we can use the current implantation using Reservation
>>> struct until someone requests manual creation. I doubt that we will
>>> need to support such.
>>
>> I would like to remove the mutable static variable and simplify
>> the macro.
> 
> It's worse than having public unsafe function (phy_drivers_unregister)?

Why would that function have to be public?

FUJITA Tomonori Oct. 19, 2023, 9:51 p.m. UTC | #11

On Thu, 19 Oct 2023 16:37:46 +0000
Benno Lossin <[e-mail]> wrote:

> On 19.10.23 17:32, FUJITA Tomonori wrote:
>>> You can just do this (I omitted the `::kernel::` prefix for
>>> readability, if you add this in the macro, please include it):
>>>
>>>       // CAST: `DriverVTable` is `repr(transparent)` and wrapping `bindings::phy_driver`.
>>>       let ptr = drv.as_mut_ptr().cast::<bindings::phy_driver>();
>>>       let len = drv.len().try_into()?;
>>>       // SAFETY: ...
>>>       to_result(unsafe { bindings::phy_drivers_register(ptr, len, module.0) })?;
>>>
>>>>                   })?;
>> 
>> The above solves DriverVTable.0 but still the macro can't access to
>> kernel::ThisModule.0. I got the following error:
> 
> I think we could just provide an `as_ptr` getter function
> for `ThisModule`. But need to check with the others.
> 

ThisModule.0 is *mut bindings::module. Drivers should not use
bindings?


>>>>> I suppose that it would be ok to call the register function multiple
>>>>> times, since it only is on module startup/shutdown and it is not
>>>>> performance critical.
>>>>
>>>> I think that we can use the current implantation using Reservation
>>>> struct until someone requests manual creation. I doubt that we will
>>>> need to support such.
>>>
>>> I would like to remove the mutable static variable and simplify
>>> the macro.
>> 
>> It's worse than having public unsafe function (phy_drivers_unregister)?
> 
> Why would that function have to be public?

If we don't make ThisModule.0 public, phy_drivers_unregister has to be
public.

FUJITA Tomonori Oct. 20, 2023, 12:34 a.m. UTC | #12

On Thu, 19 Oct 2023 15:20:51 +0000
Benno Lossin <[e-mail]> wrote:

> I would like to remove the mutable static variable and simplify
> the macro.

How about adding DriverVTable array to Registration?

/// Registration structure for a PHY driver.
///
/// # Invariants
///
/// The `drivers` slice are currently registered to the kernel via `phy_drivers_register`.
pub struct Registration<const N: usize> {
    drivers: [DriverVTable; N],
}

impl<const N: usize> Registration<{ N }> {
    /// Registers a PHY driver.
    pub fn register(
        module: &'static crate::ThisModule,
        drivers: [DriverVTable; N],
    ) -> Result<Self> {
        let mut reg = Registration { drivers };
        let ptr = reg.drivers.as_mut_ptr().cast::<bindings::phy_driver>();
        // SAFETY: The type invariants of [`DriverVTable`] ensure that all elements of the `drivers` slice
        // are initialized properly. So an FFI call with a valid pointer.
        to_result(unsafe {
            bindings::phy_drivers_register(ptr, reg.drivers.len().try_into()?, module.0)
        })?;
        // INVARIANT: The `drivers` slice is successfully registered to the kernel via `phy_drivers_register`.
        Ok(reg)
    }
}

Then the macro becomes simpler. No need to add any public
functions. Also I think that this approach supports the manual
registration.


    (drivers: [$($driver:ident),+], device_table: [$($dev:expr),+], $($f:tt)*) => {
        const N: usize = $crate::module_phy_driver!(@count_devices $($driver),+);
        struct Module {
            _reg: ::kernel::net::phy::Registration<N>,
        }

        $crate::prelude::module! {
            type: Module,
            $($f)*
        }

        impl ::kernel::Module for Module {
            fn init(module: &'static ThisModule) -> Result<Self> {
                let drivers = [$(::kernel::net::phy::create_phy_driver::<$driver>()),+];
                let reg = ::kernel::net::phy::Registration::register(module, drivers)?;
                Ok(Module { _reg: reg })
            }
        }

        $crate::module_phy_driver!(@device_table [$($dev),+]);
    }

FUJITA Tomonori Oct. 20, 2023, 12:54 p.m. UTC | #13

On Fri, 20 Oct 2023 09:34:46 +0900 (JST)
FUJITA Tomonori <[e-mail]> wrote:

> On Thu, 19 Oct 2023 15:20:51 +0000
> Benno Lossin <[e-mail]> wrote:
> 
>> I would like to remove the mutable static variable and simplify
>> the macro.
> 
> How about adding DriverVTable array to Registration?
> 
> /// Registration structure for a PHY driver.
> ///
> /// # Invariants
> ///
> /// The `drivers` slice are currently registered to the kernel via `phy_drivers_register`.
> pub struct Registration<const N: usize> {
>     drivers: [DriverVTable; N],
> }
> 
> impl<const N: usize> Registration<{ N }> {
>     /// Registers a PHY driver.
>     pub fn register(
>         module: &'static crate::ThisModule,
>         drivers: [DriverVTable; N],
>     ) -> Result<Self> {
>         let mut reg = Registration { drivers };
>         let ptr = reg.drivers.as_mut_ptr().cast::<bindings::phy_driver>();
>         // SAFETY: The type invariants of [`DriverVTable`] ensure that all elements of the `drivers` slice
>         // are initialized properly. So an FFI call with a valid pointer.
>         to_result(unsafe {
>             bindings::phy_drivers_register(ptr, reg.drivers.len().try_into()?, module.0)
>         })?;
>         // INVARIANT: The `drivers` slice is successfully registered to the kernel via `phy_drivers_register`.
>         Ok(reg)
>     }
> }

Scratch this.

This doesn't work. Also simply putting slice of DriverVTable into
Module strcut doesn't work.

We cannot move a slice of DriverVTable. Except for static allocation,
is there a simple way?

Andreas Hindborg Oct. 20, 2023, 5:26 p.m. UTC | #14

Hi,

FUJITA Tomonori <[e-mail]> writes:

<cut>

> +
> +    /// Returns true if the link is up.
> +    pub fn get_link(&self) -> bool {
> +        const LINK_IS_UP: u32 = 1;
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        let phydev = unsafe { *self.0.get() };
> +        phydev.link() == LINK_IS_UP
> +    }

I would prefer `is_link_up` or `link_is_up`.

> +
> +    /// Returns true if auto-negotiation is enabled.
> +    pub fn is_autoneg_enabled(&self) -> bool {
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        let phydev = unsafe { *self.0.get() };
> +        phydev.autoneg() == bindings::AUTONEG_ENABLE
> +    }
> +
> +    /// Returns true if auto-negotiation is completed.
> +    pub fn is_autoneg_completed(&self) -> bool {
> +        const AUTONEG_COMPLETED: u32 = 1;
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        let phydev = unsafe { *self.0.get() };
> +        phydev.autoneg_complete() == AUTONEG_COMPLETED
> +    }
> +
> +    /// Sets the speed of the PHY.
> +    pub fn set_speed(&mut self, speed: u32) {
> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        unsafe { (*phydev).speed = speed as i32 };
> +    }

If this function is called with `u32::MAX` `(*phydev).speed` will become -1. Is that OK?

<cut>

> +
> +/// An instance of a PHY driver.
> +///
> +/// Wraps the kernel's `struct phy_driver`.
> +///
> +/// # Invariants
> +///
> +/// `self.0` is always in a valid state.
> +#[repr(transparent)]
> +pub struct DriverType(Opaque<bindings::phy_driver>);

I don't like the name `DriverType`. How about `DriverDesciptor` or
something like that?

<cut>

> +
> +/// Corresponds to functions in `struct phy_driver`.
> +///
> +/// This is used to register a PHY driver.
> +#[vtable]
> +pub trait Driver {
> +    /// Defines certain other features this PHY supports.
> +    /// It is a combination of the flags in the [`flags`] module.
> +    const FLAGS: u32 = 0;
> +
> +    /// The friendly name of this PHY type.
> +    const NAME: &'static CStr;
> +
> +    /// This driver only works for PHYs with IDs which match this field.
> +    /// The default id and mask are zero.
> +    const PHY_DEVICE_ID: DeviceId = DeviceId::new_with_custom_mask(0, 0);
> +
> +    /// Issues a PHY software reset.
> +    fn soft_reset(_dev: &mut Device) -> Result {
> +        Err(code::ENOTSUPP)
> +    }
> +
> +    /// Probes the hardware to determine what abilities it has.
> +    fn get_features(_dev: &mut Device) -> Result {
> +        Err(code::ENOTSUPP)
> +    }
> +
> +    /// Returns true if this is a suitable driver for the given phydev.
> +    /// If not implemented, matching is based on [`Driver::PHY_DEVICE_ID`].
> +    fn match_phy_device(_dev: &Device) -> bool {
> +        false
> +    }
> +
> +    /// Configures the advertisement and resets auto-negotiation
> +    /// if auto-negotiation is enabled.
> +    fn config_aneg(_dev: &mut Device) -> Result {
> +        Err(code::ENOTSUPP)
> +    }
> +
> +    /// Determines the negotiated speed and duplex.
> +    fn read_status(_dev: &mut Device) -> Result<u16> {
> +        Err(code::ENOTSUPP)
> +    }
> +
> +    /// Suspends the hardware, saving state if needed.
> +    fn suspend(_dev: &mut Device) -> Result {
> +        Err(code::ENOTSUPP)
> +    }
> +
> +    /// Resumes the hardware, restoring state if needed.
> +    fn resume(_dev: &mut Device) -> Result {
> +        Err(code::ENOTSUPP)
> +    }
> +
> +    /// Overrides the default MMD read function for reading a MMD register.
> +    fn read_mmd(_dev: &mut Device, _devnum: u8, _regnum: u16) -> Result<u16> {
> +        Err(code::ENOTSUPP)
> +    }
> +
> +    /// Overrides the default MMD write function for writing a MMD register.
> +    fn write_mmd(_dev: &mut Device, _devnum: u8, _regnum: u16, _val: u16) -> Result {
> +        Err(code::ENOTSUPP)
> +    }
> +
> +    /// Callback for notification of link change.
> +    fn link_change_notify(_dev: &mut Device) {}

It is probably an error if these functions are called, and so BUG() would be
appropriate? See the discussion in [1].

[1] https://lore.kernel.org/rust-for-linux/[e-mail]/

<cut>

> +
> +    // macro use only
> +    #[doc(hidden)]
> +    pub const fn mdio_device_id(&self) -> bindings::mdio_device_id {
> +        bindings::mdio_device_id {
> +            phy_id: self.id,
> +            phy_id_mask: self.mask.as_int(),
> +        }
> +    }

Would it make sense to move this function to the macro patch?

Best regards,
Andreas

Benno Lossin Oct. 20, 2023, 5:56 p.m. UTC | #15

On 20.10.23 19:26, Andreas Hindborg (Samsung) wrote:
>> +
>> +    /// Overrides the default MMD write function for writing a MMD register.
>> +    fn write_mmd(_dev: &mut Device, _devnum: u8, _regnum: u16, _val: u16) -> Result {
>> +        Err(code::ENOTSUPP)
>> +    }
>> +
>> +    /// Callback for notification of link change.
>> +    fn link_change_notify(_dev: &mut Device) {}
> 
> It is probably an error if these functions are called, and so BUG() would be
> appropriate? See the discussion in [1].

Please do not use `BUG()` I wanted to wait for my patch [1] to be merged
before suggesting this, since then Tomo can then just use the constant
that I introduced.

> [1] https://lore.kernel.org/rust-for-linux/[e-mail]/

Andrew Lunn Oct. 20, 2023, 6:42 p.m. UTC | #16

> > +//! All the PHYLIB helper functions for `phy_device` modify some members in `phy_device`. Except for
> > +//! getter functions, [`Device`] methods take `&mut self`. This also applied to `read()`, which reads
> > +//! a hardware register and updates the stats.
> 
> I would use [`Device`] instead of `phy_device`, since the Rust reader
> might not be aware what wraps `phy_device`.

We don't want to hide phy_device too much, since at the moment, the
abstraction is very minimal. Anybody writing a driver is going to need
a good understanding of the C code in order to find the helpers they
need, and then add them to the abstraction. So i would say we need to
explain the relationship between the C structure and the Rust
structure, to aid developers.

> > +    /// Returns true if the link is up.
> > +    pub fn get_link(&self) -> bool {
> 
> I still think this name should be changed. My response at [1] has not yet
> been replied to. This has already been discussed before:
> - https://lore.kernel.org/rust-for-linux/2023100237-satirical-prance-bd57@gregkh/
> - https://lore.kernel.org/rust-for-linux/[e-mail]/
> - https://lore.kernel.org/rust-for-linux/[e-mail]/
> 
> And I want to suggest to change it to `is_link_up`.
> 
> Reasons why I do not like the name:
> - `get_`/`put_` are used for ref counting on the C side, I would like to
>    avoid confusion.
> - `get` in Rust is often used to fetch a value from e.g. a datastructure
>    such as a hashmap, so I expect the call to do some computation.
> - getters in Rust usually are not prefixed with `get_`, but rather are
>    just the name of the field.
> - in this case I like the name `is_link_up` much better, since code becomes
>    a lot more readable with that.
> - I do not want this pattern as an example for other drivers.
> 
> [1]: https://lore.kernel.org/rust-for-linux/[e-mail]/
> 
> > +        const LINK_IS_UP: u32 = 1;
> > +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> > +        let phydev = unsafe { *self.0.get() };
> > +        phydev.link() == LINK_IS_UP
> > +    }

During the reviews we have had a lot of misunderstanding what this
actually does, given its name. Some thought it poked around in
registers to get the current state of the link. Some thought it
triggered the PHY to establish a link. When in fact its just a dumb
getter. And we have a few other dumb getters and setters.

So i would prefer something which indicates its a dumb getter. If the
norm of Rust is just the field name, lets just use the field name. But
we should do that for all the getters and setters. Is there a naming
convention for things which take real actions?

And maybe we need to add a comment: Get the current link state, as
stored in the [`Device`]. Set the duplex value in [`Device`], etc.

   Andrew

Andrew Lunn Oct. 20, 2023, 7:50 p.m. UTC | #17

> I will try to explain things a bit more.
> 
> So this case is a bit difficult to figure out, because what is
> going on is not really a pattern that is used in Rust.

It is however a reasonably common pattern in the kernel. It stems from
driver writers often don't understand locking. So the core does the
locking, leaving the driver writers to just handle the problems of the
hardware.

Rust maybe makes locking more of a visible issue, but if driver
writers don't understand locking, the language itself does not make
much difference.

> We already have exclusive access to the `phy_device`, so in Rust
> you would not need to lock anything to also have exclusive access to the
> embedded `mii_bus`.

I would actually say its not the PHY drivers problem at all. The
mii_bus is a property of the MDIO layers, and it is the MDIO layers
problem to impose whatever locking it needs for its properties.

Also, mii_bus is not embedded. Its a pointer to an mii_bus. The phy
lock protects the pointer. But its the MDIO layer which needs to
protect what the pointer points to.

	Andrew

Andrew Lunn Oct. 20, 2023, 7:59 p.m. UTC | #18

On Fri, Oct 20, 2023 at 07:26:50PM +0200, Andreas Hindborg (Samsung) wrote:
> 
> Hi,
> 
> FUJITA Tomonori <[e-mail]> writes:
> 
> <cut>
> 
> > +
> > +    /// Returns true if the link is up.
> > +    pub fn get_link(&self) -> bool {
> > +        const LINK_IS_UP: u32 = 1;
> > +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> > +        let phydev = unsafe { *self.0.get() };
> > +        phydev.link() == LINK_IS_UP
> > +    }
> 
> I would prefer `is_link_up` or `link_is_up`.

Hi Andreas

Have you read the comment on the previous versions of this patch. It
seems like everybody wants a different name for this, and we are going
round and round and round. Please could you spend the time to read all
the previous comments and then see if you still want this name, and
explain why. Otherwise i doubt we are going to reach consensus.

> If this function is called with `u32::MAX` `(*phydev).speed` will become -1. Is that OK?

Have you ever seen a Copper Ethernet interface which can do u32:MAX
Mbps? Do you think such a thing will ever be possible?

> > +    /// Callback for notification of link change.
> > +    fn link_change_notify(_dev: &mut Device) {}
> 
> It is probably an error if these functions are called, and so BUG() would be
> appropriate? See the discussion in [1].

Do you really want to kill the machine dead, no syncing of the disk,
loose everything not yet written to the disk, maybe corrupt the disk
etc?

      Andrew

Andreas Hindborg Oct. 20, 2023, 8:30 p.m. UTC | #19

Andrew Lunn <[e-mail]> writes:

> On Fri, Oct 20, 2023 at 07:26:50PM +0200, Andreas Hindborg (Samsung) wrote:
>> 
>> Hi,
>> 
>> FUJITA Tomonori <[e-mail]> writes:
>> 
>> <cut>
>> 
>> > +
>> > +    /// Returns true if the link is up.
>> > +    pub fn get_link(&self) -> bool {
>> > +        const LINK_IS_UP: u32 = 1;
>> > +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
>> > +        let phydev = unsafe { *self.0.get() };
>> > +        phydev.link() == LINK_IS_UP
>> > +    }
>> 
>> I would prefer `is_link_up` or `link_is_up`.
>
> Hi Andreas
>
> Have you read the comment on the previous versions of this patch. It
> seems like everybody wants a different name for this, and we are going
> round and round and round. Please could you spend the time to read all
> the previous comments and then see if you still want this name, and
> explain why. Otherwise i doubt we are going to reach consensus.

Thanks, I'll read through it.

>
>> If this function is called with `u32::MAX` `(*phydev).speed` will become -1. Is that OK?
>
> Have you ever seen a Copper Ethernet interface which can do u32:MAX
> Mbps? Do you think such a thing will ever be possible?

Probably not. Maybe a dummy device for testing? I don't know if it would
be OK with a negative value, hence the question. If things break with a
negative value, it makes sense to force the value into the valid range.
Make it impossible to break it, instead of relying on nobody ever doing
things you did not expect.

>
>> > +    /// Callback for notification of link change.
>> > +    fn link_change_notify(_dev: &mut Device) {}
>> 
>> It is probably an error if these functions are called, and so BUG() would be
>> appropriate? See the discussion in [1].
>
> Do you really want to kill the machine dead, no syncing of the disk,
> loose everything not yet written to the disk, maybe corrupt the disk
> etc?

A WARN then? Benno suggests a compile time error, that is probably a
better approach. The code should not be called, so I would really want
to know if that was ever the case.

BR Andreas

FUJITA Tomonori Oct. 21, 2023, 3:49 a.m. UTC | #20

On Fri, 20 Oct 2023 22:30:49 +0200
"Andreas Hindborg (Samsung)" <[e-mail]> wrote:

>>> If this function is called with `u32::MAX` `(*phydev).speed` will become -1. Is that OK?
>>
>> Have you ever seen a Copper Ethernet interface which can do u32:MAX
>> Mbps? Do you think such a thing will ever be possible?
> 
> Probably not. Maybe a dummy device for testing? I don't know if it would
> be OK with a negative value, hence the question. If things break with a
> negative value, it makes sense to force the value into the valid range.
> Make it impossible to break it, instead of relying on nobody ever doing
> things you did not expect.

You can find discussion on this in the previous comments too.


>>> > +    /// Callback for notification of link change.
>>> > +    fn link_change_notify(_dev: &mut Device) {}
>>> 
>>> It is probably an error if these functions are called, and so BUG() would be
>>> appropriate? See the discussion in [1].
>>
>> Do you really want to kill the machine dead, no syncing of the disk,
>> loose everything not yet written to the disk, maybe corrupt the disk
>> etc?
> 
> A WARN then? Benno suggests a compile time error, that is probably a
> better approach. The code should not be called, so I would really want
> to know if that was ever the case.

These functions are never called so I don't think that WARN or
whatever doesn't matter. The api of vtable macro is misleading so I'm
not sure it's worth trying something with the api. I would prefer to
leave this alone until Benno's work.

FUJITA Tomonori Oct. 21, 2023, 4:01 a.m. UTC | #21

On Fri, 20 Oct 2023 19:26:50 +0200
"Andreas Hindborg (Samsung)" <[e-mail]> wrote:

>> +/// An instance of a PHY driver.
>> +///
>> +/// Wraps the kernel's `struct phy_driver`.
>> +///
>> +/// # Invariants
>> +///
>> +/// `self.0` is always in a valid state.
>> +#[repr(transparent)]
>> +pub struct DriverType(Opaque<bindings::phy_driver>);
> 
> I don't like the name `DriverType`. How about `DriverDesciptor` or
> something like that?

Benno suggested DriverVTable. I plan to use that name in the next
version.

>> +
>> +    // macro use only
>> +    #[doc(hidden)]
>> +    pub const fn mdio_device_id(&self) -> bindings::mdio_device_id {
>> +        bindings::mdio_device_id {
>> +            phy_id: self.id,
>> +            phy_id_mask: self.mask.as_int(),
>> +        }
>> +    }
> 
> Would it make sense to move this function to the macro patch?

IMHO, either is fine.

You could say that the function is used only in the macro but also you
could say that this is the method of DeviceId.

FUJITA Tomonori Oct. 21, 2023, 4:44 a.m. UTC | #22

On Fri, 20 Oct 2023 20:42:10 +0200
Andrew Lunn <[e-mail]> wrote:

>> > +//! All the PHYLIB helper functions for `phy_device` modify some members in `phy_device`. Except for
>> > +//! getter functions, [`Device`] methods take `&mut self`. This also applied to `read()`, which reads
>> > +//! a hardware register and updates the stats.
>> 
>> I would use [`Device`] instead of `phy_device`, since the Rust reader
>> might not be aware what wraps `phy_device`.
> 
> We don't want to hide phy_device too much, since at the moment, the
> abstraction is very minimal. Anybody writing a driver is going to need
> a good understanding of the C code in order to find the helpers they
> need, and then add them to the abstraction. So i would say we need to
> explain the relationship between the C structure and the Rust
> structure, to aid developers.

I'm sure that Rust readers would notice Device wraps `phy_device`
because the comment on Device clearly says so.

/// An instance of a PHY device.
///
/// Wraps the kernel's `struct phy_device`.
///
/// # Invariants
///
/// `self.0` is always in a valid state.
#[repr(transparent)]
pub struct Device(Opaque<bindings::phy_device>);


I think that the relationships between the C and Rust structures are
documented in phy.rs.

Benno Lossin Oct. 21, 2023, 7:21 a.m. UTC | #23

On 19.10.23 23:51, FUJITA Tomonori wrote:
> On Thu, 19 Oct 2023 16:37:46 +0000
> Benno Lossin <[e-mail]> wrote:
> 
>> On 19.10.23 17:32, FUJITA Tomonori wrote:
>>>> You can just do this (I omitted the `::kernel::` prefix for
>>>> readability, if you add this in the macro, please include it):
>>>>
>>>>        // CAST: `DriverVTable` is `repr(transparent)` and wrapping `bindings::phy_driver`.
>>>>        let ptr = drv.as_mut_ptr().cast::<bindings::phy_driver>();
>>>>        let len = drv.len().try_into()?;
>>>>        // SAFETY: ...
>>>>        to_result(unsafe { bindings::phy_drivers_register(ptr, len, module.0) })?;
>>>>
>>>>>                    })?;
>>>
>>> The above solves DriverVTable.0 but still the macro can't access to
>>> kernel::ThisModule.0. I got the following error:
>>
>> I think we could just provide an `as_ptr` getter function
>> for `ThisModule`. But need to check with the others.
>>
> 
> ThisModule.0 is *mut bindings::module. Drivers should not use
> bindings?

This is a special case, since it `module` is used on a lot of functions
(and it does not make sense to provide abstractions for those on
`ThisModule`). Additionally, `ThisModule` already has a public `from_raw`
function that takes a `*mut bindings::module`.

If you add a `as_ptr` function, please create a separate patch for it.

Benno Lossin Oct. 21, 2023, 7:25 a.m. UTC | #24

On 20.10.23 14:54, FUJITA Tomonori wrote:
> On Fri, 20 Oct 2023 09:34:46 +0900 (JST)
> FUJITA Tomonori <[e-mail]> wrote:
> 
>> On Thu, 19 Oct 2023 15:20:51 +0000
>> Benno Lossin <[e-mail]> wrote:
>>
>>> I would like to remove the mutable static variable and simplify
>>> the macro.
>>
>> How about adding DriverVTable array to Registration?
>>
>> /// Registration structure for a PHY driver.
>> ///
>> /// # Invariants
>> ///
>> /// The `drivers` slice are currently registered to the kernel via `phy_drivers_register`.
>> pub struct Registration<const N: usize> {
>>      drivers: [DriverVTable; N],
>> }
>>
>> impl<const N: usize> Registration<{ N }> {
>>      /// Registers a PHY driver.
>>      pub fn register(
>>          module: &'static crate::ThisModule,
>>          drivers: [DriverVTable; N],
>>      ) -> Result<Self> {
>>          let mut reg = Registration { drivers };
>>          let ptr = reg.drivers.as_mut_ptr().cast::<bindings::phy_driver>();
>>          // SAFETY: The type invariants of [`DriverVTable`] ensure that all elements of the `drivers` slice
>>          // are initialized properly. So an FFI call with a valid pointer.
>>          to_result(unsafe {
>>              bindings::phy_drivers_register(ptr, reg.drivers.len().try_into()?, module.0)
>>          })?;
>>          // INVARIANT: The `drivers` slice is successfully registered to the kernel via `phy_drivers_register`.
>>          Ok(reg)
>>      }
>> }
> 
> Scratch this.
> 
> This doesn't work. Also simply putting slice of DriverVTable into
> Module strcut doesn't work.

Why does it not work? I tried it and it compiled fine for me.

> We cannot move a slice of DriverVTable. Except for static allocation,
> is there a simple way?

I do not know what you are referring to, you can certainly move an array
of `DriverVTable`s.

FUJITA Tomonori Oct. 21, 2023, 7:30 a.m. UTC | #25

On Sat, 21 Oct 2023 07:25:17 +0000
Benno Lossin <[e-mail]> wrote:

> On 20.10.23 14:54, FUJITA Tomonori wrote:
>> On Fri, 20 Oct 2023 09:34:46 +0900 (JST)
>> FUJITA Tomonori <[e-mail]> wrote:
>> 
>>> On Thu, 19 Oct 2023 15:20:51 +0000
>>> Benno Lossin <[e-mail]> wrote:
>>>
>>>> I would like to remove the mutable static variable and simplify
>>>> the macro.
>>>
>>> How about adding DriverVTable array to Registration?
>>>
>>> /// Registration structure for a PHY driver.
>>> ///
>>> /// # Invariants
>>> ///
>>> /// The `drivers` slice are currently registered to the kernel via `phy_drivers_register`.
>>> pub struct Registration<const N: usize> {
>>>      drivers: [DriverVTable; N],
>>> }
>>>
>>> impl<const N: usize> Registration<{ N }> {
>>>      /// Registers a PHY driver.
>>>      pub fn register(
>>>          module: &'static crate::ThisModule,
>>>          drivers: [DriverVTable; N],
>>>      ) -> Result<Self> {
>>>          let mut reg = Registration { drivers };
>>>          let ptr = reg.drivers.as_mut_ptr().cast::<bindings::phy_driver>();
>>>          // SAFETY: The type invariants of [`DriverVTable`] ensure that all elements of the `drivers` slice
>>>          // are initialized properly. So an FFI call with a valid pointer.
>>>          to_result(unsafe {
>>>              bindings::phy_drivers_register(ptr, reg.drivers.len().try_into()?, module.0)
>>>          })?;
>>>          // INVARIANT: The `drivers` slice is successfully registered to the kernel via `phy_drivers_register`.
>>>          Ok(reg)
>>>      }
>>> }
>> 
>> Scratch this.
>> 
>> This doesn't work. Also simply putting slice of DriverVTable into
>> Module strcut doesn't work.
> 
> Why does it not work? I tried it and it compiled fine for me.

You can compile but the kernel crashes. The addresses of the callback
functions are invalid.


>> We cannot move a slice of DriverVTable. Except for static allocation,
>> is there a simple way?
> 
> I do not know what you are referring to, you can certainly move an array
> of `DriverVTable`s.
> 
> -- 
> Cheers,
> Benno
> 
> 
>

Benno Lossin Oct. 21, 2023, 7:36 a.m. UTC | #26

On 20.10.23 20:42, Andrew Lunn wrote:
>>> +//! All the PHYLIB helper functions for `phy_device` modify some members in `phy_device`. Except for
>>> +//! getter functions, [`Device`] methods take `&mut self`. This also applied to `read()`, which reads
>>> +//! a hardware register and updates the stats.
>>
>> I would use [`Device`] instead of `phy_device`, since the Rust reader
>> might not be aware what wraps `phy_device`.
> 
> We don't want to hide phy_device too much, since at the moment, the
> abstraction is very minimal. Anybody writing a driver is going to need
> a good understanding of the C code in order to find the helpers they
> need, and then add them to the abstraction. So i would say we need to
> explain the relationship between the C structure and the Rust
> structure, to aid developers.

There is a comment on `Device` that explains that it wraps `phy_device`.
Since [`Device`] is a link, readers who do not know what it means can
immediately click the link and find out. This is not possible with
`phy_device`, since you have to search the web for it, so I would
prefer to use the link.

>>> +    /// Returns true if the link is up.
>>> +    pub fn get_link(&self) -> bool {
>>
>> I still think this name should be changed. My response at [1] has not yet
>> been replied to. This has already been discussed before:
>> - https://lore.kernel.org/rust-for-linux/2023100237-satirical-prance-bd57@gregkh/
>> - https://lore.kernel.org/rust-for-linux/[e-mail]/
>> - https://lore.kernel.org/rust-for-linux/[e-mail]/
>>
>> And I want to suggest to change it to `is_link_up`.
>>
>> Reasons why I do not like the name:
>> - `get_`/`put_` are used for ref counting on the C side, I would like to
>>     avoid confusion.
>> - `get` in Rust is often used to fetch a value from e.g. a datastructure
>>     such as a hashmap, so I expect the call to do some computation.
>> - getters in Rust usually are not prefixed with `get_`, but rather are
>>     just the name of the field.
>> - in this case I like the name `is_link_up` much better, since code becomes
>>     a lot more readable with that.
>> - I do not want this pattern as an example for other drivers.
>>
>> [1]: https://lore.kernel.org/rust-for-linux/[e-mail]/
>>
>>> +        const LINK_IS_UP: u32 = 1;
>>> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
>>> +        let phydev = unsafe { *self.0.get() };
>>> +        phydev.link() == LINK_IS_UP
>>> +    }
> 
> During the reviews we have had a lot of misunderstanding what this
> actually does, given its name. Some thought it poked around in
> registers to get the current state of the link. Some thought it
> triggered the PHY to establish a link. When in fact its just a dumb
> getter. And we have a few other dumb getters and setters.

IMO `is_link_up` would indicate that it is a dumb getter.

> So i would prefer something which indicates its a dumb getter. If the
> norm of Rust is just the field name, lets just use the field name. But
> we should do that for all the getters and setters. Is there a naming
> convention for things which take real actions?

For bool getters it would be the norm to use `is_` as the prefix, see
[1]. In this case I would say it is also natural to not use `is_link`,
but rather `is_link_up`, since the former sounds weird.

[1]: https://doc.rust-lang.org/std/?search=is_

> And maybe we need to add a comment: Get the current link state, as
> stored in the [`Device`]. Set the duplex value in [`Device`], etc.

Sure, we can document dumb getters explicitly, I would prefer to do:
Getter for the current link state. Setter for the duplex value.

I don't think that we need to link to `Device`, since the functions
are defined on that type.

Benno Lossin Oct. 21, 2023, 8:01 a.m. UTC | #27

On 20.10.23 21:50, Andrew Lunn wrote:
>> I will try to explain things a bit more.
>>
>> So this case is a bit difficult to figure out, because what is
>> going on is not really a pattern that is used in Rust.
> 
> It is however a reasonably common pattern in the kernel. It stems from
> driver writers often don't understand locking. So the core does the
> locking, leaving the driver writers to just handle the problems of the
> hardware.

I have seen this pattern the first time here, I am sure more experienced
members such as Miguel and Wedson have seen it before.

I am not saying that it is incompatible with Rust, just that it
wouldn't be done like this if it were purely Rust.

> Rust maybe makes locking more of a visible issue, but if driver
> writers don't understand locking, the language itself does not make
> much difference.

I think Rust will make a big difference:
- you cannot access data protected by a lock without locking the
   lock beforehand.
- you cannot forget to unlock a lock.

>> We already have exclusive access to the `phy_device`, so in Rust
>> you would not need to lock anything to also have exclusive access to the
>> embedded `mii_bus`.
> 
> I would actually say its not the PHY drivers problem at all. The
> mii_bus is a property of the MDIO layers, and it is the MDIO layers
> problem to impose whatever locking it needs for its properties.

Since the MDIO layer would provide its own serialization, in Rust
we would not protect the `mdio_device` with a lock. In this case
it could just be a coincidence that both locks are locked, since
IIUC `phy_device` is locked whenever callbacks are called.

> Also, mii_bus is not embedded. Its a pointer to an mii_bus. The phy
> lock protects the pointer. But its the MDIO layer which needs to
> protect what the pointer points to.

Oh I overlooked the `*`. Then it depends what type of pointer that is,
is the `mii_bus` unique or is it shared? If it is shared, then Rust
would also need another lock there.

Benno Lossin Oct. 21, 2023, 8:37 a.m. UTC | #28

On 21.10.23 09:30, FUJITA Tomonori wrote:
> On Sat, 21 Oct 2023 07:25:17 +0000
> Benno Lossin <[e-mail]> wrote:
> 
>> On 20.10.23 14:54, FUJITA Tomonori wrote:
>>> On Fri, 20 Oct 2023 09:34:46 +0900 (JST)
>>> FUJITA Tomonori <[e-mail]> wrote:
>>>
>>>> On Thu, 19 Oct 2023 15:20:51 +0000
>>>> Benno Lossin <[e-mail]> wrote:
>>>>
>>>>> I would like to remove the mutable static variable and simplify
>>>>> the macro.
>>>>
>>>> How about adding DriverVTable array to Registration?
>>>>
>>>> /// Registration structure for a PHY driver.
>>>> ///
>>>> /// # Invariants
>>>> ///
>>>> /// The `drivers` slice are currently registered to the kernel via `phy_drivers_register`.
>>>> pub struct Registration<const N: usize> {
>>>>       drivers: [DriverVTable; N],
>>>> }
>>>>
>>>> impl<const N: usize> Registration<{ N }> {
>>>>       /// Registers a PHY driver.
>>>>       pub fn register(
>>>>           module: &'static crate::ThisModule,
>>>>           drivers: [DriverVTable; N],
>>>>       ) -> Result<Self> {
>>>>           let mut reg = Registration { drivers };
>>>>           let ptr = reg.drivers.as_mut_ptr().cast::<bindings::phy_driver>();
>>>>           // SAFETY: The type invariants of [`DriverVTable`] ensure that all elements of the `drivers` slice
>>>>           // are initialized properly. So an FFI call with a valid pointer.
>>>>           to_result(unsafe {
>>>>               bindings::phy_drivers_register(ptr, reg.drivers.len().try_into()?, module.0)
>>>>           })?;
>>>>           // INVARIANT: The `drivers` slice is successfully registered to the kernel via `phy_drivers_register`.
>>>>           Ok(reg)
>>>>       }
>>>> }
>>>
>>> Scratch this.
>>>
>>> This doesn't work. Also simply putting slice of DriverVTable into
>>> Module strcut doesn't work.
>>
>> Why does it not work? I tried it and it compiled fine for me.
> 
> You can compile but the kernel crashes. The addresses of the callback
> functions are invalid.

Can you please share your setup and the error? For me it booted fine.

FUJITA Tomonori Oct. 21, 2023, 10:27 a.m. UTC | #29

On Sat, 21 Oct 2023 08:37:08 +0000
Benno Lossin <[e-mail]> wrote:

> On 21.10.23 09:30, FUJITA Tomonori wrote:
>> On Sat, 21 Oct 2023 07:25:17 +0000
>> Benno Lossin <[e-mail]> wrote:
>> 
>>> On 20.10.23 14:54, FUJITA Tomonori wrote:
>>>> On Fri, 20 Oct 2023 09:34:46 +0900 (JST)
>>>> FUJITA Tomonori <[e-mail]> wrote:
>>>>
>>>>> On Thu, 19 Oct 2023 15:20:51 +0000
>>>>> Benno Lossin <[e-mail]> wrote:
>>>>>
>>>>>> I would like to remove the mutable static variable and simplify
>>>>>> the macro.
>>>>>
>>>>> How about adding DriverVTable array to Registration?
>>>>>
>>>>> /// Registration structure for a PHY driver.
>>>>> ///
>>>>> /// # Invariants
>>>>> ///
>>>>> /// The `drivers` slice are currently registered to the kernel via `phy_drivers_register`.
>>>>> pub struct Registration<const N: usize> {
>>>>>       drivers: [DriverVTable; N],
>>>>> }
>>>>>
>>>>> impl<const N: usize> Registration<{ N }> {
>>>>>       /// Registers a PHY driver.
>>>>>       pub fn register(
>>>>>           module: &'static crate::ThisModule,
>>>>>           drivers: [DriverVTable; N],
>>>>>       ) -> Result<Self> {
>>>>>           let mut reg = Registration { drivers };
>>>>>           let ptr = reg.drivers.as_mut_ptr().cast::<bindings::phy_driver>();
>>>>>           // SAFETY: The type invariants of [`DriverVTable`] ensure that all elements of the `drivers` slice
>>>>>           // are initialized properly. So an FFI call with a valid pointer.
>>>>>           to_result(unsafe {
>>>>>               bindings::phy_drivers_register(ptr, reg.drivers.len().try_into()?, module.0)
>>>>>           })?;
>>>>>           // INVARIANT: The `drivers` slice is successfully registered to the kernel via `phy_drivers_register`.
>>>>>           Ok(reg)
>>>>>       }
>>>>> }
>>>>
>>>> Scratch this.
>>>>
>>>> This doesn't work. Also simply putting slice of DriverVTable into
>>>> Module strcut doesn't work.
>>>
>>> Why does it not work? I tried it and it compiled fine for me.
>> 
>> You can compile but the kernel crashes. The addresses of the callback
>> functions are invalid.
> 
> Can you please share your setup and the error? For me it booted
>fine.

You use ASIX PHY hardware?

I use the following macro:

    (drivers: [$($driver:ident),+], device_table: [$($dev:expr),+], $($f:tt)*) => {
        const N: usize = $crate::module_phy_driver!(@count_devices $($driver),+);
        struct Module {
            _drivers: [::kernel::net::phy::DriverVTable; N],
        }

        $crate::prelude::module! {
            type: Module,
            $($f)*
        }

        unsafe impl Sync for Module {}

        impl ::kernel::Module for Module {
            fn init(module: &'static ThisModule) -> Result<Self> {
                let mut m = Module {
                    _drivers:[$(::kernel::net::phy::create_phy_driver::<$driver>()),+],
                };
                let ptr = m._drivers.as_mut_ptr().cast::<::kernel::bindings::phy_driver>();
                ::kernel::error::to_result(unsafe {
                    kernel::bindings::phy_drivers_register(ptr, m._drivers.len().try_into()?, module.as_ptr())
                })?;
                Ok(m)
            }
        }


[  176.809218] asix 1-7:1.0 (unnamed net_device) (uninitialized): PHY [usb-001:003:10] driver [Asix Electronics AX88772A] (irq=POLL)
[  176.812371] Asix Electronics AX88772A usb-001:003:10: attached PHY driver (mii_bus:phy_addr=usb-001:003:10, irq=POLL)
[  176.812840] asix 1-7:1.0 eth0: register 'asix' at usb-0000:00:14.0-7, ASIX AX88772 USB 2.0 Ethernet, 08:6d:41:e4:30:66
[  176.812927] usbcore: registered new interface driver asix
[  176.816371] asix 1-7:1.0 enx086d41e43066: renamed from eth0
[  176.872711] asix 1-7:1.0 enx086d41e43066: configuring for phy/internal link mode
[  179.002965] asix 1-7:1.0 enx086d41e43066: Link is Up - 100Mbps/Full - flow control off
[  319.672300] loop0: detected capacity change from 0 to 8
[  367.936371] asix 1-7:1.0 enx086d41e43066: Link is Down
[  370.459947] asix 1-7:1.0 enx086d41e43066: configuring for phy/internal link mode
[  372.599320] asix 1-7:1.0 enx086d41e43066: Link is Up - 100Mbps/Full - flow control off
[  615.277509] BUG: unable to handle page fault for address: ffffc90000752e98
[  615.277598] #PF: supervisor read access in kernel mode
[  615.277653] #PF: error_code(0x0000) - not-present page
[  615.277706] PGD 100000067 P4D 100000067 PUD 1001f0067 PMD 102dad067 PTE 0
[  615.277761] Oops: 0000 [#1] PREEMPT SMP
[  615.277793] CPU: 14 PID: 147 Comm: kworker/14:1 Tainted: G           OE      6.6.0-rc4+ #2
[  615.277877] Hardware name: HP HP Slim Desktop S01-pF3xxx/8B3C, BIOS F.05 02/08/2023
[  615.277929] Workqueue: events_power_efficient phy_state_machine
[  615.277978] RIP: 0010:phy_check_link_status+0x28/0xd0
[  615.278018] Code: 1f 00 0f 1f 44 00 00 55 48 89 e5 41 56 53 f6 87 dd 03 00 00 01 0f 85 ac 00 00 00 49 89 fe 48 8b 87 40 03 00 00 48 85 c0 74 13 <48> 8b 80 10 01 00 00 4c 89 f7 48 85 c0 74 0e ff d0 eb 0f bb fb ff
[  615.278136] RSP: 0018:ffffc90000823de8 EFLAGS: 00010286
[  615.278174] RAX: ffffc90000752d88 RBX: ffff8881023524e0 RCX: ffff888102e39980
[  615.278223] RDX: ffff88846fbb18e8 RSI: 0000000000000000 RDI: ffff888102352000
[  615.278269] RBP: ffffc90000823df8 R08: 8080808080808080 R09: fefefefefefefeff
[  615.278316] R10: 0000000000000007 R11: 6666655f7265776f R12: ffff88846fbb18c0
[  615.278364] R13: ffff888102b75000 R14: ffff888102352000 R15: ffff888102352000
[  615.278412] FS:  0000000000000000(0000) GS:ffff88846fb80000(0000) knlGS:0000000000000000
[  615.278491] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  615.278532] CR2: ffffc90000752e98 CR3: 0000000005433000 CR4: 0000000000750ee0
[  615.278579] PKRU: 55555554
[  615.278609] Call Trace:
[  615.278629]  <TASK>
[  615.278649]  ? __die_body+0x6b/0xb0
[  615.278686]  ? __die+0x86/0x90
[  615.278725]  ? page_fault_oops+0x369/0x3e0
[  615.278771]  ? usb_control_msg+0xfc/0x140
[  615.278809]  ? kfree+0x82/0x180
[  615.278838]  ? usb_control_msg+0xfc/0x140
[  615.278871]  ? kernelmode_fixup_or_oops+0xd5/0x100
[  615.278923]  ? __bad_area_nosemaphore+0x69/0x290
[  615.278972]  ? bad_area_nosemaphore+0x16/0x20
[  615.279004]  ? do_kern_addr_fault+0x7c/0x90
[  615.279036]  ? exc_page_fault+0xbc/0x220
[  615.279081]  ? asm_exc_page_fault+0x27/0x30
[  615.279120]  ? phy_check_link_status+0x28/0xd0
[  615.279167]  ? mutex_lock+0x14/0x70
[  615.279198]  phy_state_machine+0xb1/0x2c0
[  615.279231]  process_one_work+0x16f/0x3f0
[  615.279263]  ? wq_worker_running+0x11/0x90
[  615.279310]  worker_thread+0x360/0x4c0
[  615.279351]  ? __kthread_parkme+0x4c/0xb0
[  615.279384]  kthread+0xf6/0x120
[  615.279412]  ? pr_cont_work_flush+0xf0/0xf0
[  615.279442]  ? kthread_blkcg+0x30/0x30
[  615.279485]  ret_from_fork+0x35/0x40
[  615.279528]  ? kthread_blkcg+0x30/0x30
[  615.279570]  ret_from_fork_asm+0x11/0x20
[  615.279619]  </TASK>
[  615.279644] Modules linked in: asix(E) rust_ax88796b(OE) intel_rapl_msr(E) intel_rapl_common(E) intel_uncore_frequency(E) intel_uncore_frequency_common(E) rtw88_8821ce(E) rtw88_8821c(E) rtw88_pci(E) rtw88_core(E) x86_pkg_temp_thermal(E) intel_powerclamp(E) mac80211(E) coretemp(E) rapl(E) libarc4(E) nls_iso8859_1(E) mei_me(E) intel_cstate(E) input_leds(E) apple_mfi_fastcharge(E) wmi_bmof(E) ee1004(E) cfg80211(E) mei(E) acpi_pad(E) acpi_tad(E) sch_fq_codel(E) msr(E) ramoops(E) reed_solomon(E) pstore_blk(E) pstore_zone(E) efi_pstore(E) ip_tables(E) x_tables(E) hid_generic(E) usbhid(E) hid(E) usbnet(E) phylink(E) mii(E) crct10dif_pclmul(E) crc32_pclmul(E) ghash_clmulni_intel(E) sha512_ssse3(E) r8169(E) aesni_intel(E) crypto_simd(E) cryptd(E) i2c_i801(E) i2c_smbus(E) realtek(E) xhci_pci(E) xhci_pci_renesas(E) video(E) wmi(E) [last unloaded: ax88796b(E)]
[  615.280107] CR2: ffffc90000752e98
[  615.280133] ---[ end trace 0000000000000000 ]---
[  615.365054] RIP: 0010:phy_check_link_status+0x28/0xd0
[  615.365065] Code: 1f 00 0f 1f 44 00 00 55 48 89 e5 41 56 53 f6 87 dd 03 00 00 01 0f 85 ac 00 00 00 49 89 fe 48 8b 87 40 03 00 00 48 85 c0 74 13 <48> 8b 80 10 01 00 00 4c 89 f7 48 85 c0 74 0e ff d0 eb 0f bb fb ff
[  615.365104] RSP: 0018:ffffc90000823de8 EFLAGS: 00010286
[  615.365116] RAX: ffffc90000752d88 RBX: ffff8881023524e0 RCX: ffff888102e39980
[  615.365130] RDX: ffff88846fbb18e8 RSI: 0000000000000000 RDI: ffff888102352000
[  615.365144] RBP: ffffc90000823df8 R08: 8080808080808080 R09: fefefefefefefeff
[  615.365157] R10: 0000000000000007 R11: 6666655f7265776f R12: ffff88846fbb18c0
[  615.365171] R13: ffff888102b75000 R14: ffff888102352000 R15: ffff888102352000
[  615.365185] FS:  0000000000000000(0000) GS:ffff88846fb80000(0000) knlGS:0000000000000000
[  615.365210] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  615.365222] CR2: ffffc90000752e98 CR3: 0000000111635000 CR4: 0000000000750ee0
[  615.365237] PKRU: 55555554
[  615.365247] note: kworker/14:1[147] exited with irqs disabled
[  619.668322] loop0: detected capacity change from 0 to 8
[  919.664303] loop0: detected capacity change from 0 to 8
[ 1219.660223] loop0: detected capacity change from 0 to 8
[ 1519.656041] loop0: detected capacity change from 0 to 8
[ 1819.651769] loop0: detected capacity change from 0 to 8
[ 2119.647826] loop0: detected capacity change from 0 to 8
[ 2419.643470] loop0: detected capacity change from 0 to 8

Benno Lossin Oct. 21, 2023, 11:21 a.m. UTC | #30

On 21.10.23 12:27, FUJITA Tomonori wrote:
> On Sat, 21 Oct 2023 08:37:08 +0000
> Benno Lossin <[e-mail]> wrote:
> 
>> On 21.10.23 09:30, FUJITA Tomonori wrote:
>>> On Sat, 21 Oct 2023 07:25:17 +0000
>>> Benno Lossin <[e-mail]> wrote:
>>>
>>>> On 20.10.23 14:54, FUJITA Tomonori wrote:
>>>>> On Fri, 20 Oct 2023 09:34:46 +0900 (JST)
>>>>> FUJITA Tomonori <[e-mail]> wrote:
>>>>>
>>>>>> On Thu, 19 Oct 2023 15:20:51 +0000
>>>>>> Benno Lossin <[e-mail]> wrote:
>>>>>>
>>>>>>> I would like to remove the mutable static variable and simplify
>>>>>>> the macro.
>>>>>>
>>>>>> How about adding DriverVTable array to Registration?
>>>>>>
>>>>>> /// Registration structure for a PHY driver.
>>>>>> ///
>>>>>> /// # Invariants
>>>>>> ///
>>>>>> /// The `drivers` slice are currently registered to the kernel via `phy_drivers_register`.
>>>>>> pub struct Registration<const N: usize> {
>>>>>>        drivers: [DriverVTable; N],
>>>>>> }
>>>>>>
>>>>>> impl<const N: usize> Registration<{ N }> {
>>>>>>        /// Registers a PHY driver.
>>>>>>        pub fn register(
>>>>>>            module: &'static crate::ThisModule,
>>>>>>            drivers: [DriverVTable; N],
>>>>>>        ) -> Result<Self> {
>>>>>>            let mut reg = Registration { drivers };
>>>>>>            let ptr = reg.drivers.as_mut_ptr().cast::<bindings::phy_driver>();
>>>>>>            // SAFETY: The type invariants of [`DriverVTable`] ensure that all elements of the `drivers` slice
>>>>>>            // are initialized properly. So an FFI call with a valid pointer.
>>>>>>            to_result(unsafe {
>>>>>>                bindings::phy_drivers_register(ptr, reg.drivers.len().try_into()?, module.0)
>>>>>>            })?;
>>>>>>            // INVARIANT: The `drivers` slice is successfully registered to the kernel via `phy_drivers_register`.
>>>>>>            Ok(reg)
>>>>>>        }
>>>>>> }
>>>>>
>>>>> Scratch this.
>>>>>
>>>>> This doesn't work. Also simply putting slice of DriverVTable into
>>>>> Module strcut doesn't work.
>>>>
>>>> Why does it not work? I tried it and it compiled fine for me.
>>>
>>> You can compile but the kernel crashes. The addresses of the callback
>>> functions are invalid.
>>
>> Can you please share your setup and the error? For me it booted
>> fine.
> 
> You use ASIX PHY hardware?

It seems I have configured something wrong. Can you share your testing
setup? Do you use a virtual PHY device in qemu, or do you boot it from
real hardware with a real ASIX PHY device?

> I use the following macro:
> 
>      (drivers: [$($driver:ident),+], device_table: [$($dev:expr),+], $($f:tt)*) => {
>          const N: usize = $crate::module_phy_driver!(@count_devices $($driver),+);
>          struct Module {
>              _drivers: [::kernel::net::phy::DriverVTable; N],
>          }
> 
>          $crate::prelude::module! {
>              type: Module,
>              $($f)*
>          }
> 
>          unsafe impl Sync for Module {}
> 
>          impl ::kernel::Module for Module {
>              fn init(module: &'static ThisModule) -> Result<Self> {
>                  let mut m = Module {
>                      _drivers:[$(::kernel::net::phy::create_phy_driver::<$driver>()),+],
>                  };
>                  let ptr = m._drivers.as_mut_ptr().cast::<::kernel::bindings::phy_driver>();
>                  ::kernel::error::to_result(unsafe {
>                      kernel::bindings::phy_drivers_register(ptr, m._drivers.len().try_into()?, module.as_ptr())
>                  })?;
>                  Ok(m)
>              }
>          }
> 
> 
> [  176.809218] asix 1-7:1.0 (unnamed net_device) (uninitialized): PHY [usb-001:003:10] driver [Asix Electronics AX88772A] (irq=POLL)
> [  176.812371] Asix Electronics AX88772A usb-001:003:10: attached PHY driver (mii_bus:phy_addr=usb-001:003:10, irq=POLL)
> [  176.812840] asix 1-7:1.0 eth0: register 'asix' at usb-0000:00:14.0-7, ASIX AX88772 USB 2.0 Ethernet, 08:6d:41:e4:30:66
> [  176.812927] usbcore: registered new interface driver asix
> [  176.816371] asix 1-7:1.0 enx086d41e43066: renamed from eth0
> [  176.872711] asix 1-7:1.0 enx086d41e43066: configuring for phy/internal link mode
> [  179.002965] asix 1-7:1.0 enx086d41e43066: Link is Up - 100Mbps/Full - flow control off
> [  319.672300] loop0: detected capacity change from 0 to 8
> [  367.936371] asix 1-7:1.0 enx086d41e43066: Link is Down
> [  370.459947] asix 1-7:1.0 enx086d41e43066: configuring for phy/internal link mode
> [  372.599320] asix 1-7:1.0 enx086d41e43066: Link is Up - 100Mbps/Full - flow control off
> [  615.277509] BUG: unable to handle page fault for address: ffffc90000752e98
> [  615.277598] #PF: supervisor read access in kernel mode
> [  615.277653] #PF: error_code(0x0000) - not-present page
> [  615.277706] PGD 100000067 P4D 100000067 PUD 1001f0067 PMD 102dad067 PTE 0
> [  615.277761] Oops: 0000 [#1] PREEMPT SMP
> [  615.277793] CPU: 14 PID: 147 Comm: kworker/14:1 Tainted: G           OE      6.6.0-rc4+ #2
> [  615.277877] Hardware name: HP HP Slim Desktop S01-pF3xxx/8B3C, BIOS F.05 02/08/2023
> [  615.277929] Workqueue: events_power_efficient phy_state_machine
> [  615.277978] RIP: 0010:phy_check_link_status+0x28/0xd0
> [  615.278018] Code: 1f 00 0f 1f 44 00 00 55 48 89 e5 41 56 53 f6 87 dd 03 00 00 01 0f 85 ac 00 00 00 49 89 fe 48 8b 87 40 03 00 00 48 85 c0 74 13 <48> 8b 80 10 01 00 00 4c 89 f7 48 85 c0 74 0e ff d0 eb 0f bb fb ff
> [  615.278136] RSP: 0018:ffffc90000823de8 EFLAGS: 00010286
> [  615.278174] RAX: ffffc90000752d88 RBX: ffff8881023524e0 RCX: ffff888102e39980
> [  615.278223] RDX: ffff88846fbb18e8 RSI: 0000000000000000 RDI: ffff888102352000
> [  615.278269] RBP: ffffc90000823df8 R08: 8080808080808080 R09: fefefefefefefeff
> [  615.278316] R10: 0000000000000007 R11: 6666655f7265776f R12: ffff88846fbb18c0
> [  615.278364] R13: ffff888102b75000 R14: ffff888102352000 R15: ffff888102352000
> [  615.278412] FS:  0000000000000000(0000) GS:ffff88846fb80000(0000) knlGS:0000000000000000
> [  615.278491] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  615.278532] CR2: ffffc90000752e98 CR3: 0000000005433000 CR4: 0000000000750ee0
> [  615.278579] PKRU: 55555554
> [  615.278609] Call Trace:
> [  615.278629]  <TASK>
> [  615.278649]  ? __die_body+0x6b/0xb0
> [  615.278686]  ? __die+0x86/0x90
> [  615.278725]  ? page_fault_oops+0x369/0x3e0
> [  615.278771]  ? usb_control_msg+0xfc/0x140
> [  615.278809]  ? kfree+0x82/0x180
> [  615.278838]  ? usb_control_msg+0xfc/0x140
> [  615.278871]  ? kernelmode_fixup_or_oops+0xd5/0x100
> [  615.278923]  ? __bad_area_nosemaphore+0x69/0x290
> [  615.278972]  ? bad_area_nosemaphore+0x16/0x20
> [  615.279004]  ? do_kern_addr_fault+0x7c/0x90
> [  615.279036]  ? exc_page_fault+0xbc/0x220
> [  615.279081]  ? asm_exc_page_fault+0x27/0x30
> [  615.279120]  ? phy_check_link_status+0x28/0xd0
> [  615.279167]  ? mutex_lock+0x14/0x70
> [  615.279198]  phy_state_machine+0xb1/0x2c0
> [  615.279231]  process_one_work+0x16f/0x3f0
> [  615.279263]  ? wq_worker_running+0x11/0x90
> [  615.279310]  worker_thread+0x360/0x4c0
> [  615.279351]  ? __kthread_parkme+0x4c/0xb0
> [  615.279384]  kthread+0xf6/0x120
> [  615.279412]  ? pr_cont_work_flush+0xf0/0xf0
> [  615.279442]  ? kthread_blkcg+0x30/0x30
> [  615.279485]  ret_from_fork+0x35/0x40
> [  615.279528]  ? kthread_blkcg+0x30/0x30
> [  615.279570]  ret_from_fork_asm+0x11/0x20
> [  615.279619]  </TASK>
> [  615.279644] Modules linked in: asix(E) rust_ax88796b(OE) intel_rapl_msr(E) intel_rapl_common(E) intel_uncore_frequency(E) intel_uncore_frequency_common(E) rtw88_8821ce(E) rtw88_8821c(E) rtw88_pci(E) rtw88_core(E) x86_pkg_temp_thermal(E) intel_powerclamp(E) mac80211(E) coretemp(E) rapl(E) libarc4(E) nls_iso8859_1(E) mei_me(E) intel_cstate(E) input_leds(E) apple_mfi_fastcharge(E) wmi_bmof(E) ee1004(E) cfg80211(E) mei(E) acpi_pad(E) acpi_tad(E) sch_fq_codel(E) msr(E) ramoops(E) reed_solomon(E) pstore_blk(E) pstore_zone(E) efi_pstore(E) ip_tables(E) x_tables(E) hid_generic(E) usbhid(E) hid(E) usbnet(E) phylink(E) mii(E) crct10dif_pclmul(E) crc32_pclmul(E) ghash_clmulni_intel(E) sha512_ssse3(E) r8169(E) aesni_intel(E) crypto_simd(E) cryptd(E) i2c_i801(E) i2c_smbus(E) realtek(E) xhci_pci(E) xhci_pci_renesas(E) video(E) wmi(E) [last unloaded: ax88796b(E)]
> [  615.280107] CR2: ffffc90000752e98
> [  615.280133] ---[ end trace 0000000000000000 ]---
> [  615.365054] RIP: 0010:phy_check_link_status+0x28/0xd0
> [  615.365065] Code: 1f 00 0f 1f 44 00 00 55 48 89 e5 41 56 53 f6 87 dd 03 00 00 01 0f 85 ac 00 00 00 49 89 fe 48 8b 87 40 03 00 00 48 85 c0 74 13 <48> 8b 80 10 01 00 00 4c 89 f7 48 85 c0 74 0e ff d0 eb 0f bb fb ff
> [  615.365104] RSP: 0018:ffffc90000823de8 EFLAGS: 00010286
> [  615.365116] RAX: ffffc90000752d88 RBX: ffff8881023524e0 RCX: ffff888102e39980
> [  615.365130] RDX: ffff88846fbb18e8 RSI: 0000000000000000 RDI: ffff888102352000
> [  615.365144] RBP: ffffc90000823df8 R08: 8080808080808080 R09: fefefefefefefeff
> [  615.365157] R10: 0000000000000007 R11: 6666655f7265776f R12: ffff88846fbb18c0
> [  615.365171] R13: ffff888102b75000 R14: ffff888102352000 R15: ffff888102352000
> [  615.365185] FS:  0000000000000000(0000) GS:ffff88846fb80000(0000) knlGS:0000000000000000
> [  615.365210] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  615.365222] CR2: ffffc90000752e98 CR3: 0000000111635000 CR4: 0000000000750ee0
> [  615.365237] PKRU: 55555554
> [  615.365247] note: kworker/14:1[147] exited with irqs disabled
> [  619.668322] loop0: detected capacity change from 0 to 8
> [  919.664303] loop0: detected capacity change from 0 to 8
> [ 1219.660223] loop0: detected capacity change from 0 to 8
> [ 1519.656041] loop0: detected capacity change from 0 to 8
> [ 1819.651769] loop0: detected capacity change from 0 to 8
> [ 2119.647826] loop0: detected capacity change from 0 to 8
> [ 2419.643470] loop0: detected capacity change from 0 to 8

I think this is very weird, do you have any idea why this
could happen?

If you don't mind, could you try if the following changes
anything?

     (drivers: [$($driver:ident),+], device_table: [$($dev:expr),+], $($f:tt)*) => {
         const N: usize = $crate::module_phy_driver!(@count_devices $($driver),+);
         struct Module {
             _drivers: [::kernel::net::phy::DriverVTable; N],
         }

         $crate::prelude::module! {
             type: Module,
             $($f)*
         }

         unsafe impl Sync for Module {}

         impl ::kernel::Module for Module {
             fn init(module: &'static ThisModule) -> Result<Self> {
		const DRIVERS: [::kernel::net::phy::DriverVTable; N] = [$(::kernel::net::phy::create_phy_driver::<$driver>()),+];
                 let mut m = Module {
                     _drivers: unsafe { core::ptr::read(&DRIVERS) },
                 };
                 let ptr = m._drivers.as_mut_ptr().cast::<::kernel::bindings::phy_driver>();
                 ::kernel::error::to_result(unsafe {
                     kernel::bindings::phy_drivers_register(ptr, m._drivers.len().try_into()?, module.as_ptr())
                 })?;
                 Ok(m)
             }
         }

and also the variation where you replace `const DRIVERS` with
`static DRIVERS`.

FUJITA Tomonori Oct. 21, 2023, 11:36 a.m. UTC | #31

On Sat, 21 Oct 2023 11:21:12 +0000
Benno Lossin <[e-mail]> wrote:

> On 21.10.23 12:27, FUJITA Tomonori wrote:
>> On Sat, 21 Oct 2023 08:37:08 +0000
>> Benno Lossin <[e-mail]> wrote:
>> 
>>> On 21.10.23 09:30, FUJITA Tomonori wrote:
>>>> On Sat, 21 Oct 2023 07:25:17 +0000
>>>> Benno Lossin <[e-mail]> wrote:
>>>>
>>>>> On 20.10.23 14:54, FUJITA Tomonori wrote:
>>>>>> On Fri, 20 Oct 2023 09:34:46 +0900 (JST)
>>>>>> FUJITA Tomonori <[e-mail]> wrote:
>>>>>>
>>>>>>> On Thu, 19 Oct 2023 15:20:51 +0000
>>>>>>> Benno Lossin <[e-mail]> wrote:
>>>>>>>
>>>>>>>> I would like to remove the mutable static variable and simplify
>>>>>>>> the macro.
>>>>>>>
>>>>>>> How about adding DriverVTable array to Registration?
>>>>>>>
>>>>>>> /// Registration structure for a PHY driver.
>>>>>>> ///
>>>>>>> /// # Invariants
>>>>>>> ///
>>>>>>> /// The `drivers` slice are currently registered to the kernel via `phy_drivers_register`.
>>>>>>> pub struct Registration<const N: usize> {
>>>>>>>        drivers: [DriverVTable; N],
>>>>>>> }
>>>>>>>
>>>>>>> impl<const N: usize> Registration<{ N }> {
>>>>>>>        /// Registers a PHY driver.
>>>>>>>        pub fn register(
>>>>>>>            module: &'static crate::ThisModule,
>>>>>>>            drivers: [DriverVTable; N],
>>>>>>>        ) -> Result<Self> {
>>>>>>>            let mut reg = Registration { drivers };
>>>>>>>            let ptr = reg.drivers.as_mut_ptr().cast::<bindings::phy_driver>();
>>>>>>>            // SAFETY: The type invariants of [`DriverVTable`] ensure that all elements of the `drivers` slice
>>>>>>>            // are initialized properly. So an FFI call with a valid pointer.
>>>>>>>            to_result(unsafe {
>>>>>>>                bindings::phy_drivers_register(ptr, reg.drivers.len().try_into()?, module.0)
>>>>>>>            })?;
>>>>>>>            // INVARIANT: The `drivers` slice is successfully registered to the kernel via `phy_drivers_register`.
>>>>>>>            Ok(reg)
>>>>>>>        }
>>>>>>> }
>>>>>>
>>>>>> Scratch this.
>>>>>>
>>>>>> This doesn't work. Also simply putting slice of DriverVTable into
>>>>>> Module strcut doesn't work.
>>>>>
>>>>> Why does it not work? I tried it and it compiled fine for me.
>>>>
>>>> You can compile but the kernel crashes. The addresses of the callback
>>>> functions are invalid.
>>>
>>> Can you please share your setup and the error? For me it booted
>>> fine.
>> 
>> You use ASIX PHY hardware?
> 
> It seems I have configured something wrong. Can you share your testing
> setup? Do you use a virtual PHY device in qemu, or do you boot it from
> real hardware with a real ASIX PHY device?

real hardware with real ASIX PHY device.

Qemu supports a virtual PHY device?


>> I use the following macro:
>> 
>>      (drivers: [$($driver:ident),+], device_table: [$($dev:expr),+], $($f:tt)*) => {
>>          const N: usize = $crate::module_phy_driver!(@count_devices $($driver),+);
>>          struct Module {
>>              _drivers: [::kernel::net::phy::DriverVTable; N],
>>          }
>> 
>>          $crate::prelude::module! {
>>              type: Module,
>>              $($f)*
>>          }
>> 
>>          unsafe impl Sync for Module {}
>> 
>>          impl ::kernel::Module for Module {
>>              fn init(module: &'static ThisModule) -> Result<Self> {
>>                  let mut m = Module {
>>                      _drivers:[$(::kernel::net::phy::create_phy_driver::<$driver>()),+],
>>                  };
>>                  let ptr = m._drivers.as_mut_ptr().cast::<::kernel::bindings::phy_driver>();
>>                  ::kernel::error::to_result(unsafe {
>>                      kernel::bindings::phy_drivers_register(ptr, m._drivers.len().try_into()?, module.as_ptr())
>>                  })?;
>>                  Ok(m)
>>              }
>>          }
>> 

(snip)

>> [  615.365054] RIP: 0010:phy_check_link_status+0x28/0xd0
>> [  615.365065] Code: 1f 00 0f 1f 44 00 00 55 48 89 e5 41 56 53 f6 87 dd 03 00 00 01 0f 85 ac 00 00 00 49 89 fe 48 8b 87 40 03 00 00 48 85 c0 74 13 <48> 8b 80 10 01 00 00 4c 89 f7 48 85 c0 74 0e ff d0 eb 0f bb fb ff
>> [  615.365104] RSP: 0018:ffffc90000823de8 EFLAGS: 00010286
>> [  615.365116] RAX: ffffc90000752d88 RBX: ffff8881023524e0 RCX: ffff888102e39980
>> [  615.365130] RDX: ffff88846fbb18e8 RSI: 0000000000000000 RDI: ffff888102352000
>> [  615.365144] RBP: ffffc90000823df8 R08: 8080808080808080 R09: fefefefefefefeff
>> [  615.365157] R10: 0000000000000007 R11: 6666655f7265776f R12: ffff88846fbb18c0
>> [  615.365171] R13: ffff888102b75000 R14: ffff888102352000 R15: ffff888102352000
>> [  615.365185] FS:  0000000000000000(0000) GS:ffff88846fb80000(0000) knlGS:0000000000000000
>> [  615.365210] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> [  615.365222] CR2: ffffc90000752e98 CR3: 0000000111635000 CR4: 0000000000750ee0
>> [  615.365237] PKRU: 55555554
>> [  615.365247] note: kworker/14:1[147] exited with irqs disabled
> 
> I think this is very weird, do you have any idea why this
> could happen?

DriverVtable is created on kernel stack, I guess.

> If you don't mind, could you try if the following changes
> anything?

I don't think it works. If you use const for DriverTable, DriverTable
is placed on read-only pages. The C side modifies DriverVTable array
so it does't work.


>      (drivers: [$($driver:ident),+], device_table: [$($dev:expr),+], $($f:tt)*) => {
>          const N: usize = $crate::module_phy_driver!(@count_devices $($driver),+);
>          struct Module {
>              _drivers: [::kernel::net::phy::DriverVTable; N],
>          }
> 
>          $crate::prelude::module! {
>              type: Module,
>              $($f)*
>          }
> 
>          unsafe impl Sync for Module {}
> 
>          impl ::kernel::Module for Module {
>              fn init(module: &'static ThisModule) -> Result<Self> {
> 		const DRIVERS: [::kernel::net::phy::DriverVTable; N] = [$(::kernel::net::phy::create_phy_driver::<$driver>()),+];
>                  let mut m = Module {
>                      _drivers: unsafe { core::ptr::read(&DRIVERS) },
>                  };
>                  let ptr = m._drivers.as_mut_ptr().cast::<::kernel::bindings::phy_driver>();
>                  ::kernel::error::to_result(unsafe {
>                      kernel::bindings::phy_drivers_register(ptr, m._drivers.len().try_into()?, module.as_ptr())
>                  })?;
>                  Ok(m)
>              }
>          }
> 
> and also the variation where you replace `const DRIVERS` with
> `static DRIVERS`.

Probably works. But looks like similar with the current code? This is
simpler?

Benno Lossin Oct. 21, 2023, 12:13 p.m. UTC | #32

On 21.10.23 13:36, FUJITA Tomonori wrote:
> On Sat, 21 Oct 2023 11:21:12 +0000
> Benno Lossin <[e-mail]> wrote:
> 
>> On 21.10.23 12:27, FUJITA Tomonori wrote:
>>> On Sat, 21 Oct 2023 08:37:08 +0000
>>> Benno Lossin <[e-mail]> wrote:
>>>
>>>> On 21.10.23 09:30, FUJITA Tomonori wrote:
>>>>> On Sat, 21 Oct 2023 07:25:17 +0000
>>>>> Benno Lossin <[e-mail]> wrote:
>>>>>
>>>>>> On 20.10.23 14:54, FUJITA Tomonori wrote:
>>>>>>> On Fri, 20 Oct 2023 09:34:46 +0900 (JST)
>>>>>>> FUJITA Tomonori <[e-mail]> wrote:
>>>>>>>
>>>>>>>> On Thu, 19 Oct 2023 15:20:51 +0000
>>>>>>>> Benno Lossin <[e-mail]> wrote:
>>>>>>>>
>>>>>>>>> I would like to remove the mutable static variable and simplify
>>>>>>>>> the macro.
>>>>>>>>
>>>>>>>> How about adding DriverVTable array to Registration?
>>>>>>>>
>>>>>>>> /// Registration structure for a PHY driver.
>>>>>>>> ///
>>>>>>>> /// # Invariants
>>>>>>>> ///
>>>>>>>> /// The `drivers` slice are currently registered to the kernel via `phy_drivers_register`.
>>>>>>>> pub struct Registration<const N: usize> {
>>>>>>>>         drivers: [DriverVTable; N],
>>>>>>>> }
>>>>>>>>
>>>>>>>> impl<const N: usize> Registration<{ N }> {
>>>>>>>>         /// Registers a PHY driver.
>>>>>>>>         pub fn register(
>>>>>>>>             module: &'static crate::ThisModule,
>>>>>>>>             drivers: [DriverVTable; N],
>>>>>>>>         ) -> Result<Self> {
>>>>>>>>             let mut reg = Registration { drivers };
>>>>>>>>             let ptr = reg.drivers.as_mut_ptr().cast::<bindings::phy_driver>();
>>>>>>>>             // SAFETY: The type invariants of [`DriverVTable`] ensure that all elements of the `drivers` slice
>>>>>>>>             // are initialized properly. So an FFI call with a valid pointer.
>>>>>>>>             to_result(unsafe {
>>>>>>>>                 bindings::phy_drivers_register(ptr, reg.drivers.len().try_into()?, module.0)
>>>>>>>>             })?;
>>>>>>>>             // INVARIANT: The `drivers` slice is successfully registered to the kernel via `phy_drivers_register`.
>>>>>>>>             Ok(reg)
>>>>>>>>         }
>>>>>>>> }
>>>>>>>
>>>>>>> Scratch this.
>>>>>>>
>>>>>>> This doesn't work. Also simply putting slice of DriverVTable into
>>>>>>> Module strcut doesn't work.
>>>>>>
>>>>>> Why does it not work? I tried it and it compiled fine for me.
>>>>>
>>>>> You can compile but the kernel crashes. The addresses of the callback
>>>>> functions are invalid.
>>>>
>>>> Can you please share your setup and the error? For me it booted
>>>> fine.
>>>
>>> You use ASIX PHY hardware?
>>
>> It seems I have configured something wrong. Can you share your testing
>> setup? Do you use a virtual PHY device in qemu, or do you boot it from
>> real hardware with a real ASIX PHY device?
> 
> real hardware with real ASIX PHY device.

I see.

> Qemu supports a virtual PHY device?

I have no idea.

[...]

>> I think this is very weird, do you have any idea why this
>> could happen?
> 
> DriverVtable is created on kernel stack, I guess.

But how does that invalidate the function pointers?

>> If you don't mind, could you try if the following changes
>> anything?
> 
> I don't think it works. If you use const for DriverTable, DriverTable
> is placed on read-only pages. The C side modifies DriverVTable array
> so it does't work.

Did you try it? Note that I copy the `DriverVTable` into the Module
struct, so it will not be placed on a read-only page.

>>       (drivers: [$($driver:ident),+], device_table: [$($dev:expr),+], $($f:tt)*) => {
>>           const N: usize = $crate::module_phy_driver!(@count_devices $($driver),+);
>>           struct Module {
>>               _drivers: [::kernel::net::phy::DriverVTable; N],
>>           }
>>
>>           $crate::prelude::module! {
>>               type: Module,
>>               $($f)*
>>           }
>>
>>           unsafe impl Sync for Module {}
>>
>>           impl ::kernel::Module for Module {
>>               fn init(module: &'static ThisModule) -> Result<Self> {
>> 		const DRIVERS: [::kernel::net::phy::DriverVTable; N] = [$(::kernel::net::phy::create_phy_driver::<$driver>()),+];
>>                   let mut m = Module {
>>                       _drivers: unsafe { core::ptr::read(&DRIVERS) },
>>                   };
>>                   let ptr = m._drivers.as_mut_ptr().cast::<::kernel::bindings::phy_driver>();
>>                   ::kernel::error::to_result(unsafe {
>>                       kernel::bindings::phy_drivers_register(ptr, m._drivers.len().try_into()?, module.as_ptr())
>>                   })?;
>>                   Ok(m)
>>               }
>>           }
>>
>> and also the variation where you replace `const DRIVERS` with
>> `static DRIVERS`.
> 
> Probably works. But looks like similar with the current code? This is
> simpler?

Just curious if it has to do with using `static` vs `const`.

FUJITA Tomonori Oct. 21, 2023, 12:38 p.m. UTC | #33

On Sat, 21 Oct 2023 12:13:32 +0000
Benno Lossin <[e-mail]> wrote:

>>>>> Can you please share your setup and the error? For me it booted
>>>>> fine.
>>>>
>>>> You use ASIX PHY hardware?
>>>
>>> It seems I have configured something wrong. Can you share your testing
>>> setup? Do you use a virtual PHY device in qemu, or do you boot it from
>>> real hardware with a real ASIX PHY device?
>> 
>> real hardware with real ASIX PHY device.
> 
> I see.
> 
>> Qemu supports a virtual PHY device?
> 
> I have no idea.

When I had a look at Qemu several months ago, it didn't support such.

> [...]
> 
>>> I think this is very weird, do you have any idea why this
>>> could happen?
>> 
>> DriverVtable is created on kernel stack, I guess.
> 
> But how does that invalidate the function pointers?

Not only funciton pointers. You can't store something on stack for
later use.


>>> If you don't mind, could you try if the following changes
>>> anything?
>> 
>> I don't think it works. If you use const for DriverTable, DriverTable
>> is placed on read-only pages. The C side modifies DriverVTable array
>> so it does't work.
> 
> Did you try it? Note that I copy the `DriverVTable` into the Module
> struct, so it will not be placed on a read-only page.

Ah, I misunderstood code. It doesn't work. DriverVTable on stack.


>>>       (drivers: [$($driver:ident),+], device_table: [$($dev:expr),+], $($f:tt)*) => {
>>>           const N: usize = $crate::module_phy_driver!(@count_devices $($driver),+);
>>>           struct Module {
>>>               _drivers: [::kernel::net::phy::DriverVTable; N],
>>>           }
>>>
>>>           $crate::prelude::module! {
>>>               type: Module,
>>>               $($f)*
>>>           }
>>>
>>>           unsafe impl Sync for Module {}
>>>
>>>           impl ::kernel::Module for Module {
>>>               fn init(module: &'static ThisModule) -> Result<Self> {
>>> 		const DRIVERS: [::kernel::net::phy::DriverVTable; N] = [$(::kernel::net::phy::create_phy_driver::<$driver>()),+];
>>>                   let mut m = Module {
>>>                       _drivers: unsafe { core::ptr::read(&DRIVERS) },
>>>                   };
>>>                   let ptr = m._drivers.as_mut_ptr().cast::<::kernel::bindings::phy_driver>();
>>>                   ::kernel::error::to_result(unsafe {
>>>                       kernel::bindings::phy_drivers_register(ptr, m._drivers.len().try_into()?, module.as_ptr())
>>>                   })?;
>>>                   Ok(m)
>>>               }
>>>           }
>>>
>>> and also the variation where you replace `const DRIVERS` with
>>> `static DRIVERS`.
>> 
>> Probably works. But looks like similar with the current code? This is
>> simpler?
> 
> Just curious if it has to do with using `static` vs `const`.

static doesn't work too due to the same reason.

Miguel Ojeda Oct. 21, 2023, 12:47 p.m. UTC | #34

On Fri, Oct 20, 2023 at 8:42 PM Andrew Lunn <[e-mail]> wrote:
>
> We don't want to hide phy_device too much, since at the moment, the
> abstraction is very minimal. Anybody writing a driver is going to need
> a good understanding of the C code in order to find the helpers they
> need, and then add them to the abstraction. So i would say we need to
> explain the relationship between the C structure and the Rust
> structure, to aid developers.

I don't see how exposing `phy_device` in the documentation (note: not
the implementation) helps with that. If someone has to add things to
the abstraction, then at that point they need to be reading the
implementation of the abstraction, and thus they should read the
comments.

That is why the helpers should in general not be mentioned in the
documentation, i.e. a Rust API user should not care / need to know
about them.

If we mix things up in the docs, then it actually becomes harder later
on to properly split it; and in the Rust side we want to maintain this
 "API documentation" vs. "implementation comments" split. Thus it is
important to do it right in the first examples we will have in-tree.

And if an API is not abstracted yet, it should not be documented. APIs
and their docs should be added together, in the same patch, wherever
possible. Of course, implementation comments are different, and
possibly a designer of an abstraction may establish some rules or
guidelines for future APIs added -- that is fine, but if the user does
not need to know, it should not be in the docs, even if it is added
early.

Regarding this, part of the `phy` module documentation (i.e. the three
paragraphs) in this patch currently sounds more like an implementation
comment to me. It should probably be rewritten/split properly in docs
vs. comments.

> During the reviews we have had a lot of misunderstanding what this
> actually does, given its name. Some thought it poked around in
> registers to get the current state of the link. Some thought it
> triggered the PHY to establish a link. When in fact its just a dumb
> getter. And we have a few other dumb getters and setters.
>
> So i would prefer something which indicates its a dumb getter. If the

Agreed.

> norm of Rust is just the field name, lets just use the field name. But
> we should do that for all the getters and setters. Is there a naming
> convention for things which take real actions?

For the getters, there is
https://rust-lang.github.io/api-guidelines/naming.html#getter-names-follow-rust-convention-c-getter.

For "actual actions" that are non-trivial, starting with a prefix that
is not `set_*` would probably be ideal, i.e. things like read, load,
push, init, register, attach, resolve, link, lock, add, create,
find...

Cheers,
Miguel

Benno Lossin Oct. 21, 2023, 12:50 p.m. UTC | #35

On 21.10.23 14:38, FUJITA Tomonori wrote:
> On Sat, 21 Oct 2023 12:13:32 +0000
> Benno Lossin <[e-mail]> wrote:
> 
>>>>>> Can you please share your setup and the error? For me it booted
>>>>>> fine.
>>>>>
>>>>> You use ASIX PHY hardware?
>>>>
>>>> It seems I have configured something wrong. Can you share your testing
>>>> setup? Do you use a virtual PHY device in qemu, or do you boot it from
>>>> real hardware with a real ASIX PHY device?
>>>
>>> real hardware with real ASIX PHY device.
>>
>> I see.
>>
>>> Qemu supports a virtual PHY device?
>>
>> I have no idea.
> 
> When I had a look at Qemu several months ago, it didn't support such.
> 
>> [...]
>>
>>>> I think this is very weird, do you have any idea why this
>>>> could happen?
>>>
>>> DriverVtable is created on kernel stack, I guess.
>>
>> But how does that invalidate the function pointers?
> 
> Not only funciton pointers. You can't store something on stack for
> later use.

It is not stored on the stack, it is only created on the stack and
moved to a global static later on. The `module!` macro creates a
`static mut __MOD: Option<Module>` where the module data is stored in.

It seems that constructing the driver table not at that location
is somehow interfering with something?

Wedson has a patch [1] to create in-place initialized modules, but
it probably is not completely finished, as he has not yet begun to
post it to the list. But I am sure that it is mature enough for
you to test this hypothesis.

[1]: https://github.com/wedsonaf/linux/commit/484ec70025ff9887d9ca228ec631264039cee355

FUJITA Tomonori Oct. 21, 2023, 1 p.m. UTC | #36

On Sat, 21 Oct 2023 12:50:10 +0000
Benno Lossin <[e-mail]> wrote:

> On 21.10.23 14:38, FUJITA Tomonori wrote:
>> On Sat, 21 Oct 2023 12:13:32 +0000
>> Benno Lossin <[e-mail]> wrote:
>> 
>>>>>>> Can you please share your setup and the error? For me it booted
>>>>>>> fine.
>>>>>>
>>>>>> You use ASIX PHY hardware?
>>>>>
>>>>> It seems I have configured something wrong. Can you share your testing
>>>>> setup? Do you use a virtual PHY device in qemu, or do you boot it from
>>>>> real hardware with a real ASIX PHY device?
>>>>
>>>> real hardware with real ASIX PHY device.
>>>
>>> I see.
>>>
>>>> Qemu supports a virtual PHY device?
>>>
>>> I have no idea.
>> 
>> When I had a look at Qemu several months ago, it didn't support such.
>> 
>>> [...]
>>>
>>>>> I think this is very weird, do you have any idea why this
>>>>> could happen?
>>>>
>>>> DriverVtable is created on kernel stack, I guess.
>>>
>>> But how does that invalidate the function pointers?
>> 
>> Not only funciton pointers. You can't store something on stack for
>> later use.
> 
> It is not stored on the stack, it is only created on the stack and
> moved to a global static later on. The `module!` macro creates a
> `static mut __MOD: Option<Module>` where the module data is stored in.

I know. The problem is that we call phy_drivers_register() with
DriverVTable on stack. Then it was moved.


> It seems that constructing the driver table not at that location
> is somehow interfering with something?
> 
> Wedson has a patch [1] to create in-place initialized modules, but
> it probably is not completely finished, as he has not yet begun to
> post it to the list. But I am sure that it is mature enough for
> you to test this hypothesis.
> 
> [1]: https://github.com/wedsonaf/linux/commit/484ec70025ff9887d9ca228ec631264039cee355
> 
> -- 
> Cheers,
> Benno
> 
>>>>> If you don't mind, could you try if the following changes
>>>>> anything?
>>>>
>>>> I don't think it works. If you use const for DriverTable, DriverTable
>>>> is placed on read-only pages. The C side modifies DriverVTable array
>>>> so it does't work.
>>>
>>> Did you try it? Note that I copy the `DriverVTable` into the Module
>>> struct, so it will not be placed on a read-only page.
>> 
>> Ah, I misunderstood code. It doesn't work. DriverVTable on stack.
>> 
>> 
>>>>>        (drivers: [$($driver:ident),+], device_table: [$($dev:expr),+], $($f:tt)*) => {
>>>>>            const N: usize = $crate::module_phy_driver!(@count_devices $($driver),+);
>>>>>            struct Module {
>>>>>                _drivers: [::kernel::net::phy::DriverVTable; N],
>>>>>            }
>>>>>
>>>>>            $crate::prelude::module! {
>>>>>                type: Module,
>>>>>                $($f)*
>>>>>            }
>>>>>
>>>>>            unsafe impl Sync for Module {}
>>>>>
>>>>>            impl ::kernel::Module for Module {
>>>>>                fn init(module: &'static ThisModule) -> Result<Self> {
>>>>> 		const DRIVERS: [::kernel::net::phy::DriverVTable; N] = [$(::kernel::net::phy::create_phy_driver::<$driver>()),+];
>>>>>                    let mut m = Module {
>>>>>                        _drivers: unsafe { core::ptr::read(&DRIVERS) },
>>>>>                    };
>>>>>                    let ptr = m._drivers.as_mut_ptr().cast::<::kernel::bindings::phy_driver>();
>>>>>                    ::kernel::error::to_result(unsafe {
>>>>>                        kernel::bindings::phy_drivers_register(ptr, m._drivers.len().try_into()?, module.as_ptr())
>>>>>                    })?;
>>>>>                    Ok(m)
>>>>>                }
>>>>>            }
>>>>>
>>>>> and also the variation where you replace `const DRIVERS` with
>>>>> `static DRIVERS`.
>>>>
>>>> Probably works. But looks like similar with the current code? This is
>>>> simpler?
>>>
>>> Just curious if it has to do with using `static` vs `const`.
>> 
>> static doesn't work too due to the same reason.
> 
> 
>

Benno Lossin Oct. 21, 2023, 1:05 p.m. UTC | #37

On 21.10.23 15:00, FUJITA Tomonori wrote:
> On Sat, 21 Oct 2023 12:50:10 +0000
> Benno Lossin <[e-mail]> wrote:
>>>>>> I think this is very weird, do you have any idea why this
>>>>>> could happen?
>>>>>
>>>>> DriverVtable is created on kernel stack, I guess.
>>>>
>>>> But how does that invalidate the function pointers?
>>>
>>> Not only funciton pointers. You can't store something on stack for
>>> later use.
>>
>> It is not stored on the stack, it is only created on the stack and
>> moved to a global static later on. The `module!` macro creates a
>> `static mut __MOD: Option<Module>` where the module data is stored in.
> 
> I know. The problem is that we call phy_drivers_register() with
> DriverVTable on stack. Then it was moved.

I see, what exactly is the problem with that? In other words:
why does PHYLIB need `phy_driver` to stay at the same address?

This is an important requirement in Rust. Rust can ensure that
types are not moved by means of pinning them. In this case, Wedson's
patch below should fix the issue completely.

But we should also fix this in the abstractions, the `DriverVTable`
type should only be constructible in a pinned state. For this purpose
we have the `pin-init` API [2].

Are there any other things in PHY that must not change address?

[2]: https://rust-for-linux.github.io/docs/v6.6-rc2/kernel/init/index.html

FUJITA Tomonori Oct. 21, 2023, 1:31 p.m. UTC | #38

On Sat, 21 Oct 2023 13:05:59 +0000
Benno Lossin <[e-mail]> wrote:

> On 21.10.23 15:00, FUJITA Tomonori wrote:
>> On Sat, 21 Oct 2023 12:50:10 +0000
>> Benno Lossin <[e-mail]> wrote:
>>>>>>> I think this is very weird, do you have any idea why this
>>>>>>> could happen?
>>>>>>
>>>>>> DriverVtable is created on kernel stack, I guess.
>>>>>
>>>>> But how does that invalidate the function pointers?
>>>>
>>>> Not only funciton pointers. You can't store something on stack for
>>>> later use.
>>>
>>> It is not stored on the stack, it is only created on the stack and
>>> moved to a global static later on. The `module!` macro creates a
>>> `static mut __MOD: Option<Module>` where the module data is stored in.
>> 
>> I know. The problem is that we call phy_drivers_register() with
>> DriverVTable on stack. Then it was moved.
> 
> I see, what exactly is the problem with that? In other words:
> why does PHYLIB need `phy_driver` to stay at the same address?

phy_driver_register stores addresses that you passed.

> This is an important requirement in Rust. Rust can ensure that
> types are not moved by means of pinning them. In this case, Wedson's
> patch below should fix the issue completely.
> 
> But we should also fix this in the abstractions, the `DriverVTable`
> type should only be constructible in a pinned state. For this purpose
> we have the `pin-init` API [2].

You can create DriverVTable freely. The restriction is what
phy_driver_register takes. Currently, it needs &'static DriverVTable
array so it works.

The C side uses static allocation too. If someone asks for, we could
loosen the restriction with a complicated implentation. But I doubt
that someone would ask for such.


> Are there any other things in PHY that must not change address?

I don't think so.

Benno Lossin Oct. 21, 2023, 1:35 p.m. UTC | #39

On 21.10.23 15:31, FUJITA Tomonori wrote:
> On Sat, 21 Oct 2023 13:05:59 +0000
> Benno Lossin <[e-mail]> wrote:
> 
>> On 21.10.23 15:00, FUJITA Tomonori wrote:
>>> On Sat, 21 Oct 2023 12:50:10 +0000
>>> Benno Lossin <[e-mail]> wrote:
>>>>>>>> I think this is very weird, do you have any idea why this
>>>>>>>> could happen?
>>>>>>>
>>>>>>> DriverVtable is created on kernel stack, I guess.
>>>>>>
>>>>>> But how does that invalidate the function pointers?
>>>>>
>>>>> Not only funciton pointers. You can't store something on stack for
>>>>> later use.
>>>>
>>>> It is not stored on the stack, it is only created on the stack and
>>>> moved to a global static later on. The `module!` macro creates a
>>>> `static mut __MOD: Option<Module>` where the module data is stored in.
>>>
>>> I know. The problem is that we call phy_drivers_register() with
>>> DriverVTable on stack. Then it was moved.
>>
>> I see, what exactly is the problem with that? In other words:
>> why does PHYLIB need `phy_driver` to stay at the same address?
> 
> phy_driver_register stores addresses that you passed.

But the function pointers don't change?

>> This is an important requirement in Rust. Rust can ensure that
>> types are not moved by means of pinning them. In this case, Wedson's
>> patch below should fix the issue completely.
>>
>> But we should also fix this in the abstractions, the `DriverVTable`
>> type should only be constructible in a pinned state. For this purpose
>> we have the `pin-init` API [2].
> 
> You can create DriverVTable freely. The restriction is what
> phy_driver_register takes. 

Sure you can also keep it this way, I just thought only allowing
pinned creation makes things simpler.

> Currently, it needs &'static DriverVTable
> array so it works.

That is actually also incorrect. As the C side is going to modify
the `DriverVTable`, you should actually use `&'static mut DriverVTable`.
But since it is not allowed to be moved you have to use
`Pin<&'static mut DriverVTable>`.

> The C side uses static allocation too. If someone asks for, we could
> loosen the restriction with a complicated implentation. But I doubt
> that someone would ask for such.

With Wedson's patch you also would be using the static allocation
from `module!`. What my problem is, is that you are using a `static mut`
which is `unsafe` and you do not actually have to use it (with
Wedson's patch of course).

Andrew Lunn Oct. 21, 2023, 3:35 p.m. UTC | #40

> I think Rust will make a big difference:
> - you cannot access data protected by a lock without locking the
>    lock beforehand.
> - you cannot forget to unlock a lock.

It is going to be interesting to look at this in 5 to 10 years time.
By then we hopefully have Rust drivers in subsystems which do the
locking in the core and those which leave it to the drivers. We can
then see if Rust written drivers which have to handle locking do
better than C drivers, or is it still better to do it all in the core.

> >> We already have exclusive access to the `phy_device`, so in Rust
> >> you would not need to lock anything to also have exclusive access to the
> >> embedded `mii_bus`.
> > 
> > I would actually say its not the PHY drivers problem at all. The
> > mii_bus is a property of the MDIO layers, and it is the MDIO layers
> > problem to impose whatever locking it needs for its properties.
> 
> Since the MDIO layer would provide its own serialization, in Rust
> we would not protect the `mdio_device` with a lock. In this case
> it could just be a coincidence that both locks are locked, since
> IIUC `phy_device` is locked whenever callbacks are called.
> 
> > Also, mii_bus is not embedded. Its a pointer to an mii_bus. The phy
> > lock protects the pointer. But its the MDIO layer which needs to
> > protect what the pointer points to.
> 
> Oh I overlooked the `*`. Then it depends what type of pointer that is,
> is the `mii_bus` unique or is it shared? If it is shared, then Rust
> would also need another lock there.

There can be up to 32 PHY drivers using one mii_bus, but in practice
you never get this density. Because there can be multiple PHYs this is
why the mii_bus has a lock, to serialise accesses from those PHYs to
the bus.

And MDIO is to some extend a generic bus. Not everything on an MII bus
is a PHY. Some Ethernet switches are MDIO devices, and they often take
up multiple addresses on the bus. But the locking is all the same.

PHYLIB core holds a reference to the MII bus, so the bus is not going
to go away before the PHY goes away. This is all standard Linux
bus/clients locking. It gets a bit messy with hot-plug, devices like
USB devices. The physical hardware can disappear at any time, but the
software representation stays around until it gets cleaned up in a
controlled manor. So a read/write on a bus can fail because its
physically gone, but you don't have to worry about the mii_bus
structure disappearing.

    Andrew

Andrew Lunn Oct. 21, 2023, 3:41 p.m. UTC | #41

> Qemu supports a virtual PHY device?

Not really. I wish it did. Some MAC emulators have very minimal PHY
driver emulation, but they don't make use of PHYLIB.

Having a real emulated PHY would be nice, it would make testing things
like Energy Efficient Ethernet much simpler.

     Andrew

Andrew Lunn Oct. 21, 2023, 3:57 p.m. UTC | #42

> I see, what exactly is the problem with that? In other words:
> why does PHYLIB need `phy_driver` to stay at the same address?

Again, pretty standard kernel behaviour. The core keeps a linked list
of drivers which have been registered with it. So when the driver
loads, it calls phy_driver_register() and the core adds the passed
structure to a linked list of drivers. Sometime later, the bus is
enumerated and devices found. The core will read a couple of registers
which contain the manufactures ID, model and revision. The linked list
of drivers is walked and a match is performed on the IDs. When a match
is found, phydev->drv is set to the driver structure. Calls into the
driver are then performed through this pointer.

A typically C driver has statically initialised driver structures
which are placed in the data section, or better still the rodata
section. They are not going anywhere until the driver is unloaded. So
there is no problem keeping them on a linked list. Dynamically
creating them is unusual. They are just structures of pointers to
functions, everything is known at link time.

	Andrew

Benno Lossin Oct. 21, 2023, 4:31 p.m. UTC | #43

On 21.10.23 17:57, Andrew Lunn wrote:
>> I see, what exactly is the problem with that? In other words:
>> why does PHYLIB need `phy_driver` to stay at the same address?
> 
> Again, pretty standard kernel behaviour. The core keeps a linked list
> of drivers which have been registered with it. So when the driver
> loads, it calls phy_driver_register() and the core adds the passed
> structure to a linked list of drivers. Sometime later, the bus is
> enumerated and devices found. The core will read a couple of registers
> which contain the manufactures ID, model and revision. The linked list
> of drivers is walked and a match is performed on the IDs. When a match
> is found, phydev->drv is set to the driver structure. Calls into the
> driver are then performed through this pointer.

We have several examples of abstractions over things that embed linked
lists upstream already (e.g. `mutex`) and have developed a special API
that handles them very well. This API ensures that the values cannot be
moved (and if one tries to move it, the compiler errors). In this case
I was not aware of the requirement -- and it was also not noted in any
SAFETY comment (e.g. on `phy_drivers_register`).

> A typically C driver has statically initialised driver structures
> which are placed in the data section, or better still the rodata
> section. They are not going anywhere until the driver is unloaded. So
> there is no problem keeping them on a linked list. Dynamically
> creating them is unusual. They are just structures of pointers to
> functions, everything is known at link time.

In the ideal case I would just like to store them inside of the
`Module` struct (which is placed in the data section). However,
that requires Wedson's patch I linked in this thread.

FUJITA Tomonori Oct. 21, 2023, 9:45 p.m. UTC | #44

On Sat, 21 Oct 2023 13:35:57 +0000
Benno Lossin <[e-mail]> wrote:

>> Currently, it needs &'static DriverVTable
>> array so it works.
> 
> That is actually also incorrect. As the C side is going to modify
> the `DriverVTable`, you should actually use `&'static mut DriverVTable`.
> But since it is not allowed to be moved you have to use
> `Pin<&'static mut DriverVTable>`.

I updated Registration::register(). Needs to add comments on requirement?

impl Registration {
    /// Registers a PHY driver.
    pub fn register(
        module: &'static crate::ThisModule,
        drivers: Pin<&'static mut [DriverVTable]>,
    ) -> Result<Self> {
        // SAFETY: The type invariants of [`DriverVTable`] ensure that all elements of the `drivers` slice
        // are initialized properly. So an FFI call with a valid pointer.
        to_result(unsafe {
            bindings::phy_drivers_register(drivers[0].0.get(), drivers.len().try_into()?, module.0)
        })?;
        // INVARIANT: The `drivers` slice is successfully registered to the kernel via `phy_drivers_register`.
        Ok(Registration { drivers })
    }
}


>> The C side uses static allocation too. If someone asks for, we could
>> loosen the restriction with a complicated implentation. But I doubt
>> that someone would ask for such.
> 
> With Wedson's patch you also would be using the static allocation
> from `module!`. What my problem is, is that you are using a `static mut`
> which is `unsafe` and you do not actually have to use it (with
> Wedson's patch of course).

Like your vtable patch, I improve the code when something useful is
available.

FUJITA Tomonori Oct. 22, 2023, 9:47 a.m. UTC | #45

On Sat, 21 Oct 2023 14:47:04 +0200
Miguel Ojeda <[e-mail]> wrote:

> On Fri, Oct 20, 2023 at 8:42 PM Andrew Lunn <[e-mail]> wrote:
>>
>> We don't want to hide phy_device too much, since at the moment, the
>> abstraction is very minimal. Anybody writing a driver is going to need
>> a good understanding of the C code in order to find the helpers they
>> need, and then add them to the abstraction. So i would say we need to
>> explain the relationship between the C structure and the Rust
>> structure, to aid developers.
> 
> I don't see how exposing `phy_device` in the documentation (note: not
> the implementation) helps with that. If someone has to add things to
> the abstraction, then at that point they need to be reading the
> implementation of the abstraction, and thus they should read the
> comments.
> 
> That is why the helpers should in general not be mentioned in the
> documentation, i.e. a Rust API user should not care / need to know
> about them.
> 
> If we mix things up in the docs, then it actually becomes harder later
> on to properly split it; and in the Rust side we want to maintain this
>  "API documentation" vs. "implementation comments" split. Thus it is
> important to do it right in the first examples we will have in-tree.
> 
> And if an API is not abstracted yet, it should not be documented. APIs
> and their docs should be added together, in the same patch, wherever
> possible. Of course, implementation comments are different, and
> possibly a designer of an abstraction may establish some rules or
> guidelines for future APIs added -- that is fine, but if the user does
> not need to know, it should not be in the docs, even if it is added
> early.
> 
> Regarding this, part of the `phy` module documentation (i.e. the three
> paragraphs) in this patch currently sounds more like an implementation
> comment to me. It should probably be rewritten/split properly in docs
> vs. comments.

Agreed that the first three paragraphs at the top of the file are
implementation comments. Are there any other comments in the file,
which look implementation comments to you? To me, the rest look the
docs for Rust API users.

I'm not sure that a comment on the relationship between C and Rust
structures like "Wraps the kernel's `struct phy_driver`" is API docs
but the in-tree files like mutex.rs have the similar so I assume it's
fine.

Where the implementation comments are supposed to be placed?
Documentation/networking?

Miguel Ojeda Oct. 22, 2023, 11:37 a.m. UTC | #46

On Sun, Oct 22, 2023 at 11:47 AM FUJITA Tomonori
<[e-mail]> wrote:
>
> Agreed that the first three paragraphs at the top of the file are
> implementation comments. Are there any other comments in the file,
> which look implementation comments to you? To me, the rest look the
> docs for Rust API users.

I think some should be improved with that in mind, yeah. For instance,
this one seems good to me:

    /// An instance of a PHY driver.

But this one is not:

    /// Creates the kernel's `phy_driver` instance.

It is especially bad because the first line of the docs is the "short
description" used for lists by `rustdoc`.

For similar reasons, this one is bad (and in this case it is the only line!):

    /// Corresponds to the kernel's `enum phy_state`.

That line could be part of the documentation if you think it is
helpful for a reader as a practical note explaining what it is
supposed to map in the C side. But it should really not be the very
first line / short description.

Instead, the documentation should answer the question "What is this?".
And the answer should be something like "The state of the PHY ......"
as the short description. Then ideally a longer explanation of why it
is needed, how it is intended to be used, what this maps to in the C
side (if relevant), anything else that the user may need to know about
it, particular subtleties if any, examples if relevant, etc.

> I'm not sure that a comment on the relationship between C and Rust
> structures like "Wraps the kernel's `struct phy_driver`" is API docs
> but the in-tree files like mutex.rs have the similar so I assume it's
> fine.

Yes, documenting that something wraps/relies on/maps a particular C
functionality is something we do for clarity and practicality (we also
link the related C headers). This is, I assume, the kind of clarity
Andrew was asking for, i.e. to be practical and let the user know what
they are dealing with on the C side, especially early on.

But if some detail is not needed to use the API, then we should avoid
writing it in the documentation. And if it is needed, but it can be
written in a way that does not depend/reference the C side, then it
should be.

For instance, as you can see from the `mutex.rs` you mention, the
short description does not mention the C side -- it does so
afterwards, and then it goes onto explaining why it is needed, how it
is used (with examples), and so on. The fact that it exposes the C
`struct mutex` is there, because it adds value ("oh, ok, so this is
what I would use if I wanted the C mutex"), but that bit (and the
rest) is not really about explaining how `Mutex` is implemented:

    /// A mutual exclusion primitive.
    ///
    /// Exposes the kernel's [`struct mutex`]. When multiple threads
attempt to lock the same mutex,
    /// only one at a time is allowed to progress, the others will
block (sleep) until the mutex is
    /// unlocked, at which point another thread will be allowed to
wake up and make progress.
    ///
    /// Since it may block, [`Mutex`] needs to be used with care in
atomic contexts.
    ///
    /// Instances of [`Mutex`] need a lock class and to be pinned. The
recommended way to create such
    /// instances is with the [`pin_init`](crate::pin_init) and
[`new_mutex`] macros.
    ///
    /// # Examples
    ///
    /// The following example shows how to declare, allocate and
initialise a struct (`Example`) that
    /// contains an inner struct (`Inner`) that is protected by a mutex.

    ...

    /// The following example shows how to use interior mutability to
modify the contents of a struct
    /// protected by a mutex despite only having a shared reference:

    ...

`Mutex`'s docs are, in fact, a good a good example of how to write docs!

> Where the implementation comments are supposed to be placed?
> Documentation/networking?

No, they would be normal `//` comments and they should be as close to
the relevant code as possible -- please see
https://docs.kernel.org/rust/coding-guidelines.html#comments. They can
still be read in the generated docs via the "source" buttons, so they
will be there for those reading the actual implementation in the
browser.

Instead, `Documentation/` is detached from the actual code. For Rust
code, we hope to use only those for out-of-line information that is
not related to any particular API.

For instance, the "coding guidelines" document I just linked. Or
end-user / distributor documentation. Or, as another example, Wedson
at some point considered adding some high-level design documents, and
if those would not fit any particular API or if they are not intended
for users of the API, they could perhaps go into `Doc/`. Or perhaps
Boqun's idea of having a reviewers guide, etc.

Cheers,
Miguel

Andrew Lunn Oct. 22, 2023, 3:34 p.m. UTC | #47

On Sun, Oct 22, 2023 at 01:37:33PM +0200, Miguel Ojeda wrote:
> On Sun, Oct 22, 2023 at 11:47 AM FUJITA Tomonori
> <[e-mail]> wrote:
> >
> > Agreed that the first three paragraphs at the top of the file are
> > implementation comments. Are there any other comments in the file,
> > which look implementation comments to you? To me, the rest look the
> > docs for Rust API users.
> 
> I think some should be improved with that in mind, yeah. For instance,
> this one seems good to me:
> 
>     /// An instance of a PHY driver.
> 
> But this one is not:
> 
>     /// Creates the kernel's `phy_driver` instance.
> 
> It is especially bad because the first line of the docs is the "short
> description" used for lists by `rustdoc`.
> 
> For similar reasons, this one is bad (and in this case it is the only line!):
> 
>     /// Corresponds to the kernel's `enum phy_state`.
> 
> That line could be part of the documentation if you think it is
> helpful for a reader as a practical note explaining what it is
> supposed to map in the C side. But it should really not be the very
> first line / short description.
> 
> Instead, the documentation should answer the question "What is this?".
> And the answer should be something like "The state of the PHY ......"

Its the state of the state machine, not the state of the PHY. It is
already documented in kernel doc, so we don't really want to duplicate
it. So maybe just cross reference to the kdoc:

https://docs.kernel.org/networking/kapi.html#c.phy_state

> Yes, documenting that something wraps/relies on/maps a particular C
> functionality is something we do for clarity and practicality (we also
> link the related C headers). This is, I assume, the kind of clarity
> Andrew was asking for, i.e. to be practical and let the user know what
> they are dealing with on the C side, especially early on.

I don't think 'early on' is relevant. In the kernel, you pretty much
always need the bigger picture, how a pieces of the puzzle fits in
with what is above it and what is below it. Sometimes you need to
extend what is above and below. Or a reviewer will tell you to move
code into the core, so others can share it, etc.

	Andrew

Benno Lossin Oct. 23, 2023, 6:35 a.m. UTC | #48

On 21.10.23 23:45, FUJITA Tomonori wrote:
> On Sat, 21 Oct 2023 13:35:57 +0000
> Benno Lossin <[e-mail]> wrote:
> 
>>> Currently, it needs &'static DriverVTable
>>> array so it works.
>>
>> That is actually also incorrect. As the C side is going to modify
>> the `DriverVTable`, you should actually use `&'static mut DriverVTable`.
>> But since it is not allowed to be moved you have to use
>> `Pin<&'static mut DriverVTable>`.
> 
> I updated Registration::register(). Needs to add comments on requirement?
> 
> impl Registration {
>      /// Registers a PHY driver.
>      pub fn register(
>          module: &'static crate::ThisModule,
>          drivers: Pin<&'static mut [DriverVTable]>,
>      ) -> Result<Self> {
>          // SAFETY: The type invariants of [`DriverVTable`] ensure that all elements of the `drivers` slice
>          // are initialized properly. So an FFI call with a valid pointer.

This SAFETY comment needs to mention that `drivers[0].0.get()` are
pinned and will not change address.

>          to_result(unsafe {
>              bindings::phy_drivers_register(drivers[0].0.get(), drivers.len().try_into()?, module.0)
>          })?;
>          // INVARIANT: The `drivers` slice is successfully registered to the kernel via `phy_drivers_register`.
>          Ok(Registration { drivers })
>      }
> }

Otherwise this looks good.

> 
> 
>>> The C side uses static allocation too. If someone asks for, we could
>>> loosen the restriction with a complicated implentation. But I doubt
>>> that someone would ask for such.
>>
>> With Wedson's patch you also would be using the static allocation
>> from `module!`. What my problem is, is that you are using a `static mut`
>> which is `unsafe` and you do not actually have to use it (with
>> Wedson's patch of course).
> 
> Like your vtable patch, I improve the code when something useful is
> available.

Sure. If you have the time though, it would be helpful to know
if the patch actually fixes the issue. I am pretty sure it will,
but you never know unless you try.

Benno Lossin Oct. 23, 2023, 6:37 a.m. UTC | #49

On 23.10.23 08:35, Benno Lossin wrote:
> On 21.10.23 23:45, FUJITA Tomonori wrote:
>> On Sat, 21 Oct 2023 13:35:57 +0000
>> Benno Lossin <[e-mail]> wrote:
>>
>>>> Currently, it needs &'static DriverVTable
>>>> array so it works.
>>>
>>> That is actually also incorrect. As the C side is going to modify
>>> the `DriverVTable`, you should actually use `&'static mut DriverVTable`.
>>> But since it is not allowed to be moved you have to use
>>> `Pin<&'static mut DriverVTable>`.
>>
>> I updated Registration::register(). Needs to add comments on requirement?
>>
>> impl Registration {
>>       /// Registers a PHY driver.
>>       pub fn register(
>>           module: &'static crate::ThisModule,
>>           drivers: Pin<&'static mut [DriverVTable]>,
>>       ) -> Result<Self> {
>>           // SAFETY: The type invariants of [`DriverVTable`] ensure that all elements of the `drivers` slice
>>           // are initialized properly. So an FFI call with a valid pointer.
> 
> This SAFETY comment needs to mention that `drivers[0].0.get()` are

Sorry, I meant `drivers` instead of `drivers[0].0.get()`

FUJITA Tomonori Oct. 24, 2023, 1:37 a.m. UTC | #50

On Sun, 22 Oct 2023 17:34:04 +0200
Andrew Lunn <[e-mail]> wrote:

> On Sun, Oct 22, 2023 at 01:37:33PM +0200, Miguel Ojeda wrote:
>> On Sun, Oct 22, 2023 at 11:47 AM FUJITA Tomonori
>> <[e-mail]> wrote:
>> >
>> > Agreed that the first three paragraphs at the top of the file are
>> > implementation comments. Are there any other comments in the file,
>> > which look implementation comments to you? To me, the rest look the
>> > docs for Rust API users.
>> 
>> I think some should be improved with that in mind, yeah. For instance,
>> this one seems good to me:
>> 
>>     /// An instance of a PHY driver.
>> 
>> But this one is not:
>> 
>>     /// Creates the kernel's `phy_driver` instance.
>> 
>> It is especially bad because the first line of the docs is the "short
>> description" used for lists by `rustdoc`.
>> 
>> For similar reasons, this one is bad (and in this case it is the only line!):
>> 
>>     /// Corresponds to the kernel's `enum phy_state`.
>> 
>> That line could be part of the documentation if you think it is
>> helpful for a reader as a practical note explaining what it is
>> supposed to map in the C side. But it should really not be the very
>> first line / short description.
>> 
>> Instead, the documentation should answer the question "What is this?".
>> And the answer should be something like "The state of the PHY ......"
> 
> Its the state of the state machine, not the state of the PHY. It is
> already documented in kernel doc, so we don't really want to duplicate
> it. So maybe just cross reference to the kdoc:
> 
> https://docs.kernel.org/networking/kapi.html#c.phy_state

I added links to the kdoc like:

/// Corresponds to the kernel's [`enum phy_state`](https://docs.kernel.org/networking/kapi.html#c.phy_state).

But the first line needs to a short description so I copy the C
description:

/// PHY state machine states.

I revised all the comments.

Miguel Ojeda Oct. 24, 2023, 8:48 a.m. UTC | #51

On Sun, Oct 22, 2023 at 5:34 PM Andrew Lunn <[e-mail]> wrote:
>
> On Sun, Oct 22, 2023 at 01:37:33PM +0200, Miguel Ojeda wrote:
> >
> > Instead, the documentation should answer the question "What is this?".
> > And the answer should be something like "The state of the PHY ......"
>
> Its the state of the state machine, not the state of the PHY. It is

I didn't say it was the state of the PHY -- please note the dots above.

> already documented in kernel doc, so we don't really want to duplicate
> it. So maybe just cross reference to the kdoc:
>
> https://docs.kernel.org/networking/kapi.html#c.phy_state

Yes, that can be worth it for simple 1:1 cases like the `enum`
(assuming they are properly documented in the C side), but we still
want a suitable short description (e.g. "PHY state machine states"),
like Tomonori did in the version he just sent (v6).

I wondered about the docs of each variant, too, but those should be OK
too, because `rustdoc` does not create an individual page for them, so
one can always see the link to the C docs at the top from the `enum`
description.

> > Yes, documenting that something wraps/relies on/maps a particular C
> > functionality is something we do for clarity and practicality (we also
> > link the related C headers). This is, I assume, the kind of clarity
> > Andrew was asking for, i.e. to be practical and let the user know what
> > they are dealing with on the C side, especially early on.
>
> I don't think 'early on' is relevant. In the kernel, you pretty much
> always need the bigger picture, how a pieces of the puzzle fits in
> with what is above it and what is below it. Sometimes you need to
> extend what is above and below. Or a reviewer will tell you to move
> code into the core, so others can share it, etc.

"Early on" in my reply is referring to what you said earlier, i.e.
that initially abstractions are minimal.

In any case, yes, using complex systems typically requires knowing a
bit of what is going on in different parts, but that does not mean we
cannot make self-contained documentation as much as reasonably
possible. We want that using a particular Rust abstraction does not
require reading its source code or the code in the C side.

In the example that you mention, if the reviewer wants to share code,
then it should be extracted into a new Rust abstraction (and possibly
the C core depending on what it is, of course) and using it from the
driver, but also documenting the Rust abstraction.

Cheers,
Miguel

Patch

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 421d2b62918f..0faebdb184ca 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -60,6 +60,14 @@  config FIXED_PHY
 
 	  Currently tested with mpc866ads and mpc8349e-mitx.
 
+config RUST_PHYLIB_ABSTRACTIONS
+        bool "PHYLIB abstractions support"
+        depends on RUST
+        depends on PHYLIB=y
+        help
+          Adds support needed for PHY drivers written in Rust. It provides
+          a wrapper around the C phylib core.
+
 config SFP
 	tristate "SFP cage support"
 	depends on I2C && PHYLINK
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index c91a3c24f607..ec4ee09a34ad 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -8,6 +8,9 @@ 
 
 #include <kunit/test.h>
 #include <linux/errname.h>
+#include <linux/ethtool.h>
+#include <linux/mdio.h>
+#include <linux/phy.h>
 #include <linux/slab.h>
 #include <linux/refcount.h>
 #include <linux/wait.h>
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index e8811700239a..0588422e273c 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -14,6 +14,7 @@ 
 #![no_std]
 #![feature(allocator_api)]
 #![feature(coerce_unsized)]
+#![feature(const_maybe_uninit_zeroed)]
 #![feature(dispatch_from_dyn)]
 #![feature(new_uninit)]
 #![feature(receiver_trait)]
@@ -36,6 +37,8 @@ 
 pub mod ioctl;
 #[cfg(CONFIG_KUNIT)]
 pub mod kunit;
+#[cfg(CONFIG_NET)]
+pub mod net;
 pub mod prelude;
 pub mod print;
 mod static_assert;
diff --git a/rust/kernel/net.rs b/rust/kernel/net.rs
new file mode 100644
index 000000000000..fe415cb369d3
--- /dev/null
+++ b/rust/kernel/net.rs
@@ -0,0 +1,6 @@ 
+// SPDX-License-Identifier: GPL-2.0
+
+//! Networking.
+
+#[cfg(CONFIG_RUST_PHYLIB_ABSTRACTIONS)]
+pub mod phy;
diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
new file mode 100644
index 000000000000..7d4927ece32f
--- /dev/null
+++ b/rust/kernel/net/phy.rs
@@ -0,0 +1,701 @@ 
+// SPDX-License-Identifier: GPL-2.0
+
+// Copyright (C) 2023 FUJITA Tomonori <[e-mail]>
+
+//! Network PHY device.
+//!
+//! C headers: [`include/linux/phy.h`](../../../../include/linux/phy.h).
+//!
+//! The C side (PHYLIB) guarantees that there is only one thread accessing to one `phy_device` instance
+//! during a callback in `phy_driver`. The callback safely accesses to the instance exclusively.
+//! Except for `resume()` and `suspend()`, PHYLIB holds `phy_device`'s lock during a callback.
+//! PHYLIB doesn't hold `phy_device`'s lock in both `resume()` and `suspend()`. Instead, PHYLIB
+//! updates `phy_device`'s state with `phy_device`'s lock hold, to guarantee that resume() accesses
+//! to the instance exclusively. Note that `resume()` and `suspend()` also are called where only
+//! one thread can access to the instance.
+//!
+//! This abstractions creates [`Device`] instance only during a callback so it's guaranteed that
+//! only one reference exists. All its methods can accesses to the instance exclusively.
+//!
+//! All the PHYLIB helper functions for `phy_device` modify some members in `phy_device`. Except for
+//! getter functions, [`Device`] methods take `&mut self`. This also applied to `read()`, which reads
+//! a hardware register and updates the stats.
+
+use crate::{bindings, error::*, prelude::vtable, str::CStr, types::Opaque};
+use core::marker::PhantomData;
+
+/// Corresponds to the kernel's `enum phy_state`.
+#[derive(PartialEq)]
+pub enum DeviceState {
+    /// PHY device and driver are not ready for anything.
+    Down,
+    /// PHY is ready to send and receive packets.
+    Ready,
+    /// PHY is up, but no polling or interrupts are done.
+    Halted,
+    /// PHY is up, but is in an error state.
+    Error,
+    /// PHY and attached device are ready to do work.
+    Up,
+    /// PHY is currently running.
+    Running,
+    /// PHY is up, but not currently plugged in.
+    NoLink,
+    /// PHY is performing a cable test.
+    CableTest,
+}
+
+/// Represents duplex mode.
+pub enum DuplexMode {
+    /// PHY is in full-duplex mode.
+    Full,
+    /// PHY is in half-duplex mode.
+    Half,
+    /// PHY is in unknown duplex mode.
+    Unknown,
+}
+
+/// An instance of a PHY device.
+///
+/// Wraps the kernel's `struct phy_device`.
+///
+/// # Invariants
+///
+/// `self.0` is always in a valid state.
+#[repr(transparent)]
+pub struct Device(Opaque<bindings::phy_device>);
+
+impl Device {
+    /// Creates a new [`Device`] instance from a raw pointer.
+    ///
+    /// # Safety
+    ///
+    /// This function must only be called from the callbacks in `phy_driver`. PHYLIB guarantees
+    /// the exclusive access for the duration of the lifetime `'a`.
+    unsafe fn from_raw<'a>(ptr: *mut bindings::phy_device) -> &'a mut Self {
+        // CAST: `Self` is a `repr(transparent)` wrapper around `bindings::phy_device`.
+        let ptr = ptr.cast::<Self>();
+        // SAFETY: by the function requirements the pointer is valid and we have unique access for
+        // the duration of `'a`.
+        unsafe { &mut *ptr }
+    }
+
+    /// Gets the id of the PHY.
+    pub fn phy_id(&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(&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 };
+        // TODO: this conversion code will be replaced with automatically generated code by bindgen
+        // when it becomes possible.
+        // better to call WARN_ONCE() when the state is out-of-range (needs to add WARN_ONCE support).
+        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,
+            _ => DeviceState::Error,
+        }
+    }
+
+    /// Returns true if the link is up.
+    pub fn get_link(&self) -> bool {
+        const LINK_IS_UP: u32 = 1;
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        let phydev = unsafe { *self.0.get() };
+        phydev.link() == LINK_IS_UP
+    }
+
+    /// Returns true if auto-negotiation is enabled.
+    pub fn is_autoneg_enabled(&self) -> bool {
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        let phydev = unsafe { *self.0.get() };
+        phydev.autoneg() == bindings::AUTONEG_ENABLE
+    }
+
+    /// Returns true if auto-negotiation is completed.
+    pub fn is_autoneg_completed(&self) -> bool {
+        const AUTONEG_COMPLETED: u32 = 1;
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        let phydev = unsafe { *self.0.get() };
+        phydev.autoneg_complete() == AUTONEG_COMPLETED
+    }
+
+    /// Sets the speed of the PHY.
+    pub fn set_speed(&mut self, speed: u32) {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        unsafe { (*phydev).speed = speed as i32 };
+    }
+
+    /// Sets duplex mode.
+    pub fn set_duplex(&mut self, mode: DuplexMode) {
+        let phydev = self.0.get();
+        let v = match mode {
+            DuplexMode::Full => bindings::DUPLEX_FULL as i32,
+            DuplexMode::Half => bindings::DUPLEX_HALF as i32,
+            DuplexMode::Unknown => bindings::DUPLEX_UNKNOWN as i32,
+        };
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        unsafe { (*phydev).duplex = v };
+    }
+
+    /// Reads a given C22 PHY register.
+    pub fn read(&mut self, regnum: u16) -> Result<u16> {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        let ret = unsafe {
+            bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into())
+        };
+        if ret < 0 {
+            Err(Error::from_errno(ret))
+        } else {
+            Ok(ret as u16)
+        }
+    }
+
+    /// Writes a given C22 PHY register.
+    pub fn write(&mut self, regnum: u16, val: u16) -> Result {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        to_result(unsafe {
+            bindings::mdiobus_write((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into(), val)
+        })
+    }
+
+    /// Reads a paged register.
+    pub fn read_paged(&mut self, page: u16, regnum: u16) -> Result<u16> {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        let ret = unsafe { bindings::phy_read_paged(phydev, page.into(), regnum.into()) };
+        if ret < 0 {
+            Err(Error::from_errno(ret))
+        } else {
+            Ok(ret as u16)
+        }
+    }
+
+    /// Resolves the advertisements into PHY settings.
+    pub fn resolve_aneg_linkmode(&mut self) {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        unsafe { bindings::phy_resolve_aneg_linkmode(phydev) };
+    }
+
+    /// Executes software reset the PHY via `BMCR_RESET` bit.
+    pub fn genphy_soft_reset(&mut self) -> Result {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        to_result(unsafe { bindings::genphy_soft_reset(phydev) })
+    }
+
+    /// Initializes the PHY.
+    pub fn init_hw(&mut self) -> Result {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // so an FFI call with a valid pointer.
+        to_result(unsafe { bindings::phy_init_hw(phydev) })
+    }
+
+    /// Starts auto-negotiation.
+    pub fn start_aneg(&mut self) -> Result {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        to_result(unsafe { bindings::phy_start_aneg(phydev) })
+    }
+
+    /// Resumes the PHY via `BMCR_PDOWN` bit.
+    pub fn genphy_resume(&mut self) -> Result {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        to_result(unsafe { bindings::genphy_resume(phydev) })
+    }
+
+    /// Suspends the PHY via `BMCR_PDOWN` bit.
+    pub fn genphy_suspend(&mut self) -> Result {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        to_result(unsafe { bindings::genphy_suspend(phydev) })
+    }
+
+    /// Checks the link status and updates current link state.
+    pub fn genphy_read_status(&mut self) -> Result<u16> {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        let ret = unsafe { bindings::genphy_read_status(phydev) };
+        if ret < 0 {
+            Err(Error::from_errno(ret))
+        } else {
+            Ok(ret as u16)
+        }
+    }
+
+    /// Updates the link status.
+    pub fn genphy_update_link(&mut self) -> Result {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        to_result(unsafe { bindings::genphy_update_link(phydev) })
+    }
+
+    /// Reads link partner ability.
+    pub fn genphy_read_lpa(&mut self) -> Result {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        to_result(unsafe { bindings::genphy_read_lpa(phydev) })
+    }
+
+    /// Reads PHY abilities.
+    pub fn genphy_read_abilities(&mut self) -> Result {
+        let phydev = self.0.get();
+        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
+        // So an FFI call with a valid pointer.
+        to_result(unsafe { bindings::genphy_read_abilities(phydev) })
+    }
+}
+
+/// Defines certain other features this PHY supports (like interrupts).
+///
+/// These flag values are used in [`Driver::FLAGS`].
+pub mod flags {
+    /// PHY is internal.
+    pub const IS_INTERNAL: u32 = bindings::PHY_IS_INTERNAL;
+    /// PHY needs to be reset after the refclk is enabled.
+    pub const RST_AFTER_CLK_EN: u32 = bindings::PHY_RST_AFTER_CLK_EN;
+    /// Polling is used to detect PHY status changes.
+    pub const POLL_CABLE_TEST: u32 = bindings::PHY_POLL_CABLE_TEST;
+    /// Don't suspend.
+    pub const ALWAYS_CALL_SUSPEND: u32 = bindings::PHY_ALWAYS_CALL_SUSPEND;
+}
+
+/// An adapter for the registration of a PHY driver.
+struct Adapter<T: Driver> {
+    _p: PhantomData<T>,
+}
+
+impl<T: Driver> Adapter<T> {
+    /// # Safety
+    ///
+    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
+    unsafe extern "C" fn soft_reset_callback(
+        phydev: *mut bindings::phy_device,
+    ) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: Preconditions ensure `phydev` is valid.
+            let dev = unsafe { Device::from_raw(phydev) };
+            T::soft_reset(dev)?;
+            Ok(0)
+        })
+    }
+
+    /// # Safety
+    ///
+    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
+    unsafe extern "C" fn get_features_callback(
+        phydev: *mut bindings::phy_device,
+    ) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: Preconditions ensure `phydev` is valid.
+            let dev = unsafe { Device::from_raw(phydev) };
+            T::get_features(dev)?;
+            Ok(0)
+        })
+    }
+
+    /// # Safety
+    ///
+    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
+    unsafe extern "C" fn suspend_callback(phydev: *mut bindings::phy_device) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: Preconditions ensure `phydev` is valid.
+            let dev = unsafe { Device::from_raw(phydev) };
+            T::suspend(dev)?;
+            Ok(0)
+        })
+    }
+
+    /// # Safety
+    ///
+    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
+    unsafe extern "C" fn resume_callback(phydev: *mut bindings::phy_device) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: Preconditions ensure `phydev` is valid.
+            let dev = unsafe { Device::from_raw(phydev) };
+            T::resume(dev)?;
+            Ok(0)
+        })
+    }
+
+    /// # Safety
+    ///
+    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
+    unsafe extern "C" fn config_aneg_callback(
+        phydev: *mut bindings::phy_device,
+    ) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: Preconditions ensure `phydev` is valid.
+            let dev = unsafe { Device::from_raw(phydev) };
+            T::config_aneg(dev)?;
+            Ok(0)
+        })
+    }
+
+    /// # Safety
+    ///
+    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
+    unsafe extern "C" fn read_status_callback(
+        phydev: *mut bindings::phy_device,
+    ) -> core::ffi::c_int {
+        from_result(|| {
+            // SAFETY: Preconditions ensure `phydev` is valid.
+            let dev = unsafe { Device::from_raw(phydev) };
+            T::read_status(dev)?;
+            Ok(0)
+        })
+    }
+
+    /// # Safety
+    ///
+    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
+    unsafe extern "C" fn match_phy_device_callback(
+        phydev: *mut bindings::phy_device,
+    ) -> core::ffi::c_int {
+        // SAFETY: Preconditions ensure `phydev` is valid.
+        let dev = unsafe { Device::from_raw(phydev) };
+        T::match_phy_device(dev) as i32
+    }
+
+    /// # Safety
+    ///
+    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
+    unsafe extern "C" fn read_mmd_callback(
+        phydev: *mut bindings::phy_device,
+        devnum: i32,
+        regnum: u16,
+    ) -> i32 {
+        from_result(|| {
+            // SAFETY: Preconditions ensure `phydev` is valid.
+            let dev = unsafe { Device::from_raw(phydev) };
+            // CAST: the C side verifies devnum < 32.
+            let ret = T::read_mmd(dev, devnum as u8, regnum)?;
+            Ok(ret.into())
+        })
+    }
+
+    /// # Safety
+    ///
+    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
+    unsafe extern "C" fn write_mmd_callback(
+        phydev: *mut bindings::phy_device,
+        devnum: i32,
+        regnum: u16,
+        val: u16,
+    ) -> i32 {
+        from_result(|| {
+            // SAFETY: Preconditions ensure `phydev` is valid.
+            let dev = unsafe { Device::from_raw(phydev) };
+            T::write_mmd(dev, devnum as u8, regnum, val)?;
+            Ok(0)
+        })
+    }
+
+    /// # Safety
+    ///
+    /// `phydev` must be passed by the corresponding callback in `phy_driver`.
+    unsafe extern "C" fn link_change_notify_callback(phydev: *mut bindings::phy_device) {
+        // SAFETY: Preconditions ensure `phydev` is valid.
+        let dev = unsafe { Device::from_raw(phydev) };
+        T::link_change_notify(dev);
+    }
+}
+
+/// An instance of a PHY driver.
+///
+/// Wraps the kernel's `struct phy_driver`.
+///
+/// # Invariants
+///
+/// `self.0` is always in a valid state.
+#[repr(transparent)]
+pub struct DriverType(Opaque<bindings::phy_driver>);
+
+/// Creates the kernel's `phy_driver` instance.
+///
+/// This is used by [`module_phy_driver`] macro to create a static array of `phy_driver`.
+///
+/// [`module_phy_driver`]: crate::module_phy_driver
+pub const fn create_phy_driver<T: Driver>() -> DriverType {
+    // All the fields of `struct phy_driver` are initialized properly.
+    // It ensures the invariants.
+    DriverType(Opaque::new(bindings::phy_driver {
+        name: T::NAME.as_char_ptr().cast_mut(),
+        flags: T::FLAGS,
+        phy_id: T::PHY_DEVICE_ID.id,
+        phy_id_mask: T::PHY_DEVICE_ID.mask_as_int(),
+        soft_reset: if T::HAS_SOFT_RESET {
+            Some(Adapter::<T>::soft_reset_callback)
+        } else {
+            None
+        },
+        get_features: if T::HAS_GET_FEATURES {
+            Some(Adapter::<T>::get_features_callback)
+        } else {
+            None
+        },
+        match_phy_device: if T::HAS_MATCH_PHY_DEVICE {
+            Some(Adapter::<T>::match_phy_device_callback)
+        } else {
+            None
+        },
+        suspend: if T::HAS_SUSPEND {
+            Some(Adapter::<T>::suspend_callback)
+        } else {
+            None
+        },
+        resume: if T::HAS_RESUME {
+            Some(Adapter::<T>::resume_callback)
+        } else {
+            None
+        },
+        config_aneg: if T::HAS_CONFIG_ANEG {
+            Some(Adapter::<T>::config_aneg_callback)
+        } else {
+            None
+        },
+        read_status: if T::HAS_READ_STATUS {
+            Some(Adapter::<T>::read_status_callback)
+        } else {
+            None
+        },
+        read_mmd: if T::HAS_READ_MMD {
+            Some(Adapter::<T>::read_mmd_callback)
+        } else {
+            None
+        },
+        write_mmd: if T::HAS_WRITE_MMD {
+            Some(Adapter::<T>::write_mmd_callback)
+        } else {
+            None
+        },
+        link_change_notify: if T::HAS_LINK_CHANGE_NOTIFY {
+            Some(Adapter::<T>::link_change_notify_callback)
+        } else {
+            None
+        },
+        // SAFETY: The rest is zeroed out to initialize `struct phy_driver`,
+        // sets `Option<&F>` to be `None`.
+        ..unsafe { core::mem::MaybeUninit::<bindings::phy_driver>::zeroed().assume_init() }
+    }))
+}
+
+/// Corresponds to functions in `struct phy_driver`.
+///
+/// This is used to register a PHY driver.
+#[vtable]
+pub trait Driver {
+    /// Defines certain other features this PHY supports.
+    /// It is a combination of the flags in the [`flags`] module.
+    const FLAGS: u32 = 0;
+
+    /// The friendly name of this PHY type.
+    const NAME: &'static CStr;
+
+    /// This driver only works for PHYs with IDs which match this field.
+    /// The default id and mask are zero.
+    const PHY_DEVICE_ID: DeviceId = DeviceId::new_with_custom_mask(0, 0);
+
+    /// Issues a PHY software reset.
+    fn soft_reset(_dev: &mut Device) -> Result {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Probes the hardware to determine what abilities it has.
+    fn get_features(_dev: &mut Device) -> Result {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Returns true if this is a suitable driver for the given phydev.
+    /// If not implemented, matching is based on [`Driver::PHY_DEVICE_ID`].
+    fn match_phy_device(_dev: &Device) -> bool {
+        false
+    }
+
+    /// Configures the advertisement and resets auto-negotiation
+    /// if auto-negotiation is enabled.
+    fn config_aneg(_dev: &mut Device) -> Result {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Determines the negotiated speed and duplex.
+    fn read_status(_dev: &mut Device) -> Result<u16> {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Suspends the hardware, saving state if needed.
+    fn suspend(_dev: &mut Device) -> Result {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Resumes the hardware, restoring state if needed.
+    fn resume(_dev: &mut Device) -> Result {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Overrides the default MMD read function for reading a MMD register.
+    fn read_mmd(_dev: &mut Device, _devnum: u8, _regnum: u16) -> Result<u16> {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Overrides the default MMD write function for writing a MMD register.
+    fn write_mmd(_dev: &mut Device, _devnum: u8, _regnum: u16, _val: u16) -> Result {
+        Err(code::ENOTSUPP)
+    }
+
+    /// Callback for notification of link change.
+    fn link_change_notify(_dev: &mut Device) {}
+}
+
+/// Registration structure for a PHY driver.
+///
+/// # Invariants
+///
+/// The `drivers` slice are currently registered to the kernel via `phy_drivers_register`.
+pub struct Registration {
+    drivers: &'static [DriverType],
+}
+
+impl Registration {
+    /// Registers a PHY driver.
+    pub fn register(
+        module: &'static crate::ThisModule,
+        drivers: &'static [DriverType],
+    ) -> Result<Self> {
+        if drivers.is_empty() {
+            return Err(code::EINVAL);
+        }
+        // SAFETY: The type invariants of [`DriverType`] ensure that all elements of the `drivers` slice
+        // are initialized properly. So an FFI call with a valid pointer.
+        to_result(unsafe {
+            bindings::phy_drivers_register(drivers[0].0.get(), drivers.len().try_into()?, module.0)
+        })?;
+        // INVARIANT: The `drivers` slice is successfully registered to the kernel via `phy_drivers_register`.
+        Ok(Registration { drivers })
+    }
+}
+
+impl Drop for Registration {
+    fn drop(&mut self) {
+        // SAFETY: The type invariants guarantee that `self.drivers` is valid.
+        // So an FFI call with a valid pointer.
+        unsafe {
+            bindings::phy_drivers_unregister(self.drivers[0].0.get(), self.drivers.len() as i32)
+        };
+    }
+}
+
+// SAFETY: `Registration` does not expose any of its state across threads.
+unsafe impl Send for Registration {}
+
+// SAFETY: `Registration` does not expose any of its state across threads.
+unsafe impl Sync for Registration {}
+
+/// Represents the kernel's `struct mdio_device_id`.
+pub struct DeviceId {
+    id: u32,
+    mask: DeviceMask,
+}
+
+impl DeviceId {
+    /// Creates a new instance with the exact match mask.
+    pub const fn new_with_exact_mask(id: u32) -> Self {
+        DeviceId {
+            id,
+            mask: DeviceMask::Exact,
+        }
+    }
+
+    /// Creates a new instance with the model match mask.
+    pub const fn new_with_model_mask(id: u32) -> Self {
+        DeviceId {
+            id,
+            mask: DeviceMask::Model,
+        }
+    }
+
+    /// Creates a new instance with the vendor match mask.
+    pub const fn new_with_vendor_mask(id: u32) -> Self {
+        DeviceId {
+            id,
+            mask: DeviceMask::Vendor,
+        }
+    }
+
+    /// Creates a new instance with a custom match mask.
+    pub const fn new_with_custom_mask(id: u32, mask: u32) -> Self {
+        DeviceId {
+            id,
+            mask: DeviceMask::Custom(mask),
+        }
+    }
+
+    /// Creates a new instance from [`Driver`].
+    pub const fn new_with_driver<T: Driver>() -> Self {
+        T::PHY_DEVICE_ID
+    }
+
+    /// Get a `mask` as u32.
+    pub const fn mask_as_int(&self) -> u32 {
+        self.mask.as_int()
+    }
+
+    // macro use only
+    #[doc(hidden)]
+    pub const fn mdio_device_id(&self) -> bindings::mdio_device_id {
+        bindings::mdio_device_id {
+            phy_id: self.id,
+            phy_id_mask: self.mask.as_int(),
+        }
+    }
+}
+
+enum DeviceMask {
+    Exact,
+    Model,
+    Vendor,
+    Custom(u32),
+}
+
+impl DeviceMask {
+    const MASK_EXACT: u32 = !0;
+    const MASK_MODEL: u32 = !0 << 4;
+    const MASK_VENDOR: u32 = !0 << 10;
+
+    const fn as_int(&self) -> u32 {
+        match self {
+            DeviceMask::Exact => Self::MASK_EXACT,
+            DeviceMask::Model => Self::MASK_MODEL,
+            DeviceMask::Vendor => Self::MASK_VENDOR,
+            DeviceMask::Custom(mask) => *mask,
+        }
+    }
+}